Navigation should not have transition flags of PAGE_TRANSITION_FORWARD_BACK | PAGE_TRANSITION_RELOAD |
||
Issue descriptionhttps://codereview.chromium.org/2533473003/ surfaced that it's possible for a navigation to end up with both PAGE_TRANSITION_FORWARD_BACK and PAGE_TRANSITION_RELOAD in their transition type. This seems to be the cause behind issue 653051. I think this is a bug, and navigations should either be a back/forward or a reload, but not both. creis, nasko: wdyt?
,
Nov 29 2016
I saw this problem before to handle navigation type specific metrics (https://codereview.chromium.org/2091353002) In ui/base/page_transition_types.h, PAGE_TRANSITION_RELOAD is declared as one of core type, but PAGE_TRANSITION_FORWARD_BACK is one of qualifier. So, I thought it was by design and history navigation was assumed as a reload variant here. But actually, this also seems very confusing because 1. Probably many people overlook the fact these two bits can be set together. 2. It isn't trivial what PAGE_TRANSITION_RELOAD|PAGE_TRANSITION_FORWARD_BACK means For instance, here are two cases that may set both flags. case 1) visit page A, reload the page A, and navigate to page B, then history back to the page A case 2) visit page A, navigate to page B, and history back the the page A, then reload the page A But, IIRC, both flags are set only for case 1), and PAGE_TRANSITION_RELOAD is just set for case 2). One idea to avoid human errors around this is to revise comments, and add new methods PageTransitionIsReload(), PageTransitionIsForwardBack() to the file. I think it's also reasonable to upgrade PAGE_TRANSITION_FORWARD_BACK as one of core types. I can not ensure, but I haven't seen any code that want to handle the case both flags are set.
,
Nov 29 2016
,
Nov 29 2016
> One idea to avoid human errors around this is to revise comments, and add new methods > PageTransitionIsReload(), PageTransitionIsForwardBack() to the file. In this case, probably PageTransitionIs*New*Reload() is better?
,
Dec 5 2016
Interesting. I would still think that this transition should not happen in the case of bug 653051: Hit reload on page A and hit back to page B before the reload of page A completes. I would expect the navigation to not be a reload in this case. Or to get 2 navigations: one reload followed by a non-reload. |
||
►
Sign in to add a comment |
||
Comment 1 by creis@chromium.org
, Nov 28 2016