|
|
Created:
3 years, 6 months ago by lpy Modified:
3 years, 6 months ago Reviewers:
haraken, oystein (OOO til 10th of July), Primiano Tucci (use gerrit), Ken Rockot(use gerrit already) 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@chromium.org changed reviewers: + haraken@chromium.org, oysteine@chromium.org, primiano@chromium.org
PTAL.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looks good. oystein should be a better reviewer for this. https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h (right): https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h:16: : public GarbageCollectedFinalized<FrameResourceCoordinator> { What functionalities will be added to FrameResourceCoordinator? I'm curious because platform/ cannot use core/. LocalFrame is in core/.
On 2017/06/02 04:08:08, haraken wrote: > Looks good. oystein should be a better reviewer for this. I need you review as platform/ owner :) > https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... > File > third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h > (right): > > https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h:16: > : public GarbageCollectedFinalized<FrameResourceCoordinator> { > > What functionalities will be added to FrameResourceCoordinator? I'm curious > because platform/ cannot use core/. LocalFrame is in core/. I see, I think FrameResourceCoordinator would be a place to send our collected signals to grc. If I understand correctly, we will collect those signals by LocalFrame, for example when network is quiet.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/02 04:14:12, lpy wrote: > On 2017/06/02 04:08:08, haraken wrote: > > Looks good. oystein should be a better reviewer for this. > > I need you review as platform/ owner :) > > > > https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... > > File > > > third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h > > (right): > > > > > https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... > > > third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h:16: > > : public GarbageCollectedFinalized<FrameResourceCoordinator> { > > > > What functionalities will be added to FrameResourceCoordinator? I'm curious > > because platform/ cannot use core/. LocalFrame is in core/. > > I see, I think FrameResourceCoordinator would be a place to send our collected > signals to grc. If I understand correctly, we will collect those signals by > LocalFrame, for example when network is quiet. So you're trying to call methods on FrameResourceCoordinator from LocalFrame. That sounds reasonable to me.
Implementation-wise LGTM. https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.cpp (right): https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.cpp:22: return base::FeatureList::IsEnabled(features::kGlobalResourceCoordinator); It's fine to use base::FeatureList in platform/ but could you add it to platform/DEPS/? You're the first user of base::FeatureList in Blink. https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h (right): https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h:15: class PLATFORM_EXPORT FrameResourceCoordinator final Add a class-level comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
oysteine@chromium.org changed reviewers: + rockot@chromium.org
The description should probably mention that this is dependent on https://codereview.chromium.org/2913253002 landing first (at least until it does), or there's nothing providing the CoordinationUnit interface in navigation:frame and this will fail run-time (if the feature-flag is enabled). https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... File services/resource_coordinator/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... services/resource_coordinator/public/interfaces/BUILD.gn:9: visibility_blink = [ "//third_party/WebKit/Source/platform/*" ] I don't think this is right. The reason for wrapping the Mojo interfaces together with the public/cpp stuff is so the whole service can be built as a component, due to the use of statics in the clientlib. We're depending on //services/resource_coordinator/public/cpp:resource_coordinator_cpp anyway, can't that component internally depend on //services/resource_coordinator/public/interfaces:interfaces_internal_blink ? +rockot who knows this service component build magic :P.
RS-LGTM //third_party/WebKit/Source/platform/instrumentation w/ haraken comments addressed
On 2017/06/02 16:36:01, Primiano Tucci wrote: > RS-LGTM //third_party/WebKit/Source/platform/instrumentation w/ haraken > comments addressed Ah also please consider adding a short README.md which explains what is this resource_coordinator subfolder, how blink is supposed to use this and what is the relationship with //services/resource_coordinator .
> https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... > File services/resource_coordinator/public/interfaces/BUILD.gn (right): > > https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... > services/resource_coordinator/public/interfaces/BUILD.gn:9: visibility_blink = [ > "//third_party/WebKit/Source/platform/*" ] > I don't think this is right. The reason for wrapping the Mojo interfaces > together with the public/cpp stuff is so the whole service can be built as a > component, due to the use of statics in the clientlib. > > We're depending on > //services/resource_coordinator/public/cpp:resource_coordinator_cpp anyway, > can't that component internally depend on > //services/resource_coordinator/public/interfaces:interfaces_internal_blink ? > > +rockot who knows this service component build magic :P. yeah even by adding interfaces_internal_blink into resource_coordinator_cpp still result to undefined reference, not sure what I have missed though
On 2017/06/02 at 16:52:53, lpy wrote: > > https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... > > File services/resource_coordinator/public/interfaces/BUILD.gn (right): > > > > https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... > > services/resource_coordinator/public/interfaces/BUILD.gn:9: visibility_blink = [ > > "//third_party/WebKit/Source/platform/*" ] > > I don't think this is right. The reason for wrapping the Mojo interfaces > > together with the public/cpp stuff is so the whole service can be built as a > > component, due to the use of statics in the clientlib. > > > > We're depending on > > //services/resource_coordinator/public/cpp:resource_coordinator_cpp anyway, > > can't that component internally depend on > > //services/resource_coordinator/public/interfaces:interfaces_internal_blink ? > > > > +rockot who knows this service component build magic :P. > > yeah even by adding interfaces_internal_blink into resource_coordinator_cpp still result to undefined reference, not sure what I have missed though I'm guessing that the current setup works fine as long as nothing in the Blink component uses the GRC clientlib, which is fine for this CL but would be nice to fix. rockot@ mentioned something about services becoming components by default though, which would eliminate the need for the magic, but I don't know the status on that. Awaiting commenting :).
https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... File services/resource_coordinator/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... services/resource_coordinator/public/interfaces/BUILD.gn:9: visibility_blink = [ "//third_party/WebKit/Source/platform/*" ] On 2017/06/02 at 15:32:36, oystein wrote: > I don't think this is right. The reason for wrapping the Mojo interfaces together with the public/cpp stuff is so the whole service can be built as a component, due to the use of statics in the clientlib. > > We're depending on //services/resource_coordinator/public/cpp:resource_coordinator_cpp anyway, can't that component internally depend on //services/resource_coordinator/public/interfaces:interfaces_internal_blink ? > Blink and non-blink variant target outputs are totally separate. So your export_* settings below only apply to the non-blink version. If you depend on interfaces_internal_blink from your cpp component, it wouldn't export the blink mojom symbols. Now you could do that and add export_class_attribute_blink etc with the same values. Then you would of course also get rid of the interfaces_internal_blink dependency this CL is adding in Blink. Another option would be to stop using this component magic and use the other component magic, which makes your interfaces targets (e.g. "interfaces" and "interfaces_blink") components themselves. i.e. mojom_component("interfaces") { sources = [ ... ] output_prefix = "resource_coordinator_interfaces" macro_prefix = "RESOURCE_COORDINATOR_INTERFACES" } The trade-off (isn't there always a trade-off) with this new approach is that you have to split your client library into two parts to avoid circular deps: - a component for any types that are typemapped from mojom. This can't depend on the mojom itself obviously (the mojom depends on it via typemap) - a component for everything else, which depends on both the first component and the mojom target(s) as needed > +rockot who knows this service component build magic :P.
On 2017/06/02 at 17:01:23, rockot wrote: > https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... > File services/resource_coordinator/public/interfaces/BUILD.gn (right): > > https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... > services/resource_coordinator/public/interfaces/BUILD.gn:9: visibility_blink = [ "//third_party/WebKit/Source/platform/*" ] > On 2017/06/02 at 15:32:36, oystein wrote: > > I don't think this is right. The reason for wrapping the Mojo interfaces together with the public/cpp stuff is so the whole service can be built as a component, due to the use of statics in the clientlib. > > > > We're depending on //services/resource_coordinator/public/cpp:resource_coordinator_cpp anyway, can't that component internally depend on //services/resource_coordinator/public/interfaces:interfaces_internal_blink ? > > > > Blink and non-blink variant target outputs are totally separate. So your export_* settings below only apply to the non-blink version. If you depend on interfaces_internal_blink from your cpp component, it wouldn't export the blink mojom symbols. > > Now you could do that and add export_class_attribute_blink etc with the same values. Then you would of course also get rid of the interfaces_internal_blink dependency this CL is adding in Blink. > > Another option would be to stop using this component magic and use the other component magic, which makes your interfaces targets (e.g. "interfaces" and "interfaces_blink") components themselves. i.e. > > mojom_component("interfaces") { > sources = [ ... ] > output_prefix = "resource_coordinator_interfaces" > macro_prefix = "RESOURCE_COORDINATOR_INTERFACES" > } > > The trade-off (isn't there always a trade-off) with this new approach is that you have to split your client library into two parts to avoid circular deps: > - a component for any types that are typemapped from mojom. This can't depend on the mojom itself obviously (the mojom depends on it via typemap) > - a component for everything else, which depends on both the first component and the mojom target(s) as needed > > Thanks! The first option sounds less painful to me. > > +rockot who knows this service component build magic :P.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Thanks, I updated the patch, ptal. https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... File services/resource_coordinator/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2914243003/diff/1/services/resource_coordinat... services/resource_coordinator/public/interfaces/BUILD.gn:9: visibility_blink = [ "//third_party/WebKit/Source/platform/*" ] On 2017/06/02 15:32:36, oystein wrote: > I don't think this is right. The reason for wrapping the Mojo interfaces > together with the public/cpp stuff is so the whole service can be built as a > component, due to the use of statics in the clientlib. > > We're depending on > //services/resource_coordinator/public/cpp:resource_coordinator_cpp anyway, > can't that component internally depend on > //services/resource_coordinator/public/interfaces:interfaces_internal_blink ? > > +rockot who knows this service component build magic :P. Done. https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.cpp (right): https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.cpp:22: return base::FeatureList::IsEnabled(features::kGlobalResourceCoordinator); On 2017/06/02 04:28:30, haraken wrote: > > It's fine to use base::FeatureList in platform/ but could you add it to > platform/DEPS/? > > You're the first user of base::FeatureList in Blink. Done. https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h (right): https://codereview.chromium.org/2914243003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/instrumentation/resource_coordinator/FrameResourceCoordinator.h:16: : public GarbageCollectedFinalized<FrameResourceCoordinator> { On 2017/06/02 04:08:08, haraken wrote: > > What functionalities will be added to FrameResourceCoordinator? I'm curious > because platform/ cannot use core/. LocalFrame is in core/. > Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [GRC] GRC service plumbing in Blink. This patch hooks Frame-level CoordinationUnit in Blink up to GRC service behind a flag. An example of full GRC usage can be seen in https://codereview.chromium.org/2710823003 BUG=691886 ========== to ========== [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 ==========
lgtm with a nit, thanks! https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS (right): https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS:2: "+services/resource_coordinator/public/cpp", Just do "services/resource_coordinator/public" (clientlib folders are always just split up into /cpp and /interfaces anyway).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I updated DEPS, will land it when https://codereview.chromium.org/2913253002 landed. https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS (right): https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS:2: "+services/resource_coordinator/public/cpp", On 2017/06/02 20:44:50, oystein wrote: > Just do "services/resource_coordinator/public" (clientlib folders are always > just split up into /cpp and /interfaces anyway). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/06/02 at 23:16:42, lpy wrote: > Thanks, I updated DEPS, will land it when https://codereview.chromium.org/2913253002 landed. > > https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS (right): > > https://codereview.chromium.org/2914243003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/instrumentation/resource_coordinator/DEPS:2: "+services/resource_coordinator/public/cpp", > On 2017/06/02 20:44:50, oystein wrote: > > Just do "services/resource_coordinator/public" (clientlib folders are always > > just split up into /cpp and /interfaces anyway). > > Done. Dependent CL has landed, should be able to land this now.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org, oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/2914243003/#ps40001 (title: "update")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/bui...)
The CQ bit was checked by lpy@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496783870707530, "parent_rev": "a60f59beaa452df7e603b52bbdbfe733fec449a0", "commit_rev": "5c265a308a311a7d65ab37c10cf84830ddea72d5"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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/+/5c265a308a311a7d65ab37c10cf8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5c265a308a311a7d65ab37c10cf8... |