Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881593002:
Reporting: Plumb into RenderFrame via Mojo (Closed)
|
Created:
3 years, 7 months ago by Julia Tuttle Modified:
3 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, cbentzel+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dcheng, jochen (gone - plz use gerrit), mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, net-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, paulmeyer Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReporting: Plumb into RenderFrame via Mojo
BUG=704259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2881593002
Cr-Commit-Position: refs/heads/master@{#483855}
Committed: https://chromium.googlesource.com/chromium/src/+/95fe3cc592eeb332824adc8c8f4b9951f2f170d7
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #
Total comments: 4
Patch Set 4 : Add username to TODOs, rebase #
Total comments: 14
Patch Set 5 : rebase, make requested changes, format #
Total comments: 7
Patch Set 6 : #include <utility> #Patch Set 7 : Enumify. #
Total comments: 2
Patch Set 8 : Revert "Enumify." #
Total comments: 2
Patch Set 9 : rebase; yet another std::move #Patch Set 10 : rebase #
Total comments: 12
Patch Set 11 : rebase #Patch Set 12 : Make requested changes. #Patch Set 13 : Move ReportingServiceProxy to renderer_host, .mojom to Blink. #
Total comments: 1
Patch Set 14 : rebase, remove obsolete comments, format #Patch Set 15 : Fix #includes and DEPS. #
Dependent Patchsets: Messages
Total messages: 115 (78 generated)
Description was changed from ========== Reporting: Plumb into RenderFrame via Mojo BUG=704259 ========== to ========== Reporting: Plumb into RenderFrame via Mojo BUG=704259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
juliatuttle@chromium.org changed reviewers: + mkwst@chromium.org, shivanisha@chromium.org
PTAL, shivanisha and mkwst. This isn't done yet; I just want feedback on the general design before I flesh it out with comments and anything else we need.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think this looks pretty reasonable, and is pretty well in-line with what we discussed last week. The renderer builds up a `Value` (I guess we're assuming a (potentially nested) `DictionaryValue`?)), passes it up the chain to the browser, which serializes the `Value` and sends it off to the internets. That said, I'm not entirely sure what the right conversion is from something like `JSONValue` in Blink to a `base::Value` structure. dcheng@, jochen@? You've looked at more of the mojo->blink bits than I have; is there a straightforward conversion I'm missing?
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/15 07:48:51, Mike West wrote: > That said, I'm not entirely sure what the right conversion is from something > like `JSONValue` in Blink to a `base::Value` structure. dcheng@, jochen@? You've > looked at more of the mojo->blink bits than I have; is there a straightforward > conversion I'm missing? Ping?
Looks good to me, I'd suggest getting final approval from someone with Mojo expertise. Thanks. https://codereview.chromium.org/2881593002/diff/40001/content/browser/net/rep... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/40001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:42: // TODO: Get rid of this copy. nit: TODO(username) https://codereview.chromium.org/2881593002/diff/40001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:83: // TODO: Make sure making our own scoped_refptr is okay. nit: TODO(username)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mkwst@chromium.org changed reviewers: + dcheng@chromium.org
Moving dcheng@ up from CC to reviewer, as I think his input would be helpful.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Thanks, shivanisha. dcheng? https://codereview.chromium.org/2881593002/diff/40001/content/browser/net/rep... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/40001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:42: // TODO: Get rid of this copy. On 2017/05/17 18:58:28, shivanisha wrote: > nit: TODO(username) Done. https://codereview.chromium.org/2881593002/diff/40001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:83: // TODO: Make sure making our own scoped_refptr is okay. On 2017/05/17 18:58:28, shivanisha wrote: > nit: TODO(username) Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:52: request_context_getter_(request_context_getter) {} Nit: std::move() here https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:65: group, type, base::WrapUnique(raw_const_body))); How come this can't just std::move(body)? Does the parameter need to be a std::unique_ptr<const base::Value>, or can it just be a plain old std::unique_ptr<base::Value? That would let us just std::move() it all the way through. https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:77: scoped_refptr<SiteInstance> site_instance, Nit: Consider using base::RetainedRef and accepting this argument by raw pointer, as there doesn't seem to be any ownership transfer in the body. https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:86: new ReportingServiceProxyImpl(std::move(request), Should this be using mojo::MakeStrongBinding? I don't see anything that would delete this, so I think it might be leaked. https://codereview.chromium.org/2881593002/diff/60001/content/common/net/repo... File content/common/net/reporting.mojom (right): https://codereview.chromium.org/2881593002/diff/60001/content/common/net/repo... content/common/net/reporting.mojom:11: QueueReport(url.mojom.Url url, Nit: please add some comments to this interface and its methods. In particular, please describe what |group| and |type| are, and if there are any restrictions on the inputs, since they're just strings.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
PTAL, dcheng. https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:52: request_context_getter_(request_context_getter) {} On 2017/05/19 13:42:55, dcheng (in AEST) wrote: > Nit: std::move() here Done. https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:65: group, type, base::WrapUnique(raw_const_body))); On 2017/05/19 13:42:55, dcheng (in AEST) wrote: > How come this can't just std::move(body)? Does the parameter need to be a > std::unique_ptr<const base::Value>, or can it just be a plain old > std::unique_ptr<base::Value? That would let us just std::move() it all the way > through. Reporting deals in std::unique_ptr<const base::Value>, since report bodies never change. I *could* change it there, but I feel like there should be a neater cast for this? https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:77: scoped_refptr<SiteInstance> site_instance, On 2017/05/19 13:42:55, dcheng (in AEST) wrote: > Nit: Consider using base::RetainedRef and accepting this argument by raw > pointer, as there doesn't seem to be any ownership transfer in the body. Done. https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:86: new ReportingServiceProxyImpl(std::move(request), On 2017/05/19 13:42:55, dcheng (in AEST) wrote: > Should this be using mojo::MakeStrongBinding? I don't see anything that would > delete this, so I think it might be leaked. It was leaked! Thank you for teaching me the right way to do it :) https://codereview.chromium.org/2881593002/diff/60001/content/common/net/repo... File content/common/net/reporting.mojom (right): https://codereview.chromium.org/2881593002/diff/60001/content/common/net/repo... content/common/net/reporting.mojom:11: QueueReport(url.mojom.Url url, On 2017/05/19 13:42:55, dcheng (in AEST) wrote: > Nit: please add some comments to this interface and its methods. In particular, > please describe what |group| and |type| are, and if there are any restrictions > on the inputs, since they're just strings. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
juliatuttle@chromium.org changed reviewers: + rbyers@chromium.org
FYI rbyers.
Looks great Julia, thanks! Just a couple non-blocking questions from my perspective as someone who intends to use this from within blink. https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... File content/common/net/reporting.mojom (right): https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... content/common/net/reporting.mojom:11: // Attempts to queue a report using the Reporting API. What's the implications of "attempt" here? I.e. in what ways can this fail, and should we perhaps be exposing those failure down to blink? https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... content/common/net/reporting.mojom:22: // |body| is the body. It can be any valid Value. I was assuming this would be a string, not a base::Value. But maybe base::Value is better, so giving this a try sounds good to me. But if we end up having trouble properly converting a JS dictionary to a base::Value, would it be too hard to change this to a JSON-serialized string instead? In particular I'm thinking of possible minor spec conformance issues. I think the ReportingObserver spec will probably end up implying that running JSON.stringify on the body object supplied to the observer would produce EXACTLY the same JSON string that is included in the report body (there's been some debate in the past from Mozilla folks about being precise around JSON here - eg. so failure conditions are handled identically by different browsers).
https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:52: request_context_getter_(request_context_getter) {} On 2017/05/19 16:12:22, Julia Tuttle wrote: > On 2017/05/19 13:42:55, dcheng (in AEST) wrote: > > Nit: std::move() here > > Done. Followup nit: please #include <utility> https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:65: group, type, base::WrapUnique(raw_const_body))); On 2017/05/19 16:12:22, Julia Tuttle wrote: > On 2017/05/19 13:42:55, dcheng (in AEST) wrote: > > How come this can't just std::move(body)? Does the parameter need to be a > > std::unique_ptr<const base::Value>, or can it just be a plain old > > std::unique_ptr<base::Value? That would let us just std::move() it all the way > > through. > > Reporting deals in std::unique_ptr<const base::Value>, since report bodies never > change. > > I *could* change it there, but I feel like there should be a neater cast for > this? I see. I think std::unique_ptr<const T> is quite rare and probably causes more trouble than it's worth. Probably not worth investigating this particular point in more detail now, as base::Value is migrating to become a value type. https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... File content/common/net/reporting.mojom (right): https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... content/common/net/reporting.mojom:11: // Attempts to queue a report using the Reporting API. On 2017/05/19 20:35:02, Rick Byers wrote: > What's the implications of "attempt" here? I.e. in what ways can this fail, and > should we perhaps be exposing those failure down to blink? I guess this is a web spec somewhere, might be nice to link it. https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... content/common/net/reporting.mojom:20: // set to a fixed value by the feature using Reporting. Is something that could be represented as an enum, or is it mostly controlled by web content?
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
PTAL, dcheng. https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:52: request_context_getter_(request_context_getter) {} On 2017/05/22 09:30:38, dcheng wrote: > On 2017/05/19 16:12:22, Julia Tuttle wrote: > > On 2017/05/19 13:42:55, dcheng (in AEST) wrote: > > > Nit: std::move() here > > > > Done. > > Followup nit: please #include <utility> Done. https://codereview.chromium.org/2881593002/diff/60001/content/browser/net/rep... content/browser/net/reporting_service_proxy.cc:65: group, type, base::WrapUnique(raw_const_body))); On 2017/05/22 09:30:38, dcheng wrote: > On 2017/05/19 16:12:22, Julia Tuttle wrote: > > On 2017/05/19 13:42:55, dcheng (in AEST) wrote: > > > How come this can't just std::move(body)? Does the parameter need to be a > > > std::unique_ptr<const base::Value>, or can it just be a plain old > > > std::unique_ptr<base::Value? That would let us just std::move() it all the > way > > > through. > > > > Reporting deals in std::unique_ptr<const base::Value>, since report bodies > never > > change. > > > > I *could* change it there, but I feel like there should be a neater cast for > > this? > > I see. I think std::unique_ptr<const T> is quite rare and probably causes more > trouble than it's worth. Probably not worth investigating this particular point > in more detail now, as base::Value is migrating to become a value type. Oh, awesome to hear. https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... File content/common/net/reporting.mojom (right): https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... content/common/net/reporting.mojom:20: // set to a fixed value by the feature using Reporting. On 2017/05/22 09:30:38, dcheng wrote: > Is something that could be represented as an enum, or is it mostly controlled by > web content? It could be an enum, I guess -- it is controlled by web specs (e.g. csp would specify type "csp" for all reports), but not by web content, so we'd have an enum entry for each Reporting feature that can send reports from the renderer. Would that be preferable? https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... content/common/net/reporting.mojom:22: // |body| is the body. It can be any valid Value. On 2017/05/19 20:35:02, Rick Byers wrote: > I was assuming this would be a string, not a base::Value. But maybe base::Value > is better, so giving this a try sounds good to me. But if we end up having > trouble properly converting a JS dictionary to a base::Value, would it be too > hard to change this to a JSON-serialized string instead? > > In particular I'm thinking of possible minor spec conformance issues. I think > the ReportingObserver spec will probably end up implying that running > JSON.stringify on the body object supplied to the observer would produce EXACTLY > the same JSON string that is included in the report body (there's been some > debate in the past from Mozilla folks about being precise around JSON here - eg. > so failure conditions are handled identically by different browsers). It wouldn't be super difficult to make this a JSON string, but I'd end up parsing it on the browser side and then reserializing it along with other reports when it's sent, so I can't promise it will be identical in the upload.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... File content/common/net/reporting.mojom (right): https://codereview.chromium.org/2881593002/diff/80001/content/common/net/repo... content/common/net/reporting.mojom:20: // set to a fixed value by the feature using Reporting. On 2017/05/22 14:30:41, Julia Tuttle wrote: > On 2017/05/22 09:30:38, dcheng wrote: > > Is something that could be represented as an enum, or is it mostly controlled > by > > web content? > > It could be an enum, I guess -- it is controlled by web specs (e.g. csp would > specify type "csp" for all reports), but not by web content, so we'd have an > enum entry for each Reporting feature that can send reports from the renderer. > > Would that be preferable? Hmmm, I think so: in general we prefer to pass more structured types over ipc rather than strings and then try to constrain the value of the strings.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
PTAL, dcheng.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2881593002/diff/120001/content/browser/net/re... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/120001/content/browser/net/re... content/browser/net/reporting_service_proxy.cc:71: std::string type_string = ReportTypeEnumToString(type_enum); Hmm... I didn't realize we were just going to turn it back into a string. If we're not going to do anything with this value (i.e. we don't try to interpret it some way and we just forward it somewhere else), it seems better to just pass a string through. Sorry for not being clear on this point.
PTAL, dcheng. https://codereview.chromium.org/2881593002/diff/120001/content/browser/net/re... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/120001/content/browser/net/re... content/browser/net/reporting_service_proxy.cc:71: std::string type_string = ReportTypeEnumToString(type_enum); On 2017/05/24 00:23:12, dcheng wrote: > Hmm... I didn't realize we were just going to turn it back into a string. If > we're not going to do anything with this value (i.e. we don't try to interpret > it some way and we just forward it somewhere else), it seems better to just pass > a string through. > > Sorry for not being clear on this point. Oops! Will revert.
PTAL, dcheng.
mojo lgtm https://codereview.chromium.org/2881593002/diff/140001/content/browser/net/re... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/140001/content/browser/net/re... content/browser/net/reporting_service_proxy.cc:85: base::MakeUnique<ReportingServiceProxyImpl>(request_context_getter), Nit: std::move() here as well.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Thanks, dcheng. PTAL, shivanisha. https://codereview.chromium.org/2881593002/diff/140001/content/browser/net/re... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/140001/content/browser/net/re... content/browser/net/reporting_service_proxy.cc:85: base::MakeUnique<ReportingServiceProxyImpl>(request_context_getter), On 2017/05/24 22:29:25, dcheng wrote: > Nit: std::move() here as well. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
shivanisha, ping?
On 2017/05/30 at 16:16:05, juliatuttle wrote: > shivanisha, ping? RS LGTM.
The CQ bit was checked by juliatuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2881593002/#ps160001 (title: "rebase; yet another std::move")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juliatuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shivanisha@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2881593002/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
juliatuttle@chromium.org changed reviewers: + nasko@chromium.org
PTAL nasko for //content changes.
On 2017/06/05 15:48:28, Julia Tuttle wrote: > PTAL nasko for //content changes. Can we first get consensus on where this feature lives (i.e. net/ vs content/browser)?
On 2017/06/05 15:51:45, jam wrote: > On 2017/06/05 15:48:28, Julia Tuttle wrote: > > PTAL nasko for //content changes. > > Can we first get consensus on where this feature lives (i.e. net/ vs > content/browser)? Part of this CL is going to be needed in either case, and that consensus could take quite a while. I'd like to keep iterating on it in //net, and move it if the consensus is that it should be in //content/browser instead. I'm happy to move it, but I don't want to stop working on it during the discussion.
jam@chromium.org changed reviewers: + jam@chromium.org
On 2017/06/05 15:54:10, Julia Tuttle wrote: > On 2017/06/05 15:51:45, jam wrote: > > On 2017/06/05 15:48:28, Julia Tuttle wrote: > > > PTAL nasko for //content changes. > > > > Can we first get consensus on where this feature lives (i.e. net/ vs > > content/browser)? > > Part of this CL is going to be needed in either case, and that consensus could > take quite a while. What other data points do we need at this point? > > I'd like to keep iterating on it in //net, and move it if the consensus is that > it should be in //content/browser instead. I'm happy to move it, but I don't > want to stop working on it during the discussion. Ok sure. https://codereview.chromium.org/2881593002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2862: GetInterfaceRegistry()->AddInterface<mojom::ReportingServiceProxy>(base::Bind( since this interface is per profile, it can be registered in RenderProcessHostImpl instead of RenderFrameHost. that way if multiple frames in a process use it, there's no need to keep fetching the interfaceptr from the renderer (also less objects in general) https://codereview.chromium.org/2881593002/diff/180001/content/browser/net/re... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/browser/net/re... content/browser/net/reporting_service_proxy.cc:84: mojo::MakeStrongBinding(base::MakeUnique<ReportingServiceProxyImpl>( it'd be more efficient to pass the interface ptr (see PassInterface() link) and bind the message pipe on the IO thread. that way you avoid IPCs coming from IO thread to the UI thread, back to the IO thread. https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h... https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:1122: service->QueueReport(url, group, type, std::move(body)); blink can connect directly to mojo services, no need to add plumbing to content/ for this. as part of onion soup, content/renderer is meant to be removed by moving code to blink/. so we shouldn't add more code to move for new features. https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl_browsertest.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl_browsertest.cc:553: frame()->QueueReport(GURL("https://example.com/path"), "example-group", what is this test testing, that no crashes occur? seems like an end-to-end test would want to check the reporting service data structures to check that it received it?
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, jam. https://codereview.chromium.org/2881593002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2862: GetInterfaceRegistry()->AddInterface<mojom::ReportingServiceProxy>(base::Bind( On 2017/06/05 21:03:44, jam (ooo 6-14 - 6-20) wrote: > since this interface is per profile, it can be registered in > RenderProcessHostImpl instead of RenderFrameHost. that way if multiple frames in > a process use it, there's no need to keep fetching the interfaceptr from the > renderer (also less objects in general) I can't -- the ReportingServiceProxy needs to know which SiteInstance (and therefore which URLRequestContext) to route the report to, and render processes (AFAIK) can host frames from different SiteInstances. https://codereview.chromium.org/2881593002/diff/180001/content/browser/net/re... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/browser/net/re... content/browser/net/reporting_service_proxy.cc:84: mojo::MakeStrongBinding(base::MakeUnique<ReportingServiceProxyImpl>( On 2017/06/05 21:03:44, jam (ooo 6-14 - 6-20) wrote: > it'd be more efficient to pass the interface ptr (see PassInterface() link) and > bind the message pipe on the IO thread. that way you avoid IPCs coming from IO > thread to the UI thread, back to the IO thread. > > > > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h... I did this, but I didn't need to use PassInterface -- I just passed the interface *request* over to the network task runner and bound it there. Is that okay? https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:1122: service->QueueReport(url, group, type, std::move(body)); On 2017/06/05 21:03:44, jam (ooo 6-14 - 6-20) wrote: > blink can connect directly to mojo services, no need to add plumbing to content/ > for this. as part of onion soup, content/renderer is meant to be removed by > moving code to blink/. so we shouldn't add more code to move for new features. Okay! https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl_browsertest.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl_browsertest.cc:553: frame()->QueueReport(GURL("https://example.com/path"), "example-group", On 2017/06/05 21:03:44, jam (ooo 6-14 - 6-20) wrote: > what is this test testing, that no crashes occur? > > seems like an end-to-end test would want to check the reporting service data > structures to check that it received it? Yeah, this was just to check for crashes. I'll hold out on an end-to-end test until we have a place that actually calls it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2881593002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2862: GetInterfaceRegistry()->AddInterface<mojom::ReportingServiceProxy>(base::Bind( On 2017/06/15 20:33:31, Julia Tuttle wrote: > On 2017/06/05 21:03:44, jam (ooo 6-14 - 6-20) wrote: > > since this interface is per profile, it can be registered in > > RenderProcessHostImpl instead of RenderFrameHost. that way if multiple frames > in > > a process use it, there's no need to keep fetching the interfaceptr from the > > renderer (also less objects in general) > > I can't -- the ReportingServiceProxy needs to know which SiteInstance (and > therefore which URLRequestContext) to route the report to, and render processes > (AFAIK) can host frames from different SiteInstances. Do you mean StoragePartition instead of SiteInstance? Each RenderProceeHost is associated with one StoragePartition from which one can get a URLRequestContext (i.e. RenderProcessHostImpl::GetStoragePartition()->GetURLRequestContext()->GetURLRequestContext() https://codereview.chromium.org/2881593002/diff/180001/content/browser/net/re... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/browser/net/re... content/browser/net/reporting_service_proxy.cc:84: mojo::MakeStrongBinding(base::MakeUnique<ReportingServiceProxyImpl>( On 2017/06/15 20:33:32, Julia Tuttle wrote: > On 2017/06/05 21:03:44, jam (ooo 6-14 - 6-20) wrote: > > it'd be more efficient to pass the interface ptr (see PassInterface() link) > and > > bind the message pipe on the IO thread. that way you avoid IPCs coming from IO > > thread to the UI thread, back to the IO thread. > > > > > > > > > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h... > > I did this, but I didn't need to use PassInterface -- I just passed the > interface *request* over to the network task runner and bound it there. Is that > okay? sg https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:1122: service->QueueReport(url, group, type, std::move(body)); On 2017/06/15 20:33:32, Julia Tuttle wrote: > On 2017/06/05 21:03:44, jam (ooo 6-14 - 6-20) wrote: > > blink can connect directly to mojo services, no need to add plumbing to > content/ > > for this. as part of onion soup, content/renderer is meant to be removed by > > moving code to blink/. so we shouldn't add more code to move for new features. > > Okay! note: the mojom should then move to third_party/WebKit/public/platform/ so that it's accessible to blink
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, jam.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Oops. I fixed that #include problem, but now git cl upload is complaining that I can't reference third_party/WebKit from content/browser, which is where ReportingServiceProxy is. Ideas?
On 2017/06/27 18:31:35, Julia Tuttle wrote: > Oops. I fixed that #include problem, but now git cl upload is complaining that I > can't reference third_party/WebKit from content/browser, which is where > ReportingServiceProxy is. > > Ideas? Never mind, I found the place where exceptions are made, and made myself one.
lgtm, thanks sorry i missed your update until now (feel free to IM me if i don't respond in a few hours) https://codereview.chromium.org/2881593002/diff/180001/content/browser/net/re... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/180001/content/browser/net/re... content/browser/net/reporting_service_proxy.cc:84: mojo::MakeStrongBinding(base::MakeUnique<ReportingServiceProxyImpl>( On 2017/06/15 20:33:32, Julia Tuttle wrote: > On 2017/06/05 21:03:44, jam (ooo 6-14 - 6-20) wrote: > > it'd be more efficient to pass the interface ptr (see PassInterface() link) > and > > bind the message pipe on the IO thread. that way you avoid IPCs coming from IO > > thread to the UI thread, back to the IO thread. > > > > > > > > > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h... > > I did this, but I didn't need to use PassInterface -- I just passed the > interface *request* over to the network task runner and bound it there. Is that > okay? Yep. https://codereview.chromium.org/2881593002/diff/240001/content/browser/net/re... File content/browser/net/reporting_service_proxy.cc (right): https://codereview.chromium.org/2881593002/diff/240001/content/browser/net/re... content/browser/net/reporting_service_proxy.cc:75: // TODO(juliatuttle): Make sure making our own scoped_refptr is okay. yep this is safe, this is the pattern that it's used
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juliatuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shivanisha@chromium.org, dcheng@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2881593002/#ps280001 (title: "Fix #includes and DEPS.")
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": 280001, "attempt_start_ts": 1498863649905400,
"parent_rev": "e72c3cb5833379a04dfe93ca24ace1bb61e01379", "commit_rev":
"95fe3cc592eeb332824adc8c8f4b9951f2f170d7"}
Message was sent while issue was closed.
Description was changed from ========== Reporting: Plumb into RenderFrame via Mojo BUG=704259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Reporting: Plumb into RenderFrame via Mojo BUG=704259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2881593002 Cr-Commit-Position: refs/heads/master@{#483855} Committed: https://chromium.googlesource.com/chromium/src/+/95fe3cc592eeb332824adc8c8f4b... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/95fe3cc592eeb332824adc8c8f4b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
