Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2535483002:
Plumb site engagement to the renderer process. (Closed)
|
Created:
4 years ago by dominickn Modified:
4 years ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, dominickn+watch_chromium.org, eae+blinkwatch, jam, kinuko+watch, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, rwlbuis, sof, tfarina, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlumb site engagement to the renderer process.
This CL generalises the EngagementLevel concept in site engagement,
moving it to a Mojo enum in Blink's public/platform. Prior to each
navigation commit, and when the engagement level for an origin changes
between buckets, the browser process sends the new level to all renderers
which are navigated to that origin. Using the discrete levels rather
than the continuous score minimises the IPC overhead to one additional
IPC per navigation commit per frame, and one IPC per engagement level
change.
The engagement level is stored on Document within the renderer, with
a temporary stop on RenderFrameImpl if sent prior to navigation commit.
This is a first step to enabling the collection of UseCounter data for
JavaScript API usage against engagement buckets.
BUG=655415
Committed: https://crrev.com/6c1f1cf3c31b4c798708cb17d4ecd8e6db37e833
Cr-Commit-Position: refs/heads/master@{#439726}
Patch Set 1 #Patch Set 2 : Rebase. Fix Win compile #
Total comments: 6
Patch Set 3 : Address comments #
Total comments: 10
Patch Set 4 : Address comments #
Total comments: 18
Patch Set 5 : Address comments #
Total comments: 9
Patch Set 6 : Redesign helper #Patch Set 7 : Back to previous impl #Patch Set 8 : Rebase #Messages
Total messages: 93 (58 generated)
The CQ bit was checked by dominickn@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dominickn@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by dominickn@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...
Description was changed from ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojom enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process send the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is on Document within the renderer, with a temporary stop on RenderFrameImpl. This is a first step to enabling collecting UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 ========== to ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojom enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process send the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is on Document within the renderer, with a temporary stop on RenderFrameImpl. This is a first step to enabling collecting UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 ==========
dominickn@chromium.org changed reviewers: + benwells@chromium.org, dcheng@chromium.org, esprehn@chromium.org, haraken@chromium.org
dominickn@chromium.org changed reviewers: + nasko@chromium.org
benwells: PTAL at engagement/ dcheng: mojo security review esprehn, haraken: Blink changes nasko: content/, and sanity checking the navigation logic (I'm not 100% sure I have it right) Thanks everyone!
Description was changed from ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojom enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process send the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is on Document within the renderer, with a temporary stop on RenderFrameImpl. This is a first step to enabling collecting UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 ========== to ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojo enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process sends the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is stored on Document within the renderer, with a temporary stop on RenderFrameImpl if sent prior to navigation commit. This is a first step to enabling collecting UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
An initial round of review of content/. The main suggestion is to associate the engagement level with origin and always send the pair. https://codereview.chromium.org/2535483002/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/2535483002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:225: if (origin == render_frame_host->GetLastCommittedURL()) { Why not GetLastCommittedOrigin? Comparing an origin to URL doesn't seem correct. https://codereview.chromium.org/2535483002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:3597: frame_->setEngagementLevel(engagement_level_); I would feel more comfortable if this is only set if the origin of the committed document matches the origin associated with the engagement level. https://codereview.chromium.org/2535483002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/site_engagement.mojom (right): https://codereview.chromium.org/2535483002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/site_engagement.mojom:18: SetEngagementLevel(EngagementLevel level); I would suggest adding the URL as an explicit parameter, as navigations are inherently a racy part of the browser, so the idea of "current URL" can mismatch between processes. Having an explicit URL allows the receiver to verify it. The HostZoom level works in a similar way, if you are looking for equivalent feature.
The CQ bit was checked by dominickn@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/2535483002/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/2535483002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:225: if (origin == render_frame_host->GetLastCommittedURL()) { On 2016/11/28 19:18:08, nasko wrote: > Why not GetLastCommittedOrigin? Comparing an origin to URL doesn't seem correct. Done. We have a bunch of type conversion for now because site engagement doesn't yet use url::Origin internally. https://codereview.chromium.org/2535483002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:3597: frame_->setEngagementLevel(engagement_level_); On 2016/11/28 19:18:08, nasko wrote: > I would feel more comfortable if this is only set if the origin of the committed > document matches the origin associated with the engagement level. Done. https://codereview.chromium.org/2535483002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/site_engagement.mojom (right): https://codereview.chromium.org/2535483002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/site_engagement.mojom:18: SetEngagementLevel(EngagementLevel level); On 2016/11/28 19:18:08, nasko wrote: > I would suggest adding the URL as an explicit parameter, as navigations are > inherently a racy part of the browser, so the idea of "current URL" can mismatch > between processes. > Having an explicit URL allows the receiver to verify it. The HostZoom level > works in a similar way, if you are looking for equivalent feature. Done.
Description was changed from ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojo enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process sends the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is stored on Document within the renderer, with a temporary stop on RenderFrameImpl if sent prior to navigation commit. This is a first step to enabling collecting UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 ========== to ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojo enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process sends the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is stored on Document within the renderer, with a temporary stop on RenderFrameImpl if sent prior to navigation commit. This is a first step to enabling the collection of UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 ==========
https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:2669: } This seems like lots of complex logic. Why can't we just simply ask the frame for the origin of its current security context? Between always setting the level if the frame current origin matches or saving it otherwise and always setting it at commit time if found in the map, it should cover all the cases. Let me know if I've missed some. https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:2672: engagement_levels_[origin] = level; Do we need to store the level in the map if it was already set on the frame? https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4763: engagement_levels_.find(url::Origin(GURL(request.url()))); Can't we use the frame's origin at this point? Why go through this elaborate conversion, which actually is lossy too? https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4767: engagement_levels_.erase(engagement_level); The map should never grow big, right? Maybe add some form of a (D)CHECK to ensure we don't leak? Or alternatively purge the map at commit. https://codereview.chromium.org/2535483002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/site_engagement.mojom (right): https://codereview.chromium.org/2535483002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/site_engagement.mojom:19: // The browser tells the renderer the engagement level of the current URL. nit: Let's avoid using the word "current" as it is not very clear. Also, this API can probably be called with arbitrary origin, so there is no enforcement that the origin is the current one. Last, the parameter is an origin, not an URL :).
The CQ bit was checked by dominickn@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! Appreciating the content expertise. :) https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:2669: } On 2016/11/30 18:00:25, nasko wrote: > This seems like lots of complex logic. Why can't we just simply ask the frame > for the origin of its current security context? > > Between always setting the level if the frame current origin matches or saving > it otherwise and always setting it at commit time if found in the map, it should > cover all the cases. Let me know if I've missed some. I'm definitely not an expert on this (hence the complexity!), so I can't confirm if this covers all the cases. But I'm more than happy to defer. :) https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:2672: engagement_levels_[origin] = level; On 2016/11/30 18:00:25, nasko wrote: > Do we need to store the level in the map if it was already set on the frame? I don't think so - that's why it returns immediately after setting it on the frame above. https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4763: engagement_levels_.find(url::Origin(GURL(request.url()))); On 2016/11/30 18:00:25, nasko wrote: > Can't we use the frame's origin at this point? Why go through this elaborate > conversion, which actually is lossy too? Done. https://codereview.chromium.org/2535483002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4767: engagement_levels_.erase(engagement_level); On 2016/11/30 18:00:25, nasko wrote: > The map should never grow big, right? Maybe add some form of a (D)CHECK to > ensure we don't leak? Or alternatively purge the map at commit. Done - instead of erasing, I just cleared the map immediately after this. https://codereview.chromium.org/2535483002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/site_engagement.mojom (right): https://codereview.chromium.org/2535483002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/site_engagement.mojom:19: // The browser tells the renderer the engagement level of the current URL. On 2016/11/30 18:00:25, nasko wrote: > nit: Let's avoid using the word "current" as it is not very clear. Also, this > API can probably be called with arbitrary origin, so there is no enforcement > that the origin is the current one. Last, the parameter is an origin, not an URL > :). Done.
content/ LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/01 00:15:44, nasko wrote: > content/ LGTM esprehn/harken: ping :)
WebKit/ LGTM
https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2657: void RenderFrameImpl::SetEngagementLevel(const url::Origin& origin, Why do you need to pass an origin? If you connect to the per-frame ServiceRegistry then its tightly bound to the origin (ex. permissions are connected to it) , so you don't need to check again here. You'll only get deliveries about that frame. https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:4760: frame_->setEngagementLevel(engagement_level->second); This seems like something we should be able to manage inside blink? https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:6516: base::Bind(&RenderFrameImpl::BindEngagement, weak_factory_.GetWeakPtr())); You should be able to connect to this interface directly from inside blink and then not expose new API surface on the WebFrame.
https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2657: void RenderFrameImpl::SetEngagementLevel(const url::Origin& origin, On 2016/12/06 00:31:34, esprehn wrote: > Why do you need to pass an origin? If you connect to the per-frame > ServiceRegistry then its tightly bound to the origin (ex. permissions are > connected to it) , so you don't need to check again here. You'll only get > deliveries about that frame. nasko suggested passing the origin here like the HostZoom feature does. His reasoning seemed sound: navigation is inherently racey, so we should only set engagement when the current security origin matches the origin that we received. https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:4760: frame_->setEngagementLevel(engagement_level->second); On 2016/12/06 00:31:34, esprehn wrote: > This seems like something we should be able to manage inside blink? See comment below regarding this being sent pre-commit. https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:6516: base::Bind(&RenderFrameImpl::BindEngagement, weak_factory_.GetWeakPtr())); On 2016/12/06 00:31:34, esprehn wrote: > You should be able to connect to this interface directly from inside blink and > then not expose new API surface on the WebFrame. I'm not sure what you mean. The IPC from the browser process is sent pre-navigation commit, so that when we commit we've already initialized the frame state to have the correct engagement. Can we guarantee the correct behaviour if it skips the RenderFrame and goes directly to Blink?
https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2657: void RenderFrameImpl::SetEngagementLevel(const url::Origin& origin, On 2016/12/06 00:31:34, esprehn wrote: > Why do you need to pass an origin? If you connect to the per-frame > ServiceRegistry then its tightly bound to the origin (ex. permissions are > connected to it) , so you don't need to check again here. You'll only get > deliveries about that frame. Because a single frame can render multiple origins and navigations are racy by definition. By the time an IPC arrives, the frame might have navigated away to a different origin or even relaxed it with document.domain.
https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2657: void RenderFrameImpl::SetEngagementLevel(const url::Origin& origin, On 2016/12/06 at 00:50:30, nasko wrote: > On 2016/12/06 00:31:34, esprehn wrote: > > Why do you need to pass an origin? If you connect to the per-frame > > ServiceRegistry then its tightly bound to the origin (ex. permissions are > > connected to it) , so you don't need to check again here. You'll only get > > deliveries about that frame. > > Because a single frame can render multiple origins and navigations are racy by definition. By the time an IPC arrives, the frame might have navigated away to a different origin or even relaxed it with document.domain. Right, but that's true of all things using the ServiceRegistry on Frame right? ex. USB uses the registry to get permissions, you're saying that the permission could be accidentally granted to the wrong domain? That should never happen, at the DOMWindow level we should register as a client, just like USB would.
dcheng/benwells: ping. :) https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2657: void RenderFrameImpl::SetEngagementLevel(const url::Origin& origin, On 2016/12/06 00:58:44, esprehn wrote: > On 2016/12/06 at 00:50:30, nasko wrote: > > On 2016/12/06 00:31:34, esprehn wrote: > > > Why do you need to pass an origin? If you connect to the per-frame > > > ServiceRegistry then its tightly bound to the origin (ex. permissions are > > > connected to it) , so you don't need to check again here. You'll only get > > > deliveries about that frame. > > > > Because a single frame can render multiple origins and navigations are racy by > definition. By the time an IPC arrives, the frame might have navigated away to a > different origin or even relaxed it with document.domain. > > Right, but that's true of all things using the ServiceRegistry on Frame right? > ex. USB uses the registry to get permissions, you're saying that the permission > could be accidentally granted to the wrong domain? That should never happen, at > the DOMWindow level we should register as a client, just like USB would. USB doesn't need to request a permission prior to navigation commit though, whereas in this case, we're usually sending the engagement before commit and then setting it when the commit happens.
On 2016/12/06 00:58:44, esprehn wrote: > https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... > content/renderer/render_frame_impl.cc:2657: void > RenderFrameImpl::SetEngagementLevel(const url::Origin& origin, > On 2016/12/06 at 00:50:30, nasko wrote: > > On 2016/12/06 00:31:34, esprehn wrote: > > > Why do you need to pass an origin? If you connect to the per-frame > > > ServiceRegistry then its tightly bound to the origin (ex. permissions are > > > connected to it) , so you don't need to check again here. You'll only get > > > deliveries about that frame. > > > > Because a single frame can render multiple origins and navigations are racy by > definition. By the time an IPC arrives, the frame might have navigated away to a > different origin or even relaxed it with document.domain. > > Right, but that's true of all things using the ServiceRegistry on Frame right? > ex. USB uses the registry to get permissions, you're saying that the permission > could be accidentally granted to the wrong domain? That should never happen, at > the DOMWindow level we should register as a client, just like USB would. Is the per-frame ServiceRegistry actually origin-scoped? I don't think it is in any way (this is actually something I wished for when we were converting app banners to use Mojo). I've been thinking about this more, and I think the easiest long-term solution is to just make the lifetime of LocalFrame match the lifetime of LocalDOMWindow: if we get a new global object, we get a new LocalFrame.
https://codereview.chromium.org/2535483002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/2535483002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:64: &SiteEngagementService::Helper::SendEngagementLevelToFramesMatchingOrigin, It almost feels a bit wasteful to do this... all the frames matching an origin should be in the same process. Maybe it doesn't matter, since this is conceptually simpler though. https://codereview.chromium.org/2535483002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:276: GURL url = handle->GetURL(); Nit: avoid making a copy here and just call url::Origin(handle->GetURL()) Also, how does this work for things like data URLs? https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:4761: engagement_levels_.clear(); Out of curiosity... why do we need to maintain a map? I think this needs some sort of comment (I would have expected a max of one pending entry)
https://codereview.chromium.org/2535483002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/2535483002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:276: GURL url = handle->GetURL(); On 2016/12/08 08:14:07, dcheng wrote: > Nit: avoid making a copy here and just call url::Origin(handle->GetURL()) > > Also, how does this work for things like data URLs? (or filesystem: or blob: urls)
The CQ bit was checked by dominickn@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! https://codereview.chromium.org/2535483002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/2535483002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:64: &SiteEngagementService::Helper::SendEngagementLevelToFramesMatchingOrigin, On 2016/12/08 08:14:07, dcheng wrote: > It almost feels a bit wasteful to do this... all the frames matching an origin > should be in the same process. Maybe it doesn't matter, since this is > conceptually simpler though. Yeah, I did it this way to be as simple (and robust) as possible. https://codereview.chromium.org/2535483002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:276: GURL url = handle->GetURL(); On 2016/12/08 08:14:45, dcheng wrote: > On 2016/12/08 08:14:07, dcheng wrote: > > Nit: avoid making a copy here and just call url::Origin(handle->GetURL()) > > > > Also, how does this work for things like data URLs? > > (or filesystem: or blob: urls) Done. Only HTTP/HTTPS URLs earn engagement, so blobs/file/data URLs will just send NONE down to the renderer. I've added a small optimisation to avoid the extraneous IPC here because NONE is the default value on Document. https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:4761: engagement_levels_.clear(); On 2016/12/08 08:14:07, dcheng wrote: > Out of curiosity... why do we need to maintain a map? I think this needs some > sort of comment (I would have expected a max of one pending entry) I implemented it analogously to the HostZoom feature, which is also sent down in ReadyToCommitNavigation and stored in a map. I don't know enough to elaborate in a comment, but nasko suggested doing it this way.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2657: void RenderFrameImpl::SetEngagementLevel(const url::Origin& origin, On 2016/12/08 02:37:10, dominickn wrote: > On 2016/12/06 00:58:44, esprehn wrote: > > On 2016/12/06 at 00:50:30, nasko wrote: > > > On 2016/12/06 00:31:34, esprehn wrote: > > > > Why do you need to pass an origin? If you connect to the per-frame > > > > ServiceRegistry then its tightly bound to the origin (ex. permissions are > > > > connected to it) , so you don't need to check again here. You'll only get > > > > deliveries about that frame. > > > > > > Because a single frame can render multiple origins and navigations are racy > by > > definition. By the time an IPC arrives, the frame might have navigated away to > a > > different origin or even relaxed it with document.domain. > > > > Right, but that's true of all things using the ServiceRegistry on Frame right? > > ex. USB uses the registry to get permissions, you're saying that the > permission > > could be accidentally granted to the wrong domain? That should never happen, > at > > the DOMWindow level we should register as a client, just like USB would. > > USB doesn't need to request a permission prior to navigation commit though, > whereas in this case, we're usually sending the engagement before commit and > then setting it when the commit happens. If your Mojo interface is bound to the lifetime of a document, then you don't need to pass the origin. However, this interface is bound to a RenderFrame lifetime, which can render multiple origins, so we need to pass the origin to help disambiguate. https://codereview.chromium.org/2535483002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:6516: base::Bind(&RenderFrameImpl::BindEngagement, weak_factory_.GetWeakPtr())); On 2016/12/06 00:49:57, dominickn wrote: > On 2016/12/06 00:31:34, esprehn wrote: > > You should be able to connect to this interface directly from inside blink and > > then not expose new API surface on the WebFrame. > > I'm not sure what you mean. The IPC from the browser process is sent > pre-navigation commit, so that when we commit we've already initialized the > frame state to have the correct engagement. Can we guarantee the correct > behaviour if it skips the RenderFrame and goes directly to Blink? It doesn't strictly need to be a map, but it needs to be a tuple. Maybe we can just use a tuple to store it, if we don't expect more than one. In general, we do need to keep the URL and score together, since there can be races between navigations and we want to ensure that we are stamping the correct score on the correct document.
We should be able to store the ConnectionPtr (or service wrapper) on the Document/DOMWindow which binds the incoming result to the origin?
Sorry about the latency. Do you have plans to add tests for this? https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/DEPS (right): https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/DEPS:1: include_rules = [ should this go in chrome/browser/DEPS with all the others? https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:134: return CreateEngagementScore(url).GetEngagementLevel(); It looks like every time something calls CreateEngagementScore it has to have previously checked if a cleanup is needed. That feels a bit error-prone, is there a way to make it more robust? https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:125: void HelperCreated(SiteEngagementService::Helper* helper); This seems like an observer now. Apart from this, there doesn't appear to be any reason for the Helper to be a nested class. Can you: - remove the Helper nested class - add an Observer nested class - change SiteEnagementService::Helper to be SiteEngagementHelper - make the SiteEngagementHelper implement the SiteEngagementService::Observer interface - use the normal AddObserver, ObserverList etc. patterns
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/DEPS (right): https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/DEPS:1: include_rules = [ On 2016/12/09 06:11:23, benwells wrote: > should this go in chrome/browser/DEPS with all the others? Good catch, done. https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:134: return CreateEngagementScore(url).GetEngagementLevel(); On 2016/12/09 06:11:23, benwells wrote: > It looks like every time something calls CreateEngagementScore it has to have > previously checked if a cleanup is needed. That feels a bit error-prone, is > there a way to make it more robust? Not quite: OriginsWithMaxDailyEngagement, GetCountsAndLastVisitForOriginsComplete, and ResetScoreForURL, SetLastShortcutLaunchTime and CleanupEngagementScores itself all call CreateEngagementScore without needing to check if a cleanup is needed. We only need to check for a cleanup if we're about to use the score externally or add points internally. https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:125: void HelperCreated(SiteEngagementService::Helper* helper); On 2016/12/09 06:11:24, benwells wrote: > This seems like an observer now. Apart from this, there doesn't appear to be any > reason for the Helper to be a nested class. Can you: > - remove the Helper nested class > - add an Observer nested class > - change SiteEnagementService::Helper to be SiteEngagementHelper > - make the SiteEngagementHelper implement the SiteEngagementService::Observer > interface > - use the normal AddObserver, ObserverList etc. patterns I've revamped the Helper to make it more of a private implementation detail of the service. WDYT?
https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:134: return CreateEngagementScore(url).GetEngagementLevel(); On 2016/12/13 06:27:37, dominickn wrote: > On 2016/12/09 06:11:23, benwells wrote: > > It looks like every time something calls CreateEngagementScore it has to have > > previously checked if a cleanup is needed. That feels a bit error-prone, is > > there a way to make it more robust? > > Not quite: OriginsWithMaxDailyEngagement, > GetCountsAndLastVisitForOriginsComplete, and ResetScoreForURL, > SetLastShortcutLaunchTime and CleanupEngagementScores itself all call > CreateEngagementScore without needing to check if a cleanup is needed. We only > need to check for a cleanup if we're about to use the score externally or add > points internally. OK. If it was me I'd try to find a way to clean this up. I think it is tricky and undiscoverable, and likely to break in future. But it's orthogonal to this CL. https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:125: void HelperCreated(SiteEngagementService::Helper* helper); On 2016/12/13 06:27:37, dominickn wrote: > On 2016/12/09 06:11:24, benwells wrote: > > This seems like an observer now. Apart from this, there doesn't appear to be > any > > reason for the Helper to be a nested class. Can you: > > - remove the Helper nested class > > - add an Observer nested class > > - change SiteEnagementService::Helper to be SiteEngagementHelper > > - make the SiteEngagementHelper implement the SiteEngagementService::Observer > > interface > > - use the normal AddObserver, ObserverList etc. patterns > > I've revamped the Helper to make it more of a private implementation detail of > the service. WDYT? Hrmph. I think it is now more obscure. Not only is the observer not called an observer or managed like other observers, the creation feels more convoluted. I won't block you on this. I think the weirdness is all private to this class' implementation and tests, and you pretty much own all of it, so I'll defer to you. But I'd prefer the previous patch set as the helper creation was more straightforward, so will lg if you go back to that.
The CQ bit was checked by dominickn@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...
https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/2535483002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:125: void HelperCreated(SiteEngagementService::Helper* helper); On 2016/12/14 06:17:53, benwells wrote: > On 2016/12/13 06:27:37, dominickn wrote: > > On 2016/12/09 06:11:24, benwells wrote: > > > This seems like an observer now. Apart from this, there doesn't appear to be > > any > > > reason for the Helper to be a nested class. Can you: > > > - remove the Helper nested class > > > - add an Observer nested class > > > - change SiteEnagementService::Helper to be SiteEngagementHelper > > > - make the SiteEngagementHelper implement the > SiteEngagementService::Observer > > > interface > > > - use the normal AddObserver, ObserverList etc. patterns > > > > I've revamped the Helper to make it more of a private implementation detail of > > the service. WDYT? > > Hrmph. I think it is now more obscure. Not only is the observer not called an > observer or managed like other observers, the creation feels more convoluted. > > I won't block you on this. I think the weirdness is all private to this class' > implementation and tests, and you pretty much own all of it, so I'll defer to > you. But I'd prefer the previous patch set as the helper creation was more > straightforward, so will lg if you go back to that. Done. I think the main thing here is that we have what really is a private implementation detail (the helper) in a separate .h/.cc file for clarity (otherwise the service .h/.cc files would be enormous). As much as possible, we want to keep the helper -> service interface as a private detail.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
dominickn@chromium.org changed reviewers: + jochen@chromium.org
esphren/dcheng: did you have any more issues that you wanted to address here? +jochen: PTAL at chrome/browser/DEPS, thanks!
lgtm
On 2016/12/15 05:01:40, dominickn wrote: > esphren/dcheng: did you have any more issues that you wanted to address here? > > +jochen: PTAL at chrome/browser/DEPS, thanks! Any chance we can get rid of the map (perhaps in a followup)? I don't see why we'd need to accumulate more than one entry here.
lgtm
On 2016/12/15 06:30:58, dcheng wrote: > On 2016/12/15 05:01:40, dominickn wrote: > > esphren/dcheng: did you have any more issues that you wanted to address here? > > > > +jochen: PTAL at chrome/browser/DEPS, thanks! > > Any chance we can get rid of the map (perhaps in a followup)? I don't see why > we'd need to accumulate more than one entry here. Any change with the maps might want to do the same with the HostZoom feature (both are implemented the same and are called the same way). I'm happy to follow up with nasko or other content experts on whether we can remove the map, and do so for both engagement and host zoom. How does that sound?
On 2016/12/15 23:43:21, dominickn wrote: > On 2016/12/15 06:30:58, dcheng wrote: > > On 2016/12/15 05:01:40, dominickn wrote: > > > esphren/dcheng: did you have any more issues that you wanted to address > here? > > > > > > +jochen: PTAL at chrome/browser/DEPS, thanks! > > > > Any chance we can get rid of the map (perhaps in a followup)? I don't see why > > we'd need to accumulate more than one entry here. > > Any change with the maps might want to do the same with the HostZoom feature > (both are implemented the same and are called the same way). I'm happy to follow > up with nasko or other content experts on whether we can remove the map, and do > so for both engagement and host zoom. How does that sound? dcheng: thoughts on this?
mojo lgtm, I'd like to fix the map issue in a followup though. It might also be nice to note somewhere that site engagement never handles special URLs like data: URLs, though I'm not sure where the appropriate place to document that would be... I suppose a DCHECK would be one way of doing it. *shrug*
On 2016/12/20 01:01:12, dcheng wrote: > mojo lgtm, I'd like to fix the map issue in a followup though. > > It might also be nice to note somewhere that site engagement never handles > special URLs like data: URLs, though I'm not sure where the appropriate place to > document that would be... I suppose a DCHECK would be one way of doing it. > *shrug* Thanks! I'll follow up one both of those shortly.
The CQ bit was checked by dominickn@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, nasko@chromium.org, benwells@chromium.org, jochen@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2535483002/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dominickn@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": 180001, "attempt_start_ts": 1482214030412680,
"parent_rev": "bdb5513340909eea60246627ef2547883f1352ca", "commit_rev":
"f0a15d38f34dc902b92075e7e071a16b91046e2a"}
Message was sent while issue was closed.
Description was changed from ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojo enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process sends the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is stored on Document within the renderer, with a temporary stop on RenderFrameImpl if sent prior to navigation commit. This is a first step to enabling the collection of UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 ========== to ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojo enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process sends the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is stored on Document within the renderer, with a temporary stop on RenderFrameImpl if sent prior to navigation commit. This is a first step to enabling the collection of UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 Review-Url: https://codereview.chromium.org/2535483002 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojo enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process sends the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is stored on Document within the renderer, with a temporary stop on RenderFrameImpl if sent prior to navigation commit. This is a first step to enabling the collection of UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 Review-Url: https://codereview.chromium.org/2535483002 ========== to ========== Plumb site engagement to the renderer process. This CL generalises the EngagementLevel concept in site engagement, moving it to a Mojo enum in Blink's public/platform. Prior to each navigation commit, and when the engagement level for an origin changes between buckets, the browser process sends the new level to all renderers which are navigated to that origin. Using the discrete levels rather than the continuous score minimises the IPC overhead to one additional IPC per navigation commit per frame, and one IPC per engagement level change. The engagement level is stored on Document within the renderer, with a temporary stop on RenderFrameImpl if sent prior to navigation commit. This is a first step to enabling the collection of UseCounter data for JavaScript API usage against engagement buckets. BUG=655415 Committed: https://crrev.com/6c1f1cf3c31b4c798708cb17d4ecd8e6db37e833 Cr-Commit-Position: refs/heads/master@{#439726} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6c1f1cf3c31b4c798708cb17d4ecd8e6db37e833 Cr-Commit-Position: refs/heads/master@{#439726} |
