Can you link to the relevant spec in the commit note?
Also, I suspect this would require an intent to prototype and ship
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Can you link to the relevant spec in the commit note?
Also, I suspect this would require an intent to prototype and ship
Yes, this will need an intent to prototype and ship. Since it is an implementation of an existing spec, they can be combined.
This might take me a few days to review as it touches a lot of things.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ui::PAGE_TRANSITION_RELOAD,Why is this safe to remove?
Adam RiceCan you link to the relevant spec in the commit note?
Also, I suspect this would require an intent to prototype and ship
Yes, this will need an intent to prototype and ship. Since it is an implementation of an existing spec, they can be combined.
ui::PAGE_TRANSITION_RELOAD,Why is this safe to remove?
think it is safe to remove because this CL makes PAGE_TRANSITION_RELOAD web-triggerable for script-initiated reloads. Since it's now intentionally web-triggerable, it would be incorrect to test.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ui::PAGE_TRANSITION_RELOAD,Helmut JanuschkaWhy is this safe to remove?
think it is safe to remove because this CL makes PAGE_TRANSITION_RELOAD web-triggerable for script-initiated reloads. Since it's now intentionally web-triggerable, it would be incorrect to test.
But `isReloadNavigation` is a read-only attribute. JavaScript can't even set it, so how does this CL enable JavaScript to trigger PAGE_TRANSITION_RELOAD?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ui::PAGE_TRANSITION_RELOAD,Helmut JanuschkaWhy is this safe to remove?
Adam Ricethink it is safe to remove because this CL makes PAGE_TRANSITION_RELOAD web-triggerable for script-initiated reloads. Since it's now intentionally web-triggerable, it would be incorrect to test.
But `isReloadNavigation` is a read-only attribute. JavaScript can't even set it, so how does this CL enable JavaScript to trigger PAGE_TRANSITION_RELOAD?
i changed `render_frame_impl.cc` to send `PAGE_TRANSITION_RELOAD` for reload navigations (`location.reload()`). Previously, all renderer-initiated navigations used `PAGE_TRANSITION_LINK`.
Since the renderer now sends `PAGE_TRANSITION_RELOAD`, it must be added to `PageTransitionIsWebTriggerable()` - otherwise the browser would kill the renderer.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
+dcheng I need someone with domain knowledge to determine if the change in //content/renderer/render_frame_impl.cc is safe. Would you be a good person?
ui::PAGE_TRANSITION_RELOAD,Helmut JanuschkaWhy is this safe to remove?
Adam Ricethink it is safe to remove because this CL makes PAGE_TRANSITION_RELOAD web-triggerable for script-initiated reloads. Since it's now intentionally web-triggerable, it would be incorrect to test.
Helmut JanuschkaBut `isReloadNavigation` is a read-only attribute. JavaScript can't even set it, so how does this CL enable JavaScript to trigger PAGE_TRANSITION_RELOAD?
i changed `render_frame_impl.cc` to send `PAGE_TRANSITION_RELOAD` for reload navigations (`location.reload()`). Previously, all renderer-initiated navigations used `PAGE_TRANSITION_LINK`.
Since the renderer now sends `PAGE_TRANSITION_RELOAD`, it must be added to `PageTransitionIsWebTriggerable()` - otherwise the browser would kill the renderer.
Oh! I missed that important point. I think I need that part to be reviewed first. Let me find a review for it.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
+dcheng I need someone with domain knowledge to determine if the change in //content/renderer/render_frame_impl.cc is safe. Would you be a good person?
I don't understand why `BeginNavigationInternal()` changes here. Aren't the fetch API and navigation two distinct things?
Daniel Cheng+dcheng I need someone with domain knowledge to determine if the change in //content/renderer/render_frame_impl.cc is safe. Would you be a good person?
I don't understand why `BeginNavigationInternal()` changes here. Aren't the fetch API and navigation two distinct things?
Fetch API and navigation intersect for Service Worker navigation requests.
`Request.isReloadNavigation` tells the SW whether the navigation came from a reload, but for that to work we need to pass the right page transition from the renderer.
Right now `BeginNavigationInternal()` always uses `PAGE_TRANSITION_LINK` [1], even for `location.reload()`. The navigation type is already `kWebNavigationTypeReload` [2], but we weren't converting that into a reload transition. This change fixes that by using `PAGE_TRANSITION_RELOAD` for reloads.
[1] https://source.chromium.org/chromium/chromium/src/+/83f6205f0e033e3c5da53306d66b494254303cce:content/renderer/render_frame_impl.cc;l=6048
[2] https://source.chromium.org/chromium/chromium/src/+/83f6205f0e033e3c5da53306d66b494254303cce:third_party/blink/public/web/web_navigation_type.h;l=40
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ping, anything i can do to move on?