New issue
Advanced search Search tips

Issue 716565 link

Starred by 6 users
Status: Fixed
Owner:
loonyb...@chromium.org
Closed: Apr 2018
Cc:
lfg@chromium.org
nhiroki@chromium.org
hartma...@chromium.org
phistuck@gmail.com
lukasza@chromium.org
dcheng@chromium.org
alex...@chromium.org
wfh@chromium.org
creis@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocked on:
issue 819226
issue 376039
issue 759394
issue 811948
Blocking:
issue 811948



Sign in to add a comment

UseCounter won't work correctly with OOPIF

Project Member Reported by rbyers@chromium.org, Apr 28 2017 
Blink's UseCounter aims to record which blink features were used for each page load (scoped to a 'Page' instance in blink).

As currently implemented I believe OOPIF will result in a skew in our metrics - with each renderer being considered it's own "page load".  I.e. PageVisit counts will be inflated, and "% of PageVisit" ratios will go down for features used in some but not all renderers for a page.

I'm not sure if this is an issue for OOPIF extensions today. lfg@ can you point me to some details of the differences (eg. usage of "Page") and a demo I can try?

But when we launch TDI or general OOPIF this seems likely to become a significant problem with our metrics.

Perhaps we need to implement a counter-forwarding scheme similar to what nhiroki@ did for workers in  issue 376039 ?  However we'll need to be careful to limit the number of IPCs (potentially an existing issue in the worker case).

Comment 1 by lukasza@chromium.org, Apr 28 2017

Cc: lukasza@chromium.org alex...@chromium.org dcheng@chromium.org
 Issue 710637  has been merged into this issue.

Comment 2 by lfg@chromium.org, Apr 28 2017 

I'm not sure to what extent things will be broken. We only count page visits when a local main frame commits -- and the main frame never commits on an OOPIF subframe process. Also, OOPIF subframes won't survive a main frame navigation, so it shouldn't be a problem that UseCounter features aren't cleaned up for OOPIF renderers.

However, we will be inflating things that should be counted only once per page load, like mentioned in  issue 710637 .

You can try OOPIF by launching chrome with --site-per-process and navigating to http://csreis.github.io/tests/cross-site-iframe-simple.html.

Comment 3 by rbyers@chromium.org, Apr 28 2017 

Right, thanks - PageVisits won't be inflated, only counts inside of OOPIF frames which also occur in other renderers.

I wonder if we should really be doing the histogram generation in the browser process...

Comment 4 by lfg@chromium.org, Apr 28 2017 

Doing the histogram generation on the browser process would fix those issues, but I don't think it's a simple refactor -- things that are done programatically are pretty hard to be aggregated properly in the browser process, we would have to go on a case-by-case basis.

For example, if you call UseCounter::HasRecordedMeasurement before recording a measure, aggregating in the browser process we would need a different way of counting, not just adding everything up.

Comment 5 by rbyers@chromium.org, Apr 29 2017 

Yes, exactly.  If we decide to take that route then the simplest way is probably for the renderer to send an RPC to the browser the first time each feature is used (using an extra copy of the BitVectors in the renderer).  Proving there's no race condition around when the bits are reset might be tricky.  

If we decide that's an unnecessary amount of RPCs (perhaps a few hundred on some pages) then we could perhaps try batching them like the PageLoadMetrics (where updates are throttled to 1-per-second).

Comment 7 by rbyers@chromium.org, May 2 2017

Owner: rbyers@chromium.org
Status: Assigned (was: Available)
On me for now to come up with a design proposal.
Project Member

Comment 8 by bugdroid1@chromium.org, May 29 2017 

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

commit 64819bf38f1754e6677f2be17335a2b8f8103060
Author: rbyers <rbyers@chromium.org>
Date: Mon May 29 01:08:36 2017

Make UseCounter take a LocaFrame instead of any Frame

It doesn't make sense for a UseCounter to be triggered by a RemoteFrame.
Update all the callers to provide a LocalFrame (or in a couple cases
where we can't be sure to have one, skip reporting usage).

In a future patch I'd like to start depending on the fact that we know
the correct LocalFrame for a UseCounter invocation.

BUG= 716565 

Review-Url: https://codereview.chromium.org/2877893002
Cr-Commit-Position: refs/heads/master@{#475268}

[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/events/TouchEvent.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/frame/DOMWindow.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/frame/Frame.h
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/frame/LocalFrame.h
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/frame/UseCounter.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/loader/MixedContentChecker.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/loader/MixedContentChecker.h
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/loader/private/FrameClientHintsPreferencesContext.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/loader/private/FrameClientHintsPreferencesContext.h
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/page/AutoscrollController.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/64819bf38f1754e6677f2be17335a2b8f8103060/third_party/WebKit/Source/web/WebViewImpl.cpp

Comment 9 by lunalu@chromium.org, Jun 12 2017

Cc: -lunalu@chromium.org loonyb...@chromium.org
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 4 2017 

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

commit f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4
Author: Luna Lu <loonybear@chromium.org>
Date: Tue Jul 04 23:19:53 2017

Move UseCounter WebFeature definition into mojom

This enables sending UseCounter WebFeature through IPC:
https://chromium-review.googlesource.com/c/550520/

This CL also includes some optimization work in EventTarget.

Bug:  716565 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_dbg_ng
Change-Id: I156ffb951d6a2868e9757063a91fee97fdf8008b

TBR=rockot@chromium.org

Change-Id: I156ffb951d6a2868e9757063a91fee97fdf8008b
Reviewed-on: https://chromium-review.googlesource.com/555950
Commit-Queue: Luna Lu <loonybear@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Rick Byers <rbyers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484164}
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/build/check_gn_headers_whitelist.txt
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/third_party/WebKit/Source/core/events/EventTarget.cpp
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/third_party/WebKit/Source/core/events/EventTarget.h
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/third_party/WebKit/Source/core/frame/UseCounter.cpp
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/third_party/WebKit/Source/core/testing/WorkerInternals.cpp
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/third_party/WebKit/public/BUILD.gn
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/third_party/WebKit/public/platform/PRESUBMIT.py
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/third_party/WebKit/public/platform/WebFeature.h
[add] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/third_party/WebKit/public/platform/web_feature.mojom
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/f41b44927adbcbd4ad3f79cc50b385f95c3d6ce4/tools/metrics/histograms/update_use_counter_feature_enum.py
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 6 2017 

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

commit 352f17b2b6312771cf96bca75780a2b1e9b71af5
Author: Luna Lu <loonybear@chromium.org>
Date: Thu Jul 06 16:09:09 2017

Remove WebFeature.h

Now that we have public/platform/web_feature.mojom, we should be using
web_feature.mojom-shared.h on the browser side, and 
web_feature.mojom-blink.h in Blink. As a result, we can get rid of
WebFeature.h which was previously moved to public/platform to be shared
with the browser side.

Bug:  716565 
Change-Id: I60701517eda2501396a18a663e39c20e17fd4ebc
Reviewed-on: https://chromium-review.googlesource.com/559895
Commit-Queue: Luna Lu <loonybear@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484616}
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/LayoutTests/animations/usecounters/step-middle-keyword-timing-function-deprecated.html
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/LayoutTests/animations/usecounters/step-middle-timing-function-deprecated.html
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/Source/bindings/scripts/v8_utilities.py
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/Source/bindings/tests/results/core/V8TestConstants.cpp
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor.cpp
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp
[modify] https://crrev.com/352f17b2b6312771cf96bca75780a2b1e9b71af5/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp
[delete] https://crrev.com/5d1513df0539d02b7afc2790c1f54a344529cb2d/third_party/WebKit/public/platform/WebFeature.h

Comment 12 by rbyers@chromium.org, Jul 12 2017

Owner: loonyb...@chromium.org
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 18 2017 

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

commit 1423af088e7fd7b6671a6328329918936e7bd234
Author: Luna Lu <loonybear@chromium.org>
Date: Tue Jul 18 21:40:14 2017

Add a PageLoadMetricsObserver that tracks all observed features and reports a new histogram

For more information, please refer to this doc:

https: //docs.google.com/document/d/1jaUv28xBPrOrkZV9gck9btEKvgM8u_awO83SXraPbQ4/edit?usp=sharing
Bug:  716565 
Change-Id: I040ecc5e44df15768649684b4f5ee8d5e828c209
Reviewed-on: https://chromium-review.googlesource.com/534133
Commit-Queue: Luna Lu <loonybear@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Rick Byers <rbyers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487612}
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/BUILD.gn
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/DEPS
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
[add] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer.cc
[add] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer.h
[add] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/page_load_metrics_observer.h
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/page_load_tracker.cc
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/browser/page_load_metrics/page_load_tracker.h
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/common/BUILD.gn
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/common/page_load_metrics/page_load_metrics.mojom
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/chrome/test/BUILD.gn
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/third_party/WebKit/public/BUILD.gn
[modify] https://crrev.com/1423af088e7fd7b6671a6328329918936e7bd234/tools/metrics/histograms/histograms.xml
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 18 2017 

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

commit 628e9d0922332d94629b3fec38f1a1d274eff042
Author: Alexei Filippov <alph@chromium.org>
Date: Tue Jul 18 22:16:44 2017

Revert "Add a PageLoadMetricsObserver that tracks all observed features and reports a new histogram"

This reverts commit 1423af088e7fd7b6671a6328329918936e7bd234.

Reason for revert: 
Broke build 
https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20ChromeOS/builds/36753

Original change's description:
> Add a PageLoadMetricsObserver that tracks all observed features and reports a new histogram
> 
> For more information, please refer to this doc:
> 
> https: //docs.google.com/document/d/1jaUv28xBPrOrkZV9gck9btEKvgM8u_awO83SXraPbQ4/edit?usp=sharing
> Bug:  716565 
> Change-Id: I040ecc5e44df15768649684b4f5ee8d5e828c209
> Reviewed-on: https://chromium-review.googlesource.com/534133
> Commit-Queue: Luna Lu <loonybear@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Charlie Harrison <csharrison@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Mark Pearson <mpearson@chromium.org>
> Reviewed-by: Rick Byers <rbyers@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487612}

TBR=rbyers@chromium.org,dcheng@chromium.org,mpearson@chromium.org,thakis@chromium.org,bmcquade@chromium.org,csharrison@chromium.org,loonybear@chromium.org

Change-Id: I84cb3780b38511fc84394f81c04c339d0cc3a5fb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  716565 
Reviewed-on: https://chromium-review.googlesource.com/576569
Reviewed-by: Alexei Filippov <alph@chromium.org>
Commit-Queue: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487627}
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/BUILD.gn
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/DEPS
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
[delete] https://crrev.com/eaf08a1176afa708584cf99e6329a168ba75e9da/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer.cc
[delete] https://crrev.com/eaf08a1176afa708584cf99e6329a168ba75e9da/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer.h
[delete] https://crrev.com/eaf08a1176afa708584cf99e6329a168ba75e9da/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/page_load_metrics_observer.h
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/page_load_tracker.cc
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/browser/page_load_metrics/page_load_tracker.h
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/common/BUILD.gn
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/common/page_load_metrics/page_load_metrics.mojom
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/chrome/test/BUILD.gn
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/third_party/WebKit/public/BUILD.gn
[modify] https://crrev.com/628e9d0922332d94629b3fec38f1a1d274eff042/tools/metrics/histograms/histograms.xml
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 27 2017 

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

commit 7b133f7a628429e59d18371456152694f9ab5480
Author: Luna Lu <loonybear@chromium.org>
Date: Thu Jul 27 16:16:47 2017

Plumbing use counter feature usage from render to browser

Upon receiving the features, no actions is taken upon in this CL.
A use counter page load observer will be implemented in this CL
https://chromium-review.googlesource.com/c/534133/

which makes use of the feature usage received on the browser side.

Disabling PageLoadMetricsBrowserTest.BadXhtml due to its flakiness.

Bug:  716565 
Change-Id: I3f319782e383474dcd7a8cf18d4c56e38f292848
Reviewed-on: https://chromium-review.googlesource.com/550520
Commit-Queue: Luna Lu <loonybear@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489958}
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/browser/DEPS
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/common/BUILD.gn
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/common/DEPS
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/common/page_load_metrics/page_load_metrics.mojom
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/renderer/page_load_metrics/fake_page_timing_sender.cc
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/renderer/page_load_metrics/fake_page_timing_sender.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/renderer/page_load_metrics/metrics_render_frame_observer.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/renderer/page_load_metrics/page_timing_metrics_sender.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/chrome/renderer/page_load_metrics/page_timing_sender.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/content/public/renderer/render_frame_observer.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/content/renderer/render_frame_impl.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/third_party/WebKit/Source/core/frame/Deprecation.cpp
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/third_party/WebKit/Source/core/frame/UseCounter.cpp
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/third_party/WebKit/Source/core/frame/UseCounterTest.cpp
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/third_party/WebKit/public/BUILD.gn
[modify] https://crrev.com/7b133f7a628429e59d18371456152694f9ab5480/third_party/WebKit/public/web/WebFrameClient.h

Comment 16 by loonyb...@chromium.org, Jul 27 2017

Cc: -loonyb...@chromium.org hartma...@chromium.org
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 28 2017 

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

commit 4fdef4e6d146f5adb35f761498d2b501ee421172
Author: Luna Lu <loonybear@chromium.org>
Date: Fri Jul 28 18:30:37 2017

Reland "Add a PageLoadMetricsObserver that tracks all observed features and reports a new histogram"

This is a reland of 1423af088e7fd7b6671a6328329918936e7bd234
Original change's description:
> Add a PageLoadMetricsObserver that tracks all observed features and reports a new histogram
>
> For more information, please refer to this doc:
>
> https: //docs.google.com/document/d/1jaUv28xBPrOrkZV9gck9btEKvgM8u_awO83SXraPbQ4/edit?usp=sharing
> Bug:  716565 
> Change-Id: I040ecc5e44df15768649684b4f5ee8d5e828c209
> Reviewed-on: https://chromium-review.googlesource.com/534133
> Commit-Queue: Luna Lu <loonybear@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Charlie Harrison <csharrison@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Mark Pearson <mpearson@chromium.org>
> Reviewed-by: Rick Byers <rbyers@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487612}

TBR=mpearson

Bug:  716565 
Change-Id: I137792c35322876f3bbe936ea5f7a0c715ee05bb
Reviewed-on: https://chromium-review.googlesource.com/577828
Reviewed-by: Luna Lu <loonybear@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Luna Lu <loonybear@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490472}
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/BUILD.gn
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_tester.cc
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_tester.h
[add] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer.cc
[add] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer.h
[add] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/page_load_metrics_observer.h
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/page_load_tracker.cc
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/browser/page_load_metrics/page_load_tracker.h
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/test/BUILD.gn
[add] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/test/data/page_load_metrics/use_counter_features.html
[add] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/test/data/page_load_metrics/use_counter_features_in_iframe.html
[add] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/chrome/test/data/page_load_metrics/use_counter_features_in_iframes.html
[modify] https://crrev.com/4fdef4e6d146f5adb35f761498d2b501ee421172/tools/metrics/histograms/histograms.xml

Comment 18 by rbyers@chromium.org, Aug 28 2017

Blockedon: 759394

Comment 19 by cha...@chromium.org, Sep 8 2017

Components: -Blink>Infra>Predictability Internals>FeatureControl

Comment 20 by loonyb...@chromium.org, Feb 21 2018

Blockedon: 811948

Comment 21 by loonyb...@chromium.org, Feb 21 2018

Status: Started (was: Assigned)

Comment 22 by nasko@chromium.org, Mar 2 2018 

We are aiming to ship full Site Isolation by default in M67. It seems that this bug is  important to fix before that, so can it be prioritized for M67?

Comment 23 by nasko@chromium.org, Mar 2 2018

Labels: Proj-SiteIsolation-LaunchBlocking

Comment 24 by rbyers@chromium.org, Mar 4 2018 

This bug doesn't have direct impact on users / web developers, so I don't think it should block site isolation launch.  Also we're far enough along now that I think it wouldn't be that bad for us (eg. web compat decision  making) if we lost renderer UseCounters (we'd just switch to the experimental browser-side ones Luna added which seem pretty good).  Perhaps the biggest priority here now is getting a browser-side version of the CSS UseCounters.

Comment 25 by loonyb...@chromium.org, Mar 5 2018

Labels: -Proj-SiteIsolation-LaunchBlocking
Removing SiteIsolation-LaunchBlocking. 

Created  Issue 818684  to track moving CSS UseCounters to the browser-side

Comment 26 by loonyb...@chromium.org, Mar 5 2018

Blocking: 811948

Comment 27 by nasko@chromium.org, Mar 5 2018 

Should the usage of renderer-side UseCounters be gated on whether Site Isolation is enabled or not? Or potentially based on presence of OOPIFs?

Comment 28 by loonyb...@chromium.org, Mar 5 2018 

I believe the latter. But eventually it shouldn't matter because we will be moving to the browser-side UseCounter soon (hoping to do so by the end of this week).

Comment 29 by loonyb...@chromium.org, Mar 6 2018

Blockedon: 819226
Project Member

Comment 30 by bugdroid1@chromium.org, Mar 13 2018 

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

commit 398cf0803ed34aaac96e94eff016529d391a4aa3
Author: Luna Lu <loonybear@chromium.org>
Date: Tue Mar 13 21:22:46 2018

Rename UseCounter histograms

OOPIF is going to roll out in M67, this means the blink side UseCounter will no
longer be accurate. Switching to the browser side UseCounter in this CL.

However, there's still existing bugs where the browser sise UseCounter is not
working correctly, tracked in  crbug.com/811948 .

Bug:  811948 ,  716565 
Change-Id: I112f82d5f6b938dce4bbe394413068a8c3472498
Reviewed-on: https://chromium-review.googlesource.com/957472
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Rick Byers <rbyers@chromium.org>
Commit-Queue: Luna Lu <loonybear@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542913}
[modify] https://crrev.com/398cf0803ed34aaac96e94eff016529d391a4aa3/chrome/browser/page_load_metrics/observers/use_counter_page_load_metrics_observer.h
[modify] https://crrev.com/398cf0803ed34aaac96e94eff016529d391a4aa3/chrome/browser/ui/search/local_ntp_browsertest.cc
[modify] https://crrev.com/398cf0803ed34aaac96e94eff016529d391a4aa3/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/398cf0803ed34aaac96e94eff016529d391a4aa3/third_party/WebKit/Source/core/frame/UseCounter.cpp
[modify] https://crrev.com/398cf0803ed34aaac96e94eff016529d391a4aa3/third_party/WebKit/Source/core/frame/UseCounterTest.cpp
[modify] https://crrev.com/398cf0803ed34aaac96e94eff016529d391a4aa3/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp
[modify] https://crrev.com/398cf0803ed34aaac96e94eff016529d391a4aa3/tools/metrics/histograms/histograms.xml

Comment 31 by rbyers@chromium.org, Mar 22 2018 

Once the CSSFeatures and AnimatedCSSFeatures histograms have also been moved, I guess we should call this bug 'Fixed' and track remaining improvements (like issue 819226) seperately, right? Just to make it clear UseCounters DO now work correctly with OOPIF.

Comment 32 by creis@chromium.org, Apr 2 2018

Labels: -OS-All OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
loonybear@ / rbyers@: Per comment 31, can this issue be closed now that r546163 ("Plumb UseCounter histograms for CSS properties and animated CSS properties to the browser side") has landed?

Comment 33 by loonyb...@chromium.org, Apr 3 2018 

Yes I think so.
Though the CSS histograms are moved, we are still using the blink side code for logging the public CSS histograms. I will quickly verify the accuracy of the browser side histograms this week and completely replace the blink side counters by the browser side.

Thanks

Comment 34 by loonyb...@chromium.org, Apr 4 2018

Labels: Merge-Request-66
Ran a quick analysis, most features have accurate usage on the browser side. I will track the correctness of the browser side histograms and finish off the the rest of the work in another issue.

We can mark this as fixed.

Before I close this though, I would like to merge the last commit to 56 if possible:

The last CL replaced the blink implementation (flawn) for UseCounter by the browser side implementation (more accurate!!!).
The last CL only touches internal feature, it does not affect the web.
The reason I would really like to merge this is because the results of UseCounter is shown publicly on chromestatus.com. It is currently wrong. See on https://www.chromestatus.com/metrics/feature/popularity, features are not supposed to have usage over 100%. There are a couple discussions about this in github (https://github.com/GoogleChrome/chromium-dashboard/issues/545#) and on blink-dev. UseCounter stats is used as part of the blink intents process, so the earlier we have accurate stats, the better.

Thanks
Project Member

Comment 35 by sheriffbot@chromium.org, Apr 4 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 36 by cmasso@google.com, Apr 4 2018 

The cl in #30 has a lot of code change. If this is not super critical, please let's just wait to ship it in M67 instead of taking the risk of breaking other stuffs.

Comment 37 by loonyb...@chromium.org, Apr 4 2018

Cc: phistuck@gmail.com
If you take a closer look at the CL, most changes are string (names of UseCounter histograms being switched).

Comment 38 by cmasso@google.com, Apr 5 2018 

What is the overall risk associated with merging this CL? I would like to understand who the users will be affected if this is not merged into M66.

Comment 39 by loonyb...@chromium.org, Apr 6 2018 

I don't think there is any risk to be honest. 
Users will be affected: people who refer to chromestatus.com (https://www.chromestatus.com/metrics/feature/popularity) for feature usage results (this info could also be used as part of the blink intents process) and blink developers who use the stats to understand the features they own on the web.

I understand the timeline is tight and we don't want to risk for anything, so it is ok if we don't merge it to M66 also.

Thanks

Comment 40 by cmasso@google.com, Apr 6 2018

Labels: -Hotlist-Merge-Review -Merge-Review-66 Merge-Rejected-67
Thanks loonybear@. Let's just ship it with M67.

Comment 41 by creis@chromium.org, Apr 13 2018

Status: Fixed (was: Started)
Marking fixed per comment 34, and since the merge question was resolved.
►

Sign in to add a comment