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

Issue 2338133006: [NTP] Fix article suggestion clicks contributing to Most Visited tiles (Closed)

Created:
4 years, 3 months ago by mastiz
Modified:
4 years, 3 months ago
CC:
chromium-reviews, ntp-dev+reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[NTP] Fix article suggestion clicks contributing to Most Visited tiles There's a need to distinguish clicks on different elements on the NTP: a) clicks on Most Visited tiles. b) clicks on (newly introduced) article suggestions (aka snippets). The first should contribute to Most Visited tiles (i.e. boost tiles that have been clicked in the past). The second shouldn't. We choose to achieve this by specifying a referrer for article suggestion clicks. This exposes the referrer to third parties, which has been discussed and considered a desirable feature. The fix relies on such a workaround due to the current lack of infrastructure to propagate opaque feature-specific data from upper layers to navigation history (and sync). The approach competes with more intrusive/controversial alternatives to achieve the same: 1. Use page transition types (LINK vs AUTO_BOOKMARK) to distinguish tile clicks from article suggestion clicks: unfortunately both types have been used in the past (older versions of Chrome). 2. Introducing a new page transition type or qualifier: this can be considered a layering violation. 3. Introduce a dummy redirect by means of a data: schema page. BUG=645895 Committed: https://crrev.com/30406bddaaa14549fdd5474ba0a411e5fb386f27 Cr-Commit-Position: refs/heads/master@{#419242}

Patch Set 1 #

Total comments: 15

Patch Set 2 : Addressed comments. #

Patch Set 3 : Moved URL to constant. #

Total comments: 2

Patch Set 4 : Moved to HistoryTabHelper as suggested. #

Total comments: 4

Patch Set 5 : Addressed comments: new comment, arg naming. #

Total comments: 4

Patch Set 6 : Addressed comments: removed obvious comment and added docs for flag. #

Patch Set 7 : Updated trivial occurrence in unit test. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -41 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java View 1 2 8 chunks +39 lines, -18 lines 0 comments Download
M chrome/browser/history/history_tab_helper.cc View 1 2 3 4 2 chunks +15 lines, -1 line 0 comments Download
M chrome/browser/supervised_user/supervised_user_navigation_observer.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/web_dialog_web_contents_delegate_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M components/history/core/browser/history_backend.cc View 1 2 3 4 5 2 chunks +5 lines, -4 lines 0 comments Download
M components/history/core/browser/history_backend_unittest.cc View 1 2 3 4 7 chunks +38 lines, -8 lines 0 comments Download
M components/history/core/browser/history_service.cc View 1 2 3 4 2 chunks +3 lines, -2 lines 0 comments Download
M components/history/core/browser/history_types.h View 1 2 3 4 5 4 chunks +9 lines, -3 lines 0 comments Download
M components/history/core/browser/history_types.cc View 1 2 3 4 3 chunks +6 lines, -3 lines 0 comments Download

Messages

Total messages: 36 (13 generated)
mastiz
Third attempt, now IMO cleaner after we relaxed the requirement to hide the referrer from ...
4 years, 3 months ago (2016-09-15 14:20:47 UTC) #2
Marc Treib
Code LGTM, if we're okay with exposing this to third parties. https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): ...
4 years, 3 months ago (2016-09-15 14:31:23 UTC) #5
mastiz
https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java#newcode324 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:324: if (article.mCategory == KnownCategories.ARTICLES) { On 2016/09/15 14:31:22, Marc ...
4 years, 3 months ago (2016-09-15 14:37:56 UTC) #6
Marc Treib
https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java#newcode324 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:324: if (article.mCategory == KnownCategories.ARTICLES) { On 2016/09/15 14:37:56, mastiz ...
4 years, 3 months ago (2016-09-15 14:50:56 UTC) #7
mastiz
https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java#newcode324 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:324: if (article.mCategory == KnownCategories.ARTICLES) { On 2016/09/15 14:50:55, Marc ...
4 years, 3 months ago (2016-09-15 15:06:16 UTC) #8
mastiz
sky@chromium.org: Please review overall approach. This seems a cleanest approach so far, now that exposing ...
4 years, 3 months ago (2016-09-15 15:08:28 UTC) #10
Bernhard Bauer
https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java#newcode326 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:326: new Referrer("https://www.googleapis.com/auth/chrome-content-suggestions", Can you make this a constant? https://codereview.chromium.org/2338133006/diff/1/components/history/core/browser/history_backend.cc ...
4 years, 3 months ago (2016-09-15 15:10:52 UTC) #11
Marc Treib
https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java#newcode326 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:326: new Referrer("https://www.googleapis.com/auth/chrome-content-suggestions", Ah, actually one more nit: This is ...
4 years, 3 months ago (2016-09-15 15:19:21 UTC) #12
mastiz
PTAL. https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java#newcode326 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:326: new Referrer("https://www.googleapis.com/auth/chrome-content-suggestions", On 2016/09/15 15:10:52, Bernhard Bauer wrote: ...
4 years, 3 months ago (2016-09-15 15:31:57 UTC) #13
Bernhard Bauer
Java LGTM. https://codereview.chromium.org/2338133006/diff/1/components/history/core/browser/history_backend.cc File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2338133006/diff/1/components/history/core/browser/history_backend.cc#newcode353 components/history/core/browser/history_backend.cc:353: referrer != GURL(kChromeSearchContentSuggestionsReferrer))) && On 2016/09/15 15:31:57, ...
4 years, 3 months ago (2016-09-15 16:13:07 UTC) #14
sky
https://codereview.chromium.org/2338133006/diff/40001/components/history/core/browser/history_backend.cc File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2338133006/diff/40001/components/history/core/browser/history_backend.cc#newcode76 components/history/core/browser/history_backend.cc:76: "https://www.googleapis.com/auth/chrome-content-suggestions"; I like this approach much better, but I ...
4 years, 3 months ago (2016-09-15 17:46:26 UTC) #15
mastiz
https://codereview.chromium.org/2338133006/diff/40001/components/history/core/browser/history_backend.cc File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2338133006/diff/40001/components/history/core/browser/history_backend.cc#newcode76 components/history/core/browser/history_backend.cc:76: "https://www.googleapis.com/auth/chrome-content-suggestions"; On 2016/09/15 17:46:26, sky wrote: > I like ...
4 years, 3 months ago (2016-09-15 18:12:07 UTC) #16
sky
On 2016/09/15 18:12:07, mastiz wrote: > https://codereview.chromium.org/2338133006/diff/40001/components/history/core/browser/history_backend.cc > File components/history/core/browser/history_backend.cc (right): > > https://codereview.chromium.org/2338133006/diff/40001/components/history/core/browser/history_backend.cc#newcode76 > ...
4 years, 3 months ago (2016-09-15 20:07:06 UTC) #17
mastiz
On 2016/09/15 20:07:06, sky wrote: > On 2016/09/15 18:12:07, mastiz wrote: > > > https://codereview.chromium.org/2338133006/diff/40001/components/history/core/browser/history_backend.cc ...
4 years, 3 months ago (2016-09-15 21:26:36 UTC) #18
sky
Just minor stuff. https://codereview.chromium.org/2338133006/diff/60001/chrome/browser/history/history_tab_helper.cc File chrome/browser/history/history_tab_helper.cc (right): https://codereview.chromium.org/2338133006/diff/60001/chrome/browser/history/history_tab_helper.cc#newcode69 chrome/browser/history/history_tab_helper.cc:69: const bool skip_for_ntp_most_visited = Please document ...
4 years, 3 months ago (2016-09-15 22:56:10 UTC) #19
mastiz
PTAL. Let me highlight one different wrt to the original proposal: we now skip the ...
4 years, 3 months ago (2016-09-16 08:42:10 UTC) #20
sky
LGTM with the following addressed. Thanks for your patience and finding a good solution. https://codereview.chromium.org/2338133006/diff/80001/components/history/core/browser/history_backend.cc ...
4 years, 3 months ago (2016-09-16 17:18:52 UTC) #21
mastiz
All comments addressed. Thanks for your reviews and guidance, as well as the back and ...
4 years, 3 months ago (2016-09-16 17:58:21 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2338133006/100001
4 years, 3 months ago (2016-09-16 18:00:35 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_compile_dbg_ng/builds/264449)
4 years, 3 months ago (2016-09-16 18:16:10 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2338133006/120001
4 years, 3 months ago (2016-09-16 19:09:18 UTC) #33
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 3 months ago (2016-09-16 19:16:11 UTC) #34
commit-bot: I haz the power
4 years, 3 months ago (2016-09-16 19:18:32 UTC) #36
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/30406bddaaa14549fdd5474ba0a411e5fb386f27
Cr-Commit-Position: refs/heads/master@{#419242}

Powered by Google App Engine
This is Rietveld 408576698