Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885453003:
NavigationSimulator: add support for GlobalRequestIds (Closed)
|
Created:
3 years, 7 months ago by Charlie Harrison Modified:
3 years, 7 months ago CC:
chromium-reviews, csharrison+watch_chromium.org, jam, darin-cc_chromium.org, loading-reviews+metrics_chromium.org, speed-metrics-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNavigationSimulator: add support for GlobalRequestIds
This adds support for both the PlzNavigate and non-PlzNavigate paths,
and ensures that TestNavigationURLLoader always uses a non default one.
BUG=680841, 711352
Review-Url: https://codereview.chromium.org/2885453003
Cr-Commit-Position: refs/heads/master@{#472093}
Committed: https://chromium.googlesource.com/chromium/src/+/50996ef01397fc67caece4810a2c51f91f926cdb
Patch Set 1 #Patch Set 2 : dcheck #
Total comments: 10
Patch Set 3 : creis review #Patch Set 4 : creis review #
Total comments: 4
Patch Set 5 : fix comments #
Messages
Total messages: 30 (19 generated)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + arthursonzogni@chromium.org, creis@chromium.org
creis + arthursonzogni: Would you two be ok reviewers for this change? I'd ask clamy/nasko but they are out.
csharrison@chromium.org changed reviewers: + jkarlin@chromium.org
+jkarlin
I'm not super familiar with this code, but one question below about collisions. Otherwise seems ok. https://codereview.chromium.org/2885453003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_tracker.cc (right): https://codereview.chromium.org/2885453003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_tracker.cc:525: DCHECK(navigation_handle->GetGlobalRequestID() != content::GlobalRequestID()); This looks possibly redundant with line 528 now that the conditional is gone. https://codereview.chromium.org/2885453003/diff/20001/content/public/test/nav... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2885453003/diff/20001/content/public/test/nav... content/public/test/navigation_simulator.cc:255: static int request_id = 0; Are there any concerns with this overlapping with other request IDs in tests? Or is that not a concern since this only gets used in unit tests? https://codereview.chromium.org/2885453003/diff/20001/content/test/test_navig... File content/test/test_navigation_url_loader.cc (right): https://codereview.chromium.org/2885453003/diff/20001/content/test/test_navig... content/test/test_navigation_url_loader.cc:61: static int request_id = 0; Hmm, now it does look like there's a potential for collisions. Is it ever possible for both NavigationSimulator and TestNavigationURLLoader to be used in the same test?
The CQ bit was checked by csharrison@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...
https://codereview.chromium.org/2885453003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_tracker.cc (right): https://codereview.chromium.org/2885453003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_tracker.cc:525: DCHECK(navigation_handle->GetGlobalRequestID() != content::GlobalRequestID()); On 2017/05/15 20:07:43, Charlie Reis wrote: > This looks possibly redundant with line 528 now that the conditional is gone. Done. https://codereview.chromium.org/2885453003/diff/20001/content/public/test/nav... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2885453003/diff/20001/content/public/test/nav... content/public/test/navigation_simulator.cc:255: static int request_id = 0; On 2017/05/15 20:07:43, Charlie Reis wrote: > Are there any concerns with this overlapping with other request IDs in tests? > Or is that not a concern since this only gets used in unit tests? I think this could theoretically overlap with request IDs in unit tests. I'm not sure how much of a problem it is though, especially since consumers know that these navigations are not really backed by URLRequests. We could do something fancy like start at INT_MAX and subtract if you'd like. https://codereview.chromium.org/2885453003/diff/20001/content/test/test_navig... File content/test/test_navigation_url_loader.cc (right): https://codereview.chromium.org/2885453003/diff/20001/content/test/test_navig... content/test/test_navigation_url_loader.cc:61: static int request_id = 0; On 2017/05/15 20:07:43, Charlie Reis wrote: > Hmm, now it does look like there's a potential for collisions. Is it ever > possible for both NavigationSimulator and TestNavigationURLLoader to be used in > the same test? Yes, but the code I've changed in navigation_simulator will only run in non-PlzNavigate world, and NavigationURLLoader is PlzNavigate only. I've added a DCHECK in navigation_simulator so this is explicit.
Thanks. LGTM with nits. https://codereview.chromium.org/2885453003/diff/20001/content/public/test/nav... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2885453003/diff/20001/content/public/test/nav... content/public/test/navigation_simulator.cc:255: static int request_id = 0; On 2017/05/15 21:45:54, Charlie Harrison wrote: > On 2017/05/15 20:07:43, Charlie Reis wrote: > > Are there any concerns with this overlapping with other request IDs in tests? > > Or is that not a concern since this only gets used in unit tests? > > I think this could theoretically overlap with request IDs in unit tests. I'm not > sure how much of a problem it is though, especially since consumers know that > these navigations are not really backed by URLRequests. > > We could do something fancy like start at INT_MAX and subtract if you'd like. Maybe just start it at 1000 to reduce the chance of overlap in the common case? (That's assuming there isn't any code comparing request IDs to each other, of course.) No big deal either way-- just trying to make it slightly less likely to encounter odd unit test problems. https://codereview.chromium.org/2885453003/diff/20001/content/test/test_navig... File content/test/test_navigation_url_loader.cc (right): https://codereview.chromium.org/2885453003/diff/20001/content/test/test_navig... content/test/test_navigation_url_loader.cc:61: static int request_id = 0; On 2017/05/15 21:45:54, Charlie Harrison wrote: > On 2017/05/15 20:07:43, Charlie Reis wrote: > > Hmm, now it does look like there's a potential for collisions. Is it ever > > possible for both NavigationSimulator and TestNavigationURLLoader to be used > in > > the same test? > > Yes, but the code I've changed in navigation_simulator will only run in > non-PlzNavigate world, and NavigationURLLoader is PlzNavigate only. > > I've added a DCHECK in navigation_simulator so this is explicit. Ah, glad they're mutually exclusive. Can you add a similar DCHECK here that PlzNavigate is enabled? I don't see anything in this class except a comment on the class declaration.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by csharrison@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...
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_...)
https://codereview.chromium.org/2885453003/diff/20001/content/public/test/nav... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2885453003/diff/20001/content/public/test/nav... content/public/test/navigation_simulator.cc:255: static int request_id = 0; On 2017/05/15 22:02:38, Charlie Reis wrote: > On 2017/05/15 21:45:54, Charlie Harrison wrote: > > On 2017/05/15 20:07:43, Charlie Reis wrote: > > > Are there any concerns with this overlapping with other request IDs in > tests? > > > Or is that not a concern since this only gets used in unit tests? > > > > I think this could theoretically overlap with request IDs in unit tests. I'm > not > > sure how much of a problem it is though, especially since consumers know that > > these navigations are not really backed by URLRequests. > > > > We could do something fancy like start at INT_MAX and subtract if you'd like. > > Maybe just start it at 1000 to reduce the chance of overlap in the common case? > (That's assuming there isn't any code comparing request IDs to each other, of > course.) > > No big deal either way-- just trying to make it slightly less likely to > encounter odd unit test problems. Done. https://codereview.chromium.org/2885453003/diff/20001/content/test/test_navig... File content/test/test_navigation_url_loader.cc (right): https://codereview.chromium.org/2885453003/diff/20001/content/test/test_navig... content/test/test_navigation_url_loader.cc:61: static int request_id = 0; On 2017/05/15 22:02:39, Charlie Reis wrote: > On 2017/05/15 21:45:54, Charlie Harrison wrote: > > On 2017/05/15 20:07:43, Charlie Reis wrote: > > > Hmm, now it does look like there's a potential for collisions. Is it ever > > > possible for both NavigationSimulator and TestNavigationURLLoader to be used > > in > > > the same test? > > > > Yes, but the code I've changed in navigation_simulator will only run in > > non-PlzNavigate world, and NavigationURLLoader is PlzNavigate only. > > > > I've added a DCHECK in navigation_simulator so this is explicit. > > Ah, glad they're mutually exclusive. Can you add a similar DCHECK here that > PlzNavigate is enabled? I don't see anything in this class except a comment on > the class declaration. I've added a DCHECK in the constructor.
Thanks. LGTM with nits. https://codereview.chromium.org/2885453003/diff/60001/content/public/test/nav... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2885453003/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.cc:256: // network resoures (it should be rare to compare these in unit tests). Nit: s/resoures/resources https://codereview.chromium.org/2885453003/diff/60001/content/test/test_navig... File content/test/test_navigation_url_loader.cc (right): https://codereview.chromium.org/2885453003/diff/60001/content/test/test_navig... content/test/test_navigation_url_loader.cc:65: // network resoures (it should be rare to compare these in unit tests). nit: s/resoures/resources
lgtm! Thanks Charles.
The CQ bit was checked by csharrison@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...
Thanks folks https://codereview.chromium.org/2885453003/diff/60001/content/public/test/nav... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2885453003/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.cc:256: // network resoures (it should be rare to compare these in unit tests). On 2017/05/16 09:16:04, arthursonzogni wrote: > Nit: s/resoures/resources Done. https://codereview.chromium.org/2885453003/diff/60001/content/test/test_navig... File content/test/test_navigation_url_loader.cc (right): https://codereview.chromium.org/2885453003/diff/60001/content/test/test_navig... content/test/test_navigation_url_loader.cc:65: // network resoures (it should be rare to compare these in unit tests). On 2017/05/16 09:16:05, arthursonzogni wrote: > nit: s/resoures/resources Done.
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, jkarlin@chromium.org, arthursonzogni@chromium.org Link to the patchset: https://codereview.chromium.org/2885453003/#ps80001 (title: "fix comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494937424463840,
"parent_rev": "0c09719b6965b13fac67b09aff251db5c42890fd", "commit_rev":
"50996ef01397fc67caece4810a2c51f91f926cdb"}
Message was sent while issue was closed.
Description was changed from ========== NavigationSimulator: add support for GlobalRequestIds This adds support for both the PlzNavigate and non-PlzNavigate paths, and ensures that TestNavigationURLLoader always uses a non default one. BUG=680841,711352 ========== to ========== NavigationSimulator: add support for GlobalRequestIds This adds support for both the PlzNavigate and non-PlzNavigate paths, and ensures that TestNavigationURLLoader always uses a non default one. BUG=680841,711352 Review-Url: https://codereview.chromium.org/2885453003 Cr-Commit-Position: refs/heads/master@{#472093} Committed: https://chromium.googlesource.com/chromium/src/+/50996ef01397fc67caece4810a2c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/50996ef01397fc67caece4810a2c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
