Issue 722444  has been merged into this issue.

Just noticed that we need this on WebContentsTester as well. What's the problem there? Is it PlzNavigate-only or broken in both cases?

Broken in both cases. This is revealed when you make the following change: https://codereview.chromium.org/2884753002/diff/100001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc

Suddenly we start to see main frame request types come in which triggers untested code paths that check the global request id.

OK thanks for the pointer. Not sure if I'll have time to dig into that issue though.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/50996ef01397fc67caece4810a2c51f91f926cdb

commit 50996ef01397fc67caece4810a2c51f91f926cdb
Author: csharrison <csharrison@chromium.org>
Date: Tue May 16 14:05:34 2017

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}

[modify] https://crrev.com/50996ef01397fc67caece4810a2c51f91f926cdb/chrome/browser/page_load_metrics/page_load_tracker.cc
[modify] https://crrev.com/50996ef01397fc67caece4810a2c51f91f926cdb/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/50996ef01397fc67caece4810a2c51f91f926cdb/content/test/test_navigation_url_loader.cc

Charles added support for GlobalRequestIDs in NavigationSimulator. I'm going to fix up the PLM unittests such that we no longer require a workaround for missing GlobalRequestIDs in MetricsWebContentsObserver.

Note that I looked into adding support for providing GlobalRequestIDs in the WebContentsTester framework, but it quickly became complex, so I decided that wasn't worthwhile.

In particular, I tried to update this test code to invoke the NavigationHandle::CallWillProcessResponseForTesting flow, in order to assign a GlobalRequestID, but it was difficult to do this cleanly given the many existing uses of WebContentsTester and various execution ordering requirements and assumptions.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aefd605c21331b94e8e8cce0daac64f9a944e9af

commit aefd605c21331b94e8e8cce0daac64f9a944e9af
Author: Bryan McQuade <bmcquade@chromium.org>
Date: Fri Jun 23 15:56:52 2017

Remove GlobalRequestID test workaround in MetricsWebContentsObserver.

Unit test infrastructure doesn't consistently provide a GlobalRequestID
for NavigationHandles. NavigationSimulator provides support for
generating GlobalRequestIDs, but WebContentsTester does not. As a
result, page load metrics production code contains a workaround for
missing GlobalRequestIDs. This change removes that workaround.

This change does the following:
* require a GlobalRequestID for simulated main frame resource loads.
* add support in NavigationSimulator to get the GlobalRequestID
  for the navigation, after nav commit.
* add support for specifying a GlobalRequestID in
  SimulateLoadedResource.
* add tests to verify correct behavior
* switch most observers to using non-main frame resources, since
  most observers do not have any resource type specific logic

Going forward, any tests that want to simulate the load of a main
frame resource must use NavigationSimulator in order to get a valid
GlobalRequestID.

Bug:  711352 
Change-Id: I01aded2cbdc36cb627235d0ce0126045ed441c0a
Reviewed-on: https://chromium-review.googlesource.com/541516
Commit-Queue: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481904}
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/observers/lofi_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/page_load_metrics_observer.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/chrome/browser/page_load_metrics/page_load_metrics_observer.h
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/aefd605c21331b94e8e8cce0daac64f9a944e9af/content/public/test/navigation_simulator.h

Though we did not add support in WebContentsTester (despite trying to do so :)) we have solid support in NavigationSimulator such that we no longer to work around this for tests. So I'm going to close this out.