Change Clipboard to use virtual methods for testing purposes.

Change ui::Clipboard to use virtual methods for testing purposes.

Platform-specific implementations now have their own derived classes.
This allows some cleanup of platform-specific hackery in clipboard.h,
and paves the way for adding a fake clipboard implementation in the
future for testing.

BUG=319285
Committed: https://crrev.com/24f3d91568c470b69a62e6dab5998a295f2eb46b
Cr-Commit-Position: refs/heads/master@{#300063}

[dcheng, 2014-10-16 03:18:14]

dcheng@chromium.org changed reviewers:
+ sky@chromium.org

[dcheng, 2014-10-16 09:38:05]

Sorry for the size of the CL. Most of the diffs in the implementation files are
just moving blocks around to better match the order of declarations. Otherwise,
this is a pretty straightforward change. It allows me to add a FakeClipboard
implementation in a followup patch and hopefully run it through the same unit
tests as the normal Clipboard implementation, so the fake can be substituted
where appropriate in unit tests.

https://codereview.chromium.org/658963003/diff/150001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.h (left):

https://codereview.chromium.org/658963003/diff/150001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.h:116: UINT ToUINT() const { return data_.cfFormat;
}
I ended up removing this because it was awkward to give ClipboardWin access to
this.

[sky, 2014-10-16 16:16:03]

Can you break this up. A change the makes declaration and definition
match order and then the real change? You can TBR the first.

  -Scott

On Thu, Oct 16, 2014 at 2:38 AM,  <dcheng@chromium.org> wrote:
> Reviewers: sky,
>
> Message:
> Sorry for the size of the CL. Most of the diffs in the implementation files
> are
> just moving blocks around to better match the order of declarations.
> Otherwise,
> this is a pretty straightforward change. It allows me to add a FakeClipboard
> implementation in a followup patch and hopefully run it through the same
> unit
> tests as the normal Clipboard implementation, so the fake can be substituted
> where appropriate in unit tests.
>
>
>
https://codereview.chromium.org/658963003/diff/150001/ui/base/clipboard/clipb...
> File ui/base/clipboard/clipboard.h (left):
>
>
https://codereview.chromium.org/658963003/diff/150001/ui/base/clipboard/clipb...
> ui/base/clipboard/clipboard.h:116: UINT ToUINT() const { return
> data_.cfFormat; }
> I ended up removing this because it was awkward to give ClipboardWin
> access to this.
>
> Description:
> Change Clipboard to use virtual methods for testing purposes.
>
> Platform-specific implementations now have their own derived classes.
> This allows some cleanup of platform-specific hackery in clipboard.h,
> and paves the way for adding a fake clipboard implementation in the
> future for testing.
>
> BUG=319285
>
> Please review this at https://codereview.chromium.org/658963003/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+1335, -971 lines):
>   M ui/base/BUILD.gn
>   M ui/base/android/ui_base_jni_registrar.cc
>   M ui/base/clipboard/clipboard.h
>   M ui/base/clipboard/clipboard.cc
>   A ui/base/clipboard/clipboard_android.h
>   M ui/base/clipboard/clipboard_android.cc
>   D ui/base/clipboard/clipboard_android_initialization.h
>   A ui/base/clipboard/clipboard_aura.h
>   M ui/base/clipboard/clipboard_aura.cc
>   A ui/base/clipboard/clipboard_aurax11.h
>   M ui/base/clipboard/clipboard_aurax11.cc
>   A ui/base/clipboard/clipboard_mac.h
>   M ui/base/clipboard/clipboard_mac.mm
>   A ui/base/clipboard/clipboard_win.h
>   M ui/base/clipboard/clipboard_win.cc
>   M ui/base/ui_base.gyp
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2014-10-16 16:57:52]

PTAL. I've rebased this patch on top of
https://codereview.chromium.org/659103002, so it should be much easier to review
the diffs now.

[sky, 2014-10-16 19:40:12]

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_android.h (right):

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.h:19: ~ClipboardAndroid() override;
virtual and I don't believe we use override on destructors.

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.h:21: // Clipboard public overrides:
You could actually move all of this to private as in theory you shouldn't be
using ClipboardAndroid directly.

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.h:22: uint64 GetSequenceNumber(ClipboardType
type) override;
virtual on all of these.

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_aura.h (right):

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_aura.h:15: ~ClipboardAura() override;
Same comments here.

[dcheng, 2014-10-16 21:51:39]

Apparently I didn't actually send out my drafts, sorry. PTAL.

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_android.h (right):

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.h:19: ~ClipboardAndroid() override;
On 2014/10/16 19:40:11, sky wrote:
> virtual and I don't believe we use override on destructors.

The C++ style guide seems to be unambiguous on this point:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html

"Explicitly annotate overrides of virtual functions or virtual destructors with
an override or (less frequently) final specifier"

Avi brought this issue up to me as well when the clang plugin was updated in
https://codereview.chromium.org/597863002. I am inclined to follow the Google
C++ style guide on this point, in order to minimize the diffs between Chromium
and Google style.

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.h:21: // Clipboard public overrides:
On 2014/10/16 19:40:11, sky wrote:
> You could actually move all of this to private as in theory you shouldn't be
> using ClipboardAndroid directly.

Good point. Done. I've made everything private... because why not.

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.h:22: uint64 GetSequenceNumber(ClipboardType
type) override;
On 2014/10/16 19:40:11, sky wrote:
> virtual on all of these.

Style guide: "For clarity, use exactly one of override, final, or virtual when
declaring an override."

The plugin accepts (but does not require) this style now, and I'd like to save
myself some work down the road.

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_aura.h (right):

https://codereview.chromium.org/658963003/diff/170001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_aura.h:15: ~ClipboardAura() override;
On 2014/10/16 19:40:11, sky wrote:
> Same comments here.

See previous comments.

[sky, 2014-10-16 22:37:12]

Something tells me I need to read the style guide again. LGTM

[dcheng, 2014-10-17 03:41:16]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-17 03:42:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/658963003/190001

[commit-bot: I haz the power, 2014-10-17 04:32:01]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2014-10-17 04:32:56]

Patchset 11 (id:??) landed as
https://crrev.com/24f3d91568c470b69a62e6dab5998a295f2eb46b
Cr-Commit-Position: refs/heads/master@{#300063}