Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2240323003:
mus: Implement OSExchangeDataProviderMus. (Closed)
|
Created:
4 years, 4 months ago by Elliot Glaysher Modified:
4 years, 4 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmus: Implement OSExchangeDataProviderMus.
Fill in the skeleton so the current views code will create the map
representation that we use to move data across the IPC boundary to the
clipboard.
BUG=614037
Committed: https://crrev.com/e428cfc81bc9288d67b5918fd4eab925c8e8665d
Cr-Commit-Position: refs/heads/master@{#412302}
Patch Set 1 #Patch Set 2 : Rebase to tot #Patch Set 3 : Compile #Patch Set 4 : Hopefully fix win compile. #Patch Set 5 : Maybe a better way of fixing windows? #
Total comments: 7
Patch Set 6 : sky comments + fix linux non-chromeos maybe? #
Total comments: 1
Patch Set 7 : Add static assert. #Patch Set 8 : No, sky wanted a dcheck. I misunderstood. #
Messages
Total messages: 42 (33 generated)
The CQ bit was checked by erg@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...)
The CQ bit was checked by erg@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: 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-...)
The CQ bit was checked by erg@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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by erg@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 erg@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...
erg@chromium.org changed reviewers: + sky@chromium.org
This is an 80% implementation of OSExchangeDataProviderMus, and is sufficient enough for most views controls to write their data to this, and to transfer it to mus during the start of a drag. (Things needed for a complete implementation is a story about what happens regarding incoming data on a drop, but I'm leaving those details for another patch.) I'm not happy about copying the normal unit test suite here, but I think it's better than porting it to a parameterized test suite and #including it, which always struck me as a bit of a hack.
https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... File ui/views/mus/os_exchange_data_provider_mus.cc (right): https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... ui/views/mus/os_exchange_data_provider_mus.cc:29: return base::string16( This seems rather risky. Wouldn't it be better to map string16 to utf8? https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... File ui/views/mus/os_exchange_data_provider_mus.h (right): https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... ui/views/mus/os_exchange_data_provider_mus.h:17: // Translates chrome's requests for varies data types to a platform specific varies->various https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... ui/views/mus/os_exchange_data_provider_mus.h:23: class VIEWS_MUS_EXPORT OSExchangeDataProviderMus This class isn't dependent on views. Wouldn't it make more sense to put this in services/ui/public/cpp? https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... File ui/views/mus/os_exchange_data_provider_mus_unittest.cc (right): https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... ui/views/mus/os_exchange_data_provider_mus_unittest.cc:29: // the OSExchangeData test suite to a parameterized test suite, but I've We use parameterized tests in a number of places. What problem did you encounter?
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_...)
The CQ bit was checked by erg@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/2240323003/diff/80001/ui/views/mus/os_exchang... File ui/views/mus/os_exchange_data_provider_mus.cc (right): https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... ui/views/mus/os_exchange_data_provider_mus.cc:29: return base::string16( On 2016/08/15 20:31:26, sky wrote: > This seems rather risky. Wouldn't it be better to map string16 to utf8? As mentioned in person, we do this in the linux port. https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... File ui/views/mus/os_exchange_data_provider_mus.h (right): https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... ui/views/mus/os_exchange_data_provider_mus.h:23: class VIEWS_MUS_EXPORT OSExchangeDataProviderMus On 2016/08/15 20:31:26, sky wrote: > This class isn't dependent on views. Wouldn't it make more sense to put this in > services/ui/public/cpp? As mentioned in person, this would make //services/ui/public/cpp depend on //ui/base. https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... File ui/views/mus/os_exchange_data_provider_mus_unittest.cc (right): https://codereview.chromium.org/2240323003/diff/80001/ui/views/mus/os_exchang... ui/views/mus/os_exchange_data_provider_mus_unittest.cc:29: // the OSExchangeData test suite to a parameterized test suite, but I've On 2016/08/15 20:31:26, sky wrote: > We use parameterized tests in a number of places. What problem did you > encounter? (Also discussed in person).
LGTM https://codereview.chromium.org/2240323003/diff/100001/ui/views/mus/os_exchan... File ui/views/mus/os_exchange_data_provider_mus.cc (right): https://codereview.chromium.org/2240323003/diff/100001/ui/views/mus/os_exchan... ui/views/mus/os_exchange_data_provider_mus.cc:29: return base::string16( How about a DCHECK that the size if even?
The CQ bit was checked by erg@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...
erg@chromium.org changed reviewers: + rdsmith@chromium.org
+rdsmith for DEPS. This directory already includes code from another net using directory.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. (I wince a bit, just because putting /net in DEPS means that future usages of /net from mus don't have to go through /net OWNERS review, and there are dragons in that interface. But there's nothing problematic in your use of the net file APIs, and that's where they're stored, so the DEPS change is needed. It would be good to rope in someone from /net on any future API uses.)
The CQ bit was checked by erg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2240323003/#ps140001 (title: "No, sky wanted a dcheck. I misunderstood.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== mus: Implement OSExchangeDataProviderMus. Fill in the skeleton so the current views code will create the map representation that we use to move data across the IPC boundary to the clipboard. BUG=614037 ========== to ========== mus: Implement OSExchangeDataProviderMus. Fill in the skeleton so the current views code will create the map representation that we use to move data across the IPC boundary to the clipboard. BUG=614037 Committed: https://crrev.com/e428cfc81bc9288d67b5918fd4eab925c8e8665d Cr-Commit-Position: refs/heads/master@{#412302} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e428cfc81bc9288d67b5918fd4eab925c8e8665d Cr-Commit-Position: refs/heads/master@{#412302} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
