Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(347)

Issue 2914243003: [GRC] GRC service plumbing in Blink. (Closed)

Created:
3 years, 6 months ago by lpy
Modified:
3 years, 6 months ago
CC:
abarth-chromium, blink-reviews, blink-reviews-frames_chromium.org, chrome-grc-reviews_chromium.org, chromium-reviews, kinuko+watch
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[GRC] GRC service plumbing in Blink. This patch hooks Frame-level CoordinationUnit in Blink up to GRC service behind a flag. This patch depends on https://codereview.chromium.org/2913253002 landing first, or there's nothing providing the CoordinationUnit interface in navigation:frame and will fail run-time (if the feature-flag is enabled). An example of full GRC usage can be seen in https://codereview.chromium.org/2710823003 BUG=691886 Review-Url: https://codereview.chromium.org/2914243003 Cr-Commit-Position: refs/heads/master@{#477423} Committed: https://chromium.googlesource.com/chromium/src/+/5c265a308a311a7d65ab37c10cf84830ddea72d5

Patch Set 1 #

Total comments: 8

Patch Set 2 : Updated BUILD.gn and fixed comments #

Total comments: 2

Patch Set 3 : update #

Messages

Total messages: 45 (25 generated)
lpy
PTAL.
3 years, 6 months ago (2017-06-01 23:45:30 UTC) #2
haraken
Looks good. oystein should be a better reviewer for this. https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h (right): https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h#newcode16 ...
3 years, 6 months ago (2017-06-02 04:08:08 UTC) #7
lpy
On 2017/06/02 04:08:08, haraken wrote: > Looks good. oystein should be a better reviewer for ...
3 years, 6 months ago (2017-06-02 04:14:12 UTC) #8
haraken
On 2017/06/02 04:14:12, lpy wrote: > On 2017/06/02 04:08:08, haraken wrote: > > Looks good. ...
3 years, 6 months ago (2017-06-02 04:28:22 UTC) #11
haraken
Implementation-wise LGTM. https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.cpp File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.cpp (right): https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.cpp#newcode22 third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.cpp:22: return base::FeatureList::IsEnabled(features::kGlobalResourceCoordinator); It's fine to use base::FeatureList ...
3 years, 6 months ago (2017-06-02 04:28:30 UTC) #12
oystein (OOO til 10th of July)
The description should probably mention that this is dependent on https://codereview.chromium.org/2913253002 landing first (at least ...
3 years, 6 months ago (2017-06-02 15:32:36 UTC) #16
Primiano Tucci (use gerrit)
RS-LGTM //third_party/WebKit/Source/platform/instrumentation w/ haraken comments addressed
3 years, 6 months ago (2017-06-02 16:36:01 UTC) #17
Primiano Tucci (use gerrit)
On 2017/06/02 16:36:01, Primiano Tucci wrote: > RS-LGTM //third_party/WebKit/Source/platform/instrumentation w/ haraken > comments addressed Ah ...
3 years, 6 months ago (2017-06-02 16:37:15 UTC) #18
lpy
> https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinator/public/interfaces/BUILD.gn > File services/resource_coordinator/public/interfaces/BUILD.gn (right): > > https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinator/public/interfaces/BUILD.gn#newcode9 > services/resource_coordinator/public/interfaces/BUILD.gn:9: visibility_blink = [ > ...
3 years, 6 months ago (2017-06-02 16:52:53 UTC) #19
oystein (OOO til 10th of July)
On 2017/06/02 at 16:52:53, lpy wrote: > > https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinator/public/interfaces/BUILD.gn > > File services/resource_coordinator/public/interfaces/BUILD.gn (right): > ...
3 years, 6 months ago (2017-06-02 16:57:01 UTC) #20
Ken Rockot(use gerrit already)
https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinator/public/interfaces/BUILD.gn File services/resource_coordinator/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinator/public/interfaces/BUILD.gn#newcode9 services/resource_coordinator/public/interfaces/BUILD.gn:9: visibility_blink = [ "//third_party/WebKit/Source/platform/*" ] On 2017/06/02 at 15:32:36, ...
3 years, 6 months ago (2017-06-02 17:01:23 UTC) #21
oystein (OOO til 10th of July)
On 2017/06/02 at 17:01:23, rockot wrote: > https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinator/public/interfaces/BUILD.gn > File services/resource_coordinator/public/interfaces/BUILD.gn (right): > > https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinator/public/interfaces/BUILD.gn#newcode9 ...
3 years, 6 months ago (2017-06-02 17:13:17 UTC) #22
lpy
Thanks, I updated the patch, ptal. https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinator/public/interfaces/BUILD.gn File services/resource_coordinator/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinator/public/interfaces/BUILD.gn#newcode9 services/resource_coordinator/public/interfaces/BUILD.gn:9: visibility_blink = [ ...
3 years, 6 months ago (2017-06-02 20:23:52 UTC) #24
oystein (OOO til 10th of July)
lgtm with a nit, thanks! https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS (right): https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS#newcode2 third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS:2: "+services/resource_coordinator/public/cpp", Just do "services/resource_coordinator/public" ...
3 years, 6 months ago (2017-06-02 20:44:50 UTC) #27
lpy
Thanks, I updated DEPS, will land it when https://codereview.chromium.org/2913253002 landed. https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS (right): https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS#newcode2 ...
3 years, 6 months ago (2017-06-02 23:16:42 UTC) #32
oystein (OOO til 10th of July)
On 2017/06/02 at 23:16:42, lpy wrote: > Thanks, I updated DEPS, will land it when ...
3 years, 6 months ago (2017-06-06 17:56:25 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2914243003/40001
3 years, 6 months ago (2017-06-06 18:02:20 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/231703)
3 years, 6 months ago (2017-06-06 19:22:47 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2914243003/40001
3 years, 6 months ago (2017-06-06 21:18:16 UTC) #42
commit-bot: I haz the power
3 years, 6 months ago (2017-06-06 21:51:25 UTC) #45
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5c265a308a311a7d65ab37c10cf8...

Powered by Google App Engine
This is Rietveld 408576698