Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2174293002:
NonValidatingReload: Monitor reload operations in NavigationControllerImpl (Closed)
|
Created:
4 years, 5 months ago by Takashi Toyoshima Modified:
4 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, loading-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNonValidatingReload: Monitor reload operations in NavigationControllerImpl
This patch introduces monitoring reload operations.
This is for reporting metrics on subsequent reload
operations as it is in this patch, and will be used
to manage adaptive reload behaviors later.
BUG=612701
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/a63c2a63ec73f968a05413c49f1e38dc2c83cd0c
Cr-Commit-Position: refs/heads/master@{#421783}
Patch Set 1 #Patch Set 2 : revert ui change #Patch Set 3 : histograms #Patch Set 4 : pretty_print.py #
Total comments: 2
Patch Set 5 : not for review yet #Patch Set 6 : use NavigationEntryImpl to keep ReloadType #
Total comments: 20
Patch Set 7 : plumb ClientRedirectPolicy #Patch Set 8 : address other comments in #19 #
Total comments: 3
Patch Set 9 : fix with debug code #
Total comments: 2
Patch Set 10 : cleanup #
Total comments: 25
Patch Set 11 : review #45 #Patch Set 12 : with a browser test #Patch Set 13 : rebase #
Total comments: 3
Patch Set 14 : #59 #
Total comments: 1
Patch Set 15 : simplified #Patch Set 16 : [rebase] #
Messages
Total messages: 80 (39 generated)
Description was changed from ========== NoValidatingReload: Add NavigationTracker to NavigationControllerImpl This patch introduces NavigationTracker. This is used to report metrics on subsequent reload operations in this patch, and will be used to manage adaptive reload behaviors in the NavigationControllerImpl. BUG=612701 ========== to ========== NoValidatingReload: Add NavigationTracker to NavigationControllerImpl This patch introduces NavigationTracker. This is used to report metrics on subsequent reload operations in this patch, and will be used to manage adaptive reload behaviors in the NavigationControllerImpl. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== NoValidatingReload: Add NavigationTracker to NavigationControllerImpl This patch introduces NavigationTracker. This is used to report metrics on subsequent reload operations in this patch, and will be used to manage adaptive reload behaviors in the NavigationControllerImpl. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== NonValidatingReload: Add NavigationTracker to NavigationControllerImpl This patch introduces NavigationTracker. This is used to report metrics on subsequent reload operations in this patch, and will be used to manage adaptive reload behaviors in the NavigationControllerImpl. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
toyoshim@chromium.org changed reviewers: + creis@chromium.org
Description was changed from ========== NonValidatingReload: Add NavigationTracker to NavigationControllerImpl This patch introduces NavigationTracker. This is used to report metrics on subsequent reload operations in this patch, and will be used to manage adaptive reload behaviors in the NavigationControllerImpl. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== NonValidatingReload: Add NavigationTracker to NavigationControllerImpl This patch introduces NavigationTracker. This is used to report metrics on subsequent reload operations in this patch, and will be used to manage adaptive reload behaviors inside NavigationControllerImpl later. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Can you take a look? I'm looking field trial results, and see visible number change on reload counts. But it's hard to know what this means without this kind of information. Also, this can be implemented inside NavigationControllerImpl, but I prefer to implement related logic in a separate class so that I can manage them easily. Also the reason why I do not implement this as a kind of common delegate is that this class will work closely with NavigationControllerImpl to manage adaptive reload behavior, e.g. subsequent reload in a short time would trigger bypassing reload automatically, etc.
Maybe we should chat about the design here and what you need. I don't think we'll want to add a class like this given the many ways the state could get out of sync, but maybe we can come up with an approach that works for your goals.
I'm almost busy in the next two days, but can we have a VC chat if we can find a time-slot? Here are documents explainingg backgrounds. About metrics: https://docs.google.com/document/d/1PBtpwN-lCcXWoYh7x8WSXOpVecH70_bcVBK9j7fet... About new reload UX: https://docs.google.com/document/d/1vwx8WiUASKyC2I-j2smNhaJaQQhcWREh7PC3HiIAQ...
On 2016/07/27 08:48:45, toyoshim wrote: > I'm almost busy in the next two days, but can we have a VC chat if we can find a > time-slot? Early next week could work if you want to pick a time. I generally need to leave by 5 pm PDT, but I could possibly set aside some time at home if needed. Otherwise we can try to make progress here if the scheduling proves tricky. > Here are documents explainingg backgrounds. > About metrics: > https://docs.google.com/document/d/1PBtpwN-lCcXWoYh7x8WSXOpVecH70_bcVBK9j7fet... > About new reload UX: > https://docs.google.com/document/d/1vwx8WiUASKyC2I-j2smNhaJaQQhcWREh7PC3HiIAQ... Thanks-- I'm familiar with the general project but not what problem this CL is trying to address. I left a comment about one of the races I'm concerned about below. Also, "NavigationTracker" is a bit too broad of a term for most developers to understand, especially alongside NavigationController and Navigator. :) Putting this logic either inside NavigationController or a reload-specific WebContentsObserver (which watches DidStartNavigationToPendingEntry and DidFinishNavigation) might be a cleaner approach. https://codereview.chromium.org/2174293002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_tracker.cc (right): https://codereview.chromium.org/2174293002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_tracker.cc:38: if (last_reload_type_ == NavigationController::RELOAD_MAIN_RESOURCE) { This is racy. The user could click reload just before an unrelated DidCommitProvisionalLoad IPC arrives from the renderer. We'll misclassify the unrelated commit as a reload. Can we track the reload type on the NavigationHandle instead, so that we don't need to keep it as state in a new class?
Sorry for late response. Now I finished a course that made me busy. Let me discuss here. > but not what problem this CL is trying to address. We have two goals with this CL. 1) Understand how often users would kick reload subsequently on the same page. If a user kick another reload just after the first reload, it can be a sign of broken pages, or bad loading experience. 2) Provide a shift-reload feature to mobiles. We discussed with UX team, and we agreed that we do not want to have a tricky UI for this, but just kicking the shift-reload automatically when Chrome detects users trying to fix broken contents. We believe that this metric could be a sign to trigger the shift-reload. To realize 2, NavigationControllerImpl::Reload* methods will need to call a decision making method of NavigationTracker(though the class and method name is TBD). If I implement this class as a WebContentsObserver, can I call the instance from NavigationControllerImpl? I feel calling a method that isn't one of WebContentsObserver's virtual methods from the controller is curious in the viewpoint of design. For naming: I do not have a good sense of naming in English, but how about ReloadNavigationHelper? If this inherits the WebContentsObsrever, it would be ReloadNavigationObserver or something. I will address the race issue and other possible problems, once we decide a direction how to implement it.
On 2016/08/02 10:08:45, toyoshim wrote: > Sorry for late response. Now I finished a course that made me busy. Let me > discuss here. > > > but not what problem this CL is trying to address. > > We have two goals with this CL. > > 1) Understand how often users would kick reload subsequently on the same page. > > If a user kick another reload just after the first reload, it can be a sign of > broken pages, or bad loading experience. > > > 2) Provide a shift-reload feature to mobiles. > > We discussed with UX team, and we agreed that we do not want to have a tricky UI > for this, but just kicking the shift-reload automatically when Chrome detects > users trying to fix broken contents. We believe that this metric could be a sign > to trigger the shift-reload. Does "detecting" mean just seeing a second reload in a row? Or is there a more complex heuristic? Hopefully we'll have something simple, and NavigationController itself might be able to track it as a small bit of state. > > To realize 2, NavigationControllerImpl::Reload* methods will need to call a > decision making method of NavigationTracker(though the class and method name is > TBD). If I implement this class as a WebContentsObserver, can I call the > instance from NavigationControllerImpl? I feel calling a method that isn't one > of WebContentsObserver's virtual methods from the controller is curious in the > viewpoint of design. Agreed that an observer is not the right pattern if NavigationController is going to be calling into it to decide how the next reload should behave. In that case, this does sound more like something NavigationController itself should track. > > For naming: I do not have a good sense of naming in English, but how about > ReloadNavigationHelper? If this inherits the WebContentsObsrever, it would be > ReloadNavigationObserver or something. If we really need a separate class (or inner class?), ReloadTracker might be more accurate, depending on what else it will be doing. It would be nice not to need a separate class, though, if we can keep the extra logic relatively small. > > I will address the race issue and other possible problems, once we decide a > direction how to implement it.
> Does "detecting" mean just seeing a second reload in a row? Yes, that's my current plan thoughwe may consider having more complicated heuristics if the simple one does not work. OK. I'll update my change not t o have a separate class as discussed. I may have a separate class in the future, but it should be only when we need complicated one.
Description was changed from ========== NonValidatingReload: Add NavigationTracker to NavigationControllerImpl This patch introduces NavigationTracker. This is used to report metrics on subsequent reload operations in this patch, and will be used to manage adaptive reload behaviors inside NavigationControllerImpl later. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== NonValidatingReload: Monitor reload operations in NavigationControllerImpl This patch introduces monitoring reload operations. This is for reporting metrics on subsequent reload operations as it is in this patch, and will be used to manage adaptive reload behaviors later. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
didn't finish yet, but let me share what I tried. https://codereview.chromium.org/2174293002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_tracker.cc (right): https://codereview.chromium.org/2174293002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_tracker.cc:38: if (last_reload_type_ == NavigationController::RELOAD_MAIN_RESOURCE) { I roughly understood how to make the ReloadType a member of NavigationHandle, but I could not figure out how to get the NavigationHandle instance here. In NavigationControllerImpl, we use it in RendererDidNavigateToNewPage(), but there we call RenderFrameHostImpl::navigation_handle(). I think this call also could have the same race issue, and another big problem is that we can not access to RFH from here. Probably, I need to store the reload type to an object that can be accessed through the LoadCommittedDetails object. One candidate seems to be ui::PageTransition.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
How about using NavigationEntryImpl to keep ReloadType? Even in this case, we still need another variable to keep *previous* ReloadType, but it can be updated in NotifyNavigationEntryCommitted().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks-- I think we're headed in a better direction. It tends to be tricky to add things to NavigationEntry, though, since much of that class is persisted between sessions. I'm also wondering if this is broken for location.reload(), which probably doesn't go through ReloadInternal. I'm wondering if we can have a parameter with DidCommitProvisionalLoad that gives us what we need. Or do we not track it at a fine enough granularity in the renderer? https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:349: entry->set_reload_type(reload_type); What happens if the renderer initiates a reload (e.g., location.reload() in Javascript)? That won't go through here, will it? Maybe there's a parameter we can check at commit time instead of storing the bit on NavigationEntry? https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.h:451: // Used to track if the last navigation type is for reload operations. We should be specific about whether this refers to the last commit. (It could be the last started navigation, which would have a different meaning and might run into problems with renderer-initiated navigations.) Might even be clearer to call it last_commit_was_reload_. https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.h:458: base::Time last_navigation_time_; I'm wondering if this is already tracked somewhere. Could we use GetTimestamp from GetLastCommittedEntry? https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:22: #include "content/public/browser/navigation_controller.h" We've tried hard to avoid having a circular dependency between NavigationController and NavigationEntry. If we continue down this path, maybe we can put ReloadType into a file of its own. https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:414: // For all new fields, update |Clone| and possibly |ResetForCommit|. Please see this warning about adding things to NavigationEntry. In general, we try to avoid it. :) It's a little easier if the data can be cleared at commit time and doesn't need to be persisted across sessions, but we need to think carefully about it either way. https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53546: + The buckets indicate whether or not the " stranded" Chrome install Unintentional?
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I upload a new Patch Set 7 that passes ClientRedirectPolicy in the CommitLoad delegate chain to solve the issue we need to distinguish JavaScript initiated reload. Since it wasn't small change, I didn't touch other commented places in this Patch Set. I will address them in the next Patch Set. https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:349: entry->set_reload_type(reload_type); Right. Javascript initiated reload won't be captured here. It directly calls FrameLoader::load() with FrameLoadTypeReload from ScheduleReload::fire(). But reloads I'm interested here are ones come from user interfaces. It's fine that JavaScript initiated reload is ignored. Probably, I should update comments to clarify that. If we want to capture the JavaScript initiated reloads too, probably we can track it in RendererDidNavigate() as one of NAVIGATION_TYPE_EXISTING_PAGE, and entry_index and url being not changed? Another problem I noticed here is stored reload type isn't updated on JavaScript initiated reload. That means if I follow steps below, it mistakenly report ReloadToRealodDuration metric. 1. Navigate to a page 2. Reload user action 3. location.reload() => stored information isn't updated, and entry still has the previous reload_type. this results in reporting ReloadToReloadDuration metric. To fix this issue, I should identify JavaScript initiated reloads and user actions initiated reloads. ClientRedirectPolicy in Blink is the one I can use for that. But it isn't available here. Shall we plumb it from FrameLoader on CommitLoad timing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1961: transition & ui::PAGE_TRANSITION_FORWARD_BACK; This line broke Windows build. https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.h:451: // Used to track if the last navigation type is for reload operations. This is confusing, but when NotifyNavigationEntryCommitted() is called, the last committed entry means one for the current completing navigation. But what we keep here is the previously finished navigation. So, previous_commit_was_reload_ would be better? https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.h:458: base::Time last_navigation_time_; At this point, the entry is already overwritten with one of completing navigation, and what we want to track with this variable is one for previous navigation. So, we still need to have this, I think. But, using GetTimestamp looks better than base::Time::Now(). https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:22: #include "content/public/browser/navigation_controller.h" Oh, is this why we have another RestoreType enum in NavigationEntry? Let me make a separate change to solve this and RestoreType. Probably, I'd make public/browser/navigation_types.h to have both two types. https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:414: // For all new fields, update |Clone| and possibly |ResetForCommit|. Oh, I didn't notice this WARNING. So, at the next change, I just add a copy code in |Clone| tentatively. If it's possible to reset reload_type_ at the commit time, I'll revisit this again. But it will be after discussing ClientRedirectPolicy plumbing. https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53546: + The buckets indicate whether or not the " stranded" Chrome install This is not related to my CL, but presubmit check could not pass without this format fix unfortunately. https://codereview.chromium.org/2174293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2174293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:640: #if defined(OS_ANDROID) Maybe this copy should be done?
PTAL. Now this change contains many files. If you agreed ClientRedirectPolicy change, I will split that part from this CL so that we can focus main logic.
Thanks for pushing forward, but I still have some concerns about this approach. Unfortunately, I had my first stability sheriff shift today, so I didn't have time to go through this in depth. Hopefully I can set aside some time tomorrow to better understand it and propose a way forward. https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:349: entry->set_reload_type(reload_type); On 2016/08/09 07:35:26, toyoshim wrote: > Right. Javascript initiated reload won't be captured here. It directly calls > FrameLoader::load() with FrameLoadTypeReload from ScheduleReload::fire(). > > But reloads I'm interested here are ones come from user interfaces. It's fine > that JavaScript initiated reload is ignored. Probably, I should update comments > to clarify that. > > If we want to capture the JavaScript initiated reloads too, probably we can > track it in RendererDidNavigate() as one of NAVIGATION_TYPE_EXISTING_PAGE, and > entry_index and url being not changed? > > Another problem I noticed here is stored reload type isn't updated on JavaScript > initiated reload. That means if I follow steps below, it mistakenly report > ReloadToRealodDuration metric. > > 1. Navigate to a page > 2. Reload user action > 3. location.reload() => stored information isn't updated, and entry still has > the previous reload_type. this results in reporting ReloadToReloadDuration > metric. > > To fix this issue, I should identify JavaScript initiated reloads and user > actions initiated reloads. ClientRedirectPolicy in Blink is the one I can use > for that. But it isn't available here. Shall we plumb it from FrameLoader on > CommitLoad timing? Isn't this a problem for any renderer-initiated navigation, not just location.reload? I'm still concerned about the overall strategy. https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:22: #include "content/public/browser/navigation_controller.h" On 2016/08/09 12:06:17, toyoshim wrote: > Oh, is this why we have another RestoreType enum in NavigationEntry? > Let me make a separate change to solve this and RestoreType. > Probably, I'd make public/browser/navigation_types.h to have both two types. I think this may be getting ahead of ourselves. I'm not convinced yet that we want to add this to NavigationEntry, given the problems I mentioned below. https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:414: // For all new fields, update |Clone| and possibly |ResetForCommit|. On 2016/08/09 12:06:17, toyoshim wrote: > Oh, I didn't notice this WARNING. > So, at the next change, I just add a copy code in |Clone| tentatively. > If it's possible to reset reload_type_ at the commit time, I'll revisit this > again. But it will be after discussing ClientRedirectPolicy plumbing. I don't think it's safe to add something that isn't persisted and isn't cleared at commit time. https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53546: + The buckets indicate whether or not the " stranded" Chrome install On 2016/08/09 12:06:17, toyoshim wrote: > This is not related to my CL, but presubmit check could not pass without this > format fix unfortunately. Huh. Seems like a strange requirement from the presubmit check, since it's the wrong thing to do to the description (i.e., it inserts a typo). I won't object, but it might be worth looking into whether there's a bug in the presubmit. https://codereview.chromium.org/2174293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2174293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:640: #if defined(OS_ANDROID) On 2016/08/09 12:06:18, toyoshim wrote: > Maybe this copy should be done? Good catch! Looks like qinmin@ missed that in https://codereview.chromium.org/1243253004/. Can you pull this part out into a separate CL that we can land first?
https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:349: entry->set_reload_type(reload_type); If we assume this reload type is valid only when the page transition type indicates reload, it does not matter. My proposed way, using ClientRedirectPolicy, just works becase as far as I know, all JavaScript initiated reload-ish navigation are classified to "clientRedirect". https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53546: + The buckets indicate whether or not the " stranded" Chrome install Precisely said, pretty_print.py could not parse if there is no space after '&NAME;' around the modified code. OK, I'll investigate this in parallel. https://codereview.chromium.org/2174293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2174293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:640: #if defined(OS_ANDROID) OK, I made this https://codereview.chromium.org/2230793003/
https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53546: + The buckets indicate whether or not the " stranded" Chrome install I checked if this is sill needed on trunk, and it turned out that we do not need this workaround I added at Patch Set 4 any more. Let me revert this in the newer patch set.
Thanks. Still working through this. I'm not stability sheriff tomorrow, so I should be able to get a proposal to you then.
Ok, let's take a step back and look at the requirements, and I think that will lead us to a simpler solution. I have a few design-level questions that need answering first, though. 1) Which exact time interval are you trying to measure? Suppose two reloads happen consecutively via the UI, with the following events: UserAction1, Commit1, UserAction2, Commit2. The current CL is measuring commit-to-commit, but I think that's probably misleading data. Suppose each reload takes 10 seconds to commit, due to a slow server. From your motivating goal (i.e., determining whether the user is reloading again to fix a broken page), it seems like we actually care about the time from Commit1 to the time the user tries to start the next reload (UserAction2). It doesn't matter if the second reload is slow, so we don't care about Commit2. 2) Which sorts of events should clear this reload tracking state? If the user types in a different URL between one reload and the next, that obviously should clear the state. Same for link clicks. However, there are many events that probably shouldn't clear the state. AUTO_SUBFRAME commits (when a subframe is added to the page after a reload) almost certainly shouldn't. NEW_SUBFRAME commits (when the user clicks a link in a subframe and creates a new NavigationEntry) probably should reset the state. What about in-page navigations? A replaceState probably shouldn't. (I think Twitter generates a bunch of these when you load the page.) A pushState is less clear. Maybe all browser-initiated navigations clear the state, and renderer-initiated navigations only clear it if they have either a user gesture (NavigationGestureUser) or a different document sequence number on the root FrameNavigationEntry? I'm curious about your answers for those, but if my suggested answers seem reasonable, here's a way forward: To track the state, store the ReloadType on the pending entry as you're doing now, and clear it at commit time. (I wasn't thrilled with adding it to NavigationEntry, but I'm ok with it if we clear it in ResetForCommit.) At commit time (probably in RendererDidNavigate), if the pending entry matches the committing navigation, we store the type and timestamp in members of NavigationController, similar to what you're doing now. If the commit doesn't match and it's either browser-initiated or renderer-initiated with a user gesture (or if it's a main frame commit with a different document sequence number), reset the state. (I think we can check whether the commit and pending entry match using pending_entry_->GetUniqueID() == params.nav_entry_id.) Logging the data should probably happen in ReloadInternal itself, if we want to measure the time interval I describe above. If this sounds reasonable, we will need your https://codereview.chromium.org/2225343002/ to be able to put ReloadType on NavigationEntry. I don't think we'll need to plumb the ClientRedirectPolicy, since a location.reload() without a user gesture doesn't really seem like a reason to clear the state. Thoughts? (Note that I'll be on vacation next week, but we're far enough along now that I can probably have Nasko help with the review while I'm gone.)
Hi, I have been busy for fixing a high priority issue while you are on vacation. But now I'm finishing it. Now you are back too, let me resume this CL. I'll reply your comments at #31 tomorrow.
Thank you for a clear guideline. 1) Sounds fine. But, we may consider to measure time from UserAction1 to UserAction2. This is because user may try another reload even before the commit. There may be room for discussion if we should count this or or. But eventually, users will invent a "double-reload" action to intend a hard reload. So, probably, it's better we start measuring it as UserAction1 to UserAction2. I assume this change does not introduce extra complexity. 2) Thanks. This explains what I didn't understand well. I agreed your suggestion. For location.reload(), I do not want to duration that is triggered by location.reload(), but IIUC, with your suggestion, we never count such cases. I updated https://codereview.chromium.org/2225343002/ today, and tomorrow I will make new patch set for this that is based on your idea at #31.
On 2016/09/07 12:27:54, toyoshim wrote: > Thank you for a clear guideline. > > 1) Sounds fine. But, we may consider to measure time from UserAction1 to > UserAction2. > This is because user may try another reload even before the commit. There may be > room for discussion if we should count this or or. But eventually, users will > invent a "double-reload" action to intend a hard reload. So, probably, it's > better we start measuring it as UserAction1 to UserAction2. I assume this change > does not introduce extra complexity. Hmm, I'm a bit skeptical about this. If the user reloads and reloads again before commit, that seems more like a signal that the server was slow to respond than that the page was broken and needs a different ReloadType (since the user didn't even see the page after the first reload attempt). I can't think of a reason to expect that a double-reload would trigger a hard reload. Even if we measured that interval, it would be hard to tell the difference between users doing it due to a slow server (where it's an effective strategy to recover) and users wishfully thinking that it might trigger a hard reload (which it doesn't). I'm still inclined to think that the Commit1-to-UseAction2 might be the best thing to measure. > > 2) Thanks. This explains what I didn't understand well. I agreed your > suggestion. > > For location.reload(), I do not want to duration that is triggered by > location.reload(), but IIUC, with your suggestion, we never count such cases. > Right. > I updated https://codereview.chromium.org/2225343002/ today, and tomorrow I will > make new patch set for this that is based on your idea at #31. Thanks!
> I'm still inclined to think that the Commit1-to-UseAction2 might be the best > thing to measure. OK, I agreed that the real reason for slow "time to commit" is just network is slow. My concern was user may want to invent double-click like new action, "double-pull", when they get to be familiar with this new hard reload sequence, "reload, wait a little, then reload". But this shouldn't be important until many people want such a new action. So, I'm fine with "commit 1 to action 2" approach. I couldn't finish the new patch set today, but will upload soon. Thank you for kind review and many ideas!
I upload a patch set that is based on your idea. But, I could not find a right condition to capture commits of reload for twitter.com. I will continue to investigate tomorrow, but you may know what happens. https://codereview.chromium.org/2174293002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:340: LOG(ERROR) << "Reload: " << delta; LOG(ERROR) at line 340 and 344 are for debug. Will remove once the problem is solved. https://codereview.chromium.org/2174293002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:824: // FIXME: Remove debug code below. Line 824-845 is also for debug. Following code works in most cases, but does not for twitter.com. What I saw with following LOG(ERROR) code against https://twitter.com/ (without login) was: @@@ 3, 0, [EXISTING_PAGE], [GestureUnknown] @@@ 5, 3, [EXISTING_PAGE], [GestureAuto ] @@@ *, 0, [AUTO_SUBFRAME], [GestureUnknown] @@@ *, 0, [AUTO_SUBFRAME], [GestureUnknown] @@@ *, 0, [AUTO_SUBFRAME], [GestureUnknown] @@@ *, 0, [AUTO_SUBFRAME], [GestureUnknown] @@@ *, 0, [AUTO_SUBFRAME], [GestureUnknown] @@@ *, 0, [EXISTING_PAGE], [GestureUnknown] RendererDidNavigate() was called with params.nav_entry_id == 0 for an unknown reason, then another call happens with expected params.nav_entry_id == 3. But at this second call place, pending_entry was updated with another unique ID by someone. As a result, this code fails to capture the commit of a reload. For other sites, the first call has the same nav_entry_id with pending entry's unique ID. Is this an expected navigation sequence? Or hitting an existing minor bug?
I checked what twitter.com did, and noticed that if XHR was requested inside onunload event handler, the reload received RendererDidNavigate() with nav_entry_id == 0. If this is an expected, I need to handle it correctly. But if it's a bug, I'd move this change forward, and discuss the bug separately. Here is a simple example to see this behavior; http://yuri.twintail.org/chrome/reload/unload.html
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> if XHR was requested Sorry, this isn't a right condition. I can see the same behavior if DevTools is open, but it isn't related to the unload event handler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/13 10:22:14, toyoshim wrote: > > if XHR was requested > Sorry, this isn't a right condition. I can see the same behavior if DevTools is > open, but it isn't related to the unload event handler. I'll have to patch in and investigate what you're seeing with Twitter. Should be able to do that tomorrow or Friday (after perf).
I dived this issue, and took notes at crbug.com/646730.
Now I get a simple code to reproduce it.
window.addEventListener("beforeunload", function (event) {
history.replaceState({}, "title", location.href);
);
With this JavaScript code, DidCommitProvisionalLoad() for the replaceState() is
called before DidStartProvisionalLoad() and DidCommitProvisionalLoad() are
called for the original reload.
The DidCommitProvisionalLoad() for the replaceState() is handled as
browser-initiated navigation since it does not have a preceding
DidStartProvisionalLoad() call, and it updates the pending_entry. Thus,
NavigationController can not recognize succeeding callbacks were for the first
original reload.
Thanks, this is looking much better. We'll want to include some tests that check that this is working after consecutive reloads (including with non-user-initiated navigations between) and reset if other user-initiated navigations take place between. base::HistogramTester should make that pretty easy to check from a browser test (e.g., NavigationControllerImplBrowserTest). Not sure how to fix the Twitter bug (https://crbug.com/646730) yet, but I posted some thoughts there. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:333: // Check if previous navigation was reload to track reload operations. nit: was a reload, to track consecutive reload operations. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:336: time_smoother_.GetSmoothedTime(get_timestamp_callback_.Run()); nit: Wrong indent. git cl format can take care of that. :) https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:337: base::TimeDelta delta = now - last_committed_reload_time_; Maybe include a sanity check that last_committed_reload_time_ is valid and earlier than now, and skip sending the UMA if not? https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:347: entry->set_reload_type(reload_type); Side note: While debugging I discovered (much to my surprise) that we don't get here for reloads if DevTools is open. It appears ReloadInternal in browser_commands.cc is to blame, where there's code to intercept reloads and send them to DevTools's ReloadInspectedWebContents. Hopefully it's ok to miss out on data in that case. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:824: if (pending_entry_ && pending_entry_->GetUniqueID() == params.nav_entry_id) { I would suggest using PendingEntryMatchesHandle instead. (Sadly, that alone won't fix https://crbug.com/646730.) This also needs to check whether it's a reload (either using pending_entry_->reload_type(), params.transition, or both). Otherwise we'll set the reload type/time fields for other browser-initiated navigations with pending entries, like omnibox navigations and back/forward. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:828: last_committed_reload_time_ = pending_entry_->GetTimestamp(); This won't be the right timestamp, because it doesn't get assigned until later in this method, on line 905. I can see that you might not want to move this whole block down there, since that's after the pending entry has been cleared/committed. Maybe we can generate our own timestamp here (despite being a bit redundant)? https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:832: details->type != NAVIGATION_TYPE_AUTO_SUBFRAME)) { This doesn't look like the right condition. We want to reset the state if the navigation is browser-initiated (!pending_entry_->is_renderer_initiated()) or if it has a user gesture, regardless of what details->type is. Again, we'll want to be sure to use PendingEntryMatchesHandle. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:840: last_committed_reload_type_ = ReloadType::NONE; Also clear the timestamp, to avoid having old data there. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.h:454: // navigation. nit: Rephrase to: Used for tracking consecutive reload requests. If the last user-initiated navigation (either browser-initiated or renderer-initiated with a user gesture) was a reload, these hold the ReloadType and timestamp. Otherwise these are ReloadType::NONE and a null timestamp, respectively. (Since they're closely related and can be covered by one comment, you can also remove the blank line between them.) https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:644: copy->extra_data_ = extra_data_; nit: Add "// ResetForCommit: reload_type_" before this line. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:315: void set_reload_type(ReloadType type) { Please document these. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:514: // Set when a reload is requested to check navigation type at commit time. Please mention that this is ReloadType::NONE for all non-reloads, and include the phrase "Reset at commit and not persisted."
Thank you for reviewing this. I'm working on updating this patch set based on your review comments, but it's slow because of perf-sheriff duty and Japanese two holidays in this week.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:333: // Check if previous navigation was reload to track reload operations. On 2016/09/19 22:23:51, Charlie Reis (slow) wrote: > nit: was a reload, to track consecutive reload operations. Done. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:336: time_smoother_.GetSmoothedTime(get_timestamp_callback_.Run()); Oops. Sorry, I forgot to run it for this patch set. Done. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:337: base::TimeDelta delta = now - last_committed_reload_time_; On 2016/09/19 22:23:51, Charlie Reis (slow) wrote: > Maybe include a sanity check that last_committed_reload_time_ is valid and > earlier than now, and skip sending the UMA if not? Done. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:347: entry->set_reload_type(reload_type); Yeah, I noticed this problem too. I think it's fine to ignore such cases since it means users are doing something special that beyond end general users use-cases, and that would be noise for my purpose. I imagine a case there developers take many reload operations to debug their sites. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:824: if (pending_entry_ && pending_entry_->GetUniqueID() == params.nav_entry_id) { Done. Is my fix correct? I use rfh->navigation_handle() to obtain the Handle for PendingEntryMatchesHandle. For reload type check, I prefer to check only reload_type() but do you have any concern about this? If we want to check ui::PageTransition, the check could be a little complicated since it could have both PAGE_TRANSITION_FORWARD_BACK and PAGE_TRANSITION_RELOAD when history navigation happens against a reloaded page. Also, I assumed reload_type() always returns NONE for other browser-initiated navigations. But now it sounds better to check it here and reset last_committed_reload_time_ inside 'else if (){ ... }'. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:828: last_committed_reload_time_ = pending_entry_->GetTimestamp(); On 2016/09/19 22:23:51, Charlie Reis (slow) wrote: > This won't be the right timestamp, because it doesn't get assigned until later > in this method, on line 905. > > I can see that you might not want to move this whole block down there, since > that's after the pending entry has been cleared/committed. Maybe we can > generate our own timestamp here (despite being a bit redundant)? Done. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:832: details->type != NAVIGATION_TYPE_AUTO_SUBFRAME)) { On 2016/09/19 22:23:51, Charlie Reis (slow) wrote: > This doesn't look like the right condition. > > We want to reset the state if the navigation is browser-initiated > (!pending_entry_->is_renderer_initiated()) or if it has a user gesture, > regardless of what details->type is. Again, we'll want to be sure to use > PendingEntryMatchesHandle. Done. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:840: last_committed_reload_type_ = ReloadType::NONE; On 2016/09/19 22:23:51, Charlie Reis (slow) wrote: > Also clear the timestamp, to avoid having old data there. Done. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.h:454: // navigation. On 2016/09/19 22:23:51, Charlie Reis (slow) wrote: > nit: Rephrase to: > Used for tracking consecutive reload requests. If the last user-initiated > navigation (either browser-initiated or renderer-initiated with a user gesture) > was a reload, these hold the ReloadType and timestamp. Otherwise these are > ReloadType::NONE and a null timestamp, respectively. > > (Since they're closely related and can be covered by one comment, you can also > remove the blank line between them.) Done. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:644: copy->extra_data_ = extra_data_; On 2016/09/19 22:23:51, Charlie Reis (slow) wrote: > nit: Add "// ResetForCommit: reload_type_" before this line. Done. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:315: void set_reload_type(ReloadType type) { On 2016/09/19 22:23:52, Charlie Reis (slow) wrote: > Please document these. Done. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:514: // Set when a reload is requested to check navigation type at commit time. On 2016/09/19 22:23:52, Charlie Reis (slow) wrote: > Please mention that this is ReloadType::NONE for all non-reloads, and include > the phrase "Reset at commit and not persisted." Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Great! I think this is pretty much ready, apart from needing a test. I left a comment with some ideas last time, but it might have fallen between the cracks: On 2016/09/19 22:23:51, Charlie Reis (slow) wrote: > We'll want to include some tests that check that this is working after > consecutive reloads (including with non-user-initiated navigations between) and > reset if other user-initiated navigations take place between. > base::HistogramTester should make that pretty easy to check from a browser test > (e.g., NavigationControllerImplBrowserTest). Apart from that, I'm ok with landing this before or after the fix for https://crbug.com/646730, depending on whether you're concerned about replaceState cases affecting your data. https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:824: if (pending_entry_ && pending_entry_->GetUniqueID() == params.nav_entry_id) { On 2016/09/26 12:23:34, toyoshim wrote: > Done. Is my fix correct? I use rfh->navigation_handle() to obtain the Handle for > PendingEntryMatchesHandle. Yes, that looks right to me, thanks. > For reload type check, I prefer to check only reload_type() but do you have any > concern about this? If we want to check ui::PageTransition, the check could be a > little complicated since it could have both PAGE_TRANSITION_FORWARD_BACK and > PAGE_TRANSITION_RELOAD when history navigation happens against a reloaded page. ReloadType should be fine. (For reference, you should be able to use PageTransitionCoreTypeIs to separate PAGE_TRANSITION_RELOAD from PAGE_TRANSITION_FORWARD_BACK, but I don't see a need for it here.) > > Also, I assumed reload_type() always returns NONE for other browser-initiated > navigations. But now it sounds better to check it here and reset > last_committed_reload_time_ inside 'else if (){ ... }'.
Ah, I just forgot about adding tests. Let me add it in the next patch set. For ui::PageTransition, as far as I know, PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_RELOAD) returns true if history navigation happens to the page that was reloaded at the previous visit. This is because PAGE_TRANSITION_FORWARD_BACK isn't core type, and can be set aside PAGE_TRANSITION_RELOAD. I just noticed this problem in the code review for https://codereview.chromium.org/2091353002/diff/280001/chrome/browser/page_lo...
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Now the last patch set 13 contains a browser test for the reload tracking.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great, thanks for the test, and for bearing with me through all this! LGTM with one request for some extra test cases. https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6183: histogram.ExpectTotalCount(kReloadMainResourceToReloadMetricName, 2); Can you add cases after this showing that renderer-initiated navigations with no user gesture (e.g., pushState(foo) and location.href=simple_page_2.html) don't reset the count? I think you should be able to do that with ExecuteScript, though it's possible that might count as a user gesture. (If so, maybe a setTimeout would lose it.)
https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6183: histogram.ExpectTotalCount(kReloadMainResourceToReloadMetricName, 2); I tried to use setTimeout, but it still run with the user gesture flag. I called RFH->ExecuteJavaScriptForTests() directly, but if there is another simple way, I'd like to use it.
Thanks for the extra test case! LGTM with one simplification. https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6183: histogram.ExpectTotalCount(kReloadMainResourceToReloadMetricName, 2); On 2016/09/28 10:37:21, toyoshim wrote: > I tried to use setTimeout, but it still run with the user gesture flag. > I called RFH->ExecuteJavaScriptForTests() directly, but if there is another > simple way, I'd like to use it. ExecuteJavaScriptForTests seems right, thanks. https://codereview.chromium.org/2174293002/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2174293002/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6207: ->current_frame_host() Just call GetMainFrame() on the WebContents, rather than GetFrameTree()->root()->render_manager()->current_frame_host().
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
toyoshim@chromium.org changed reviewers: + haraken@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
toyoshim@chromium.org changed reviewers: + isherman@chromium.org - haraken@chromium.org
+Ilya for metrics.
metrics lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2174293002/#ps300001 (title: "[rebase]")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== NonValidatingReload: Monitor reload operations in NavigationControllerImpl This patch introduces monitoring reload operations. This is for reporting metrics on subsequent reload operations as it is in this patch, and will be used to manage adaptive reload behaviors later. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== NonValidatingReload: Monitor reload operations in NavigationControllerImpl This patch introduces monitoring reload operations. This is for reporting metrics on subsequent reload operations as it is in this patch, and will be used to manage adaptive reload behaviors later. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== NonValidatingReload: Monitor reload operations in NavigationControllerImpl This patch introduces monitoring reload operations. This is for reporting metrics on subsequent reload operations as it is in this patch, and will be used to manage adaptive reload behaviors later. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== NonValidatingReload: Monitor reload operations in NavigationControllerImpl This patch introduces monitoring reload operations. This is for reporting metrics on subsequent reload operations as it is in this patch, and will be used to manage adaptive reload behaviors later. BUG=612701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/a63c2a63ec73f968a05413c49f1e38dc2c83cd0c Cr-Commit-Position: refs/heads/master@{#421783} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/a63c2a63ec73f968a05413c49f1e38dc2c83cd0c Cr-Commit-Position: refs/heads/master@{#421783} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
