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?
Adam Ricei 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.
Helmut JanuschkaOh! I missed that important point. I think I need that part to be reviewed first. Let me find a review for it.
Adam Riceany update?
Alex MoshchukSorry for the delay.
Helmut JanuschkaOn first glance, this change seems risky to me. It's hard to judge all the implications, but it seems that we've been previously relying on PAGE_TRANSITION_RELOAD to mean that the navigation had been browser-initiated, and now it will be renderer-initiated instead: see comments [here](https://source.chromium.org/chromium/chromium/src/+/main:ui/base/page_transition_types.h;l=83-90;drc=1d1fcea6ebbc1c2c3d04ab5538c74f742c66444f). That might mean that the navigation would be subject to fewer security checks than before, e.g. it seems that a few places that use this to initialize is_renderer_initiated ([example 1](https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/network_service/aw_web_resource_request.cc;l=28-29;drc=1d1fcea6ebbc1c2c3d04ab5538c74f742c66444f), [example 2](https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/safe_browsing/aw_url_checker_delegate_impl.cc;l=54;drc=1d1fcea6ebbc1c2c3d04ab5538c74f742c66444f)) would now be potentially wrong. It might have UI implications as well (where browser-initiated navigations are treated slightly differently from renderer-initiated ones). I'll see if I can discuss this with a couple of other navigation owners and report back here.
Thanks for the detailed analysis, Alex! You're absolutely right about the security implications.
I've refactored the CL to avoid changing `PageTransitionIsWebTriggerable()` entirely:
The `Request.isReloadNavigation` spec requirement is now satisfied via this dedicated field, while keeping existing security invariants intact.
PTAL!
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thanks, that resolves my immediate concerns with changes in //content, but I'll defer to ricea@ and others to actually review this from the networking and fetch API perspective. Happy to approve content when everybody's happy.
info->navigation_type == blink::kWebNavigationTypeFormResubmittedReload) {Why is this change necessary, and what are its implications?
info->navigation_type == blink::kWebNavigationTypeFormResubmittedReload) {Why is this change necessary, and what are its implications?
This makes form-resubmitted reloads map to NavigationType::RELOAD, instead of falling back to DIFFERENT_DOCUMENT. I need that so Request.isReloadNavigation is true for real reloads, including form resubmissions, per the Fetch spec.
Implications should be limited to code that checks NavigationTypeUtils::IsReload and only for these already-reload navigations. It does not change page transition or renderer/browser initiation semantics beyond marking them as reloads, consistent with existing behavior for non-form reloads.
@ri...@chromium.org, ping. i have a question once you CR+1. i'll wait and send: https://chromestatus.com/feature/5154214529597440 to API-OWNERS and once that is green, the CL can land? is that understanding valid?
| Code-Review | +1 |
lgtm, sorry to keep you waiting!
getter isReloadNavigationOh, right, this is web visible, so you'll need API owner approval to land, or put it in //third_party/blink/renderer/platform/runtime_enabled_features.json5 as experimental until you have approval.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ui::PAGE_TRANSITION_RELOAD,Done
Oh, right, this is web visible, so you'll need API owner approval to land, or put it in //third_party/blink/renderer/platform/runtime_enabled_features.json5 as experimental until you have approval.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
<int value="68" label="is_reload_navigation"/>nit: Is this supposed to be 67 instead of 68?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
<int value="68" label="is_reload_navigation"/>nit: Is this supposed to be 67 instead of 68?
Nevermind! I see that the previous Enum for 67 was deprecated. Ignore!
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/58181.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
<int value="68" label="is_reload_navigation"/>Sorry for thrash- but could you actually add a line above this for consistency:
```
<int value="67" label="allow_unsafe_redirect_schemes (deprecated)"/>
```
Thanks!
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sorry for thrash- but could you actually add a line above this for consistency:
```
<int value="67" label="allow_unsafe_redirect_schemes (deprecated)"/>
```Thanks!
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
21 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/metrics/histograms/metadata/network/enums.xml
Insertions: 1, Deletions: 0.
@@ -1473,6 +1473,7 @@
<int value="64" label="expected_public_keys"/>
<int value="65" label="permissions_policy"/>
<int value="66" label="client_side_content_decoding_enabled"/>
+ <int value="67" label="allow_unsafe_redirect_schemes (deprecated)"/>
<int value="68" label="is_reload_navigation"/>
</enum>
```
Add Request.isReloadNavigation attribute
Spec: https://fetch.spec.whatwg.org/#dom-request-isreloadnavigation
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
The exported PR, https://github.com/web-platform-tests/wpt/pull/58181, has failed the following check(s) on GitHub:
wpt-firefox-nightly-stability (https://github.com/web-platform-tests/wpt/runs/65603641380)
These failures will block the export. They may represent new or existing problems; please take a look at the output and see if it can be fixed. Unresolved failures will be looked at by the Ecosystem-Infra sheriff after this CL has been landed in Chromium; if you need earlier help please contact blin...@chromium.org.
Any suggestions to improve this service are welcome; crbug.com/1027618.
Gerrit CL SHA: Latest
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |