Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521823008:
Migrate chrome_net_benchmarking_message_filter to mojo (Closed)
DescriptionMigrate chrome_net_benchmarking_message_filter to mojo
BUG=577685
Committed: https://crrev.com/233f57ab4869984231b841dc58dff2dde3545072
Cr-Commit-Position: refs/heads/master@{#439996}
Patch Set 1 #Patch Set 2 : Added mojom file #Patch Set 3 : Figure out where to put the add interface #Patch Set 4 : Added first client interface call #Patch Set 5 : Added all used interfaces, removed old IPC definitions #Patch Set 6 : update and retry bot #Patch Set 7 : Change class and mojo names to adjust to new norm #Patch Set 8 : Fixed upstream issues #Patch Set 9 : Fixed unused error in android build #Patch Set 10 : Added mojo interface to json manifest #Patch Set 11 : Change default thread fro ChromeNetBenchmarkingImpl #Patch Set 12 : change ClearPredictorCache to run on UI thread via PostTask #
Total comments: 12
Patch Set 13 : Renamed mojo class to net_benchmarking #
Total comments: 8
Patch Set 14 : Modified methods to sync behavior #Patch Set 15 : Change ClearPredictorCache to PostTaskAndReply #
Total comments: 6
Patch Set 16 : Fix sync call to ClearCache #
Total comments: 2
Patch Set 17 : change all calls to use default sync method #
Total comments: 13
Patch Set 18 : Moved CloseCurrentConnections to NetBenchmarkingTest interface #Patch Set 19 : reverted split interface #
Total comments: 7
Patch Set 20 : Do not add NetBenchmaring interface if benchmarking is disabled #Messages
Total messages: 87 (58 generated)
The CQ bit was checked by dvallet@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
update and retry bot
The CQ bit was checked by dvallet@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Change class and mojo names to adjust to new norm
The CQ bit was checked by dvallet@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by dvallet@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_...)
The CQ bit was checked by dvallet@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dvallet@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dvallet@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dvallet@chromium.org changed reviewers: + sammc@chromium.org, tibell@chromium.org
Description was changed from ========== Migrate chrome_net_benchamrking_message_filter to mojo BUG=577685 ========== to ========== Migrate chrome_net_benchmarking_message_filter to mojo BUG=577685 ==========
https://codereview.chromium.org/2521823008/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_net_benchmarking_impl.cc (right): https://codereview.chromium.org/2521823008/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_net_benchmarking_impl.cc:64: if (!CheckBenchmarkingEnabled()) { Check this in Create() instead and if not enabled, early return. https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... File chrome/common/chrome_net_benchmarking.mojom (right): https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... chrome/common/chrome_net_benchmarking.mojom:7: interface ChromeNetBenchmarking { Let's call this NetBenchmarking instead. https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... chrome/common/chrome_net_benchmarking.mojom:10: CloseCurrentConnections(); These messages should [Sync]. https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... chrome/common/chrome_net_benchmarking.mojom:14: SetCacheMode(bool enabled); This doesn't appear to be used. I think we can delete it. https://codereview.chromium.org/2521823008/diff/220001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (right): https://codereview.chromium.org/2521823008/diff/220001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:69: thread->GetRemoteInterfaces()->GetInterface(&chrome_net_benchmarking); I think I'd prefer a single shared connection instead. Something like static chrome::mojom::NetBenchmarking& GetNetBenchmarking() { CR_DEFINE_STATIC_LOCAL(chrome::mojom::NetBenchmarkingPtr, net_benchmarking, ConnectToBrowser()); return *net_benchmarking; } static chrome::mojom::NetBenchmarkingPtr ConnectToBrowser() { chrome::mojom::NetBenchmarkingPtr net_benchmarking; content::RenderThread::Get()->GetRemoteInterfaces()->GetInterface( &net_benchmarking); return net_benchmarking; } should do the right thing.
The CQ bit was checked by dvallet@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2521823008/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_net_benchmarking_impl.cc (right): https://codereview.chromium.org/2521823008/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_net_benchmarking_impl.cc:64: if (!CheckBenchmarkingEnabled()) { On 2016/12/05 at 06:57:09, Sam McNally wrote: > Check this in Create() instead and if not enabled, early return. Done. I've remove te NOTREACHED message since I guess now we'll not be binding the IPC methods when Benchmarking is disabled https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... File chrome/common/chrome_net_benchmarking.mojom (right): https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... chrome/common/chrome_net_benchmarking.mojom:7: interface ChromeNetBenchmarking { On 2016/12/05 at 06:57:10, Sam McNally wrote: > Let's call this NetBenchmarking instead. Done https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... chrome/common/chrome_net_benchmarking.mojom:10: CloseCurrentConnections(); On 2016/12/05 at 06:57:10, Sam McNally wrote: > These messages should [Sync]. Done. What's your reasoning for this having to be sync? https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... chrome/common/chrome_net_benchmarking.mojom:14: SetCacheMode(bool enabled); On 2016/12/05 at 06:57:09, Sam McNally wrote: > This doesn't appear to be used. I think we can delete it. Done https://codereview.chromium.org/2521823008/diff/220001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (right): https://codereview.chromium.org/2521823008/diff/220001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:69: thread->GetRemoteInterfaces()->GetInterface(&chrome_net_benchmarking); On 2016/12/05 at 06:57:10, Sam McNally wrote: > I think I'd prefer a single shared connection instead. Something like > > static chrome::mojom::NetBenchmarking& GetNetBenchmarking() { > CR_DEFINE_STATIC_LOCAL(chrome::mojom::NetBenchmarkingPtr, net_benchmarking, > ConnectToBrowser()); > return *net_benchmarking; > } > > static chrome::mojom::NetBenchmarkingPtr ConnectToBrowser() { > chrome::mojom::NetBenchmarkingPtr net_benchmarking; > content::RenderThread::Get()->GetRemoteInterfaces()->GetInterface( > &net_benchmarking); > return net_benchmarking; > } > > should do the right thing. Done, not sure if correctly though, PTAL
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... File chrome/common/chrome_net_benchmarking.mojom (right): https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... chrome/common/chrome_net_benchmarking.mojom:10: CloseCurrentConnections(); On 2016/12/07 02:21:02, dvallet wrote: > On 2016/12/05 at 06:57:10, Sam McNally wrote: > > These messages should [Sync]. > > Done. What's your reasoning for this having to be sync? The messages they replace were declared with IPC_SYNC_MESSAGE. So all of the remaining messages should be sync. https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:223: "net_benchmarking_impl.cc", Resort these. https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:44: #include "chrome/browser/net_benchmarking_impl.h" Resort. https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/net_ben... File chrome/browser/net_benchmarking_impl.h (right): https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/net_ben... chrome/browser/net_benchmarking_impl.h:20: class NetBenchmarkingImpl Please run git cl format. https://codereview.chromium.org/2521823008/diff/240001/skia/ext/SkOpts_hsw_st... File skia/ext/SkOpts_hsw_stub.cc (left): https://codereview.chromium.org/2521823008/diff/240001/skia/ext/SkOpts_hsw_st... skia/ext/SkOpts_hsw_stub.cc:13: Revert this file.
The CQ bit was checked by dvallet@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dvallet@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.
PTAL https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... File chrome/common/chrome_net_benchmarking.mojom (right): https://codereview.chromium.org/2521823008/diff/220001/chrome/common/chrome_n... chrome/common/chrome_net_benchmarking.mojom:10: CloseCurrentConnections(); On 2016/12/08 at 22:57:46, Sam McNally wrote: > On 2016/12/07 02:21:02, dvallet wrote: > > On 2016/12/05 at 06:57:10, Sam McNally wrote: > > > These messages should [Sync]. > > > > Done. What's your reasoning for this having to be sync? > > The messages they replace were declared with IPC_SYNC_MESSAGE. So all of the remaining messages should be sync. Done https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:223: "net_benchmarking_impl.cc", On 2016/12/08 at 22:57:46, Sam McNally wrote: > Resort these. Done https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:44: #include "chrome/browser/net_benchmarking_impl.h" On 2016/12/08 at 22:57:46, Sam McNally wrote: > Resort. Done https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/net_ben... File chrome/browser/net_benchmarking_impl.h (right): https://codereview.chromium.org/2521823008/diff/240001/chrome/browser/net_ben... chrome/browser/net_benchmarking_impl.h:20: class NetBenchmarkingImpl On 2016/12/08 at 22:57:46, Sam McNally wrote: > Please run git cl format. Done, that's handy thanks. https://codereview.chromium.org/2521823008/diff/240001/skia/ext/SkOpts_hsw_st... File skia/ext/SkOpts_hsw_stub.cc (left): https://codereview.chromium.org/2521823008/diff/240001/skia/ext/SkOpts_hsw_st... skia/ext/SkOpts_hsw_stub.cc:13: On 2016/12/08 at 22:57:46, Sam McNally wrote: > Revert this file. Done
https://codereview.chromium.org/2521823008/diff/280001/chrome/browser/net_ben... File chrome/browser/net_benchmarking_impl.cc (right): https://codereview.chromium.org/2521823008/diff/280001/chrome/browser/net_ben... chrome/browser/net_benchmarking_impl.cc:117: base::Bind(&ClearPredictorCacheOnUIThread, profile_), Reformat. https://codereview.chromium.org/2521823008/diff/280001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (left): https://codereview.chromium.org/2521823008/diff/280001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:68: WebCache::clear(); I think this is still needed. https://codereview.chromium.org/2521823008/diff/280001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (right): https://codereview.chromium.org/2521823008/diff/280001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:81: GetNetBenchmarking().ClearCache(callback); Sync mojo methods generate sync and async overloads. The sync versions take pointers for out parameters instead of a callback. See https://www.chromium.org/developers/design-documents/mojo/synchronous-calls
https://codereview.chromium.org/2521823008/diff/280001/chrome/browser/net_ben... File chrome/browser/net_benchmarking_impl.cc (right): https://codereview.chromium.org/2521823008/diff/280001/chrome/browser/net_ben... chrome/browser/net_benchmarking_impl.cc:117: base::Bind(&ClearPredictorCacheOnUIThread, profile_), On 2016/12/12 at 22:23:52, Sam McNally wrote: > Reformat. Done https://codereview.chromium.org/2521823008/diff/280001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (left): https://codereview.chromium.org/2521823008/diff/280001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:68: WebCache::clear(); On 2016/12/12 at 22:23:52, Sam McNally wrote: > I think this is still needed. Oops, done. https://codereview.chromium.org/2521823008/diff/280001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (right): https://codereview.chromium.org/2521823008/diff/280001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:81: GetNetBenchmarking().ClearCache(callback); On 2016/12/12 at 22:23:52, Sam McNally wrote: > Sync mojo methods generate sync and async overloads. The sync versions take pointers for out parameters instead of a callback. See https://www.chromium.org/developers/design-documents/mojo/synchronous-calls Done. That's neat
The CQ bit was checked by dvallet@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...
https://codereview.chromium.org/2521823008/diff/300001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (right): https://codereview.chromium.org/2521823008/diff/300001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:88: GetNetBenchmarking().ClearHostResolverCache(callback); Change the rest of these calls to use the sync versions too.
https://codereview.chromium.org/2521823008/diff/300001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (right): https://codereview.chromium.org/2521823008/diff/300001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:88: GetNetBenchmarking().ClearHostResolverCache(callback); On 2016/12/12 22:57:04, Sam McNally wrote: > Change the rest of these calls to use the sync versions too. Done. Gotcha, I thought it was only for methods that return a value
lgtm
lgtm
dvallet@chromium.org changed reviewers: + dcheng@chromium.org, sky@chromium.org
I'm changing mojo IPC as part of my chromie noogler project. @dcheng: Could you please check security for the new mojo interface? @sky: Could you please check that I haven't messed up anything? Thanks!
https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2936: void ChromeContentBrowserClient::ExposeInterfacesToRenderer( What thread does this code run on? https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/net_ben... File chrome/browser/net_benchmarking_impl.cc (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/net_ben... chrome/browser/net_benchmarking_impl.cc:56: if (!CheckBenchmarkingEnabled()) { no {} https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/net_ben... File chrome/browser/net_benchmarking_impl.h (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/net_ben... chrome/browser/net_benchmarking_impl.h:20: class NetBenchmarkingImpl : public chrome::mojom::NetBenchmarking { The reason we use mojom in the namespace everywhere is so you don't need to change the implementation name. In other words, use NetBenchmarking as the class name. https://codereview.chromium.org/2521823008/diff/320001/chrome/common/net_benc... File chrome/common/net_benchmarking.mojom (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/common/net_benc... chrome/common/net_benchmarking.mojom:8: // Message sent from the renderer to the browser to request that the browser I think test only functions should live in separate interfaces. That way you can control access to it better. https://codereview.chromium.org/2521823008/diff/320001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:68: (ConnectToBrowser())); Do you really need the parens around ConnectToBrowser() here?
The CQ bit was checked by dvallet@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...
https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2936: void ChromeContentBrowserClient::ExposeInterfacesToRenderer( On 2016/12/13 02:02:52, sky wrote: > What thread does this code run on? BrowserThread::IO https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/net_ben... File chrome/browser/net_benchmarking_impl.cc (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/net_ben... chrome/browser/net_benchmarking_impl.cc:56: if (!CheckBenchmarkingEnabled()) { On 2016/12/13 02:02:52, sky wrote: > no {} Done. https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/net_ben... File chrome/browser/net_benchmarking_impl.h (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/net_ben... chrome/browser/net_benchmarking_impl.h:20: class NetBenchmarkingImpl : public chrome::mojom::NetBenchmarking { On 2016/12/13 02:02:52, sky wrote: > The reason we use mojom in the namespace everywhere is so you don't need to > change the implementation name. In other words, use NetBenchmarking as the class > name. Done. Just to note that from the documentation it implied to use the *Ipml format, and it seems like in the codebase is somehow mixed: https://cs.chromium.org/search/?q=%22:+public+mojom::%22&p=1&sq=package:chrom... https://codereview.chromium.org/2521823008/diff/320001/chrome/common/net_benc... File chrome/common/net_benchmarking.mojom (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/common/net_benc... chrome/common/net_benchmarking.mojom:8: // Message sent from the renderer to the browser to request that the browser On 2016/12/13 02:02:52, sky wrote: > I think test only functions should live in separate interfaces. That way you can > control access to it better. Done. Like so? https://codereview.chromium.org/2521823008/diff/320001/chrome/renderer/net_be... File chrome/renderer/net_benchmarking_extension.cc (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/renderer/net_be... chrome/renderer/net_benchmarking_extension.cc:68: (ConnectToBrowser())); On 2016/12/13 02:02:52, sky wrote: > Do you really need the parens around ConnectToBrowser() here? Maybe? If removed, I get the following error: error: no matching constructor for initialization of 'chrome::mojom::NetBenchmarkingPtr' (aka 'InterfacePtr<chrome::mojom::NetBenchmarking>')
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2936: void ChromeContentBrowserClient::ExposeInterfacesToRenderer( On 2016/12/13 05:20:04, dvallet wrote: > On 2016/12/13 02:02:52, sky wrote: > > What thread does this code run on? > > BrowserThread::IO I didn't think profile lookup is thread safe (line 2942 new). Am I wrong there?
https://codereview.chromium.org/2521823008/diff/320001/chrome/common/net_benc... File chrome/common/net_benchmarking.mojom (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/common/net_benc... chrome/common/net_benchmarking.mojom:8: // Message sent from the renderer to the browser to request that the browser On 2016/12/13 02:02:52, sky wrote: > I think test only functions should live in separate interfaces. That way you can > control access to it better. All the messages in this interface are test-only.
https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2521823008/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2936: void ChromeContentBrowserClient::ExposeInterfacesToRenderer( On 2016/12/13 14:09:59, sky wrote: > On 2016/12/13 05:20:04, dvallet wrote: > > On 2016/12/13 02:02:52, sky wrote: > > > What thread does this code run on? > > > > BrowserThread::IO > > I didn't think profile lookup is thread safe (line 2942 new). Am I wrong there? Sorry, after talking to Sam and Johan this is actually running in UI thread
Ok, LGTM
On 2016/12/13 22:31:46, Sam McNally wrote: > https://codereview.chromium.org/2521823008/diff/320001/chrome/common/net_benc... > File chrome/common/net_benchmarking.mojom (right): > > https://codereview.chromium.org/2521823008/diff/320001/chrome/common/net_benc... > chrome/common/net_benchmarking.mojom:8: // Message sent from the renderer to the > browser to request that the browser > On 2016/12/13 02:02:52, sky wrote: > > I think test only functions should live in separate interfaces. That way you > can > > control access to it better. > > All the messages in this interface are test-only. I've reverted that change.
https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2947: base::Bind(&NetBenchmarking::Create, profile, context)); Let's avoid installing the interface at all if benchmarking isn't enabled. https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_manifest_overlay.json:11: "chrome::mojom::NetBenchmarkingTest", Is line 11 needed? https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/net_ben... File chrome/browser/net_benchmarking.cc (right): https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/net_ben... chrome/browser/net_benchmarking.cc:71: checked = true; Given that we'll only need to call this once, I don't think it's necessary to keep the static bools around; just return the result directly. https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/net_ben... chrome/browser/net_benchmarking.cc:99: } Nit: omit { }
https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2947: base::Bind(&NetBenchmarking::Create, profile, context)); On 2016/12/20 09:14:45, dcheng wrote: > Let's avoid installing the interface at all if benchmarking isn't enabled. Done. https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_manifest_overlay.json:11: "chrome::mojom::NetBenchmarkingTest", On 2016/12/20 09:14:45, dcheng wrote: > Is line 11 needed? Done. https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/net_ben... File chrome/browser/net_benchmarking.cc (right): https://codereview.chromium.org/2521823008/diff/360001/chrome/browser/net_ben... chrome/browser/net_benchmarking.cc:99: } On 2016/12/20 09:14:45, dcheng wrote: > Nit: omit { } Done.
mojo lgtm
The CQ bit was checked by dvallet@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 dvallet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, tibell@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2521823008/#ps380001 (title: "Do not add NetBenchmaring interface if benchmarking is disabled")
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": 380001, "attempt_start_ts": 1482289921839480,
"parent_rev": "2145bf4c592b674e849fbced2fbea471f0b23ad4", "commit_rev":
"899c1c11d4cf9f88dd18ad9d17f67e674c18367f"}
Message was sent while issue was closed.
Description was changed from ========== Migrate chrome_net_benchmarking_message_filter to mojo BUG=577685 ========== to ========== Migrate chrome_net_benchmarking_message_filter to mojo BUG=577685 Review-Url: https://codereview.chromium.org/2521823008 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Migrate chrome_net_benchmarking_message_filter to mojo BUG=577685 Review-Url: https://codereview.chromium.org/2521823008 ========== to ========== Migrate chrome_net_benchmarking_message_filter to mojo BUG=577685 Committed: https://crrev.com/233f57ab4869984231b841dc58dff2dde3545072 Cr-Commit-Position: refs/heads/master@{#439996} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/233f57ab4869984231b841dc58dff2dde3545072 Cr-Commit-Position: refs/heads/master@{#439996} |
