New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 669008 link

Starred by 3 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Navigation should not have transition flags of PAGE_TRANSITION_FORWARD_BACK | PAGE_TRANSITION_RELOAD

Project Member Reported by clamy@chromium.org, Nov 28 2016

Issue description

https://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?
 

Comment 1 by creis@chromium.org, Nov 28 2016

Cc: toyoshim@chromium.org groby@chromium.org
That sounds plausible to me on the surface, though I haven't looked to see what that implies for the code.

It does sounds like PAGE_TRANSITION_RELOAD is used for session restore and undo tab close, according to the comment below.  Would it happen if you middle clicked the back button, ending up as both FORWARD_BACK and RELOAD?  I wonder if there's a downside to changing that.

  // The user "reloaded" the page, either by hitting the reload button or by
  // hitting enter in the address bar.  NOTE: This is distinct from the
  // concept of whether a particular load uses "reload semantics" (i.e.
  // bypasses cached data).  For this reason, lots of code needs to pass
  // around the concept of whether a load should be treated as a "reload"
  // separately from their tracking of this transition type, which is mainly
  // used for proper scoring for consumers who care about how frequently a
  // user typed/visited a particular URL.
  //
  // SessionRestore and undo tab close use this transition type too.
  PAGE_TRANSITION_RELOAD = 8,

toyoshim@: What are your thoughts on this?  I know you've dealt with reload a lot.

(Also CC'ing groby@ from issue 653051.)
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.
Labels: -OS-Android Reload OS-All
> 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?
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