Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(22)

Issue 2174293002: NonValidatingReload: Monitor reload operations in NavigationControllerImpl (Closed)

Created:
3 years, 4 months ago by Takashi Toyoshima
Modified:
3 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.

Description

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}

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] #

Unified diffs Side-by-side diffs Delta from patch set Stats (+194 lines, -3 lines) Patch
M content/browser/frame_host/navigation_controller_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +7 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +38 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_controller_impl_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +113 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_entry_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +12 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_entry_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +5 lines, -1 line 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 80 (39 generated)
Takashi Toyoshima
Can you take a look? I'm looking field trial results, and see visible number change ...
3 years, 4 months ago (2016-07-26 07:59:09 UTC) #5
Charlie Reis
Maybe we should chat about the design here and what you need. I don't think ...
3 years, 4 months ago (2016-07-27 00:07:01 UTC) #6
Takashi Toyoshima
I'm almost busy in the next two days, but can we have a VC chat ...
3 years, 4 months ago (2016-07-27 08:48:45 UTC) #7
Charlie Reis
On 2016/07/27 08:48:45, toyoshim wrote: > I'm almost busy in the next two days, but ...
3 years, 4 months ago (2016-07-27 23:21:15 UTC) #8
Takashi Toyoshima
Sorry for late response. Now I finished a course that made me busy. Let me ...
3 years, 4 months ago (2016-08-02 10:08:45 UTC) #9
Charlie Reis
On 2016/08/02 10:08:45, toyoshim wrote: > Sorry for late response. Now I finished a course ...
3 years, 4 months ago (2016-08-02 22:20:40 UTC) #10
Takashi Toyoshima
> Does "detecting" mean just seeing a second reload in a row? Yes, that's my ...
3 years, 4 months ago (2016-08-05 10:24:38 UTC) #11
Takashi Toyoshima
didn't finish yet, but let me share what I tried. https://codereview.chromium.org/2174293002/diff/60001/content/browser/frame_host/navigation_tracker.cc File content/browser/frame_host/navigation_tracker.cc (right): https://codereview.chromium.org/2174293002/diff/60001/content/browser/frame_host/navigation_tracker.cc#newcode38 ...
3 years, 4 months ago (2016-08-08 09:59:57 UTC) #13
Takashi Toyoshima
How about using NavigationEntryImpl to keep ReloadType? Even in this case, we still need another ...
3 years, 4 months ago (2016-08-08 11:46:20 UTC) #16
Charlie Reis
Thanks-- I think we're headed in a better direction. It tends to be tricky to ...
3 years, 4 months ago (2016-08-08 23:57:54 UTC) #19
Takashi Toyoshima
I upload a new Patch Set 7 that passes ClientRedirectPolicy in the CommitLoad delegate chain ...
3 years, 4 months ago (2016-08-09 07:35:26 UTC) #22
Takashi Toyoshima
https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_host/navigation_controller_impl.cc File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_host/navigation_controller_impl.cc#newcode1961 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_host/navigation_controller_impl.h ...
3 years, 4 months ago (2016-08-09 12:06:18 UTC) #25
Takashi Toyoshima
PTAL. Now this change contains many files. If you agreed ClientRedirectPolicy change, I will split ...
3 years, 4 months ago (2016-08-09 12:07:35 UTC) #26
Charlie Reis
Thanks for pushing forward, but I still have some concerns about this approach. Unfortunately, I ...
3 years, 4 months ago (2016-08-10 05:05:14 UTC) #27
Takashi Toyoshima
https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_host/navigation_controller_impl.cc File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/100001/content/browser/frame_host/navigation_controller_impl.cc#newcode349 content/browser/frame_host/navigation_controller_impl.cc:349: entry->set_reload_type(reload_type); If we assume this reload type is valid ...
3 years, 4 months ago (2016-08-10 06:34:15 UTC) #28
Takashi Toyoshima
https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2174293002/diff/100001/tools/metrics/histograms/histograms.xml#newcode53546 tools/metrics/histograms/histograms.xml:53546: + The buckets indicate whether or not the " ...
3 years, 4 months ago (2016-08-10 07:26:46 UTC) #29
Charlie Reis
Thanks. Still working through this. I'm not stability sheriff tomorrow, so I should be able ...
3 years, 4 months ago (2016-08-10 23:22:19 UTC) #30
Charlie Reis
Ok, let's take a step back and look at the requirements, and I think that ...
3 years, 4 months ago (2016-08-11 23:17:10 UTC) #31
Takashi Toyoshima
Hi, I have been busy for fixing a high priority issue while you are on ...
3 years, 3 months ago (2016-09-06 09:19:18 UTC) #32
Takashi Toyoshima
Thank you for a clear guideline. 1) Sounds fine. But, we may consider to measure ...
3 years, 3 months ago (2016-09-07 12:27:54 UTC) #33
Charlie Reis
On 2016/09/07 12:27:54, toyoshim wrote: > Thank you for a clear guideline. > > 1) ...
3 years, 3 months ago (2016-09-09 05:58:49 UTC) #34
Takashi Toyoshima
> I'm still inclined to think that the Commit1-to-UseAction2 might be the best > thing ...
3 years, 3 months ago (2016-09-09 13:32:09 UTC) #35
Takashi Toyoshima
I upload a patch set that is based on your idea. But, I could not ...
3 years, 3 months ago (2016-09-12 10:42:00 UTC) #36
Takashi Toyoshima
I checked what twitter.com did, and noticed that if XHR was requested inside onunload event ...
3 years, 3 months ago (2016-09-13 09:21:06 UTC) #37
Takashi Toyoshima
> if XHR was requested Sorry, this isn't a right condition. I can see the ...
3 years, 3 months ago (2016-09-13 10:22:14 UTC) #40
Charlie Reis
On 2016/09/13 10:22:14, toyoshim wrote: > > if XHR was requested > Sorry, this isn't ...
3 years, 3 months ago (2016-09-14 21:01:35 UTC) #43
Takashi Toyoshima
I dived this issue, and took notes at crbug.com/646730. Now I get a simple code ...
3 years, 3 months ago (2016-09-15 11:32:16 UTC) #44
Charlie Reis
Thanks, this is looking much better. We'll want to include some tests that check that ...
3 years, 2 months ago (2016-09-19 22:23:51 UTC) #45
Takashi Toyoshima
Thank you for reviewing this. I'm working on updating this patch set based on your ...
3 years, 2 months ago (2016-09-21 10:14:09 UTC) #46
Takashi Toyoshima
https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_host/navigation_controller_impl.cc File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2174293002/diff/180001/content/browser/frame_host/navigation_controller_impl.cc#newcode333 content/browser/frame_host/navigation_controller_impl.cc:333: // Check if previous navigation was reload to track ...
3 years, 2 months ago (2016-09-26 12:23:34 UTC) #48
Charlie Reis
Great! I think this is pretty much ready, apart from needing a test. I left ...
3 years, 2 months ago (2016-09-26 23:34:39 UTC) #52
Takashi Toyoshima
Ah, I just forgot about adding tests. Let me add it in the next patch ...
3 years, 2 months ago (2016-09-27 07:15:47 UTC) #53
Takashi Toyoshima
Now the last patch set 13 contains a browser test for the reload tracking.
3 years, 2 months ago (2016-09-27 11:24:43 UTC) #56
Charlie Reis
Great, thanks for the test, and for bearing with me through all this! LGTM with ...
3 years, 2 months ago (2016-09-28 00:05:24 UTC) #59
Takashi Toyoshima
https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_host/navigation_controller_impl_browsertest.cc File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_host/navigation_controller_impl_browsertest.cc#newcode6183 content/browser/frame_host/navigation_controller_impl_browsertest.cc:6183: histogram.ExpectTotalCount(kReloadMainResourceToReloadMetricName, 2); I tried to use setTimeout, but it ...
3 years, 2 months ago (2016-09-28 10:37:21 UTC) #60
Charlie Reis
Thanks for the extra test case! LGTM with one simplification. https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_host/navigation_controller_impl_browsertest.cc File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2174293002/diff/240001/content/browser/frame_host/navigation_controller_impl_browsertest.cc#newcode6183 ...
3 years, 2 months ago (2016-09-28 16:58:59 UTC) #61
Takashi Toyoshima
+Ilya for metrics.
3 years, 2 months ago (2016-09-29 06:55:34 UTC) #70
Ilya Sherman
metrics lgtm
3 years, 2 months ago (2016-09-29 07:13:40 UTC) #71
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2174293002/300001
3 years, 2 months ago (2016-09-29 08:33:49 UTC) #76
commit-bot: I haz the power
Committed patchset #16 (id:300001)
3 years, 2 months ago (2016-09-29 09:03:49 UTC) #78
commit-bot: I haz the power
3 years, 2 months ago (2016-09-29 09:05:47 UTC) #80
Message was sent while issue was closed.
Patchset 16 (id:??) landed as
https://crrev.com/a63c2a63ec73f968a05413c49f1e38dc2c83cd0c
Cr-Commit-Position: refs/heads/master@{#421783}

Powered by Google App Engine
This is Rietveld 408576698