Enable Speculation Rules mobile viewport heuristics in DevTools mobile emulation [chromium/src : main]
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
+nhiroki for speculationrules related changes in blink
Related details
- Helmut Januschka
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
- Helmut Januschka
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 3 comments![]( "Open in Gerrit")
It seems that this CL changes how `Preloading{Eager/Moderate}ViewportHeuristics` feature flags are handled. At the same time, this CL lacks a feature flag for the kill switch. How about enabling `Preloading{Eager/Moderate}ViewportHeuristics` by default for DESKTOP and adding new feature params like boolean `only_for_devtools_emulation` to the features, which is `true` by default? I think we can minimize the changes of the semantics of the existing feature flags in that way.
int EffectiveSamplingPeriod() const;nit: Could you make this a private method?
AnchorElementMetricsSender::GetForFrame(GetDocument()->GetFrame());This seems like an incomplete code.
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
It seems that this CL changes how `Preloading{Eager/Moderate}ViewportHeuristics` feature flags are handled. At the same time, this CL lacks a feature flag for the kill switch. How about enabling `Preloading{Eager/Moderate}ViewportHeuristics` by default for DESKTOP and adding new feature params like boolean `only_for_devtools_emulation` to the features, which is `true` by default? I think we can minimize the changes of the semantics of the existing feature flags in that way.
changed the feature setup to keep the existing viewport heuristic features as the kill switch, enabled them by default on desktop, and added `only_for_devtools_emulation` params that default to true on desktop.
nit: Could you make this a private method?
Done
AnchorElementMetricsSender::GetForFrame(GetDocument()->GetFrame());This seems like an incomplete code.
Done. I now use the `GetForFrame()` result explicitly and return early if no metrics sender exists, instead of relying on an ignored side effect.
Related details
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 13 comments![]( "Open in Gerrit")
blink::features::kNavigationPredictorNewViewportFeatures) &&Hmm. This feature looks outdated now. Let me take time to discuss this internally and get back to you. Probably I would remove this feature flag.
bool ViewportHeuristicAllowed(const base::Feature& feature) {
return base::FeatureList::IsEnabled(feature);
}
This seems like just a function alias. Could you remove this?
// The renderer already validates this IPC.I don't think this comment is necessary. It is hard to understand what the renderer "validates" just by reading this code. Could you remove this or break down your intents?
CHECK(base::FeatureList::IsEnabled(
blink::features::kPreloadingModerateViewportHeuristics));Could you restore this CHECK?
CHECK(base::FeatureList::IsEnabled(
blink::features::kPreloadingEagerViewportHeuristics));ditto
return page->GetSettings().GetViewportStyle() ==
mojom::blink::ViewportStyle::kMobile;q: what is the value of `ViewportStyle` for Android Desktop mode or for Android Tablet?
if (&feature == &features::kPreloadingModerateViewportHeuristics) {
only_for_devtools_emulation =
features::kPreloadingModerateViewportHeuristicsOnlyForDevToolsEmulation
.Get();
} else if (&feature == &features::kPreloadingEagerViewportHeuristics) {
only_for_devtools_emulation =
features::kPreloadingEagerViewportHeuristicsOnlyForDevToolsEmulation
.Get();
} else {
NOTREACHED();
}This pattern doesn't seem good. Could you split the function to the Moderate version and the Eager version instead?
if (base::FeatureList::IsEnabled(
blink::features::kPreloadingModerateViewportHeuristics) ||
base::FeatureList::IsEnabled(
blink::features::kPreloadingEagerViewportHeuristics)) {After the flag change, we should keep this guard.
if (!ShouldRunMobileViewportHeuristic(
*GetDocument(),
blink::features::kPreloadingEagerViewportHeuristics)) {
eager_viewport_heuristics_candidates_.clear();
eager_viewport_heuristic_timer_.Stop();
return;
}
// TODO(https://crbug.com/505056924): The state of IsPreloadingEligible may be
// dynamically changed in the future changes, make sure the state is reflected
// correctly at the point.
if (!IsPreloadingEligible()) {
eager_viewport_heuristics_candidates_.clear();
eager_viewport_heuristic_timer_.Stop();
return;
}nit: we can merge these `if`.
```suggestion
// TODO(https://crbug.com/505056924): The state of IsPreloadingEligible may be
// dynamically changed in the future changes, make sure the state is reflected
// correctly at the point.
if (!ShouldRunMobileEagerViewportHeuristic(*GetDocument()) || !IsPreloadingEligible()) {
eager_viewport_heuristics_candidates_.clear();
eager_viewport_heuristic_timer_.Stop();
return;
}
```
if (!ShouldRunMobileViewportHeuristic(
*GetDocument(),
blink::features::kPreloadingModerateViewportHeuristics)) {
largest_anchor_element_in_viewport_ = nullptr;
moderate_viewport_heuristic_timer_.Stop();
return;
}
// TODO(https://crbug.com/505056924): The state of IsPreloadingEligible may be
// dynamically changed in the future changes, make sure the state is reflected
// correctly at the point.
if (!IsPreloadingEligible()) {
largest_anchor_element_in_viewport_ = nullptr;
moderate_viewport_heuristic_timer_.Stop();
return;
}ditto
CHECK(ShouldRunMobileViewportHeuristic(
*GetDocument(), blink::features::kPreloadingModerateViewportHeuristics));The value of `ShouldRunMobileViewportHeuristic()` can be changed dynamically. How about removing this CHECK and dynamically check this together with `IsPreloadingEligible()` below?
if (IsPreloadingEligible()) {```suggestion
if (ShouldRunMobileModerateViewportHeuristic() && IsPreloadingEligible()) {
```
CHECK(ShouldRunMobileViewportHeuristic(ditto
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 12 comments![]( "Open in Gerrit")
blink::features::kNavigationPredictorNewViewportFeatures) &&Hmm. This feature looks outdated now. Let me take time to discuss this internally and get back to you. Probably I would remove this feature flag.
any updates?
bool ViewportHeuristicAllowed(const base::Feature& feature) {
return base::FeatureList::IsEnabled(feature);
}
This seems like just a function alias. Could you remove this?
Done
I don't think this comment is necessary. It is hard to understand what the renderer "validates" just by reading this code. Could you remove this or break down your intents?
removed it.
CHECK(base::FeatureList::IsEnabled(
blink::features::kPreloadingModerateViewportHeuristics));Could you restore this CHECK?
Done
CHECK(base::FeatureList::IsEnabled(
blink::features::kPreloadingEagerViewportHeuristics));Helmut Januschkaditto
Done
return page->GetSettings().GetViewportStyle() ==
mojom::blink::ViewportStyle::kMobile;q: what is the value of `ViewportStyle` for Android Desktop mode or for Android Tablet?
`ViewportStyle` defaults to `kMobile` on Android and `kDefault` elsewhere. In `WebContentsImpl::ComputeWebPreferences`, Android overrides it back to `kDefault` when the device form factor is automotive or when the smallest screen width is >= `kAndroidMinimumTabletWidthDp`. So:
- Android Tablet: `kDefault` (treated as non-mobile here).
- Android Desktop mode: `viewport_style` is not changed by RDS itself, only `viewport_meta_enabled` is flipped to false. So on a phone in RDS it stays `kMobile`, and on a tablet it stays `kDefault`. The mobile-viewport heuristic therefore still fires on a phone in RDS, which matches the intent (the device is still a phone).
if (&feature == &features::kPreloadingModerateViewportHeuristics) {
only_for_devtools_emulation =
features::kPreloadingModerateViewportHeuristicsOnlyForDevToolsEmulation
.Get();
} else if (&feature == &features::kPreloadingEagerViewportHeuristics) {
only_for_devtools_emulation =
features::kPreloadingEagerViewportHeuristicsOnlyForDevToolsEmulation
.Get();
} else {
NOTREACHED();
}This pattern doesn't seem good. Could you split the function to the Moderate version and the Eager version instead?
Done. Split into `ShouldRunModerateMobileViewportHeuristic` and `ShouldRunEagerMobileViewportHeuristic`
if (!ShouldRunMobileViewportHeuristic(
*GetDocument(),
blink::features::kPreloadingEagerViewportHeuristics)) {
eager_viewport_heuristics_candidates_.clear();
eager_viewport_heuristic_timer_.Stop();
return;
}
// TODO(https://crbug.com/505056924): The state of IsPreloadingEligible may be
// dynamically changed in the future changes, make sure the state is reflected
// correctly at the point.
if (!IsPreloadingEligible()) {
eager_viewport_heuristics_candidates_.clear();
eager_viewport_heuristic_timer_.Stop();
return;
}nit: we can merge these `if`.
```suggestion
// TODO(https://crbug.com/505056924): The state of IsPreloadingEligible may be
// dynamically changed in the future changes, make sure the state is reflected
// correctly at the point.
if (!ShouldRunMobileEagerViewportHeuristic(*GetDocument()) || !IsPreloadingEligible()) {
eager_viewport_heuristics_candidates_.clear();
eager_viewport_heuristic_timer_.Stop();
return;
}
```
Done
if (!ShouldRunMobileViewportHeuristic(
*GetDocument(),
blink::features::kPreloadingModerateViewportHeuristics)) {
largest_anchor_element_in_viewport_ = nullptr;
moderate_viewport_heuristic_timer_.Stop();
return;
}
// TODO(https://crbug.com/505056924): The state of IsPreloadingEligible may be
// dynamically changed in the future changes, make sure the state is reflected
// correctly at the point.
if (!IsPreloadingEligible()) {
largest_anchor_element_in_viewport_ = nullptr;
moderate_viewport_heuristic_timer_.Stop();
return;
}Helmut Januschkaditto
Done
CHECK(ShouldRunMobileViewportHeuristic(
*GetDocument(), blink::features::kPreloadingModerateViewportHeuristics));The value of `ShouldRunMobileViewportHeuristic()` can be changed dynamically. How about removing this CHECK and dynamically check this together with `IsPreloadingEligible()` below?
Done
```suggestion
if (ShouldRunMobileModerateViewportHeuristic() && IsPreloadingEligible()) {
```
Done
CHECK(ShouldRunMobileViewportHeuristic(Helmut Januschkaditto
Done
Related details
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
hi all i think this was pretty much near landing, anything from my side i could do to move on with this CL?
Takashi Nakayama (Gerrit)
Takashi Nakayama added 3 comments![]( "Open in Gerrit")
Helmut JanuschkaIt seems that this CL changes how `Preloading{Eager/Moderate}ViewportHeuristics` feature flags are handled. At the same time, this CL lacks a feature flag for the kill switch. How about enabling `Preloading{Eager/Moderate}ViewportHeuristics` by default for DESKTOP and adding new feature params like boolean `only_for_devtools_emulation` to the features, which is `true` by default? I think we can minimize the changes of the semantics of the existing feature flags in that way.
changed the feature setup to keep the existing viewport heuristic features as the kill switch, enabled them by default on desktop, and added `only_for_devtools_emulation` params that default to true on desktop.
LGTM w.r.t. the flag change.
hi all i think this was pretty much near landing, anything from my side i could do to move on with this CL?
Apologies. Please resolve other comments from me.
blink::features::kNavigationPredictorNewViewportFeatures) &&Helmut JanuschkaHmm. This feature looks outdated now. Let me take time to discuss this internally and get back to you. Probably I would remove this feature flag.
any updates?
I sincerely apologize for the delay. The situation is rather complex, as the original authors of this feature are no longer on the team, yet some folks may still be utilizing it. I'd like to keep the feature as-is for the time being. Could you please just ignore this flag and attempt to avoid calling `ReportAnchorElementsPositionUpdate()` when DevTools is attached on Desktop?
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
Takashi Nakayamahi all i think this was pretty much near landing, anything from my side i could do to move on with this CL?
Apologies. Please resolve other comments from me.
Done
thank you, and no worries if it takes a while, really appreciate your review time for an external.
blink::features::kNavigationPredictorNewViewportFeatures) &&Helmut JanuschkaHmm. This feature looks outdated now. Let me take time to discuss this internally and get back to you. Probably I would remove this feature flag.
Takashi Nakayamaany updates?
I sincerely apologize for the delay. The situation is rather complex, as the original authors of this feature are no longer on the team, yet some folks may still be utilizing it. I'd like to keep the feature as-is for the time being. Could you please just ignore this flag and attempt to avoid calling `ReportAnchorElementsPositionUpdate()` when DevTools is attached on Desktop?
Done. Kept the browser-side feature flag check as-is and restricted the renderer-side DevTools mobile-emulation fallback to Android, so desktop DevTools no longer sends `ReportAnchorElementsPositionUpdate()` while the flag is disabled.
if (base::FeatureList::IsEnabled(
blink::features::kPreloadingModerateViewportHeuristics) ||
base::FeatureList::IsEnabled(
blink::features::kPreloadingEagerViewportHeuristics)) {Helmut JanuschkaAfter the flag change, we should keep this guard.
Done.
Related details
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 2 comments![]( "Open in Gerrit")
bool ShouldReportViewportPositions(const Document& document) {
return base::FeatureList::IsEnabled(
features::kNavigationPredictorNewViewportFeatures) ||
IsDevToolsMobileEmulationActive(document);
}I think we can keep this function as-is, i.e., don't have to report viewport positions here for DevTools emulations.
{"PreloadingModerateViewportHeuristicsOnlyForDevToolsEmulation",`only_for_devtools_emulation`? (ditto for the following)
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
bool ShouldReportViewportPositions(const Document& document) {
return base::FeatureList::IsEnabled(
features::kNavigationPredictorNewViewportFeatures) ||
IsDevToolsMobileEmulationActive(document);
}I think we can keep this function as-is, i.e., don't have to report viewport positions here for DevTools emulations.
I'd prefer to keep the `IsDevToolsMobileEmulationActive(document)` check here. `kNavigationPredictorNewViewportFeatures` is disabled by default off-Android, so without this branch `ShouldReportViewportPositions()` returns false on desktop and the position tracker never dispatches anchor position updates.
{"PreloadingModerateViewportHeuristicsOnlyForDevToolsEmulation",`only_for_devtools_emulation`? (ditto for the following)
```
third_party/blink/common/features.cc:2113: The 5-argument BASE_FEATURE_PARAM macro with a string literal name is discouraged. Use the 4-argument version instead.
```
fails then, and 4-arg version cant use snake case names :/
Related details
- Takashi Nakayama
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 2 comments![]( "Open in Gerrit")
// Copyright 2022 The Chromium AuthorsThis file seems unrelated to this patch and reformat changes only.
Can we exclude this kind of things from the patch unless you have another essential change for the same file?
#if BUILDFLAG(IS_ANDROID)Maybe Android and others are swapped mistakenly here?
Related details
- Helmut Januschka
- Hiroki Nakagawa
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |