Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1495723004:
Minimum implementation of ARC clipboard Bridge (Closed)
|
Created:
5 years ago by Chi-Ngai Wan Modified:
4 years, 11 months ago CC:
dcheng, chromium-reviews, davemoore+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMinimum implementation of ARC Clipboard Bridge.
All change sets include:
- http://crrev.com/1495723004 (Chromium)
- http://ag/833451 (Android device/google/cheets)
- http://ag/833422 (Android platform/frameworks/base)
BUG=565283
TEST=Build both sides. Ctrl-C/Ctrl-V from either side reflect to the other.
Committed: https://crrev.com/f5601134a83382fdda85405fa41904e8ceb4f4ef
Cr-Commit-Position: refs/heads/master@{#368547}
Patch Set 1 #Patch Set 2 : Rebased to origin/master #Patch Set 3 : Rebased to catch arc++ notification cl #
Total comments: 13
Patch Set 4 : Rebased and implement in mojo #Patch Set 5 : Rebased, migrated to MOJO, moved from c/b/chromeos to components/arc/clipboard #Patch Set 6 : Rebased #Patch Set 7 : Fix the fakes #
Total comments: 14
Patch Set 8 : Rebased and refactored #
Total comments: 1
Patch Set 9 : #
Total comments: 2
Patch Set 10 : Fixes for reviewer's comments #Patch Set 11 : #
Total comments: 4
Patch Set 12 : #Patch Set 13 : Rebased and introduced OnClipboardVersionReady/CloseClipboardChannel #Patch Set 14 : Fixed code comment #Patch Set 15 : Fixed typo #Patch Set 16 : Fixed tryboy warnings #
Total comments: 6
Patch Set 17 : Rebased and address review comments #Messages
Total messages: 83 (38 generated)
Description was changed from ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes set of the IPC message definitions as well. BUG=565283 ========== to ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes set of the IPC message definitions as well. BUG=565283 BUG=b/24307886 ==========
Description was changed from ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes set of the IPC message definitions as well. BUG=565283 BUG=b/24307886 ========== to ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 BUG=b/24307886 ==========
cnwan@google.com changed reviewers: + dcheng@chromium.org, elijahtaylor@google.com, jochen@chromium.org
cnwan@google.com changed reviewers: + elijahtaylor@chromium.org
cnwan@google.com changed reviewers: + cnwan@google.com
Hi, This is my implementation of ARC++ clipboard manager in Chrome. The related Android CL is WIP that should come in a few days. Please take a look. Thanks.
Description was changed from ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 BUG=b/24307886 ========== to ========== Minimum implementation of ARC++ clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 BUG=b/24307886 ==========
Description was changed from ========== Minimum implementation of ARC++ clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 BUG=b/24307886 ========== to ========== Minimum implementation of ARC++ clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 BUG=b/24307886 ==========
cnwan@google.com changed reviewers: + lhchavez@chromium.org - cnwan@google.com
Description was changed from ========== Minimum implementation of ARC++ clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 BUG=b/24307886 ========== to ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 BUG=b/24307886 ==========
https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.cc (right): https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.cc:36: arc::ArcBridgeService* bridge_service = arc::ArcBridgeService::Get(); you don't need bridge_service below, so you could remove this and the check, and it should be impossible that you get this message without the service up anyway https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.cc:49: if (!bridge_service) { maybe a CHECK? This method should only be called from the bridge itself, so it would be super unlikely this would be null https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.h (right): https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.h:12: namespace chromeos { other people may have opinions about this, maybe it should be namespaced to arc instead of chromeos like other ARC services? https://codereview.chromium.org/1495723004/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1495723004/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.h:57: bool SendClipboardContentToAndroid(const ClipboardData data); should this be passed by reference? https://codereview.chromium.org/1495723004/diff/40001/components/arc/common/a... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1495723004/diff/40001/components/arc/common/a... components/arc/common/arc_host_messages.h:60: // or service tries to retrieve the android clipboard content. nit: capitalize Android
https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:86: scoped_ptr<ArcClipboardManager> arc_clipboard_manager_; denniskempin@ is creating a new class to hold all ARC-related services: https://codereview.chromium.org/1496193002/ Looks like your code does not actually depend on Chrome (it only has ui/ includes), so you might be able to leverage it and avoid adding this directly to ChromeBrowserMain.
On 2015/12/04 20:52:03, Luis Héctor Chávez wrote: > https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): > > https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/chrome_browser_main_chromeos.h:86: > scoped_ptr<ArcClipboardManager> arc_clipboard_manager_; > denniskempin@ is creating a new class to hold all ARC-related services: > https://codereview.chromium.org/1496193002/ > > Looks like your code does not actually depend on Chrome (it only has ui/ > includes), so you might be able to leverage it and avoid adding this directly to > ChromeBrowserMain. To clarify further: if possible, move your code in chrome/browser/chromeos/arc/clipboard/ to components/arc/clipboard/ and add your service to ArcServiceManager.
https://codereview.chromium.org/1495723004/diff/40001/components/arc/common/a... File components/arc/common/arc_message_types.h (right): https://codereview.chromium.org/1495723004/diff/40001/components/arc/common/a... components/arc/common/arc_message_types.h:66: enum ClipboardDataType : int { enum class =)
please don't rely on global getters, make lifetime relationships explicit please also remove the buganizer reference from the CL description https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.cc (right): https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.cc:16: arc::ArcBridgeService* bridge_service = arc::ArcBridgeService::Get(); please make this a ctor parameter so it can be mocked for unit tests https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.cc:18: LOG(ERROR) << "Bridge service is not available. Cannot initialize."; can this ever occur? if yes, how will the user be notified about this? also, no LOG https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.cc:36: arc::ArcBridgeService* bridge_service = arc::ArcBridgeService::Get(); also, you should just store the service as a member https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.h (right): https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.h:12: namespace chromeos { On 2015/12/04 at 20:36:51, elijahtaylor1 wrote: > other people may have opinions about this, maybe it should be namespaced to arc instead of chromeos like other ARC services? the arc namespace should only be used by the arc component https://codereview.chromium.org/1495723004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/clipboard/arc_clipboard_manager.h:23: }; private: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1495723004/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1495723004/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.cc:163: LOG(ERROR) << "Called SendClipboardContentToAndroid when the service is" no LOG please use DVLOG/VLOG the user won't see this errors, if this situation can actually occur in practice, please plumb the error back to the caller so they can show appropriate UI to the user
Description was changed from ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 BUG=b/24307886 ========== to ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 ==========
Description was changed from ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 ========== to ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 ==========
cnwan@google.com changed reviewers: + avi@chromium.org - elijahtaylor@google.com, jochen@chromium.org
cnwan@google.com changed reviewers: + cnwan@google.com
Hi, I've rebased, migrated the implementation to MOJO and moved the code to arc_service_manager. Please take another look. Thanks.
Description was changed from ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. Note: Android CL is WIP. BUG=565283 ========== to ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. BUG=565283 ==========
Description was changed from ========== Minimum implementation of ARC clipboard manager (on Chrome side). This CL includes the set of the IPC message definitions as well. BUG=565283 ========== to ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 ==========
Description was changed from ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 ========== to ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 ==========
https://codereview.chromium.org/1495723004/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_service.h:122: // or a service tries to retrieve the Android clipboard content. nit: alignment https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... File components/arc/clipboard/clipboard_bridge_impl.cc (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... components/arc/clipboard/clipboard_bridge_impl.cc:18: DCHECK(bridge_service_->state() == ArcBridgeService::State::STOPPED); is this required? https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... components/arc/clipboard/clipboard_bridge_impl.cc:36: ui::Clipboard* clipboard = ui::Clipboard::GetForCurrentThread(); do we need to check which thread we're on to get a consistent view? just curious what "GetForCurrentThread" is for, as if there would be multiple clipboards. https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... File components/arc/clipboard/clipboard_bridge_impl.h (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... components/arc/clipboard/clipboard_bridge_impl.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. I think we've developed a bit of an anti-pattern in components/arc with these impl.h files, and impls without tests. I also think we're using the term Bridge too much, though I'm not sure a better term... ClipboardService? ClipboardManager? I don't know. There are two things we should do IMO: 1) either find a way to test this implementation (preffered) or get rid of the ClipboardBridge / ClipboardBridgeImpl split 2) if you keep this split, clipboard_bridge_impl.h can be inlined in clipboard_bridge_impl.cc
https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... File components/arc/clipboard/clipboard_bridge_impl.cc (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... components/arc/clipboard/clipboard_bridge_impl.cc:18: DCHECK(bridge_service_->state() == ArcBridgeService::State::STOPPED); On 2015/12/15 01:17:05, elijahtaylor1 wrote: > is this required? Shouldn't be. It was required by input, but is not needed anymore. https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... File components/arc/clipboard/clipboard_bridge_impl.h (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... components/arc/clipboard/clipboard_bridge_impl.h:18: public ArcBridgeService::ClipboardObserver { Can you make this inherit ArcBridgeService::Observer and arc::ClipboardHost instead? https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... components/arc/common/arc_bridge.mojom:147: OnSetClipboardContent(string text); Can you split off this into its own interface? clipboard.mojom: module arc; interface ClipboardHost { // Called when the Android clipboard content is changed. This is // typically fired when a user initiates a "Copy" action. // |text| is the text being set in the Android Clipboard. OnSetClipboardContent(string text); // Called when the Android clipboard decides to update its content. // This is triggered when a "Paste" action is fired or an app // or a service tries to retrieve the Android clipboard content. OnGetClipboardContent(); }; interface ClipboardInstance { Init(ClipboardHost host_ptr); // Sends the Chrome clipboard content to Android. // |text| is the clipboard content being sent. SendClipboardContentToAndroid(string text); }; See https://codereview.chromium.org/1523643002/ for reference and further examples.
I'll send out another patch later for splitting out clipboard's mojom from arc_bridge.mojom once crrev.com/1541653002 is relanded. Other than that this patch addresses other review comments. PTAL https://codereview.chromium.org/1495723004/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_service.h:122: // or a service tries to retrieve the Android clipboard content. On 2015/12/15 01:17:05, elijahtaylor1 wrote: > nit: alignment Done. https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... File components/arc/clipboard/clipboard_bridge_impl.cc (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... components/arc/clipboard/clipboard_bridge_impl.cc:18: DCHECK(bridge_service_->state() == ArcBridgeService::State::STOPPED); On 2015/12/16 23:16:23, Luis Héctor Chávez wrote: > On 2015/12/15 01:17:05, elijahtaylor1 wrote: > > is this required? > > Shouldn't be. It was required by input, but is not needed anymore. Done. https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... components/arc/clipboard/clipboard_bridge_impl.cc:36: ui::Clipboard* clipboard = ui::Clipboard::GetForCurrentThread(); On 2015/12/15 01:17:05, elijahtaylor1 OOO until Jan 4 wrote: > do we need to check which thread we're on to get a consistent view? just > curious what "GetForCurrentThread" is for, as if there would be multiple > clipboards. Done. https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... File components/arc/clipboard/clipboard_bridge_impl.h (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/clipboa... components/arc/clipboard/clipboard_bridge_impl.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/12/15 01:17:05, elijahtaylor1 OOO until Jan 4 wrote: > I think we've developed a bit of an anti-pattern in components/arc with these > impl.h files, and impls without tests. I also think we're using the term Bridge > too much, though I'm not sure a better term... ClipboardService? > ClipboardManager? I don't know. I am OK with Service/Manager/Bridge/... I picked 'XXXBridge' just to be consistent with other implementations in components/arc (input/notification/settings). If I had to choose, I would pick XXXService since we have a ServiceManager which in theory manages other arc-related services such as InputService/ClipboardService/SettingsService/... but for now I am a bit towards 'the consistent side'. https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... components/arc/common/arc_bridge.mojom:147: OnSetClipboardContent(string text); On 2015/12/16 23:16:23, Luis Héctor Chávez wrote: > Can you split off this into its own interface? > > clipboard.mojom: > > module arc; > > interface ClipboardHost { > // Called when the Android clipboard content is changed. This is > // typically fired when a user initiates a "Copy" action. > // |text| is the text being set in the Android Clipboard. > OnSetClipboardContent(string text); > > // Called when the Android clipboard decides to update its content. > // This is triggered when a "Paste" action is fired or an app > // or a service tries to retrieve the Android clipboard content. > OnGetClipboardContent(); > }; > > > interface ClipboardInstance { > Init(ClipboardHost host_ptr); > > // Sends the Chrome clipboard content to Android. > // |text| is the clipboard content being sent. > SendClipboardContentToAndroid(string text); > }; > > See https://codereview.chromium.org/1523643002/ for reference and further > examples. Will do once crrev.com/1541653002 is relanded.
https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... components/arc/common/arc_bridge.mojom:147: OnSetClipboardContent(string text); On 2015/12/21 11:18:01, cnwan wrote: > On 2015/12/16 23:16:23, Luis Héctor Chávez wrote: > > Can you split off this into its own interface? > > > > clipboard.mojom: > > > > module arc; > > > > interface ClipboardHost { > > // Called when the Android clipboard content is changed. This is > > // typically fired when a user initiates a "Copy" action. > > // |text| is the text being set in the Android Clipboard. > > OnSetClipboardContent(string text); > > > > // Called when the Android clipboard decides to update its content. > > // This is triggered when a "Paste" action is fired or an app > > // or a service tries to retrieve the Android clipboard content. > > OnGetClipboardContent(); > > }; > > > > > > interface ClipboardInstance { > > Init(ClipboardHost host_ptr); > > > > // Sends the Chrome clipboard content to Android. > > // |text| is the clipboard content being sent. > > SendClipboardContentToAndroid(string text); > > }; > > > > See https://codereview.chromium.org/1523643002/ for reference and further > > examples. > > Will do once crrev.com/1541653002 is relanded. Will this only ever support text? Is copy/pasting other types of content a non-goal? Will that use the same methods with changed parameters or will additional methods be used to support that?
On 2015/12/21 17:33:50, Luis Héctor Chávez wrote: > https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... > File components/arc/common/arc_bridge.mojom (right): > > https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... > components/arc/common/arc_bridge.mojom:147: OnSetClipboardContent(string text); > On 2015/12/21 11:18:01, cnwan wrote: > > On 2015/12/16 23:16:23, Luis Héctor Chávez wrote: > > > Can you split off this into its own interface? > > > > > > clipboard.mojom: > > > > > > module arc; > > > > > > interface ClipboardHost { > > > // Called when the Android clipboard content is changed. This is > > > // typically fired when a user initiates a "Copy" action. > > > // |text| is the text being set in the Android Clipboard. > > > OnSetClipboardContent(string text); > > > > > > // Called when the Android clipboard decides to update its content. > > > // This is triggered when a "Paste" action is fired or an app > > > // or a service tries to retrieve the Android clipboard content. > > > OnGetClipboardContent(); > > > }; > > > > > > > > > interface ClipboardInstance { > > > Init(ClipboardHost host_ptr); > > > > > > // Sends the Chrome clipboard content to Android. > > > // |text| is the clipboard content being sent. > > > SendClipboardContentToAndroid(string text); > > > }; > > > > > > See https://codereview.chromium.org/1523643002/ for reference and further > > > examples. > > > > Will do once crrev.com/1541653002 is relanded. > > Will this only ever support text? Is copy/pasting other types of content a > non-goal? Will that use the same methods with changed parameters or will > additional methods be used to support that? It is possible that we could support other types of clipboard content that are both supported on Android/Chrome. However the way that Android Clipboard works is quite different than Chrome's especially when the content type is non-text. As an Android app, retrieving non-textual content from the clipboard requires two reads (1 to get the clipdata.getUri(), another to retrieve the actual binary data via a ContentProvider with that uri.) This may imply that the Android ArcClipboard implementation needs to cache additional data of variable size. For simplicity we can support text/plain and text/html for now. In my old patches, I created a ArcClipboardData struct to encapsulate a content-type field and the string data that we sent but I found it quite troublesome to convert the mojo generated classes back and fro with JNI. Using directly the MOJO-generated Java types in the Android side would be much better. I'd like to change the mojom interface as follows in the future once we can use the MOJO-generated Java types directly. module arc; enum ClipType { TEXT_PLAIN, TEXT_HTML } struct ClipData { ClipType type; string text; // optional array<uint8> binary_data; // optional } interface ClipboardHost { SetContent(ClipData data); GetContent() => (ClipData data); } interface ClipboardInstance { Init(ClipboardHost host); }
On 2015/12/22 07:53:23, cnwan wrote: > On 2015/12/21 17:33:50, Luis Héctor Chávez wrote: > > > https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... > > File components/arc/common/arc_bridge.mojom (right): > > > > > https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... > > components/arc/common/arc_bridge.mojom:147: OnSetClipboardContent(string > text); > > On 2015/12/21 11:18:01, cnwan wrote: > > > On 2015/12/16 23:16:23, Luis Héctor Chávez wrote: > > > > Can you split off this into its own interface? > > > > > > > > clipboard.mojom: > > > > > > > > module arc; > > > > > > > > interface ClipboardHost { > > > > // Called when the Android clipboard content is changed. This is > > > > // typically fired when a user initiates a "Copy" action. > > > > // |text| is the text being set in the Android Clipboard. > > > > OnSetClipboardContent(string text); > > > > > > > > // Called when the Android clipboard decides to update its content. > > > > // This is triggered when a "Paste" action is fired or an app > > > > // or a service tries to retrieve the Android clipboard content. > > > > OnGetClipboardContent(); > > > > }; > > > > > > > > > > > > interface ClipboardInstance { > > > > Init(ClipboardHost host_ptr); > > > > > > > > // Sends the Chrome clipboard content to Android. > > > > // |text| is the clipboard content being sent. > > > > SendClipboardContentToAndroid(string text); > > > > }; > > > > > > > > See https://codereview.chromium.org/1523643002/ for reference and further > > > > examples. > > > > > > Will do once crrev.com/1541653002 is relanded. relanded :) > > > > Will this only ever support text? Is copy/pasting other types of content a > > non-goal? Will that use the same methods with changed parameters or will > > additional methods be used to support that? > > > It is possible that we could support other types of clipboard content that are > both supported on Android/Chrome. However the way that Android Clipboard works > is quite different than Chrome's especially when the content type is non-text. > As an Android app, retrieving non-textual content from the clipboard requires > two reads (1 to get the clipdata.getUri(), another to retrieve the actual binary > data via a ContentProvider with that uri.) This may imply that the Android > ArcClipboard implementation needs to cache additional data of variable size. For > simplicity we can support text/plain and text/html for now. > > In my old patches, I created a ArcClipboardData struct to encapsulate a > content-type field and the string data that we sent but I found it quite > troublesome to convert the mojo generated classes back and fro with JNI. Using > directly the MOJO-generated Java types in the Android side would be much better. > > I'd like to change the mojom interface as follows in the future once we can use > the MOJO-generated Java types directly. > > module arc; > > enum ClipType { > TEXT_PLAIN, TEXT_HTML > } > > struct ClipData { > ClipType type; > string text; // optional > array<uint8> binary_data; // optional > } > > interface ClipboardHost { > SetContent(ClipData data); > GetContent() => (ClipData data); > } > > interface ClipboardInstance { > Init(ClipboardHost host); > } plan sounds good. in that case can you change the names of the methods in the interface to something like SetTextContent and GetTextContent in the meantime?
https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... components/arc/common/arc_bridge.mojom:147: OnSetClipboardContent(string text); On 2015/12/21 17:33:50, Luis Héctor Chávez wrote: > On 2015/12/21 11:18:01, cnwan wrote: > > On 2015/12/16 23:16:23, Luis Héctor Chávez wrote: > > > Can you split off this into its own interface? > > > > > > clipboard.mojom: > > > > > > module arc; > > > > > > interface ClipboardHost { > > > // Called when the Android clipboard content is changed. This is > > > // typically fired when a user initiates a "Copy" action. > > > // |text| is the text being set in the Android Clipboard. > > > OnSetClipboardContent(string text); > > > > > > // Called when the Android clipboard decides to update its content. > > > // This is triggered when a "Paste" action is fired or an app > > > // or a service tries to retrieve the Android clipboard content. > > > OnGetClipboardContent(); > > > }; > > > > > > > > > interface ClipboardInstance { > > > Init(ClipboardHost host_ptr); > > > > > > // Sends the Chrome clipboard content to Android. > > > // |text| is the clipboard content being sent. > > > SendClipboardContentToAndroid(string text); > > > }; > > > > > > See https://codereview.chromium.org/1523643002/ for reference and further > > > examples. > > > > Will do once crrev.com/1541653002 is relanded. > > Will this only ever support text? Is copy/pasting other types of content a > non-goal? Will that use the same methods with changed parameters or will > additional methods be used to support that? Btw, please use OnClipboardInstanceReady@109.
On 2015/12/23 00:07:01, Luis Héctor Chávez wrote: > https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... > File components/arc/common/arc_bridge.mojom (right): > > https://codereview.chromium.org/1495723004/diff/120001/components/arc/common/... > components/arc/common/arc_bridge.mojom:147: OnSetClipboardContent(string text); > On 2015/12/21 17:33:50, Luis Héctor Chávez wrote: > > On 2015/12/21 11:18:01, cnwan wrote: > > > On 2015/12/16 23:16:23, Luis Héctor Chávez wrote: > > > > Can you split off this into its own interface? > > > > > > > > clipboard.mojom: > > > > > > > > module arc; > > > > > > > > interface ClipboardHost { > > > > // Called when the Android clipboard content is changed. This is > > > > // typically fired when a user initiates a "Copy" action. > > > > // |text| is the text being set in the Android Clipboard. > > > > OnSetClipboardContent(string text); > > > > > > > > // Called when the Android clipboard decides to update its content. > > > > // This is triggered when a "Paste" action is fired or an app > > > > // or a service tries to retrieve the Android clipboard content. > > > > OnGetClipboardContent(); > > > > }; > > > > > > > > > > > > interface ClipboardInstance { > > > > Init(ClipboardHost host_ptr); > > > > > > > > // Sends the Chrome clipboard content to Android. > > > > // |text| is the clipboard content being sent. > > > > SendClipboardContentToAndroid(string text); > > > > }; > > > > > > > > See https://codereview.chromium.org/1523643002/ for reference and further > > > > examples. > > > > > > Will do once crrev.com/1541653002 is relanded. > > > > Will this only ever support text? Is copy/pasting other types of content a > > non-goal? Will that use the same methods with changed parameters or will > > additional methods be used to support that? > > Btw, please use OnClipboardInstanceReady@109. Rebased, refactored according to crrev.com/1541653002 and addressed review comments. PTAL. Thanks.
https://codereview.chromium.org/1495723004/diff/140001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1495723004/diff/140001/components/arc/common/... components/arc/common/arc_bridge.mojom:24: OnClipboardInstanceReady@109(ClipboardInstance instance_ptr); Can you add [MinVersion=1] before the method name? If by the time this is ready to be checked in, https://codereview.chromium.org/1451353002/ has already landed, make that [MinVersion=2].
On 2015/12/28 23:57:49, Luis Héctor Chávez wrote: > https://codereview.chromium.org/1495723004/diff/140001/components/arc/common/... > File components/arc/common/arc_bridge.mojom (right): > > https://codereview.chromium.org/1495723004/diff/140001/components/arc/common/... > components/arc/common/arc_bridge.mojom:24: > OnClipboardInstanceReady@109(ClipboardInstance instance_ptr); > Can you add [MinVersion=1] before the method name? > > If by the time this is ready to be checked in, > https://codereview.chromium.org/1451353002/ has already landed, make that > [MinVersion=2]. MinVersion added to both Chrome and Android CL's. PTAL. Thank you.
https://codereview.chromium.org/1495723004/diff/160001/components/arc/clipboa... File components/arc/clipboard/arc_clipboard_bridge.cc (right): https://codereview.chromium.org/1495723004/diff/160001/components/arc/clipboa... components/arc/clipboard/arc_clipboard_bridge.cc:54: clipboard_instance->Init(std::move(host)); #include <utility> https://codereview.chromium.org/1495723004/diff/160001/components/arc/clipboa... components/arc/clipboard/arc_clipboard_bridge.cc:58: DCHECK(CalledOnValidThread()); Just to double-check... is this the UI thread?
On 2015/12/30 19:51:52, dcheng wrote: > https://codereview.chromium.org/1495723004/diff/160001/components/arc/clipboa... > File components/arc/clipboard/arc_clipboard_bridge.cc (right): > > https://codereview.chromium.org/1495723004/diff/160001/components/arc/clipboa... > components/arc/clipboard/arc_clipboard_bridge.cc:54: > clipboard_instance->Init(std::move(host)); > #include <utility> > > https://codereview.chromium.org/1495723004/diff/160001/components/arc/clipboa... > components/arc/clipboard/arc_clipboard_bridge.cc:58: > DCHECK(CalledOnValidThread()); > Just to double-check... is this the UI thread? Made minor changes according to your comments. PTAL. Thank you.
LGTM https://codereview.chromium.org/1495723004/diff/200001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1495723004/diff/200001/components/arc/arc_ser... components/arc/arc_service_manager.h:18: class ArcClipboardBridge; nit: Lexicographic order. https://codereview.chromium.org/1495723004/diff/200001/components/arc/arc_ser... components/arc/arc_service_manager.h:41: scoped_ptr<ArcClipboardBridge> arc_clipboard_bridge_; nit: Lexicographic order.
On 2016/01/04 16:46:39, Luis Héctor Chávez wrote: > LGTM > > https://codereview.chromium.org/1495723004/diff/200001/components/arc/arc_ser... > File components/arc/arc_service_manager.h (right): > > https://codereview.chromium.org/1495723004/diff/200001/components/arc/arc_ser... > components/arc/arc_service_manager.h:18: class ArcClipboardBridge; > nit: Lexicographic order. > > https://codereview.chromium.org/1495723004/diff/200001/components/arc/arc_ser... > components/arc/arc_service_manager.h:41: scoped_ptr<ArcClipboardBridge> > arc_clipboard_bridge_; > nit: Lexicographic order.
The CQ bit was checked by cnwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495723004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495723004/200001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/1495723004/diff/200001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1495723004/diff/200001/components/arc/arc_ser... components/arc/arc_service_manager.h:18: class ArcClipboardBridge; On 2016/01/04 16:46:39, Luis Héctor Chávez wrote: > nit: Lexicographic order. Done. https://codereview.chromium.org/1495723004/diff/200001/components/arc/arc_ser... components/arc/arc_service_manager.h:41: scoped_ptr<ArcClipboardBridge> arc_clipboard_bridge_; On 2016/01/04 16:46:39, Luis Héctor Chávez wrote: > nit: Lexicographic order. Done.
The CQ bit was checked by cnwan@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495723004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495723004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/01/05 07:21:13, commit-bot: I haz the power wrote: > Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are > accepted. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Mind rebasing and make changes similar to https://codereview.chromium.org/1548833002/ and https://codereview.chromium.org/1551763002/ ?
On 2016/01/05 19:02:14, Luis Héctor Chávez wrote: > On 2016/01/05 07:21:13, commit-bot: I haz the power wrote: > > Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are > > accepted. > > Even if an L-G-T-M may have been provided, it was from a non-committer, > > _not_ a full super star committer. > > See http://www.chromium.org/getting-involved/become-a-committer > > Note that this has nothing to do with OWNERS files. > > Mind rebasing and make changes similar to > https://codereview.chromium.org/1548833002/ and > https://codereview.chromium.org/1551763002/ ? Done. OnClipboardVersionReady/CloseClipboardChannel/... are introduced. PTAL. Thank you.
Description was changed from ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 ========== to ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 TEST=Build both sides. Ctrl-C/Ctrl-V from either side reflect to the other. ==========
Description was changed from ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 TEST=Build both sides. Ctrl-C/Ctrl-V from either side reflect to the other. ========== to ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 TEST=Build both sides. Ctrl-C/Ctrl-V from either side reflect to the other. ==========
cnwan@google.com changed reviewers: + wuchengli@chromium.org
The CQ bit was checked by cnwan@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495723004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495723004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by cnwan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/1495723004/#ps280001 (title: "Fixed typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495723004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495723004/280001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
cnwan@google.com changed reviewers: + owenlin@chromium.org - wuchengli@chromium.org
lgtm
The CQ bit was checked by cnwan@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495723004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495723004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
cnwan@google.com changed reviewers: + vadimsh@chromium.org - avi@chromium.org, cnwan@google.com
On 2016/01/07 06:43:00, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Added owners for LGTM +vadimsh: mentioning //ui:base in DEPS +dcheng: for adding new mojom files
https://codereview.chromium.org/1495723004/diff/300001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1495723004/diff/300001/components/arc/common/... components/arc/common/arc_bridge.mojom:24: [MinVersion=1] OnClipboardInstanceReady@109(ClipboardInstance instance_ptr); FYI another change went in. Bump to MinVersion=2.
adding lgtm for components/arc (I think Luis' unfortunately doesn't count yet until he's a full committer) https://codereview.chromium.org/1495723004/diff/300001/components/arc/clipboa... File components/arc/clipboard/arc_clipboard_bridge.h (right): https://codereview.chromium.org/1495723004/diff/300001/components/arc/clipboa... components/arc/clipboard/arc_clipboard_bridge.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 for new files now
dcheng@chromium.org changed reviewers: + cnwan@google.com
lgtm with nit addressed https://codereview.chromium.org/1495723004/diff/300001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1495723004/diff/300001/components/arc/arc_ser... components/arc/arc_service_manager.cc:33: arc_clipboard_bridge_.reset( Initialize this in the initializer list: https://codereview.chromium.org/1553613002/ already moves lines 31/32 in into the initializer list.
cnwan@google.com changed reviewers: + thakis@chromium.org
On 2016/01/07 22:12:33, dcheng wrote: > lgtm with nit addressed > > https://codereview.chromium.org/1495723004/diff/300001/components/arc/arc_ser... > File components/arc/arc_service_manager.cc (right): > > https://codereview.chromium.org/1495723004/diff/300001/components/arc/arc_ser... > components/arc/arc_service_manager.cc:33: arc_clipboard_bridge_.reset( > Initialize this in the initializer list: > https://codereview.chromium.org/1553613002/ already moves lines 31/32 in into > the initializer list. Added another reviewer (thakis@) for +ui/base DEPS
Rebased and addressed reviewers' comments https://codereview.chromium.org/1495723004/diff/300001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1495723004/diff/300001/components/arc/arc_ser... components/arc/arc_service_manager.cc:33: arc_clipboard_bridge_.reset( On 2016/01/07 22:12:33, dcheng wrote: > Initialize this in the initializer list: > https://codereview.chromium.org/1553613002/ already moves lines 31/32 in into > the initializer list. Done. https://codereview.chromium.org/1495723004/diff/300001/components/arc/clipboa... File components/arc/clipboard/arc_clipboard_bridge.h (right): https://codereview.chromium.org/1495723004/diff/300001/components/arc/clipboa... components/arc/clipboard/arc_clipboard_bridge.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/07 22:04:29, elijahtaylor1 wrote: > nit: 2016 for new files now Done. https://codereview.chromium.org/1495723004/diff/300001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1495723004/diff/300001/components/arc/common/... components/arc/common/arc_bridge.mojom:24: [MinVersion=1] OnClipboardInstanceReady@109(ClipboardInstance instance_ptr); On 2016/01/07 17:04:54, Luis Héctor Chávez wrote: > FYI another change went in. Bump to MinVersion=2. Done. Also sync the Android side mojom file at http://ag/833451
deps lgtm
The CQ bit was checked by cnwan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, elijahtaylor@chromium.org, owenlin@chromium.org, dcheng@chromium.org, cnwan@google.com Link to the patchset: https://codereview.chromium.org/1495723004/#ps320001 (title: "Rebased and address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495723004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495723004/320001
Message was sent while issue was closed.
Description was changed from ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 TEST=Build both sides. Ctrl-C/Ctrl-V from either side reflect to the other. ========== to ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 TEST=Build both sides. Ctrl-C/Ctrl-V from either side reflect to the other. ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 TEST=Build both sides. Ctrl-C/Ctrl-V from either side reflect to the other. ========== to ========== Minimum implementation of ARC Clipboard Bridge. All change sets include: - http://crrev.com/1495723004 (Chromium) - http://ag/833451 (Android device/google/cheets) - http://ag/833422 (Android platform/frameworks/base) BUG=565283 TEST=Build both sides. Ctrl-C/Ctrl-V from either side reflect to the other. Committed: https://crrev.com/f5601134a83382fdda85405fa41904e8ceb4f4ef Cr-Commit-Position: refs/heads/master@{#368547} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/f5601134a83382fdda85405fa41904e8ceb4f4ef Cr-Commit-Position: refs/heads/master@{#368547} |
