Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724433002:
Remove the retargeting notification (Closed)
|
Created:
3 years, 9 months ago by Patrick Noland Modified:
3 years, 9 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, grt+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove the retargeting notification
Replace the retargeting notification with an additional call site for
WebContentsObserver::DidOpenRequestedURL. Adjust the two consumers of
the retargeting notification to record the information using
DidOpenRequestedURL instead.
R=nasko@chromium.org, creis@chromium.org
BUG=696787
Review-Url: https://codereview.chromium.org/2724433002
Cr-Commit-Position: refs/heads/master@{#454953}
Committed: https://chromium.googlesource.com/chromium/src/+/aae574ee52af5df55c8b7175ff096481a5bfb703
Patch Set 1 : Remove the retargeting notification #
Total comments: 16
Patch Set 2 : Remove the retargeting notification #
Total comments: 4
Patch Set 3 : Remove comments in safe browsing tests #Messages
Total messages: 53 (36 generated)
The CQ bit was checked by pnoland@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 pnoland@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 pnoland@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.
Patchset #2 (id:20001) has been deleted
Description was changed from
==========
Get rid of retargeting
BUG=
==========
to
==========
Remove the retargeting notification
Replace the retargeting notification with an additional call site for
WebContentsObserver::DidOpenRequestedURL. Adjust the two consumers of
the retargeting notification to record the information using
DidOpenRequestedURL instead.
R=nasko@chromium.org, creis@chromium.org
BUG=696787
==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
pnoland@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
Nasko, Charlie, PTAL. (I think the android trybot failures are unrelated to this change; the compile step fails with and without the patch)
This is awesome! Few small comments and it should be good to go. Thanks! https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:137: int64_t source_render_frame_id, nit: Why use int64_t when the routing id is just a plain old int? https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:392: if (api) { Would api ever be null? https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:394: if (router) { Since this is the last code in the method, it is better to use the pattern of: if (!ptr) return; ptr->foo(); It doesn't lead to multiple levels of indenting and returns early. https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right): https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.h:225: static const bool kServiceRedirectedInIncognito = true; Why do we need this? https://codereview.chromium.org/2724433002/diff/60001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/2724433002/diff/60001/content/public/browser/... content/public/browser/web_contents_observer.h:279: bool not_yet_in_tabstrip) {} Technically, TabStrip is chrome/ concept and content/ on its own doesn't have such a thing. Can we rename this to reflect better how it is set? It is true for renderer-initiated WebContents creations and it is set to false for browser-initiated ones. So renderer_initiated?
https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:137: int64_t source_render_frame_id, On 2017/03/01 00:48:37, nasko (slow) wrote: > nit: Why use int64_t when the routing id is just a plain old int? No clue, I copied this from RetargetingDetails. I'll fix it. https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:392: if (api) { On 2017/03/01 00:48:37, nasko (slow) wrote: > Would api ever be null? I don't know enough to say for sure. The fact that it was null for incognito browser contexts without the kServiceRedirectedInIncognito flag scared me enough to add this check. https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right): https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.h:225: static const bool kServiceRedirectedInIncognito = true; On 2017/03/01 00:48:37, nasko (slow) wrote: > Why do we need this? Without this, attempts to access the WebNavigationAPI instance for an incognito browser context return null. This makes it impossible for the WebNavigationTabObserver to report the event up to the router in DidOpenRequestedURL. Not reporting the event for an incognito browser context caused tests to fail, so it seemed like we needed that to continue to happen.
nasko@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Adding Devlin, to look at the couple of comments in web_navigation_api code. https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:392: if (api) { On 2017/03/01 00:57:09, Patrick Noland wrote: > On 2017/03/01 00:48:37, nasko (slow) wrote: > > Would api ever be null? > > I don't know enough to say for sure. The fact that it was null for incognito > browser contexts without the kServiceRedirectedInIncognito flag scared me enough > to add this check. Since you've added the flag, I expect this will never be null, so I'd suggest removing the check. https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right): https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.h:225: static const bool kServiceRedirectedInIncognito = true; On 2017/03/01 00:57:09, Patrick Noland wrote: > On 2017/03/01 00:48:37, nasko (slow) wrote: > > Why do we need this? > > Without this, attempts to access the WebNavigationAPI instance for an incognito > browser context return null. This makes it impossible for the > WebNavigationTabObserver to report the event up to the router in > DidOpenRequestedURL. Not reporting the event for an incognito browser context > caused tests to fail, so it seemed like we needed that to continue to happen. Thanks for the info! Devlin, this make sense to me, but having an extra pair of eyes to confirm will be great.
The CQ bit was checked by pnoland@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/2724433002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:137: int64_t source_render_frame_id, On 2017/03/01 00:48:37, nasko (slow) wrote: > nit: Why use int64_t when the routing id is just a plain old int? Done. https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:392: if (api) { On 2017/03/01 18:36:31, nasko (slow) wrote: > On 2017/03/01 00:57:09, Patrick Noland wrote: > > On 2017/03/01 00:48:37, nasko (slow) wrote: > > > Would api ever be null? > > > > I don't know enough to say for sure. The fact that it was null for incognito > > browser contexts without the kServiceRedirectedInIncognito flag scared me > enough > > to add this check. > > Since you've added the flag, I expect this will never be null, so I'd suggest > removing the check. Done. https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:394: if (router) { On 2017/03/01 00:48:37, nasko (slow) wrote: > Since this is the last code in the method, it is better to use the pattern of: > > if (!ptr) > return; > > ptr->foo(); > > It doesn't lead to multiple levels of indenting and returns early. Done. https://codereview.chromium.org/2724433002/diff/60001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/2724433002/diff/60001/content/public/browser/... content/public/browser/web_contents_observer.h:279: bool not_yet_in_tabstrip) {} On 2017/03/01 00:48:38, nasko (slow) wrote: > Technically, TabStrip is chrome/ concept and content/ on its own doesn't have > such a thing. Can we rename this to reflect better how it is set? It is true for > renderer-initiated WebContents creations and it is set to false for > browser-initiated ones. So renderer_initiated? Done.
pnoland@chromium.org changed reviewers: + jialiul@chromium.org
Adding jialiul for safe browsing
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is awesome! Thanks! LGTM safe_browsing_navigation_observer related code is currently gated by finch experiment. In case you want to try manual test, you can use the following flag --enable-features="DownloadAttribution<DownloadAttributionPerformanceTest" --force-fieldtrials=DownloadAttributionPerformanceTest/DownloadAttributionEnabled https://codereview.chromium.org/2724433002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc (right): https://codereview.chromium.org/2724433002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:586: // The next NavigationEvent was obtained from NOIFICATION_RETARGETING. Change line 586 into // The next NavigationEvent opens a new tab. And remove comment line 587-590. Thanks! https://codereview.chromium.org/2724433002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:725: // TODO(jialiul): After https://crbug.com/651895 is fixed, we'll no longer Could you remove 725-727? thanks!
https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right): https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.h:225: static const bool kServiceRedirectedInIncognito = true; On 2017/03/01 18:36:31, nasko (slow) wrote: > On 2017/03/01 00:57:09, Patrick Noland wrote: > > On 2017/03/01 00:48:37, nasko (slow) wrote: > > > Why do we need this? > > > > Without this, attempts to access the WebNavigationAPI instance for an > incognito > > browser context return null. This makes it impossible for the > > WebNavigationTabObserver to report the event up to the router in > > DidOpenRequestedURL. Not reporting the event for an incognito browser context > > caused tests to fail, so it seemed like we needed that to continue to happen. > > Thanks for the info! Devlin, this make sense to me, but having an extra pair of > eyes to confirm will be great. This makes sense if you want to share an instance between incognito and on-the-record. If instead there should be two separate instances, you can use kServiceHasOwnInstanceInIncognito. I *think* this was designed to be used across the incognito boundary, so this would be correct.
https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right): https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.h:225: static const bool kServiceRedirectedInIncognito = true; On 2017/03/02 01:33:36, Devlin wrote: > On 2017/03/01 18:36:31, nasko (slow) wrote: > > On 2017/03/01 00:57:09, Patrick Noland wrote: > > > On 2017/03/01 00:48:37, nasko (slow) wrote: > > > > Why do we need this? > > > > > > Without this, attempts to access the WebNavigationAPI instance for an > > incognito > > > browser context return null. This makes it impossible for the > > > WebNavigationTabObserver to report the event up to the router in > > > DidOpenRequestedURL. Not reporting the event for an incognito browser > context > > > caused tests to fail, so it seemed like we needed that to continue to > happen. > > > > Thanks for the info! Devlin, this make sense to me, but having an extra pair > of > > eyes to confirm will be great. > > This makes sense if you want to share an instance between incognito and > on-the-record. If instead there should be two separate instances, you can use > kServiceHasOwnInstanceInIncognito. I *think* this was designed to be used > across the incognito boundary, so this would be correct. There's a browser test (TargetBlankIncognito) that relies on the service instance being shared across the incognito boundary, so I think kServiceRedirectedInIncognito is the option we want.
On 2017/03/02 22:15:21, Patrick Noland wrote: > https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... > File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right): > > https://codereview.chromium.org/2724433002/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/web_navigation/web_navigation_api.h:225: static > const bool kServiceRedirectedInIncognito = true; > On 2017/03/02 01:33:36, Devlin wrote: > > On 2017/03/01 18:36:31, nasko (slow) wrote: > > > On 2017/03/01 00:57:09, Patrick Noland wrote: > > > > On 2017/03/01 00:48:37, nasko (slow) wrote: > > > > > Why do we need this? > > > > > > > > Without this, attempts to access the WebNavigationAPI instance for an > > > incognito > > > > browser context return null. This makes it impossible for the > > > > WebNavigationTabObserver to report the event up to the router in > > > > DidOpenRequestedURL. Not reporting the event for an incognito browser > > context > > > > caused tests to fail, so it seemed like we needed that to continue to > > happen. > > > > > > Thanks for the info! Devlin, this make sense to me, but having an extra pair > > of > > > eyes to confirm will be great. > > > > This makes sense if you want to share an instance between incognito and > > on-the-record. If instead there should be two separate instances, you can use > > kServiceHasOwnInstanceInIncognito. I *think* this was designed to be used > > across the incognito boundary, so this would be correct. > > There's a browser test (TargetBlankIncognito) that relies on the service > instance being shared across the incognito boundary, so I think > kServiceRedirectedInIncognito is the option we want. Given that opening a link in incognito window is signalled in the extension, the design is to be able to track both. Also, the retargeting notification basically accomplished the same thing and wasn't necessarily profile bound. LGTM!
(I don't think you need my stamp with Nasko's, and all this seems reasonable - lemme know if you wanted me to take a closer look.)
pnoland@chromium.org changed reviewers: + sky@chromium.org
+sky@ for chrome/browser and chrome/browser/ui files https://codereview.chromium.org/2724433002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc (right): https://codereview.chromium.org/2724433002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:586: // The next NavigationEvent was obtained from NOIFICATION_RETARGETING. On 2017/03/02 00:36:52, Jialiu Lin wrote: > Change line 586 into > // The next NavigationEvent opens a new tab. > > And remove comment line 587-590. Thanks! Done. https://codereview.chromium.org/2724433002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:725: // TODO(jialiul): After https://crbug.com/651895 is fixed, we'll no longer On 2017/03/02 00:36:52, Jialiu Lin wrote: > Could you remove 725-727? thanks! Done.
The CQ bit was checked by pnoland@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.
This looks reasonable to me, but do you have an extensions owner reviewing the extensions changes? Devlin is an extensions OWNER, but he indicated he wasn't needed?
On 2017/03/06 18:00:16, sky wrote: > This looks reasonable to me, but do you have an extensions owner reviewing the > extensions changes? Devlin is an extensions OWNER, but he indicated he wasn't > needed? Nasko OWNs the web_navigation API, and he's on the review. :)
Ok, or random c/b files not owned by other reviewers LGTM
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2724433002/#ps100001 (title: "Remove comments in safe browsing tests")
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": 100001, "attempt_start_ts": 1488828818233330,
"parent_rev": "e8e3eea556d5713612f304b2c35e193658fa4f90", "commit_rev":
"aae574ee52af5df55c8b7175ff096481a5bfb703"}
Message was sent while issue was closed.
Description was changed from
==========
Remove the retargeting notification
Replace the retargeting notification with an additional call site for
WebContentsObserver::DidOpenRequestedURL. Adjust the two consumers of
the retargeting notification to record the information using
DidOpenRequestedURL instead.
R=nasko@chromium.org, creis@chromium.org
BUG=696787
==========
to
==========
Remove the retargeting notification
Replace the retargeting notification with an additional call site for
WebContentsObserver::DidOpenRequestedURL. Adjust the two consumers of
the retargeting notification to record the information using
DidOpenRequestedURL instead.
R=nasko@chromium.org, creis@chromium.org
BUG=696787
Review-Url: https://codereview.chromium.org/2724433002
Cr-Commit-Position: refs/heads/master@{#454953}
Committed:
https://chromium.googlesource.com/chromium/src/+/aae574ee52af5df55c8b7175ff09...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/aae574ee52af5df55c8b7175ff09... |
