[soft navs] Add support for replaceState [chromium/src : main]
Scott Haseley (Gerrit)
Scott Haseley voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Annie Sullivan
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 7 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
7 comments
This is great! So much plumbing and test updates for a tiny implementation change >< but I love that this is literally returning the NavigationType value.
flag (SoftNavigationDetectionIncludeReplaceState).I'm fine with this, but I wonder if we should have a common "SNH_Experimental" flag for all the new features?
enum class SoftNavigationTypeForReporting {I'm fine with this, but wonder if we can/should re-use `V8NavigationType::Enum`?
Perhaps we need a redefinition for public/ exposure, anyway, but maybe that enum also needs to be added to the LINT check?
WebFrameLoadType load_type_ = WebFrameLoadType::kStandard;Nit: Would it be cleaner to just convert to V8NavigationType::Enum here?
Perhaps before AddUrl() call, even. This way the soft-nav parts only think in navigation API terms...
Especially with the direction of "url is all you need" for soft-navs, I think there is no value in being lazy about converting.
switch (NavigationApi::WebFrameLoadTypeToNavigationType(type)) {...This seems like another sign that we should convert eagerly?
navigate();`clickAndExpectSoftNavigation` expects a `url` param, but then blindly calls `navigate()`.
Consider passing `url` to `navigate()` callback so that the test can just pass that to push/replaceState and you don't have to duplicate strings and risk getting out of sync.
(and you can just ignore for traversals)
history.pushState({}, '', url);Super minor nit, but perhaps override navigate with a default value at the top, instead of doing this here.
```if (!navigate) navigate = () => history.pushState(...)```
Related details
- Annie Sullivan
- Johannes Henkel
- Scott Haseley
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. |
Annie Sullivan (Gerrit)
Annie Sullivan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Annie Sullivan
| Code-Review | +1 |
1 comment
Looks good to me % Michal's comments.
Also for the new UKM did you follow the process at go/ukm#adding-ukms ?
Related details
- Johannes Henkel
- Scott Haseley
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
Looks good to me % Michal's comments.
Also for the new UKM did you follow the process at go/ukm#adding-ukms ?
Thanks, meant to ask you earlier for that. First time!
Related details
- Johannes Henkel
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. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 7 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Code-Review | +1 |
7 comments
Nice cl!!!
"http://example.com:%d/soft_navigation_basics.html#text", port)));I think this is what you mentioned during the meeting and it's cool imho - basically a result of not doing an additional pushstate when the back button is pressed, but rather it reflects what url the browser puts.
PAGE_LOAD_HISTOGRAM("PageLoad.SoftNavigation.StartTime",Do you want to put a histogram, in the spirit of this one as well? I don't know what it takes exactly but figure it may be cool to get a grand summary.
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.What you write here feels like a subset of go/protodosdonts, maybe scan through that and see if there's any other consideration that may apply. I was thinking about go/protodosdonts#unspecified-enum in particular. Or maybe not, but I think it's healthy to think before committing. :-)
timing->navigation_type = mojom::SoftNavigationType::kPush;This is where the unspecified value would be set instead, if we were to follow the advice from go/protodosdonts#unspecified-enum
case V8NavigationType::Enum::kReplace:
return SoftNavigationTypeForReporting::kReplace;Maybe below this case, put
NOTREACHED();
?
type: 'push',I think making these two field names slightly more elaborate may make it friendlier to read for someone who isn't completely familiar with soft navs etc. ?
type -> expectedNavigationType
url -> expectedURL
Related details
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. |