Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1162123002:
Using render IPC to record RAPPOR metric ContentSettings.MixedScript.RanMixedScript (Closed)
|
Created:
5 years, 6 months ago by Jialiu Lin Modified:
5 years, 6 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUsing render IPC to record rappor metric ContentSettings.MixedScript.RanMixedScript
BUG=490462
Committed: https://crrev.com/1931f075a257061079dc4ea9011ad7f435eafe83
Cr-Commit-Position: refs/heads/master@{#334119}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 20 (7 generated)
jialiul@chromium.org changed reviewers: + felt@chromium.org, holte@google.com, lgarron@chromium.org
This is a better way to record ContentSettings.MixedScript.RanMixedScript RAPPOR metric comparing to my previous CL. Thanks to render's RecordRapporURL IPC.
holte@chromium.org changed reviewers: + holte@chromium.org
lgtm
jialiul@chromium.org changed reviewers: + nasko@chromium.org
Add nasko@chromium.org for content/renderer/render_frame_impl.cc owner review. Thank you!
The code looks good, but the comment seems wrong to me. https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... tools/metrics/rappor/rappor.xml:142: The eTLD+1 of a URL that when mixed script actually ran. The code will log the origin, not the eTLD+1, right? If we have foo.bar.some.sub.domain.com, origin will include the full string, where eTLD will be just domain.com.
https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... tools/metrics/rappor/rappor.xml:142: The eTLD+1 of a URL that when mixed script actually ran. On 2015/06/01 20:52:50, nasko wrote: > The code will log the origin, not the eTLD+1, right? If we have > http://foo.bar.some.sub.domain.com, origin will include the full string, where eTLD > will be just http://domain.com. All RAPPOR metrics are actually recorded as eTLD+1 for the time being: https://www.chromium.org/developers/design-documents/rappor See uses of the the neatly named "GetDomainAndRegistrySampleFromGURL" function.
LGTM https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... tools/metrics/rappor/rappor.xml:142: The eTLD+1 of a URL that when mixed script actually ran. On 2015/06/01 20:57:04, lgarron wrote: > On 2015/06/01 20:52:50, nasko wrote: > > The code will log the origin, not the eTLD+1, right? If we have > > http://foo.bar.some.sub.domain.com, origin will include the full string, where > eTLD > > will be just http://domain.com. > > All RAPPOR metrics are actually recorded as eTLD+1 for the time being: > https://www.chromium.org/developers/design-documents/rappor > > See uses of the the neatly named "GetDomainAndRegistrySampleFromGURL" function. I think it isn't entirely reasonable to expect reviewers to read the design doc for every CL. Adding this bit of data on the comment of the ContentRendererClient::RecordRapporURL interface will be fairly useful.
jialiul@chromium.org changed reviewers: - holte@google.com
The CQ bit was checked by jialiul@chromium.org
The CQ bit was unchecked by jialiul@chromium.org
https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... tools/metrics/rappor/rappor.xml:142: The eTLD+1 of a URL that when mixed script actually ran. On 2015/06/01 at 21:07:02, nasko wrote: > On 2015/06/01 20:57:04, lgarron wrote: > > On 2015/06/01 20:52:50, nasko wrote: > > > The code will log the origin, not the eTLD+1, right? If we have > > > http://foo.bar.some.sub.domain.com, origin will include the full string, where > > eTLD > > > will be just http://domain.com. > > > > All RAPPOR metrics are actually recorded as eTLD+1 for the time being: > > https://www.chromium.org/developers/design-documents/rappor > > > > See uses of the the neatly named "GetDomainAndRegistrySampleFromGURL" function. > > I think it isn't entirely reasonable to expect reviewers to read the design doc for every CL. Adding this bit of data on the comment of the ContentRendererClient::RecordRapporURL interface will be fairly useful. The design doc isn't necessary, but anyone working with RAPPOR in Chrome will presumably know this. Also, the implementation of the convenience functions could be updated to report different parts of the GURL (e.g. scheme, entire host).
Gently ping Adrienne~ Do you have any other comment?
lgtm https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... tools/metrics/rappor/rappor.xml:142: The eTLD+1 of a URL that when mixed script actually ran. On 2015/06/01 22:56:47, lgarron wrote: > On 2015/06/01 at 21:07:02, nasko wrote: > > On 2015/06/01 20:57:04, lgarron wrote: > > > On 2015/06/01 20:52:50, nasko wrote: > > > > The code will log the origin, not the eTLD+1, right? If we have > > > > http://foo.bar.some.sub.domain.com, origin will include the full string, > where > > > eTLD > > > > will be just http://domain.com. > > > > > > All RAPPOR metrics are actually recorded as eTLD+1 for the time being: > > > https://www.chromium.org/developers/design-documents/rappor > > > > > > See uses of the the neatly named "GetDomainAndRegistrySampleFromGURL" > function. > > > > I think it isn't entirely reasonable to expect reviewers to read the design > doc for every CL. Adding this bit of data on the comment of the > ContentRendererClient::RecordRapporURL interface will be fairly useful. > > The design doc isn't necessary, but anyone working with RAPPOR in Chrome will > presumably know this. > Also, the implementation of the convenience functions could be updated to report > different parts of the GURL (e.g. scheme, entire host). FWIW, I came into this file with the same question as Nasko. ("Isn't the code logging the origin, not the eTLD+1?") I don't think it needs a code comment because it's standard for RAPPOR metrics, but it would have been useful as a note to the reviewer when the CL was mailed. :)
On 2015/06/12 00:32:52, felt wrote: > lgtm > > https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... > File tools/metrics/rappor/rappor.xml (right): > > https://codereview.chromium.org/1162123002/diff/1/tools/metrics/rappor/rappor... > tools/metrics/rappor/rappor.xml:142: The eTLD+1 of a URL that when mixed script > actually ran. > On 2015/06/01 22:56:47, lgarron wrote: > > On 2015/06/01 at 21:07:02, nasko wrote: > > > On 2015/06/01 20:57:04, lgarron wrote: > > > > On 2015/06/01 20:52:50, nasko wrote: > > > > > The code will log the origin, not the eTLD+1, right? If we have > > > > > http://foo.bar.some.sub.domain.com, origin will include the full string, > > where > > > > eTLD > > > > > will be just http://domain.com. > > > > > > > > All RAPPOR metrics are actually recorded as eTLD+1 for the time being: > > > > https://www.chromium.org/developers/design-documents/rappor > > > > > > > > See uses of the the neatly named "GetDomainAndRegistrySampleFromGURL" > > function. > > > > > > I think it isn't entirely reasonable to expect reviewers to read the design > > doc for every CL. Adding this bit of data on the comment of the > > ContentRendererClient::RecordRapporURL interface will be fairly useful. > > > > The design doc isn't necessary, but anyone working with RAPPOR in Chrome will > > presumably know this. > > Also, the implementation of the convenience functions could be updated to > report > > different parts of the GURL (e.g. scheme, entire host). > > FWIW, I came into this file with the same question as Nasko. ("Isn't the code > logging the origin, not the eTLD+1?") I don't think it needs a code comment > because it's standard for RAPPOR metrics, but it would have been useful as a > note to the reviewer when the CL was mailed. :) You're right. I probably should put a note in the description. Thank you, all reviewers.
The CQ bit was checked by jialiul@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162123002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/1931f075a257061079dc4ea9011ad7f435eafe83 Cr-Commit-Position: refs/heads/master@{#334119} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
