Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2718443002:
Network traffic annotation added to logo_tracker. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to logo_tracker.
Network traffic annotation is added to network request of
search_provider_logos/logo_tracker.
BUG=656607
Review-Url: https://codereview.chromium.org/2718443002
Cr-Commit-Position: refs/heads/master@{#455694}
Committed: https://chromium.googlesource.com/chromium/src/+/4d30c69479ce696af07f93f0677ba7baac484cab
Patch Set 1 #
Total comments: 22
Patch Set 2 : Annotation updated. #
Total comments: 2
Patch Set 3 : Annotation updated. #
Total comments: 6
Patch Set 4 : Annotation updated. #
Total comments: 2
Patch Set 5 : Annotation updated. #
Total comments: 7
Patch Set 6 : Comments addressed. #Messages
Total messages: 35 (7 generated)
rhalavati@chromium.org changed reviewers: + justincohen@chromium.org
We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified logo_tracker and added annotation template to it. Please review it and suggest the required answers for the empty parts. Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
justincohen@chromium.org changed reviewers: + treib@chromium.org
treib@ is doing a refactor here (see go/ntp-doodles) and may have better thoughts on what this should look like too. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:233: sender: "..." logo_tracker https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:234: description: "..." Provides the logo image for a search provider (e.g. Google or Yahoo!) https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:235: trigger: "..." Displaying the new tab page on iOS or Android. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:236: data: "..." Not sure here? https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:237: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER +treib@ The destination is set by SetServerAPI. For example, in ios/chrome/browser/google/google_logo_service.mm this is set in GetGoogleDoodleURL as a GOOGLE_OWNED_SERVICE It's not really known within FetchLogo if this is really a GOOGLE_OWNED_SERVICE. FWIW, there's a refactor coming to logo_tracker that I believe will remove this ambiguity, see go/ntp-doodles https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:240: cookies_allowed: false/true Not sure here, I think yes? https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:242: setting: "..." You can't really disable this, not sure what goes here?
https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:237: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/23 15:14:30, justincohen wrote: > +treib@ > > The destination is set by SetServerAPI. For example, in > ios/chrome/browser/google/google_logo_service.mm this is set in > GetGoogleDoodleURL as a GOOGLE_OWNED_SERVICE > > It's not really known within FetchLogo if this is really a GOOGLE_OWNED_SERVICE. I think it's more in line with the intention of this code to call this "OTHER", even though in practice it's always set to Google. > FWIW, there's a refactor coming to logo_tracker that I believe will remove this > ambiguity, see go/ntp-doodles Indeed, the new components/doodle will make this unambiguously GOOGLE_OWNED_SERVICE. But it'll still be a while before we can remove this code. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:242: setting: "..." On 2017/02/23 15:14:29, justincohen wrote: > You can't really disable this, not sure what goes here? Well, choosing a non-Google default search engine will effectively disable this, at least on Android.
https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:242: setting: "..." On 2017/02/23 15:27:43, Marc Treib wrote: > On 2017/02/23 15:14:29, justincohen wrote: > > You can't really disable this, not sure what goes here? > > Well, choosing a non-Google default search engine will effectively disable this, > at least on Android. That's correct -- although the logo tracker doesn't know this. By design other search engines could implement their own end points. In practice, like 'destination' above, this will be disabled.
Annotation updated, please review. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:233: sender: "..." On 2017/02/23 15:14:29, justincohen wrote: > logo_tracker Done. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:234: description: "..." On 2017/02/23 15:14:30, justincohen wrote: > Provides the logo image for a search provider (e.g. Google or Yahoo!) Done. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:235: trigger: "..." On 2017/02/23 15:14:30, justincohen wrote: > Displaying the new tab page on iOS or Android. Done. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:236: data: "..." On 2017/02/23 15:14:30, justincohen wrote: > Not sure here? Isn't it the link to the logo image? https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:237: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/23 15:27:43, Marc Treib wrote: > On 2017/02/23 15:14:30, justincohen wrote: > > +treib@ > > > > The destination is set by SetServerAPI. For example, in > > ios/chrome/browser/google/google_logo_service.mm this is set in > > GetGoogleDoodleURL as a GOOGLE_OWNED_SERVICE > > > > It's not really known within FetchLogo if this is really a > GOOGLE_OWNED_SERVICE. > > I think it's more in line with the intention of this code to call this "OTHER", > even though in practice it's always set to Google. > > > FWIW, there's a refactor coming to logo_tracker that I believe will remove > this > > ambiguity, see go/ntp-doodles > > Indeed, the new components/doodle will make this unambiguously > GOOGLE_OWNED_SERVICE. But it'll still be a while before we can remove this code. Done. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:240: cookies_allowed: false/true On 2017/02/23 15:14:30, justincohen wrote: > Not sure here, I think yes? Yes, it's enabled by default, but if you don't need it, I can first create a CL to disable it and then land this one with false. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:242: setting: "..." On 2017/02/23 15:29:56, justincohen wrote: > On 2017/02/23 15:27:43, Marc Treib wrote: > > On 2017/02/23 15:14:29, justincohen wrote: > > > You can't really disable this, not sure what goes here? > > > > Well, choosing a non-Google default search engine will effectively disable > this, > > at least on Android. > > That's correct -- although the logo tracker doesn't know this. By design other > search engines could implement their own end points. In practice, like > 'destination' above, this will be disabled. Please verify the update.
Looks fine to me; I'll let Justin approve as owner. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:236: data: "..." On 2017/02/24 10:37:15, Ramin Halavati wrote: > On 2017/02/23 15:14:30, justincohen wrote: > > Not sure here? > > Isn't it the link to the logo image? It can also contain the logo image data itself, and there's some metadata (image size, time-to-live, link to an animated image, probably some other things). https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:240: cookies_allowed: false/true On 2017/02/24 10:37:15, Ramin Halavati wrote: > On 2017/02/23 15:14:30, justincohen wrote: > > Not sure here, I think yes? > > Yes, it's enabled by default, but if you don't need it, I can first create a CL > to disable it and then land this one with false. I *think* cookies aren't required here, but it's probably fine to just leave it as-is for now. https://codereview.chromium.org/2718443002/diff/20001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/20001/components/search_provi... components/search_provider_logos/logo_tracker.cc:245: "Choosing a non-Google search engine in Chromium settings under " Should this be "Chrome" rather than "Chromium"? I seem to remember that some other annotations use "Chrome".
https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:236: data: "..." On 2017/02/24 17:05:51, Marc Treib wrote: > On 2017/02/24 10:37:15, Ramin Halavati wrote: > > On 2017/02/23 15:14:30, justincohen wrote: > > > Not sure here? > > > > Isn't it the link to the logo image? > > It can also contain the logo image data itself, and there's some metadata (image > size, time-to-live, link to an animated image, probably some other things). This still needs to be updated? treib@ is correct here.
Comments addressed, please review. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:236: data: "..." On 2017/02/24 20:29:41, justincohen (OOO until Mar 20) wrote: > On 2017/02/24 17:05:51, Marc Treib wrote: > > On 2017/02/24 10:37:15, Ramin Halavati wrote: > > > On 2017/02/23 15:14:30, justincohen wrote: > > > > Not sure here? > > > > > > Isn't it the link to the logo image? > > > > It can also contain the logo image data itself, and there's some metadata > (image > > size, time-to-live, link to an animated image, probably some other things). > > This still needs to be updated? treib@ is correct here. Done. https://codereview.chromium.org/2718443002/diff/1/components/search_provider_... components/search_provider_logos/logo_tracker.cc:240: cookies_allowed: false/true On 2017/02/24 17:05:51, Marc Treib wrote: > On 2017/02/24 10:37:15, Ramin Halavati wrote: > > On 2017/02/23 15:14:30, justincohen wrote: > > > Not sure here, I think yes? > > > > Yes, it's enabled by default, but if you don't need it, I can first create a > CL > > to disable it and then land this one with false. > > I *think* cookies aren't required here, but it's probably fine to just leave it > as-is for now. Done. https://codereview.chromium.org/2718443002/diff/20001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/20001/components/search_provi... components/search_provider_logos/logo_tracker.cc:245: "Choosing a non-Google search engine in Chromium settings under " On 2017/02/24 17:05:51, Marc Treib wrote: > Should this be "Chrome" rather than "Chromium"? I seem to remember that some > other annotations use "Chrome". We have started to use Chromium instead and will updated previous annotations later.
https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Link to logo image and metadata." Wait, if I interpret the proto description correctly, then this should describe what Chrome *sends*, right? The logo image and metadata are *received* by Chrome; I don't think we send anything noteworthy. Sorry for the back and forth!
https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Link to logo image and metadata." On 2017/02/27 09:52:59, Marc Treib wrote: > Wait, if I interpret the proto description correctly, then this should describe > what Chrome *sends*, right? > The logo image and metadata are *received* by Chrome; I don't think we send > anything noteworthy. > Sorry for the back and forth! Yes, this is what Chrome sends, so is the current annotation for |data| correct or you'd like it rephrased?
https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Link to logo image and metadata." On 2017/02/27 10:04:30, Ramin Halavati wrote: > On 2017/02/27 09:52:59, Marc Treib wrote: > > Wait, if I interpret the proto description correctly, then this should > describe > > what Chrome *sends*, right? > > The logo image and metadata are *received* by Chrome; I don't think we send > > anything noteworthy. > > Sorry for the back and forth! > > Yes, this is what Chrome sends, so is the current annotation for |data| correct > or you'd like it rephrased? The current formulation refers to what Chrome receives, so it needs to be changed. As far as I can tell, the only thing Chrome sends (apart from cookies, which are probably not even required) is a "fingerprint" for the logo that we currently have cached. Note that this isn't user-specific, it just identifies the image.
Annotation updated, please review. https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Link to logo image and metadata." On 2017/02/27 10:12:47, Marc Treib wrote: > On 2017/02/27 10:04:30, Ramin Halavati wrote: > > On 2017/02/27 09:52:59, Marc Treib wrote: > > > Wait, if I interpret the proto description correctly, then this should > > describe > > > what Chrome *sends*, right? > > > The logo image and metadata are *received* by Chrome; I don't think we send > > > anything noteworthy. > > > Sorry for the back and forth! > > > > Yes, this is what Chrome sends, so is the current annotation for |data| > correct > > or you'd like it rephrased? > > The current formulation refers to what Chrome receives, so it needs to be > changed. > > As far as I can tell, the only thing Chrome sends (apart from cookies, which are > probably not even required) is a "fingerprint" for the logo that we currently > have cached. Note that this isn't user-specific, it just identifies the image. Done, in case cookies are not required, I can create a CL that disables them and base this one on that.
lgtm https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Link to logo image and metadata." On 2017/02/27 10:16:54, Ramin Halavati wrote: > On 2017/02/27 10:12:47, Marc Treib wrote: > > On 2017/02/27 10:04:30, Ramin Halavati wrote: > > > On 2017/02/27 09:52:59, Marc Treib wrote: > > > > Wait, if I interpret the proto description correctly, then this should > > > describe > > > > what Chrome *sends*, right? > > > > The logo image and metadata are *received* by Chrome; I don't think we > send > > > > anything noteworthy. > > > > Sorry for the back and forth! > > > > > > Yes, this is what Chrome sends, so is the current annotation for |data| > > correct > > > or you'd like it rephrased? > > > > The current formulation refers to what Chrome receives, so it needs to be > > changed. > > > > As far as I can tell, the only thing Chrome sends (apart from cookies, which > are > > probably not even required) is a "fingerprint" for the logo that we currently > > have cached. Note that this isn't user-specific, it just identifies the image. > > Done, in case cookies are not required, I can create a CL that disables them and > base this one on that. The problem is, I'm not sure how we could properly test this. Justin is OOO, and I don't want to make that call by myself. How about just adding a TODO for now?
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/40001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Link to logo image and metadata." On 2017/02/27 10:23:02, Marc Treib wrote: > On 2017/02/27 10:16:54, Ramin Halavati wrote: > > On 2017/02/27 10:12:47, Marc Treib wrote: > > > On 2017/02/27 10:04:30, Ramin Halavati wrote: > > > > On 2017/02/27 09:52:59, Marc Treib wrote: > > > > > Wait, if I interpret the proto description correctly, then this should > > > > describe > > > > > what Chrome *sends*, right? > > > > > The logo image and metadata are *received* by Chrome; I don't think we > > send > > > > > anything noteworthy. > > > > > Sorry for the back and forth! > > > > > > > > Yes, this is what Chrome sends, so is the current annotation for |data| > > > correct > > > > or you'd like it rephrased? > > > > > > The current formulation refers to what Chrome receives, so it needs to be > > > changed. > > > > > > As far as I can tell, the only thing Chrome sends (apart from cookies, which > > are > > > probably not even required) is a "fingerprint" for the logo that we > currently > > > have cached. Note that this isn't user-specific, it just identifies the > image. > > > > Done, in case cookies are not required, I can create a CL that disables them > and > > base this one on that. > > The problem is, I'm not sure how we could properly test this. Justin is OOO, and > I don't want to make that call by myself. > How about just adding a TODO for now? I see, 'send' is just metadata in this case. What do you mean 'disables them and base this one on that' ?
LGTM
+battre@, msramek@: Do you have privacy comments? justincohen@: I meant if this request doesn't need cookies, I can 1-Create a CL that adds do not use cookies to load flags of this network request. 2-Change uses_cookies field of annotation to false. 3-Base this CL on the one step 1.
I do not know if the current endpoint supports birthday doodles -- but if it does, we will likely want to keep cookie support.
On 2017/02/27 14:33:04, justincohen (OOO until Mar 20) wrote: > I do not know if the current endpoint supports birthday doodles -- but if it > does, we will likely want to keep cookie support. OK, thanks.
https://codereview.chromium.org/2718443002/diff/60001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/60001/components/search_provi... components/search_provider_logos/logo_tracker.cc:236: "Yahoo!)." Does it make sense to list Yahoo here if this only works for Google? How about "Provides the logo image (ake Doodle) if Google is your configured search provider."?
Comment addressed, please review. https://codereview.chromium.org/2718443002/diff/60001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/60001/components/search_provi... components/search_provider_logos/logo_tracker.cc:236: "Yahoo!)." On 2017/02/28 08:17:11, battre wrote: > Does it make sense to list Yahoo here if this only works for Google? > > How about "Provides the logo image (ake Doodle) if Google is your configured > search provider."? Done.
https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... components/search_provider_logos/logo_tracker.cc:235: "Provides the logo image (ake Doodle) if Google is your configured " sorry, this should be "aka" (also known as) https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Fingerprint for the logo." What does "Fingerprint for the logo." mean? That sounds very weird to me. Add: "Contains the user's Google cookies to show for example birthday doodles at appropriate times."
https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Fingerprint for the logo." On 2017/03/08 14:23:01, battre wrote: > What does "Fingerprint for the logo." mean? That sounds very weird to me. > > Add: "Contains the user's Google cookies to show for example birthday doodles at > appropriate times." The fingerprint is used for caching the doodle. The doodle metadata includes a fingerprint, and when requesting the next doodle the fingerprint is submitted along with the doodle request.
https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Fingerprint for the logo." On 2017/03/08 16:27:57, justincohen (OOO until Mar 20) wrote: > On 2017/03/08 14:23:01, battre wrote: > > What does "Fingerprint for the logo." mean? That sounds very weird to me. > > > > Add: "Contains the user's Google cookies to show for example birthday doodles > at > > appropriate times." > > The fingerprint is used for caching the doodle. The doodle metadata includes a > fingerprint, and when requesting the next doodle the fingerprint is submitted > along with the doodle request. Would it be fair to refer to this as an ID? I think that fingerprints are typically associated with user tracking based on properties of the user's computer.
https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Fingerprint for the logo." On 2017/03/08 16:32:14, battre wrote: > On 2017/03/08 16:27:57, justincohen (OOO until Mar 20) wrote: > > On 2017/03/08 14:23:01, battre wrote: > > > What does "Fingerprint for the logo." mean? That sounds very weird to me. > > > > > > Add: "Contains the user's Google cookies to show for example birthday > doodles > > at > > > appropriate times." > > > > The fingerprint is used for caching the doodle. The doodle metadata includes > a > > fingerprint, and when requesting the next doodle the fingerprint is submitted > > along with the doodle request. > > Would it be fair to refer to this as an ID? I think that fingerprints are > typically associated with user tracking based on properties of the user's > computer. sgtm
Comments addressed, landing. https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... File components/search_provider_logos/logo_tracker.cc (right): https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... components/search_provider_logos/logo_tracker.cc:235: "Provides the logo image (ake Doodle) if Google is your configured " On 2017/03/08 14:23:01, battre wrote: > sorry, this should be "aka" (also known as) Done. https://codereview.chromium.org/2718443002/diff/80001/components/search_provi... components/search_provider_logos/logo_tracker.cc:238: data: "Fingerprint for the logo." On 2017/03/08 16:33:06, justincohen (OOO until Mar 20) wrote: > On 2017/03/08 16:32:14, battre wrote: > > On 2017/03/08 16:27:57, justincohen (OOO until Mar 20) wrote: > > > On 2017/03/08 14:23:01, battre wrote: > > > > What does "Fingerprint for the logo." mean? That sounds very weird to me. > > > > > > > > Add: "Contains the user's Google cookies to show for example birthday > > doodles > > > at > > > > appropriate times." > > > > > > The fingerprint is used for caching the doodle. The doodle metadata > includes > > a > > > fingerprint, and when requesting the next doodle the fingerprint is > submitted > > > along with the doodle request. > > > > Would it be fair to refer to this as an ID? I think that fingerprints are > > typically associated with user tracking based on properties of the user's > > computer. > > sgtm Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, justincohen@chromium.org Link to the patchset: https://codereview.chromium.org/2718443002/#ps100001 (title: "Comments addressed.")
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": 1489042778162900,
"parent_rev": "98832b81e0bb8f06df0c49ce9b0a262d4b47dea3", "commit_rev":
"4d30c69479ce696af07f93f0677ba7baac484cab"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to logo_tracker. Network traffic annotation is added to network request of search_provider_logos/logo_tracker. BUG=656607 ========== to ========== Network traffic annotation added to logo_tracker. Network traffic annotation is added to network request of search_provider_logos/logo_tracker. BUG=656607 Review-Url: https://codereview.chromium.org/2718443002 Cr-Commit-Position: refs/heads/master@{#455694} Committed: https://chromium.googlesource.com/chromium/src/+/4d30c69479ce696af07f93f0677b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4d30c69479ce696af07f93f0677b... |
