PageLoadMetrics: Add AssertPageLoadMetricsObserver [chromium/src : main]

Ken Okada (Gerrit)

unread,

Aug 3, 2022, 5:36:57 AM8/3/22

to bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, Takashi Toyoshima, Kouhei Ueno, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Kouhei Ueno, Takashi Toyoshima.

View Change

1 comment:

Patchset:
- Patch Set #5:
  Could you have a look?

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Aug 3, 2022, 10:43:05 PM8/3/22

to Ken Okada, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, Kouhei Ueno, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Ken Okada, Kouhei Ueno.

View Change

7 comments:

Patchset:
- Patch Set #5:
  Thanks, this is really a great idea!
File components/page_load_metrics/browser/page_load_metrics_update_dispatcher.cc:
- Patch Set #5, Line 154:
```
  // If the page is non prerendered, `parse_start <= first_paint`.
  // If the page is prerendered, `parse_start <= activation_start <=
  // first_paint`.
```
  The first part explains the check from line 164, and the second part does the check below. It's a little confusing to me. Maybe one root of the confusion is the comment says "if ... prerendered...", but we didn't check `is_prerendered` in the following checks.
  How about omitting the first part, and just explains the prerendering case here?
- Patch Set #5, Line 157: if
  Also, should this be `if (is_prerendered && !EventsInOrder(...))` or move this inside the `if (is_prerendered)` below?
- Patch Set #5, Line 161: INVALID_ORDER_PARSE_START_FIRST_PAINT
  Probably, better to define a new enum.
- Patch Set #5, Line 176: INVALID_ORDER_PARSE_START_FIRST_PAINT
  ditto
- Patch Set #5, Line 722: bool
  const bool, or directly passing the expression into the second argument below.
- Patch Set #5, Line 903: bool
  ditto

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Kouhei Ueno (Gerrit)

unread,

Aug 4, 2022, 1:07:35 AM8/4/22

to Ken Okada, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, Takashi Toyoshima, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Ken Okada.

Patch set 5:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #5:
  lgtm % toyoshim review comments

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Ken Okada (Gerrit)

unread,

Aug 5, 2022, 3:10:41 AM8/5/22

to bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, mparch-reviews, Kevin McNee, Kouhei Ueno, Takashi Toyoshima, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Kouhei Ueno, Takashi Toyoshima.

View Change

6 comments:

File components/page_load_metrics/browser/page_load_metrics_update_dispatcher.cc:

Patch Set #5, Line 154:

  // If the page is non prerendered, `parse_start <= first_paint`.
  // If the page is prerendered, `parse_start <= activation_start <=
  // first_paint`.

- The first part explains the check from line 164, and the second part does the check below. […]
  Thanks! Refactored.

- Patch Set #5, Line 157: if

- Also, should this be `if (is_prerendered && !EventsInOrder(... […]
  Done

- Patch Set #5, Line 161: INVALID_ORDER_PARSE_START_FIRST_PAINT
  Probably, better to define a new enum.

- Done

- Patch Set #5, Line 176: INVALID_ORDER_PARSE_START_FIRST_PAINT
  ditto

- Done

- Patch Set #5, Line 722: bool
  const bool, or directly passing the expression into the second argument below.

- Done

- Patch Set #5, Line 903: bool
  ditto

- Done

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Aug 5, 2022, 4:22:24 AM8/5/22

to Ken Okada, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, mparch-reviews, Kevin McNee, Kouhei Ueno, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Ken Okada, Kouhei Ueno.

View Change

3 comments:

File components/page_load_metrics/browser/observers/assert_page_load_metrics_observer.h:
- Patch Set #7, Line 17: // have no (non trivial) assumption on callback timings.
  optional:
  It may be good to inherit PLMOInterface rather than PLMO.
  It enforce you to define all methods below, and you will need some extra works as I can guess from the comment above.
  But, it encourages other people to keep this AssertPLMO up-to-date. People will notice this class and may add some fancy checks when they add a new interface.

File components/page_load_metrics/browser/observers/assert_page_load_metrics_observer.cc:

Patch Set #7, Line 15:

// For ease of reading.
#define IF_PRERENDERED(body)                  \
  if (GetDelegate().GetPrerenderingState() != \
      PrerenderingState::kNoPrerendering) {   \
    body;                                     \
  }

I prefer to define a private method, IsPrerendered, and write it as
```
if (IsPrerendered())
  body;
```

Patch Set #7, Line 190: crrev.com/c/3767770
can you insert `https://`? complete url helps for some developer tools to pick up the link automatically.
ditto for others.

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Ken Okada (Gerrit)

unread,

Aug 8, 2022, 2:57:13 AM8/8/22

to bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, mparch-reviews, Kevin McNee, Kouhei Ueno, Takashi Toyoshima, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Kouhei Ueno, Takashi Toyoshima.

View Change

3 comments:

File components/page_load_metrics/browser/observers/assert_page_load_metrics_observer.h:

- Patch Set #7, Line 17: // have no (non trivial) assumption on callback timings.

- optional: […]
  Makes sense.
  This work is non trivial and makes diff large. I'll separate this as the next CL.
File components/page_load_metrics/browser/observers/assert_page_load_metrics_observer.cc:

Patch Set #7, Line 15:

// For ease of reading.
#define IF_PRERENDERED(body)                  \
  if (GetDelegate().GetPrerenderingState() != \
      PrerenderingState::kNoPrerendering) {   \
    body;                                     \
  }

- I prefer to define a private method, IsPrerendered, and write it as […]
  Done

- Patch Set #7, Line 190: crrev.com/c/3767770

- can you insert `https://`? complete url helps for some developer tools to pick up the link automatic […]
  Done

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Kouhei Ueno (Gerrit)

unread,

Aug 8, 2022, 11:09:49 AM8/8/22

to Ken Okada, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, mparch-reviews, Kevin McNee, Takashi Toyoshima, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Ken Okada, Takashi Toyoshima.

Patch set 8:Code-Review +1

View Change

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Takashi Toyoshima (Gerrit)

unread,

Aug 9, 2022, 1:12:59 AM8/9/22

to Ken Okada, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, Kouhei Ueno, mparch-reviews, Kevin McNee, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Ken Okada.

Patch set 8:Code-Review +1

View Change

2 comments:

File components/page_load_metrics/browser/observers/assert_page_load_metrics_observer.h:

- Patch Set #7, Line 17: // have no (non trivial) assumption on callback timings.

- Makes sense. […]
  sg
File components/page_load_metrics/browser/observers/assert_page_load_metrics_observer.cc:
- Patch Set #8, Line 234:
```
  if (IsPrerendered()) {
    DCHECK(activated_);
  }
```
  fyi, `DCHECK(!IsPrerendered() || activated_);` may be a well-seen idiom to be used for this kind of checks. I'm not a big fun of it personally as it takes a little time to understand what it means, but just in case if you prefer I'm fine with either.

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Ken Okada (Gerrit)

unread,

Aug 9, 2022, 1:14:56 AM8/9/22

to bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, Takashi Toyoshima, Kouhei Ueno, mparch-reviews, Kevin McNee, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

View Change

1 comment:

File components/page_load_metrics/browser/observers/assert_page_load_metrics_observer.cc:

Patch Set #8, Line 234:

  if (IsPrerendered()) {
    DCHECK(activated_);
  }

- fyi, `DCHECK(!IsPrerendered() || activated_);` may be a well-seen idiom to be used for this kind of […]
  Thanks. I have the same opinion.

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Ken Okada (Gerrit)

unread,

Aug 9, 2022, 1:15:09 AM8/9/22

to bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, Takashi Toyoshima, Kouhei Ueno, mparch-reviews, Kevin McNee, Yoav Weiss, Tricium, Chromium LUCI CQ, chromium...@chromium.org

Patch set 8:Commit-Queue +2

View Change

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Aug 9, 2022, 1:19:02 AM8/9/22

to Ken Okada, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, mparch-rev...@chromium.org, speed-metr...@chromium.org, tommcke...@chromium.org, Takashi Toyoshima, Kouhei Ueno, mparch-reviews, Kevin McNee, Yoav Weiss, Tricium, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Kouhei Ueno: Looks good to me
  Ken Okada: Commit
  Takashi Toyoshima: Looks good to me

PageLoadMetrics: Add AssertPageLoadMetricsObserver

As prerendering is added to navigation concept, the context and
assumptions of PageLoadMetricsObserver's callback become complex.

This CL

- adds AssertPageLoadMetricsObserver that describes the context
  and assumptions of callbacks for ease of implementation of PLMOs.
- adds assertion about PageLoadTiming in
  PageLoadMetricsUpdateDispatcher.
- updates comments and README.md.

Bug: 1317494, 1349308
Change-Id: I24f1231c7b45762f99315f4c457b287fe1caada4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3804328
Reviewed-by: Kouhei Ueno <kou...@chromium.org>
Commit-Queue: Ken Okada <ken...@chromium.org>
Reviewed-by: Takashi Toyoshima <toyo...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032878}
---
M chrome/browser/page_load_metrics/observers/README.md
M chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
M components/page_load_metrics/browser/BUILD.gn
A components/page_load_metrics/browser/observers/assert_page_load_metrics_observer.cc
A components/page_load_metrics/browser/observers/assert_page_load_metrics_observer.h
M components/page_load_metrics/browser/page_load_metrics_observer_interface.h
M components/page_load_metrics/browser/page_load_metrics_update_dispatcher.cc
M components/page_load_metrics/browser/page_load_metrics_update_dispatcher.h
M components/page_load_metrics/browser/page_load_tracker.cc
M tools/metrics/histograms/enums.xml
10 files changed, 588 insertions(+), 25 deletions(-)

To view, visit change 3804328. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward