Shunya Shishido (Gerrit)

unread,

2:03 AM (22 hours ago) 2:03 AM

to Yoshisato Yanagisawa, Yoshisato Yanagisawa, Kouhei Ueno, Minoru Chikamune, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, creis...@chromium.org, navigation...@chromium.org

Attention needed from Kouhei Ueno and Minoru Chikamune

Shunya Shishido added 1 comment

Commit Message

Line 7, Patchset 5:Add breakdown metrics for BeginNavigatiion

Yoshisato Yanagisawa . resolved

nit but Navigation.
You can omit one of two 'i's.

Shunya Shishido

Oh! Thanks for pointing it out. Fixed.

Open in Gerrit

Related details

Attention is currently required from:

Kouhei Ueno
Minoru Chikamune

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Minoru Chikamune (Gerrit)

unread,

2:11 AM (22 hours ago) 2:11 AM

to Shunya Shishido, Minoru Chikamune, Yoshisato Yanagisawa, Yoshisato Yanagisawa, Kouhei Ueno, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, creis...@chromium.org, navigation...@chromium.org

Attention needed from Kouhei Ueno and Shunya Shishido

Minoru Chikamune voted and added 2 comments

Votes added by Minoru Chikamune

Code-Review

+1

2 comments

File content/browser/web_contents/web_contents_impl.cc

Line 6511, Patchset 5:

  base::ElapsedTimer duration;
  {
    SCOPED_UMA_HISTOGRAM_TIMER("WebContentsObserver.DidStartNavigation");
    observers_.NotifyObservers(&WebContentsObserver::DidStartNavigation,
                               navigation_handle);
  }
  base::UmaHistogramTimes(
      base::StrCat(
          {"WebContentsObserver.DidStartNavigation.",
           navigation_handle->IsInMainFrame() ? "MainFrame" : "Subframe"}),
      duration.Elapsed());

Minoru Chikamune . unresolved

nit: Perhaps it's better to use one ElapsedTimer.
```
  base::ElapsedTimer duration;
  observers_.NotifyObservers(&WebContentsObserver::DidStartNavigation,
                             navigation_handle);
  base::TimeDelta elapsed = duration.Elapsed();
  base::UmaHistogramTimes("WebContentsObserver.DidStartNavigation", elapsed);
  base::UmaHistogramTimes(
      base::StrCat(
          {"WebContentsObserver.DidStartNavigation.",
           navigation_handle->IsInMainFrame() ? "MainFrame" : "Subframe"}),
      elapsed);
```

File tools/metrics/histograms/metadata/others/histograms.xml

Line 9934, Patchset 4: expires_after="2024-12-02">

Minoru Chikamune . resolved

[WARNING] This histogram is expired for more than a month. It might have already stopped reporting. If you're extending this histogram, please be careful of data discontinuity: https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#extending.
Please fix.
Actually the fllowing two entries are duplicated.
WebContentsObserver.DidStartNavigation.{FrameType}
WebContentsObserver.{Method}
I think you need to merge them into one such as:
WebContentsObserver.{Method}{FrameType}
And define FrameType like the following.
```
  <token key="FrameType">
    <variant name=".MainFrame"/>
    <variant name=".Subframe"/>
  </token>
```

Shunya Shishido

Thanks. Merged them into one entry. Also added `<variant name="" summary="all frames"/>` to`FrameType` because FrameType based breakdown UMAs are reported only for DidStartNavigation.

Minoru Chikamune

Thank you. LGTM.

Open in Gerrit

Related details

Attention is currently required from:

Kouhei Ueno
Shunya Shishido

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Shunya Shishido (Gerrit)

unread,

2:20 AM (22 hours ago) 2:20 AM

to Minoru Chikamune, Yoshisato Yanagisawa, Yoshisato Yanagisawa, Kouhei Ueno, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, creis...@chromium.org, navigation...@chromium.org

Attention needed from Kouhei Ueno

Shunya Shishido added 1 comment

File content/browser/web_contents/web_contents_impl.cc

Line 6511, Patchset 5:

  base::ElapsedTimer duration;
  {
    SCOPED_UMA_HISTOGRAM_TIMER("WebContentsObserver.DidStartNavigation");
    observers_.NotifyObservers(&WebContentsObserver::DidStartNavigation,
                               navigation_handle);
  }
  base::UmaHistogramTimes(
      base::StrCat(
          {"WebContentsObserver.DidStartNavigation.",
           navigation_handle->IsInMainFrame() ? "MainFrame" : "Subframe"}),
      duration.Elapsed());

Minoru Chikamune . resolved

nit: Perhaps it's better to use one ElapsedTimer.
```
  base::ElapsedTimer duration;
  observers_.NotifyObservers(&WebContentsObserver::DidStartNavigation,
                             navigation_handle);
  base::TimeDelta elapsed = duration.Elapsed();
  base::UmaHistogramTimes("WebContentsObserver.DidStartNavigation", elapsed);
  base::UmaHistogramTimes(
      base::StrCat(
          {"WebContentsObserver.DidStartNavigation.",
           navigation_handle->IsInMainFrame() ? "MainFrame" : "Subframe"}),
      elapsed);
```

Shunya Shishido

Done

Open in Gerrit

Related details

Attention is currently required from:

Kouhei Ueno

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Shunya Shishido (Gerrit)

unread,

2:21 AM (21 hours ago) 2:21 AM

to Takashi Toyoshima, Rakina Zata Amni, Minoru Chikamune, Yoshisato Yanagisawa, Yoshisato Yanagisawa, Kouhei Ueno, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, creis...@chromium.org, navigation...@chromium.org

Attention needed from Kouhei Ueno, Rakina Zata Amni and Takashi Toyoshima

Shunya Shishido added 1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Shunya Shishido . resolved

PTAL, thanks!

Open in Gerrit

Related details

Attention is currently required from:

Kouhei Ueno
Rakina Zata Amni
Takashi Toyoshima

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Takashi Toyoshima (Gerrit)

unread,

9:15 PM (3 hours ago) 9:15 PM

to Shunya Shishido, Rakina Zata Amni, Minoru Chikamune, Yoshisato Yanagisawa, Yoshisato Yanagisawa, Kouhei Ueno, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, alexmo...@chromium.org, asvitkine...@chromium.org, creis...@chromium.org, navigation...@chromium.org

Attention needed from Kouhei Ueno, Rakina Zata Amni and Shunya Shishido

Takashi Toyoshima voted and added 3 comments

Votes added by Takashi Toyoshima

Code-Review

+1

3 comments

File content/browser/renderer_host/navigation_throttle_runner.cc

Line 147, Patchset 7 (Latest): bool is_primary_main_frame_for_uma = is_primary_main_frame_;

Takashi Toyoshima . unresolved

nit: const bool

File tools/metrics/histograms/metadata/navigation/histograms.xml

Line 1516, Patchset 7 (Latest): <summary>

Takashi Toyoshima . unresolved

Can you give the summary attributes to the FrameTypes variants definition, and embed the {FrameType} in the summary so that the description can explain the variants?

Line 1562, Patchset 7 (Latest): Measures the time taken to register navigation throttles in

Takashi Toyoshima . unresolved

ditto

Open in Gerrit

Related details

Attention is currently required from:

Kouhei Ueno
Rakina Zata Amni

Shunya Shishido

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Add breakdown metrics for BeginNavigation [chromium/src : main]

Shunya Shishido (Gerrit)

Shunya Shishido added 1 comment

Related details

Minoru Chikamune (Gerrit)

Minoru Chikamune voted and added 2 comments

Votes added by Minoru Chikamune

2 comments

Related details

Shunya Shishido (Gerrit)

Shunya Shishido added 1 comment

Related details

Shunya Shishido (Gerrit)

Shunya Shishido added 1 comment

Related details

Takashi Toyoshima (Gerrit)

Takashi Toyoshima voted and added 3 comments

Votes added by Takashi Toyoshima

3 comments

Related details