Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
426093002:
Adds support for thumbnail click pings for Most Visited (Closed)
|
Created:
6 years, 4 months ago by fserb Modified:
6 years, 4 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdds support for thumbnail click pings for Most Visited
We use sendBeacon when enabled, otherwise fallback to "a ping".
BUG=398590
TEST=Manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287161
Patch Set 1 #Patch Set 2 : Cleanup code to support generic ping URLs from same origin #Patch Set 3 : Force the same origin for URL ping #Patch Set 4 : Cleanup #Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Total comments: 2
Patch Set 7 : Only ping for server suggestions #
Messages
Total messages: 15 (0 generated)
-samarth, +jered Not quite ready for review, please ping again when it is :)
On 2014/07/29 21:18:52, Mathieu Perreault wrote: > -samarth, +jered > > Not quite ready for review, please ping again when it is :) I think it's better now. Care to take a look?
Have a look! https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:65: * @const You can simplify to @const {string} https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:130: if (params.ping) { nit: you can omit braces https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:144: // Else follow <a> normally, so transition type would be LINK. This comment seems to say that for server suggestions we use <a> tags for navigation, so perhaps we can use the 'ping' there? https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:235: // sendBeacon is still in experimental, so for now we use 'a ping' you could say: "If sendBeacon is not enabled, use 'a ping'" And mention that we use both in the CL description
thanks for the comments! :) https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:65: * @const On 2014/07/30 14:12:54, Mathieu Perreault wrote: > You can simplify to @const {string} Done. https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:130: if (params.ping) { On 2014/07/30 14:12:54, Mathieu Perreault wrote: > nit: you can omit braces Acknowledged. https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:144: // Else follow <a> normally, so transition type would be LINK. On 2014/07/30 14:12:54, Mathieu Perreault wrote: > This comment seems to say that for server suggestions we use <a> tags for > navigation, so perhaps we can use the 'ping' there? as discussed offline, trying not to rely on a ping for the future. https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:235: // sendBeacon is still in experimental, so for now we use 'a ping' On 2014/07/30 14:12:54, Mathieu Perreault wrote: > you could say: "If sendBeacon is not enabled, use 'a ping'" > > And mention that we use both in the CL description Done.
On 2014/07/31 18:18:47, fserb wrote: > thanks for the comments! :) > > https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... > File chrome/browser/resources/local_ntp/most_visited_util.js (right): > > https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... > chrome/browser/resources/local_ntp/most_visited_util.js:65: * @const > On 2014/07/30 14:12:54, Mathieu Perreault wrote: > > You can simplify to @const {string} > > Done. > > https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... > chrome/browser/resources/local_ntp/most_visited_util.js:130: if (params.ping) { > On 2014/07/30 14:12:54, Mathieu Perreault wrote: > > nit: you can omit braces > > Acknowledged. > > https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... > chrome/browser/resources/local_ntp/most_visited_util.js:144: // Else follow <a> > normally, so transition type would be LINK. > On 2014/07/30 14:12:54, Mathieu Perreault wrote: > > This comment seems to say that for server suggestions we use <a> tags for > > navigation, so perhaps we can use the 'ping' there? > > as discussed offline, trying not to rely on a ping for the future. > > https://codereview.chromium.org/426093002/diff/80001/chrome/browser/resources... > chrome/browser/resources/local_ntp/most_visited_util.js:235: // sendBeacon is > still in experimental, so for now we use 'a ping' > On 2014/07/30 14:12:54, Mathieu Perreault wrote: > > you could say: "If sendBeacon is not enabled, use 'a ping'" > > > > And mention that we use both in the CL description > > Done. lgtm, jered will need to approve
https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:129: if (params.ping) { A possibility, while avoiding a separate request just for the ping. if navigator.sendBeacon is defined, we could use it here (as we're doing now). If it's not defined, we could set link.ping (since link is going to be clicked anyway at the end of this block?).
https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:129: if (params.ping) { Can you provide a bit more context for this change? I'm uneasy about accidentally pinging a user's local MV navigations, e.g. file:// URLs, to a remote logger that has no business knowing that.
On 2014/08/01 16:09:44, Jered wrote: > https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... > File chrome/browser/resources/local_ntp/most_visited_util.js (right): > > https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... > chrome/browser/resources/local_ntp/most_visited_util.js:129: if (params.ping) { > Can you provide a bit more context for this change? I'm uneasy about > accidentally pinging a user's local MV navigations, e.g. file:// URLs, to a > remote logger that has no business knowing that. (Please add a detailed comment here explaining what we want to log and why, and also what we're not logging.)
On 2014/08/01 16:12:22, Jered wrote: > On 2014/08/01 16:09:44, Jered wrote: > > > https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... > > File chrome/browser/resources/local_ntp/most_visited_util.js (right): > > > > > https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... > > chrome/browser/resources/local_ntp/most_visited_util.js:129: if (params.ping) > { > > Can you provide a bit more context for this change? I'm uneasy about > > accidentally pinging a user's local MV navigations, e.g. file:// URLs, to a > > remote logger that has no business knowing that. > > (Please add a detailed comment here explaining what we want to log and why, and > also what we're not logging.) Jered, hi. There are 2 important things here: 1. Those pings URLs are only created for server-side suggestions, never for MV. 2. The URLs we create log only the number of the tile being clicked, not the actual URLs. An example ping URL would be /chromesuggestions/click?cd=3&d=1234 (where d is the timestamp the suggestions were generated). If you think it would be better, I could add a check to only call the ping if I've asserted it's a server side suggestions. Does this address your issue?
On 2014/08/01 18:17:24, fserb wrote: > On 2014/08/01 16:12:22, Jered wrote: > > On 2014/08/01 16:09:44, Jered wrote: > > > > > > https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... > > > File chrome/browser/resources/local_ntp/most_visited_util.js (right): > > > > > > > > > https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... > > > chrome/browser/resources/local_ntp/most_visited_util.js:129: if > (params.ping) > > { > > > Can you provide a bit more context for this change? I'm uneasy about > > > accidentally pinging a user's local MV navigations, e.g. file:// URLs, to a > > > remote logger that has no business knowing that. > > > > (Please add a detailed comment here explaining what we want to log and why, > and > > also what we're not logging.) > > Jered, hi. > > There are 2 important things here: > > 1. Those pings URLs are only created for server-side suggestions, never for MV. > > 2. The URLs we create log only the number of the tile being clicked, not the > actual URLs. An example ping URL would be /chromesuggestions/click?cd=3&d=1234 > (where d is the timestamp the suggestions were generated). > > If you think it would be better, I could add a check to only call the ping if > I've asserted it's a server side suggestions. > > Does this address your issue? lgtm Hmm, we can see from looking at the chromium code that ping is unused. I forgot exactly how this worked, but seeing "ping" and "most visited" near each other made me uneasy. Please add a brief note about this (similar to your comment) to the code where we send the ping so that people reading it will understand what is meant here. I believe the absence of "ping" for native suggestions is enough to ensure we don't ping.
On 2014/08/01 18:22:19, Jered wrote: > On 2014/08/01 18:17:24, fserb wrote: > > On 2014/08/01 16:12:22, Jered wrote: > > > On 2014/08/01 16:09:44, Jered wrote: > > > > > > > > > > https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... > > > > File chrome/browser/resources/local_ntp/most_visited_util.js (right): > > > > > > > > > > > > > > https://codereview.chromium.org/426093002/diff/100001/chrome/browser/resource... > > > > chrome/browser/resources/local_ntp/most_visited_util.js:129: if > > (params.ping) > > > { > > > > Can you provide a bit more context for this change? I'm uneasy about > > > > accidentally pinging a user's local MV navigations, e.g. file:// URLs, to > a > > > > remote logger that has no business knowing that. > > > > > > (Please add a detailed comment here explaining what we want to log and why, > > and > > > also what we're not logging.) > > > > Jered, hi. > > > > There are 2 important things here: > > > > 1. Those pings URLs are only created for server-side suggestions, never for > MV. > > > > 2. The URLs we create log only the number of the tile being clicked, not the > > actual URLs. An example ping URL would be /chromesuggestions/click?cd=3&d=1234 > > (where d is the timestamp the suggestions were generated). > > > > If you think it would be better, I could add a check to only call the ping if > > I've asserted it's a server side suggestions. > > > > Does this address your issue? > > lgtm > > Hmm, we can see from looking at the chromium code that ping is unused. I forgot > exactly > how this worked, but seeing "ping" and "most visited" near each other made me > uneasy. > Please add a brief note about this (similar to your comment) to the code where > we send the > ping so that people reading it will understand what is meant here. > > I believe the absence of "ping" for native suggestions is enough to ensure we > don't ping. Done both for clarity.
The CQ bit was checked by fserb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fserb@chromium.org/426093002/120001
Message was sent while issue was closed.
Change committed as 287161 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
