Various cleanups for page_load_metrics_browsertest

Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using PageLoadMetricsWaiter
* add support for monitoring both main and child frame updates
* fix broken DocumentWriteAsync test

BUG=720534
Review-Url: https://codereview.chromium.org/2868213003
Cr-Commit-Position: refs/heads/master@{#471005}
Committed: https://chromium.googlesource.com/chromium/src/+/a1692d2da9038b8515e47f4590f24df81b56c521

[Bryan McQuade, 2017-05-10 17:45:19]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[Bryan McQuade, 2017-05-10 17:46:04]

Description was changed from

==========
Various cleanups for page_load_metrics_browsertest

BUG=
==========

to

==========
Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using
PageLoadMetricsWaiter
* fix broken DocumentWriteAsync test

BUG=
==========

[commit-bot: I haz the power, 2017-05-10 17:46:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-10 17:49:20]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 17:49:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-10 17:50:59]

Description was changed from

==========
Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using
PageLoadMetricsWaiter
* fix broken DocumentWriteAsync test

BUG=
==========

to

==========
Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using
PageLoadMetricsWaiter
* fix broken DocumentWriteAsync test

BUG=720534
==========

[Bryan McQuade, 2017-05-10 17:51:23]

bmcquade@chromium.org changed reviewers:
+ jkarlin@chromium.org

[Bryan McQuade, 2017-05-10 17:51:49]

PTAL. I decided to factor all of the various browsertest improvements into its
own change. This should look familiar to the child frame test, with a few other
additions. Thanks!

[Bryan McQuade, 2017-05-10 17:53:14]

Description was changed from

==========
Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using
PageLoadMetricsWaiter
* fix broken DocumentWriteAsync test

BUG=720534
==========

to

==========
Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using
PageLoadMetricsWaiter
* add support for monitoring both main and child frame updates
* fix broken DocumentWriteAsync test

BUG=720534
==========

[lpy, 2017-05-10 18:03:50]

Description was changed from

==========
Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using
PageLoadMetricsWaiter
* add support for monitoring both main and child frame updates
* fix broken DocumentWriteAsync test

BUG=720534
==========

to

==========
Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using
PageLoadMetricsWaiter
* add support for monitoring both main and child frame updates
* fix broken DocumentWriteAsync test

BUG=720534
==========

[jkarlin, 2017-05-10 18:45:05]

Looks good.

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:48: new
net::FailingHttpTransactionFactory(cache->GetSession(),
base::MakeUnique while we're here :)

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:49:
net::ERR_FAILED));
Include the net error header

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:104: bool
empty() const { return bitmask_ == 0; }
Empty

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:107: void
set(ExpectedTimingField field) { bitmask_ |= static_cast<int>(field); }
Set

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:110: void
clear(const ExpectedTimingFieldBitSet& other) {
Typically I think of clear as clearing everything. Perhaps, ClearMatchingBits?

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:219:
std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
optional: auto seems reasonable here

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:220:
waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_PAINT);
Seems like it would be useful to have a constructor like so:

auto waiter = CreatePageLoadMetricsWaiter(ExpectedTimingField::FIRST_PAINT);
then you could construct a waiter with all of the flags you want or'd together.

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:250:
std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
What if you could also create a set of unexpected matching bits that the test
would fail on if they were detected?

[Bryan McQuade, 2017-05-10 19:07:59]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 19:09:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-10 19:09:51]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 19:10:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-10 19:12:40]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 19:13:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-10 19:14:33]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:48: new
net::FailingHttpTransactionFactory(cache->GetSession(),
On 2017/05/10 at 18:45:05, jkarlin wrote:
> base::MakeUnique while we're here :)

Done

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:49:
net::ERR_FAILED));
On 2017/05/10 at 18:45:04, jkarlin wrote:
> Include the net error header

Done

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:104: bool
empty() const { return bitmask_ == 0; }
On 2017/05/10 at 18:45:04, jkarlin wrote:
> Empty

Done. I interpreted the style guide as indicating this is ok style, but perhaps
these aren't quite accessors / mutators:
https://google.github.io/styleguide/cppguide.html#Function_Names "accessors and
mutators may be named like variables."

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:107: void
set(ExpectedTimingField field) { bitmask_ |= static_cast<int>(field); }
On 2017/05/10 at 18:45:05, jkarlin wrote:
> Set

Done

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:110: void
clear(const ExpectedTimingFieldBitSet& other) {
On 2017/05/10 at 18:45:04, jkarlin wrote:
> Typically I think of clear as clearing everything. Perhaps, ClearMatchingBits?

changed to ClearMatching.

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:219:
std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
On 2017/05/10 at 18:45:05, jkarlin wrote:
> optional: auto seems reasonable here

done

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:220:
waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_PAINT);
On 2017/05/10 at 18:45:04, jkarlin wrote:
> Seems like it would be useful to have a constructor like so:
> 
> auto waiter = CreatePageLoadMetricsWaiter(ExpectedTimingField::FIRST_PAINT);
then you could construct a waiter with all of the flags you want or'd together.

I'm disinclined to do this since we have main and child frames & it's not
totally clear at the callsite whether the constructor is for main or child. I
think main is a logical default but overall I think the loss of clarity doesn't
make up for saving one line of code.

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:250:
std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
On 2017/05/10 at 18:45:05, jkarlin wrote:
> What if you could also create a set of unexpected matching bits that the test
would fail on if they were detected?

Sure, I added an API for this.

[Bryan McQuade, 2017-05-10 19:18:07]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 19:18:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-10 19:20:51]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 19:21:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 21:29:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:29:14]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jkarlin, 2017-05-11 14:04:06]

lgtm w/ comments

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:104: bool
empty() const { return bitmask_ == 0; }
On 2017/05/10 19:14:32, Bryan McQuade wrote:
> On 2017/05/10 at 18:45:04, jkarlin wrote:
> > Empty
> 
> Done. I interpreted the style guide as indicating this is ok style, but
perhaps
> these aren't quite accessors / mutators:
> https://google.github.io/styleguide/cppguide.html#Function_Names "accessors
and
> mutators may be named like variables."

I mostly agree with you that they could be lowercase, but clear and empty are
slightly more complex than getting and setting. Grep wars shows 3x as many
Empty() functions as empty().

https://codereview.chromium.org/2868213003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:220:
waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_PAINT);
On 2017/05/10 19:14:32, Bryan McQuade wrote:
> On 2017/05/10 at 18:45:04, jkarlin wrote:
> > Seems like it would be useful to have a constructor like so:
> > 
> > auto waiter = CreatePageLoadMetricsWaiter(ExpectedTimingField::FIRST_PAINT);
> then you could construct a waiter with all of the flags you want or'd
together.
> 
> I'm disinclined to do this since we have main and child frames & it's not
> totally clear at the callsite whether the constructor is for main or child. I
> think main is a logical default but overall I think the loss of clarity
doesn't
> make up for saving one line of code.

Fair point. The constructor could take two values, but then it wouldn't be clear
which is which.

https://codereview.chromium.org/2868213003/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2868213003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:292: 
EXPECT_FALSE(waiter->DidObserveInMainFrame(TimingField::FIRST_PAINT));

https://codereview.chromium.org/2868213003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:316: 
EXPECT_FALSE(waiter->DidObserveInMainFrame(TimingField::FIRST_PAINT));

https://codereview.chromium.org/2868213003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:327: 
EXPECT_FALSE(waiter->DidObserveInMainFrame(TimingField::FIRST_PAINT));

[Bryan McQuade, 2017-05-11 14:07:03]

Thanks!

https://codereview.chromium.org/2868213003/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2868213003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:292: 
On 2017/05/11 at 14:04:06, jkarlin wrote:
> EXPECT_FALSE(waiter->DidObserveInMainFrame(TimingField::FIRST_PAINT));

For better or worse, title1.html does paint, so we can't add this.

https://codereview.chromium.org/2868213003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:316: 
On 2017/05/11 at 14:04:05, jkarlin wrote:
> EXPECT_FALSE(waiter->DidObserveInMainFrame(TimingField::FIRST_PAINT));

Same

https://codereview.chromium.org/2868213003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:327: 
On 2017/05/11 at 14:04:06, jkarlin wrote:
> EXPECT_FALSE(waiter->DidObserveInMainFrame(TimingField::FIRST_PAINT));

Same

[Bryan McQuade, 2017-05-11 17:47:39]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2017-05-11 17:48:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 17:57:01]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494524859276360,
"parent_rev": "43c5f35105c185d6d48b580d7a2ad98ca3be21eb", "commit_rev":
"a1692d2da9038b8515e47f4590f24df81b56c521"}

[commit-bot: I haz the power, 2017-05-11 17:57:12]

Description was changed from

==========
Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using
PageLoadMetricsWaiter
* add support for monitoring both main and child frame updates
* fix broken DocumentWriteAsync test

BUG=720534
==========

to

==========
Various cleanups for page_load_metrics_browsertest

* clean up and generalize PageLoadMetricsWaiter
* where possible, switch tests from calling NavigateToUntrackedUrl() to using
PageLoadMetricsWaiter
* add support for monitoring both main and child frame updates
* fix broken DocumentWriteAsync test

BUG=720534

Review-Url: https://codereview.chromium.org/2868213003
Cr-Commit-Position: refs/heads/master@{#471005}
Committed:
https://chromium.googlesource.com/chromium/src/+/a1692d2da9038b8515e47f4590f2...
==========

[commit-bot: I haz the power, 2017-05-11 17:57:13]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a1692d2da9038b8515e47f4590f2...