Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1014543003:
Add an IPC method for Blink to call RapporService::RecordSample() (Closed)
|
Created:
5 years, 9 months ago by kojii Modified:
5 years, 8 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, 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. |
DescriptionAdd an IPC method for Blink to call RapporService::RecordSample()
This patch adds an IPC method for Blink to RapporService::RecordSample()
to send data to RAPPOR. Combined with a Blink CL to collect hosts of Web
Component usage (the link below), the data collected in Blink is sent
to RapporService. Blink patch is in the following CL:
https://codereview.chromium.org/986583002/
rappor.xml change is in a separate CL at:
https://codereview.chromium.org/1071463003/
Another accompanying method to record a domain and registry of a URL is
also added for bug 467648. Its Blink CL is at:
https://codereview.chromium.org/1017433002/
BUG=461671, 429901
Committed: https://crrev.com/0f931925f7c0b966c2eb83321886077d30127964
Cr-Commit-Position: refs/heads/master@{#324563}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase #Patch Set 3 : Changed to ETLD_PLUS_ONE #
Total comments: 3
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Add IPC_MESSAGE_EXPORT #Patch Set 7 : #Patch Set 8 : ContentRendererClient #Patch Set 9 : Comments and cleanup #
Total comments: 5
Patch Set 10 : ChromeRenderMessageFilter, and add a URL version for bug 467648 #
Total comments: 13
Patch Set 11 : Rebase #Patch Set 12 : Fix all review comments, ran "git cl format" #
Total comments: 13
Patch Set 13 : Rebase #Patch Set 14 : Add comments as suggested by asvitkine@ #Patch Set 15 : Add URL to the comments suggested by asvitkine@ #Patch Set 16 : Use SampleString in rappor_utils #Messages
Total messages: 62 (21 generated)
kojii@chromium.org changed reviewers: + asvitkine@chromium.org, hayato@chromium.org, holte@chromium.org, joi@chromium.org, tkent@chromium.org
PTAL.
I'm guessing you added me as a reviewer for content/public, but I'm no longer an OWNER there. Please check the OWNERS file. Cheers, Jói On Tue, Mar 17, 2015 at 6:41 AM, <kojii@chromium.org> wrote: > Reviewers: Steven Holte, hayato, tkent, Alexei Svitkine (slow), Jói, > > Message: > PTAL. > > Description: > Add an IPC method for Blink to call RapporService::RecordSample() > > This patch adds an IPC method for Blink to > RapporService::RecordSample() to send data to > RAPPOR. Combined with a Blink CL to collect hosts > of Web Component usage (the link below), the data > collected in Blink is sent to RapporService. > > https://codereview.chromium.org/986583002/ > > BUG=461671 > > Please review this at https://codereview.chromium.org/1014543003/ > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+135, -0 lines): > M base/metrics/user_metrics.h > M base/metrics/user_metrics.cc > M components/rappor/rappor_service.h > M components/rappor/rappor_service.cc > M content/browser/renderer_host/render_process_host_impl.h > M content/browser/renderer_host/render_process_host_impl.cc > M content/browser/user_metrics.cc > M content/common/view_messages.h > M content/public/browser/user_metrics.h > M content/public/renderer/render_thread.h > M content/public/test/mock_render_thread.h > M content/public/test/mock_render_thread.cc > M content/renderer/render_thread_impl.h > M content/renderer/render_thread_impl.cc > M content/renderer/renderer_blink_platform_impl.h > M content/renderer/renderer_blink_platform_impl.cc > M ios/web/public/user_metrics.h > M tools/metrics/rappor/rappor.xml > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/03/18 10:45:14, Jói wrote: > I'm guessing you added me as a reviewer for content/public, but I'm no > longer an OWNER there. Please check the OWNERS file. Sorry for bothering, I referred to RAPPOR implementation CL https://codereview.chromium.org/49753002 without checking updates since then. I'll update reviewers.
kojii@chromium.org changed reviewers: + isherman@chromium.org, sky@chromium.org - joi@chromium.org
https://codereview.chromium.org/1014543003/diff/1/components/rappor/rappor_se... File components/rappor/rappor_service.cc (right): https://codereview.chromium.org/1014543003/diff/1/components/rappor/rappor_se... components/rappor/rappor_service.cc:198: RecordSample(metric, COARSE_RAPPOR_TYPE, sample); This should probably be ETLD_PLUS_ONE type or just passed as a parameter. COARSE_RAPPOR_TYPE is just for SafeBrowsing metrics now. https://codereview.chromium.org/1014543003/diff/1/tools/metrics/rappor/rappor... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1014543003/diff/1/tools/metrics/rappor/rappor... tools/metrics/rappor/rappor.xml:238: <rappor-metric name="WebComponents.DocumentRegisterElement" type="COARSE_RAPPOR_TYPE"> These metrics should use ETLD_PLUS_ONE type.
What files did you want me to review?
Thank you Steven. https://codereview.chromium.org/1014543003/diff/1/tools/metrics/rappor/rappor... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1014543003/diff/1/tools/metrics/rappor/rappor... tools/metrics/rappor/rappor.xml:238: <rappor-metric name="WebComponents.DocumentRegisterElement" type="COARSE_RAPPOR_TYPE"> On 2015/03/23 20:34:06, Steven Holte wrote: > These metrics should use ETLD_PLUS_ONE type. Done, the call to RecordSample was also changed.
isherman@, sorry for not being clear.
On 2015/03/23 20:54:31, Ilya Sherman wrote:
> What files did you want me to review?
It'd be great if you could review base/metrics/user_metrics.{h,cc}. Thank you
for your support in advance.
https://codereview.chromium.org/1014543003/diff/40001/content/renderer/render... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1014543003/diff/40001/content/renderer/render... content/renderer/renderer_blink_platform_impl.cc:1014: RenderThreadImpl::current()->RecordRappor(metric, sample.utf8()); What's the format of the sample? Is it a full host like "codereview.chromium.org"? Most places that use RAPPOR currently use GetDomainAndRegistrySampleFromGURL to reduce samples down to just domain and registry like "chromium.org". If you include more this may make it a little more difficult to analyze your results, since you will have a longer long tail that will be lost in the noise, and will need a longer list of candidate host names.
timvolodine@chromium.org changed reviewers: + timvolodine@chromium.org
https://codereview.chromium.org/1014543003/diff/40001/content/renderer/render... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1014543003/diff/40001/content/renderer/render... content/renderer/renderer_blink_platform_impl.cc:1013: const char* metric, const blink::WebString& sample) { koji@: can we simply use blink::WebURL here? and then send the "sample" as a GURL to the browser where GetDomainAndRegistrySampleFromGURL can be used on it. In my patch I am using ContentBrowserClient::RecordURLMetric(metric, url) and I am afraid your implementation is not exactly equivalent to this.
I'd strongly prefer to find a way to implement this change without modifying code in the //base or //content layers. Is there a reason why this IPC message cannot be defined with the RAPPOR component? https://codereview.chromium.org/1014543003/diff/40001/base/metrics/user_metri... File base/metrics/user_metrics.h (right): https://codereview.chromium.org/1014543003/diff/40001/base/metrics/user_metri... base/metrics/user_metrics.h:54: const std::string& sample); RAPPOR is not a concept that current exists in base. I'm not really convinced that it's appropriate to add it here. But, if we do decide that //base is the right place to add it, please create a new file for it. The user_metrics files are for user actions, not for all UMA metrics. (I do realize that the name is somewhat confusing :/)
On 2015/03/25 20:51:02, Ilya Sherman wrote: > I'd strongly prefer to find a way to implement this change without modifying > code in the //base or //content layers. Is there a reason why this IPC message > cannot be defined with the RAPPOR component? > > https://codereview.chromium.org/1014543003/diff/40001/base/metrics/user_metri... > File base/metrics/user_metrics.h (right): > > https://codereview.chromium.org/1014543003/diff/40001/base/metrics/user_metri... > base/metrics/user_metrics.h:54: const std::string& sample); > RAPPOR is not a concept that current exists in base. I'm not really convinced > that it's appropriate to add it here. But, if we do decide that //base is the > right place to add it, please create a new file for it. The user_metrics files > are for user actions, not for all UMA metrics. (I do realize that the name is > somewhat confusing :/) Looking for cues from https://sites.google.com/a/chromium.org/dev/developers/design-documents/multi... Maybe instead of the code in base/metrics with plumbing in content/browser, add WebContentObserver or BrowserMessageFilter derived class to the components/rappor code, and register an instance of it from chrome/browser (maybe chrome/browser/chrome_content_browser_client.cc?)
On 2015/03/25 17:57:26, Steven Holte wrote: > https://codereview.chromium.org/1014543003/diff/40001/content/renderer/render... > File content/renderer/renderer_blink_platform_impl.cc (right): > > https://codereview.chromium.org/1014543003/diff/40001/content/renderer/render... > content/renderer/renderer_blink_platform_impl.cc:1014: > RenderThreadImpl::current()->RecordRappor(metric, sample.utf8()); > What's the format of the sample? Is it a full host like > "codereview.chromium.org"? Yes. > Most places that use RAPPOR currently use > GetDomainAndRegistrySampleFromGURL to reduce samples down to just domain and > registry like "chromium.org". If you include more this may make it a little > more difficult to analyze your results, since you will have a longer long tail > that will be lost in the noise, and will need a longer list of candidate host > names. Yeah, I discussed with PM, and he prefers longer list in this case. His response pasted below. | Ideally, if the page is https://myawesome.example.co.uk/something/blah.html | => myawesome.example.co.uk | That being said, I don't think RAPPOR lets you do anything beyond eTLD+1 | so we would only know example.co.uk but this is fine too.
On 2015/03/25 18:56:19, timvolodine wrote: > https://codereview.chromium.org/1014543003/diff/40001/content/renderer/render... > File content/renderer/renderer_blink_platform_impl.cc (right): > > https://codereview.chromium.org/1014543003/diff/40001/content/renderer/render... > content/renderer/renderer_blink_platform_impl.cc:1013: const char* metric, const > blink::WebString& sample) { > koji@: can we simply use blink::WebURL here? and then send the "sample" as a > GURL to the browser where GetDomainAndRegistrySampleFromGURL can be used on it. > > In my patch I am using ContentBrowserClient::RecordURLMetric(metric, url) and I > am afraid your implementation is not exactly equivalent to this. As in comment #15, my use case has mild preference not to call GetDomainAndRegistrySampleFromGURL(). Sounds like we need two methods, or a flag whether we want full hostname or domain and registry only. That needs to go back to change Blink API first. Let me investigate what the best way to handle both requests is.
I think it makes the most sense for this to be handled by the renderer, so the sampling API just takes a string. The code for computing a domain sample should only have real dependencies on net/url On Mar 26, 2015 10:45 PM, <kojii@chromium.org> wrote: > On 2015/03/25 18:56:19, timvolodine wrote: > > https://codereview.chromium.org/1014543003/diff/40001/ > content/renderer/renderer_blink_platform_impl.cc > >> File content/renderer/renderer_blink_platform_impl.cc (right): >> > > > https://codereview.chromium.org/1014543003/diff/40001/ > content/renderer/renderer_blink_platform_impl.cc#newcode1013 > >> content/renderer/renderer_blink_platform_impl.cc:1013: const char* >> metric, >> > const > >> blink::WebString& sample) { >> koji@: can we simply use blink::WebURL here? and then send the "sample" >> as a >> GURL to the browser where GetDomainAndRegistrySampleFromGURL can be used >> on >> > it. > > In my patch I am using ContentBrowserClient::RecordURLMetric(metric, >> url) and >> > I > >> am afraid your implementation is not exactly equivalent to this. >> > > As in comment #15, my use case has mild preference not to call > GetDomainAndRegistrySampleFromGURL(). > > Sounds like we need two methods, or a flag whether we want full hostname or > domain and registry only. That needs to go back to change Blink API first. > Let > me investigate what the best way to handle both requests is. > > https://codereview.chromium.org/1014543003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/03/25 20:51:02, Ilya Sherman wrote: > I'd strongly prefer to find a way to implement this change without modifying > code in the //base or //content layers. Is there a reason why this IPC message > cannot be defined with the RAPPOR component? > > https://codereview.chromium.org/1014543003/diff/40001/base/metrics/user_metri... > File base/metrics/user_metrics.h (right): > > https://codereview.chromium.org/1014543003/diff/40001/base/metrics/user_metri... > base/metrics/user_metrics.h:54: const std::string& sample); > RAPPOR is not a concept that current exists in base. I'm not really convinced > that it's appropriate to add it here. But, if we do decide that //base is the > right place to add it, please create a new file for it. The user_metrics files > are for user actions, not for all UMA metrics. (I do realize that the name is > somewhat confusing :/) Thank you isherman@ for the feedback. I thought RAPPOR is one of user metrics and intentionally tried to go the same route as user_metrics, but now I understand that was a wrong choice. I'll change to IPC in the RAPPOR component.
kojii@chromium.org changed reviewers: + jam@chromium.org
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
content can't depend on components (ignore content/shell, and on e existing incorrect dependency). so you have two choices: -make content call to chrome (either in the browser through ContentBrowserClient after it dispatches the IPC, or in the renderer to ContentRendererClient) -chrome can use dependency injection to give blink an interface to record these messages, i.e. see WebAutofillClient
Patchset #4 (id:190001) has been deleted
Thank you jam@, I was wondering which depends on which and tried to figure that out, that's helpful. Reading Layered component design: http://www.chromium.org/developers/design-documents/layered-components-design so content can't depend on component, but component/content can, correct? I think I need to share a struct, either the DI interface or IPC_MESSAGE, between the two, so defining that in component/content/common is the correct way, no? With unresolved external error, now I found IPC_MESSAGE_EXPORT macro, I'm trying that.
On 2015/04/01 04:13:31, koji wrote: > Thank you jam@, I was wondering which depends on which and tried to figure that > out, that's helpful. > > Reading Layered component design: > http://www.chromium.org/developers/design-documents/layered-components-design > so content can't depend on component, but component/content can, correct? code in components can depend on content. I'd ignore the layered-components stuff for now since that's for code that's shared with ios and the rest of the platforms, but this is clearly only blink. > > I think I need to share a struct, either the DI interface or IPC_MESSAGE, > between the two, so defining that in component/content/common is the correct > way, no? which struct do you want to share? IPC messages don't cross module boundaries, which is why I suggested one of these two forms. > > With unresolved external error, now I found IPC_MESSAGE_EXPORT macro, I'm trying > that.
On 2015/04/01 15:52:07, jam wrote: > > > > I think I need to share a struct, either the DI interface or IPC_MESSAGE, > > between the two, so defining that in component/content/common is the correct > > way, no? > > which struct do you want to share? IPC messages don't cross module boundaries, > which is why I suggested one of these two forms. I've added IPC_MESSAGE_EXPOEPRT to rappor_messages.h https://codereview.chromium.org/1014543003/diff/260001/components/rappor/cont... which successfully compile on my local win/mac, but not on bots. Looks like bots are using some different settings? What do you mean by "IPC messages don't cross module boundaries"? Is that wrong to use IPC_MESSAGE_EXPORT to define it in content and refer it from browser? Maybe I should give up doing this, add a method to ContentBrowserClient and pass all by arguments? I'll try this next.
kojii@chromium.org changed reviewers: - asvitkine@chromium.org, isherman@chromium.org, sky@chromium.org
Patchset #9 (id:300001) has been deleted
Tried ContentBrowserClient. mac_chromimum_ref_ng is failing, stil haven't figured out why, but I ended up with: * ChromeContentRendererClient to Send() because I need RenderThread. * ChromeContentBrowserClient to AddFilter() because I need g_browser_process. Is this correct combination? I can keep the pointer to RapporService and avoid using g_browser_process if both of these two should be done in ChromeContentRendererClient, but I don't have good idea whether this is ok, or both of these two should be done in the same class.
this seems like a lot of plumbing to make the IPC in componetns and the filter there. since chrome already calls the rappor service, why not just put the IPC and the filtering (in ChromeRenderMessageFilter) and then just call out to components there? https://codereview.chromium.org/1014543003/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1014543003/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:859: host->AddFilter(new rappor::RapporMessageFilter( nit: add this above where the other filters are added https://codereview.chromium.org/1014543003/diff/320001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/1014543003/diff/320001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:178: const std::string& metric, const std::string& sample) override; nit: this should match the order of the interface, and also there shouldn't be these blank lines. so moved this up to https://codereview.chromium.org/1014543003/diff/320001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/1014543003/diff/320001/content/public/rendere... content/public/renderer/content_renderer_client.h:297: const std::string& metric, const std::string& sample); nit: for void methods, just define this inline with {}
Thank you so much for the prompt review. On 2015/04/02 16:24:14, jam wrote: > this seems like a lot of plumbing to make the IPC in componetns and the filter > there. since chrome already calls the rappor service, why not just put the IPC > and the filtering (in ChromeRenderMessageFilter) and then just call out to > components there? I have little clue to where I should add this, I've been working on Blink and this is the first non-trivial patch to Chrome, so your advice helps me a lot. I looked into ChromeRenderMessageFilter. If I understand correctly: * Add the IPC message to ChromeMsg * In ChromeRenderMessageFilter::OnMessageReceived, call g_browser_process->rappor_service() from ChromeRenderMessageFilter Is that correct? I'll try to see if it works.
On 2015/04/02 17:52:32, koji wrote: > Thank you so much for the prompt review. > > On 2015/04/02 16:24:14, jam wrote: > > this seems like a lot of plumbing to make the IPC in componetns and the filter > > there. since chrome already calls the rappor service, why not just put the IPC > > and the filtering (in ChromeRenderMessageFilter) and then just call out to > > components there? > > I have little clue to where I should add this, I've been working on Blink and > this is the first non-trivial patch to Chrome, so your advice helps me a lot. No worries, I feel the same way in Blink :) > > I looked into ChromeRenderMessageFilter. If I understand correctly: > * Add the IPC message to ChromeMsg sounds good > * In ChromeRenderMessageFilter::OnMessageReceived, call > g_browser_process->rappor_service() from ChromeRenderMessageFilter > Is that correct? yep > > I'll try to see if it works.
Patchset #10 (id:340001) has been deleted
PTAL. Using ChromeRenderMessageFilter simplified a lot. Also added a URL version for bug 467648 as requested by timvolodine@. The URL version uses logic in net, so we need another entry (or a flag to turn on/off the behavior.) I chose another version because GURL is easier for the callers. https://codereview.chromium.org/1014543003/diff/320001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/1014543003/diff/320001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:178: const std::string& metric, const std::string& sample) override; On 2015/04/02 16:24:14, jam wrote: > nit: this should match the order of the interface, and also there shouldn't be > these blank lines. so moved this up to Done -- to be precise, methods above are not in the order of interface, so I put as the last one to override. Hope this is ok? https://codereview.chromium.org/1014543003/diff/320001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/1014543003/diff/320001/content/public/rendere... content/public/renderer/content_renderer_client.h:297: const std::string& metric, const std::string& sample); On 2015/04/02 16:24:14, jam wrote: > nit: for void methods, just define this inline with {} Done.
lgtm I'm sorry for not seeing your update earlier, I missed it :( In the future, feel free to ping me over IM if I don't respond quickly. https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.h:128: nit: no blank line https://codereview.chromium.org/1014543003/diff/360001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/1014543003/diff/360001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:150: nit: no blank line https://codereview.chromium.org/1014543003/diff/360001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1014543003/diff/360001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1031: content::GetContentClient()->renderer()->RecordRapporURL(metric, url); here and above, remove "content::"
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:410: SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(), Isn't this function in namespace rappor? https://codereview.chromium.org/1014543003/diff/360001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1014543003/diff/360001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1025: const char* metric, const blink::WebString& sample) { Nit: 1 param per line when the first one wraps. I think "git cl format" can format this correctly - consider using? https://codereview.chromium.org/1014543003/diff/360001/tools/metrics/rappor/r... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1014543003/diff/360001/tools/metrics/rappor/r... tools/metrics/rappor/rappor.xml:359: <rappor-metric name="WebComponents.DocumentRegisterElement" This CL doesn't actually implement these, right? If so probably better to have them in a separate CL that can be reviewed together with the CL adding them.
https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:410: SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(), On 2015/04/07 21:01:56, Alexei Svitkine wrote: > Isn't this function in namespace rappor? (If so, how did this even compile on the bots?)
PTAL. Thank you reviewers, esp. jam@, you're actually the fastest and the most helpful reviewer for me! https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:410: SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(), On 2015/04/07 21:03:09, Alexei Svitkine wrote: > On 2015/04/07 21:01:56, Alexei Svitkine wrote: > > Isn't this function in namespace rappor? > > (If so, how did this even compile on the bots?) Yes it is...no idea how did this compile on all bots, tried to find "using rappor" somewhere but could not find. Anyway, it should namespace prefix even if compilers do not complain. Done. https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/1014543003/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.h:128: On 2015/04/07 20:57:44, jam wrote: > nit: no blank line Done. https://codereview.chromium.org/1014543003/diff/360001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/1014543003/diff/360001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:150: On 2015/04/07 20:57:44, jam wrote: > nit: no blank line Done. https://codereview.chromium.org/1014543003/diff/360001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1014543003/diff/360001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1025: const char* metric, const blink::WebString& sample) { On 2015/04/07 21:01:56, Alexei Svitkine wrote: > Nit: 1 param per line when the first one wraps. I think "git cl format" can > format this correctly - consider using? Done, didn't know "git cl format", very nice, thank you! https://codereview.chromium.org/1014543003/diff/360001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1031: content::GetContentClient()->renderer()->RecordRapporURL(metric, url); On 2015/04/07 20:57:44, jam wrote: > here and above, remove "content::" Done. https://codereview.chromium.org/1014543003/diff/360001/tools/metrics/rappor/r... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1014543003/diff/360001/tools/metrics/rappor/r... tools/metrics/rappor/rappor.xml:359: <rappor-metric name="WebComponents.DocumentRegisterElement" On 2015/04/07 21:01:56, Alexei Svitkine wrote: > This CL doesn't actually implement these, right? > > If so probably better to have them in a separate CL that can be reviewed > together with the CL adding them. Done, the CL is at https://codereview.chromium.org/1071463003/, and updated the description.
kojii@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@, could you PTAL at chrome/common/render_messages.h? I think all other files are good thanks to jam@.
lgtm % comments https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... content/public/renderer/content_renderer_client.h:295: // Record a sample string to a Rappor metric. Nit: Use a consistent tense with the other comments here. (i.e. Record -> Records). https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... content/public/renderer/content_renderer_client.h:297: const std::string& sample) {} Nit: Add a blank line under this, since these have comments. https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... content/public/renderer/content_renderer_client.h:298: // Record a domain and registry of a url to a Rappor metric. I'm slightly worried that someone (who's not a project member) reading this code and not knowing what Rappor is may get the wrong impression that Chrome is logging URLs. How about changing both of these to: "... Rappor privacy-preserving metric. See: https://www.chromium.org/developers/design-documents/rappor". https://codereview.chromium.org/1014543003/diff/400001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.h (right): https://codereview.chromium.org/1014543003/diff/400001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.h:162: virtual void recordRappor(const char* metric, const blink::WebString& sample); Maybe add the same comment about Rappor here that I suggest above.
https://codereview.chromium.org/1014543003/diff/400001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1014543003/diff/400001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:407: rappor::SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(), I'm a little curious--why is RecordSample() a member function, but SampleDomainAndRegistryFromGURL is a static function that takes a RapporService? It seems like there should be some more consistency here.
https://codereview.chromium.org/1014543003/diff/400001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1014543003/diff/400001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:407: rappor::SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(), On 2015/04/08 15:15:20, dcheng wrote: > I'm a little curious--why is RecordSample() a member function, but > SampleDomainAndRegistryFromGURL is a static function that takes a RapporService? > It seems like there should be some more consistency here. I agree. One advantage of the static is it removed the need for doing a null check on rappor service in the calling code, so it makes sense to me to add a static for RecordSample(), but probably in a separate CL.
dcheng@, PTAL. Appreciate your ok for chrome/common/render_messages.h if you're fine with below. * Consistent static method I prefer in a separate CL by holte@ * Comments in content_renderer_client.h done * Comments in renderer_blink_platform_impl.h I think should be done in the abstract class in Blink. See more inline: https://codereview.chromium.org/1014543003/diff/400001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1014543003/diff/400001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:407: rappor::SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(), On 2015/04/08 16:42:55, Alexei Svitkine wrote: > On 2015/04/08 15:15:20, dcheng wrote: > > I'm a little curious--why is RecordSample() a member function, but > > SampleDomainAndRegistryFromGURL is a static function that takes a > RapporService? > > It seems like there should be some more consistency here. > > I agree. One advantage of the static is it removed the need for doing a null > check on rappor service in the calling code, so it makes sense to me to add a > static for RecordSample(), but probably in a separate CL. I agree too, but a separate CL is appreciated. I can suggest to holte@, the owner of RAPPOR, to clean this up along with updating his document at: http://www.chromium.org/developers/design-documents/rappor https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... content/public/renderer/content_renderer_client.h:295: // Record a sample string to a Rappor metric. On 2015/04/08 14:57:11, Alexei Svitkine wrote: > Nit: Use a consistent tense with the other comments here. (i.e. Record -> > Records). Done. https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... content/public/renderer/content_renderer_client.h:297: const std::string& sample) {} On 2015/04/08 14:57:11, Alexei Svitkine wrote: > Nit: Add a blank line under this, since these have comments. Done. https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... content/public/renderer/content_renderer_client.h:298: // Record a domain and registry of a url to a Rappor metric. On 2015/04/08 14:57:11, Alexei Svitkine wrote: > I'm slightly worried that someone (who's not a project member) reading this code > and not knowing what Rappor is may get the wrong impression that Chrome is > logging URLs. > > How about changing both of these to: "... Rappor privacy-preserving metric. See: > https://www.chromium.org/developers/design-documents/rappor%22. Done. https://codereview.chromium.org/1014543003/diff/400001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.h (right): https://codereview.chromium.org/1014543003/diff/400001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.h:162: virtual void recordRappor(const char* metric, const blink::WebString& sample); On 2015/04/08 14:57:11, Alexei Svitkine wrote: > Maybe add the same comment about Rappor here that I suggest above. Hm, these two are virtual override of platform.h in Blink, and jam@ suggested no blank lines for all override methods. Maybe comments should be there? timvolodine@ has a CL for Platform to support other features, so I can suggest there. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
IPC changes LGTM
lgtm % comments https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... content/public/renderer/content_renderer_client.h:298: // Record a domain and registry of a url to a Rappor metric. On 2015/04/08 17:31:46, koji wrote: > On 2015/04/08 14:57:11, Alexei Svitkine wrote: > > I'm slightly worried that someone (who's not a project member) reading this > code > > and not knowing what Rappor is may get the wrong impression that Chrome is > > logging URLs. > > > > How about changing both of these to: "... Rappor privacy-preserving metric. > See: > > https://www.chromium.org/developers/design-documents/rappor%22. > > Done. My suggestion was to also include "See: https://www.chromium.org/developers/design-documents/rappor" in the comments. https://codereview.chromium.org/1014543003/diff/400001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.h (right): https://codereview.chromium.org/1014543003/diff/400001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.h:162: virtual void recordRappor(const char* metric, const blink::WebString& sample); On 2015/04/08 17:31:46, koji wrote: > On 2015/04/08 14:57:11, Alexei Svitkine wrote: > > Maybe add the same comment about Rappor here that I suggest above. > > Hm, these two are virtual override of platform.h in Blink, and jam@ suggested no > blank lines for all override methods. Maybe comments should be there? > timvolodine@ has a CL for Platform to support other features, so I can suggest > there. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Fair enough.
On 2015/04/08 17:33:53, dcheng wrote: > IPC changes LGTM Thank you! The consistent function CL is at: https://codereview.chromium.org/1070863002/ If holte@ can review it reasonably quickly, I can update this CL before landing.
On 2015/04/08 17:53:50, Alexei Svitkine wrote: > lgtm % comments > > https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... > File content/public/renderer/content_renderer_client.h (right): > > https://codereview.chromium.org/1014543003/diff/400001/content/public/rendere... > content/public/renderer/content_renderer_client.h:298: // Record a domain and > registry of a url to a Rappor metric. > On 2015/04/08 17:31:46, koji wrote: > > On 2015/04/08 14:57:11, Alexei Svitkine wrote: > > > I'm slightly worried that someone (who's not a project member) reading this > > code > > > and not knowing what Rappor is may get the wrong impression that Chrome is > > > logging URLs. > > > > > > How about changing both of these to: "... Rappor privacy-preserving metric. > > See: > > > https://www.chromium.org/developers/design-documents/rappor%22. > > > > Done. > > My suggestion was to also include "See: > https://www.chromium.org/developers/design-documents/rappor%22 in the comments. Will do before landing. Thought that was a message to me, sorry about that.
lgtm Thanks for implementing this!
Patchset #16 (id:480001) has been deleted
Patchset #16 (id:500001) has been deleted
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, asvitkine@chromium.org, dcheng@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/1014543003/#ps520001 (title: "Use SampleString in rappor_utils")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1014543003/520001
Message was sent while issue was closed.
Committed patchset #16 (id:520001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/0f931925f7c0b966c2eb83321886077d30127964 Cr-Commit-Position: refs/heads/master@{#324563} |
