Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2545323002:
Implemented stub ShareService mojo service, for navigator.share. (Closed)
|
Created:
4 years ago by constantina Modified:
3 years, 11 months ago CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplemented stub ShareService mojo service, for navigator.share.
For desktop Linux, Chrome OS and Windows only.
Only active behind the
--enable-experimental-web-platform-features flag.
BUG=668389
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/c8b2173b722aacf39f77de2f98b7a6336d2a3953
Cr-Commit-Position: refs/heads/master@{#438763}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 1
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : Wrote unit test. Passed. #Patch Set 16 : Small refactor of test. Actually added test file. #Patch Set 17 : Refactor test. #Patch Set 18 : Refactor test. #Patch Set 19 : Comment and log cleanup. #Patch Set 20 : Comment and log cleanup. #
Total comments: 27
Patch Set 21 : Added 'expected' parameter to callback, as per feedback. #
Total comments: 2
Patch Set 22 : Rename variable, as per feedback. #Patch Set 23 : #Patch Set 24 : Added OWNERS file to webshare directory #
Total comments: 26
Patch Set 25 : Renamed methods, used aliases, etc. as per feedback. #Patch Set 26 : Added TearDown #Patch Set 27 : Refactor to move implementation skeleton to content instead of chrome #Patch Set 28 : Nits #Patch Set 29 : Nits #
Total comments: 2
Patch Set 30 : Deleted sneaky file #Patch Set 31 : Fix nit, as per feedback #Patch Set 32 : Reverted to chrome version #Patch Set 33 : Sync and rebase #Patch Set 34 : Rebase and sync again #Dependent Patchsets: Messages
Total messages: 81 (47 generated)
Description was changed from ========== fixed all compiler errors; removed references to browser context BUG= ========== to ========== fixed all compiler errors; removed references to browser context BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== fixed all compiler errors; removed references to browser context BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implemented 'share' mojo service on desktop browser. BUG=668389 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by constantina@google.com 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...)
constantina@google.com changed reviewers: + mgiuca@chromium.org
The CQ bit was checked by constantina@google.com 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: 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-...)
https://codereview.chromium.org/2545323002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2545323002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2326: switches::kEnableExperimentalWebPlatformFeatures)) { You shouldn't check for this switch directly; you can check directly for the WebShare feature flag (which, in turn, is controlled by kEnableExperimentalWebPlatformFeatures). I think you should be able to use RuntimeEnabledFeatures::webShareEnabled() by including blink/platform/RuntimeEnabledFeatures.h (see https://cs.chromium.org/chromium/src/out/Debug/gen/blink/platform/RuntimeEnab...). Try this and see if it works. I think we shouldn't leave this bare, because we don't want a compromised renderer to be able to access this untested feature.
The CQ bit was checked by constantina@google.com 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.
https://codereview.chromium.org/2545323002/diff/140001/content/browser/websha... File content/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2545323002/diff/140001/content/browser/websha... content/browser/webshare/share_service_impl.h:13: namespace content { This should be moved to chrome (note that the Android implementation is in chrome/android/java). That should fix the layering violation of accessing the origin trial flag status.
On 2016/12/05 05:57:46, Matt Giuca wrote: > https://codereview.chromium.org/2545323002/diff/140001/content/browser/websha... > File content/browser/webshare/share_service_impl.h (right): > > https://codereview.chromium.org/2545323002/diff/140001/content/browser/websha... > content/browser/webshare/share_service_impl.h:13: namespace content { > This should be moved to chrome (note that the Android implementation is in > chrome/android/java). That should fix the layering violation of accessing the > origin trial flag status. Done
The CQ bit was checked by constantina@google.com 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 constantina@google.com 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by constantina@google.com 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 constantina@google.com 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...
Tests added.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great start! https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. CL description: it's a bit misleading to say you implemented the service, since it doesn't actually do anything yet. Say you implemented a stub service and wired it up to navigator.share. Also say the platforms are Linux, Chrome OS and Windows only. Also say that this is only active behind the --enable-experimental-web-platform-features flag. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3046: #if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_WIN) You shouldn't need OS_CHROMEOS because LINUX implies CHROMEOS. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:23: NOTIMPLEMENTED() << "Share called"; Add TODO(constantina): .... Explain briefly that you plan to implement WST here. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:23: NOTIMPLEMENTED() << "Share called"; Get rid of the string (just "NOTIMPLEMENTED();"). If you look at the log message, it is quite verbose as it is: [25461:25461:1208/173659.851063:ERROR:share_service_impl.cc(23)] Not implemented reached in virtual void ShareServiceImpl::Share(const std::string &, const std::string &, const GURL &, const ShareCallback &)Share called So there is no need to write "Share called", and FYI if you do, you have to put a space in front of it or it looks ugly. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:14: // third_party/WebKit/public/platform/modules/webshare/webshare.mojom. Don't think this is necessary (a code search will show where blink::mojom::ShareService is found if it isn't obvious from the #includes). Unless this is idiomatic for Mojo subclasses? https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:15: // chrome/browser/webshare/share_service_impl.h No comment required here. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:22: ChromeRenderViewHostTestHarness::SetUp(); nit: Blank line after. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:26: void callback(const base::Optional<std::string>& str) { Add an argument "const base::Optional<std::string>& expected" which you pass in to base::Bind below. Because each test will expect a different result (potentially), so you want the expected value (either null or a string) to be in the test, not in the class definition. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:40: LOG(ERROR) << "ShareCallbackSuccess entered"; Remove this debug log. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:43: base::RunLoop run_loop; Move this into SetUp (and make run_loop a class variable so it can be Run() from the test). https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:46: base::Callback<void(const base::Optional<std::string>&)> cb = style: Don't abbreviate words in variable names (cb -> callback). https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:50: run_loop.Run(); You *might* be able to get away with putting this in TearDown() instead of having to put it at the end of each test. Try it and see if it works (and fails correctly too). (As I said, it might be wrong to do that since technically the callback checking will happen in the TearDown, but I think that might be preferable if it works, to having to call Run() at the end of every test.)
Description was changed from ========== Implemented 'share' mojo service on desktop browser. BUG=668389 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implemented stub ShareService mojo service, for navigator.share. For desktop Linux, Chrome OS and Windows only. Only active behind the --enable-experimental-web-platform-features flag. BUG=668389 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/12/08 06:42:56, Matt Giuca wrote: > CL description: it's a bit misleading to say you implemented the service, since > it doesn't actually do anything yet. > > Say you implemented a stub service and wired it up to navigator.share. > > Also say the platforms are Linux, Chrome OS and Windows only. > > Also say that this is only active behind the > --enable-experimental-web-platform-features flag. Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3046: #if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_WIN) On 2016/12/08 06:42:56, Matt Giuca wrote: > You shouldn't need OS_CHROMEOS because LINUX implies CHROMEOS. Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:23: NOTIMPLEMENTED() << "Share called"; On 2016/12/08 06:42:56, Matt Giuca wrote: > Get rid of the string (just "NOTIMPLEMENTED();"). > > If you look at the log message, it is quite verbose as it is: > > [25461:25461:1208/173659.851063:ERROR:share_service_impl.cc(23)] Not implemented > reached in virtual void ShareServiceImpl::Share(const std::string &, const > std::string &, const GURL &, const ShareCallback &)Share called > > So there is no need to write "Share called", and FYI if you do, you have to put > a space in front of it or it looks ugly. Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:23: NOTIMPLEMENTED() << "Share called"; On 2016/12/08 06:42:56, Matt Giuca wrote: > Add TODO(constantina): .... > > Explain briefly that you plan to implement WST here. Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:14: // third_party/WebKit/public/platform/modules/webshare/webshare.mojom. On 2016/12/08 06:42:56, Matt Giuca wrote: > Don't think this is necessary (a code search will show where > blink::mojom::ShareService is found if it isn't obvious from the #includes). > > Unless this is idiomatic for Mojo subclasses? Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:15: // chrome/browser/webshare/share_service_impl.h On 2016/12/08 06:42:56, Matt Giuca wrote: > No comment required here. Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:22: ChromeRenderViewHostTestHarness::SetUp(); On 2016/12/08 06:42:56, Matt Giuca wrote: > nit: Blank line after. Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:26: void callback(const base::Optional<std::string>& str) { On 2016/12/08 06:42:56, Matt Giuca wrote: > Add an argument "const base::Optional<std::string>& expected" which you pass in > to base::Bind below. > > Because each test will expect a different result (potentially), so you want the > expected value (either null or a string) to be in the test, not in the class > definition. Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:40: LOG(ERROR) << "ShareCallbackSuccess entered"; On 2016/12/08 06:42:56, Matt Giuca wrote: > Remove this debug log. Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:43: base::RunLoop run_loop; On 2016/12/08 06:42:56, Matt Giuca wrote: > Move this into SetUp (and make run_loop a class variable so it can be Run() from > the test). Can't be done easily. In addition, run_loop can't be elegantly used multiple times in the same test. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:46: base::Callback<void(const base::Optional<std::string>&)> cb = On 2016/12/08 06:42:56, Matt Giuca wrote: > style: Don't abbreviate words in variable names (cb -> callback). Done. https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:50: run_loop.Run(); On 2016/12/08 06:42:56, Matt Giuca wrote: > You *might* be able to get away with putting this in TearDown() instead of > having to put it at the end of each test. Try it and see if it works (and fails > correctly too). > > (As I said, it might be wrong to do that since technically the callback checking > will happen in the TearDown, but I think that might be preferable if it works, > to having to call Run() at the end of every test.) Acknowledged.
https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:23: NOTIMPLEMENTED() << "Share called"; On 2016/12/09 02:24:14, constantina wrote: > On 2016/12/08 06:42:56, Matt Giuca wrote: > > Add TODO(constantina): .... > > > > Explain briefly that you plan to implement WST here. > > Done. nit: No "@" in the TODO; capital "I" in "Implement". https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:43: base::RunLoop run_loop; On 2016/12/09 02:24:14, constantina wrote: > On 2016/12/08 06:42:56, Matt Giuca wrote: > > Move this into SetUp (and make run_loop a class variable so it can be Run() > from > > the test). > > Can't be done easily. In addition, run_loop can't be elegantly used multiple > times in the same test. Acknowledged. https://codereview.chromium.org/2545323002/diff/400001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2545323002/diff/400001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:29: if (!quit_closure_.is_null()) Maybe rename this to |on_callback_|, because this class is oblivious to the fact that we're quitting a run-loop here.
https://codereview.chromium.org/2545323002/diff/400001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2545323002/diff/400001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:29: if (!quit_closure_.is_null()) On 2016/12/09 02:32:21, Matt Giuca wrote: > Maybe rename this to |on_callback_|, because this class is oblivious to the fact > that we're quitting a run-loop here. I like it. Done.
https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2545323002/diff/380001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:23: NOTIMPLEMENTED() << "Share called"; On 2016/12/09 02:32:21, Matt Giuca wrote: > On 2016/12/09 02:24:14, constantina wrote: > > On 2016/12/08 06:42:56, Matt Giuca wrote: > > > Add TODO(constantina): .... > > > > > > Explain briefly that you plan to implement WST here. > > > > Done. > > nit: No "@" in the TODO; capital "I" in "Implement". Done.
Beautiful! lgtm
Please add an OWNERS file in c/b/webshare with sammc and myself.
constantina@google.com changed reviewers: + jochen@chromium.org, sammc@chromium.org
sammc@ PTAL jochen@ for OWNERS on chrome/browser and chrome/test c/b/webshare will be the directory for the browser-side desktop implementation of Web Share.
https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:12: ShareServiceImpl::~ShareServiceImpl() {} = default; https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:15: void ShareServiceImpl::Create(mojo::InterfaceRequest<ShareService> request) { s/mojo::InterfaceRequest<ShareService>/ShareServiceRequest/ https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:16: new ShareServiceImpl(std::move(request)); This will leak the ShareServiceImpl. Use StrongBinding instead: mojo::MakeStrongBinding(base::MakeUnique<ShareServiceImpl>(), std::move(request)); and remove |binding_|. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:7: #include <string> https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:9: #include "mojo/public/cpp/bindings/strong_binding.h" Move this into the .cc file. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:11: #include "url/gurl.h" Forward declare GURL instead. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:14: class ShareServiceTest : public ChromeRenderViewHostTestHarness { Does this need to be a ChromeRenderViewHostTestHarness rather than a testing::Test? https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:16: ShareServiceTest() {} = default; https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:17: ~ShareServiceTest() override {} = default; https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:25: void callback(const base::Optional<std::string>& expected, DidShare() https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:38: TEST_F(ShareServiceTest, ShareCallbackSuccess) { It's not exactly a successful share.
why is this service in chrome (as opposed to content)? https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:28: }; disallow copy/assign
On 2016/12/09 08:30:19, jochen wrote: > why is this service in chrome (as opposed to content)? I'm never really sure where things are supposed to go... I thought Onion Soup meant we were trying to move things out of content and up into Chrome or down into Blink. On the practical side, this feature will (in future CLs) use Chrome-specific UI so that might be a reason for it to live in Chrome and other browsers that want it would have to implement their own Mojo service and UI. But if you don't see that as an issue we can move it down into content.
On 2016/12/09 08:30:19, jochen wrote: > why is this service in chrome (as opposed to content)? In addition to what Matt said, we have a dependency on ChromeRenderViewHostTestHarness. Currently looking into alternatives.
https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:12: ShareServiceImpl::~ShareServiceImpl() {} On 2016/12/09 04:23:45, Sam McNally wrote: > = default; Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:15: void ShareServiceImpl::Create(mojo::InterfaceRequest<ShareService> request) { On 2016/12/09 04:23:45, Sam McNally wrote: > s/mojo::InterfaceRequest<ShareService>/ShareServiceRequest/ Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:16: new ShareServiceImpl(std::move(request)); On 2016/12/09 04:23:45, Sam McNally wrote: > This will leak the ShareServiceImpl. Use StrongBinding instead: > > mojo::MakeStrongBinding(base::MakeUnique<ShareServiceImpl>(), > std::move(request)); > > and remove |binding_|. Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:7: On 2016/12/09 04:23:45, Sam McNally wrote: > #include <string> Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:9: #include "mojo/public/cpp/bindings/strong_binding.h" On 2016/12/09 04:23:45, Sam McNally wrote: > Move this into the .cc file. Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:11: #include "url/gurl.h" On 2016/12/09 04:23:45, Sam McNally wrote: > Forward declare GURL instead. Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:28: }; On 2016/12/09 08:30:19, jochen wrote: > disallow copy/assign Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:14: class ShareServiceTest : public ChromeRenderViewHostTestHarness { On 2016/12/09 04:23:45, Sam McNally wrote: > Does this need to be a ChromeRenderViewHostTestHarness rather than a > testing::Test? I believe it is needed to make |run_loop| work correctly. Currently looking into using RenderViewHostTestHarness instead of ChromeRenderViewHostTestHarness, to remove chrome dependency. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:16: ShareServiceTest() {} On 2016/12/09 04:23:45, Sam McNally wrote: > = default; Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:17: ~ShareServiceTest() override {} On 2016/12/09 04:23:45, Sam McNally wrote: > = default; Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:25: void callback(const base::Optional<std::string>& expected, On 2016/12/09 04:23:45, Sam McNally wrote: > DidShare() Done. https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:38: TEST_F(ShareServiceTest, ShareCallbackSuccess) { On 2016/12/09 04:23:45, Sam McNally wrote: > It's not exactly a successful share. Changed. This test will probably be changed/removed with the next CL; it's mostly a placeholder.
https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:12: ShareServiceImpl::~ShareServiceImpl() {} On 2016/12/12 02:50:32, constantina wrote: > On 2016/12/09 04:23:45, Sam McNally wrote: > > = default; > > Done. I think this still has to be in the .cc file to avoid it being duplicated in all the clients. Does it work if you write that here: ShareServiceImpl::~ShareServiceImpl() = default; ? https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2545323002/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:14: class ShareServiceTest : public ChromeRenderViewHostTestHarness { On 2016/12/12 02:50:32, constantina wrote: > On 2016/12/09 04:23:45, Sam McNally wrote: > > Does this need to be a ChromeRenderViewHostTestHarness rather than a > > testing::Test? > > I believe it is needed to make |run_loop| work correctly. Currently looking into > using RenderViewHostTestHarness instead of ChromeRenderViewHostTestHarness, to > remove chrome dependency. dominickn@ said this was necessary to make it actually run with a browser process. But did you ever try it with the run_loop but without the ChromeRenderViewHostTestHarness (and just a normal test)? I assume it wouldn't work but might be worth trying.
content has RenderViewTestHarness which is the baseclass of ChromeRenderViewTestHarness - so you should be able to write a content_browsertest just as easily. I agree that having the UI in chrome makes sense, but having the "business logic" in content enables you to write layout tests for it
The CQ bit was checked by constantina@google.com 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. Refactored so the ShareServiceImpl mojo service implementation and registration is in content/ instead of browser/.
lgtm again (with nit). https://codereview.chromium.org/2545323002/diff/560001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2545323002/diff/560001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2301: OriginTrialPolicy* otp = GetContentClient()->GetOriginTrialPolicy(); nit: You could call this |policy| instead of |otp| (since abbreviations are frowned upon, although I don't super mind in this case).
https://codereview.chromium.org/2545323002/diff/560001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2545323002/diff/560001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2301: OriginTrialPolicy* otp = GetContentClient()->GetOriginTrialPolicy(); On 2016/12/13 06:06:08, Matt Giuca wrote: > nit: You could call this |policy| instead of |otp| (since abbreviations are > frowned upon, although I don't super mind in this case). Done.
Hi all, I have moved the skeleton implementation from chrome to content, however, the actual implementation will primarily be using chrome methods; creating a new tab, requesting the installed web apps... If the implementation stays in content, it will almost entirely be calls to a delegate. Is using a delegate for everything okay? Or is do we want the implementation to live in chrome, where it won't use the delegate as a proxy?
i'll trust your judgement which version makes more sense, so lgtm for either one
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2545323002/#ps610001 (title: "Reverted to chrome version")
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by constantina@google.com 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 constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2545323002/#ps650001 (title: "Rebase and sync again")
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: 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 constantina@google.com
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": 650001, "attempt_start_ts": 1481777302662740,
"parent_rev": "700d8b1e119ba57653245c2f4d282c9b10be38c5", "commit_rev":
"8b1fbbe5e372a7d046e58ef292cbc35f5f1f8e41"}
Message was sent while issue was closed.
Description was changed from ========== Implemented stub ShareService mojo service, for navigator.share. For desktop Linux, Chrome OS and Windows only. Only active behind the --enable-experimental-web-platform-features flag. BUG=668389 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implemented stub ShareService mojo service, for navigator.share. For desktop Linux, Chrome OS and Windows only. Only active behind the --enable-experimental-web-platform-features flag. BUG=668389 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2545323002 ==========
Message was sent while issue was closed.
Committed patchset #34 (id:650001)
Message was sent while issue was closed.
Description was changed from ========== Implemented stub ShareService mojo service, for navigator.share. For desktop Linux, Chrome OS and Windows only. Only active behind the --enable-experimental-web-platform-features flag. BUG=668389 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2545323002 ========== to ========== Implemented stub ShareService mojo service, for navigator.share. For desktop Linux, Chrome OS and Windows only. Only active behind the --enable-experimental-web-platform-features flag. BUG=668389 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c8b2173b722aacf39f77de2f98b7a6336d2a3953 Cr-Commit-Position: refs/heads/master@{#438763} ==========
Message was sent while issue was closed.
Patchset 34 (id:??) landed as https://crrev.com/c8b2173b722aacf39f77de2f98b7a6336d2a3953 Cr-Commit-Position: refs/heads/master@{#438763}
Message was sent while issue was closed.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Message was sent while issue was closed.
Normally, we don't add things to mojoms without reviewing the implementation as well. Please make sure to include an IPC security reviewer on followup CLs that actually implement the browser-side service. Thanks.
Message was sent while issue was closed.
Description was changed from ========== Implemented stub ShareService mojo service, for navigator.share. For desktop Linux, Chrome OS and Windows only. Only active behind the --enable-experimental-web-platform-features flag. BUG=668389 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c8b2173b722aacf39f77de2f98b7a6336d2a3953 Cr-Commit-Position: refs/heads/master@{#438763} ========== to ========== Implemented stub ShareService mojo service, for navigator.share. For desktop Linux, Chrome OS and Windows only. Only active behind the --enable-experimental-web-platform-features flag. BUG=668389 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c8b2173b722aacf39f77de2f98b7a6336d2a3953 Cr-Commit-Position: refs/heads/master@{#438763} ========== |
