Move Android view based ColorPicker from ui/ to components/

Move Android view based ColorPicker from ui/ to components/

BUG=651988
Committed: https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c
Cr-Commit-Position: refs/heads/master@{#431088}

[wychen, 2016-10-05 07:20:43]

wychen@chromium.org changed reviewers:
+ tedchoc@chromium.org

[wychen, 2016-10-05 07:20:43]

PTAL

[wychen, 2016-10-05 07:20:56]

The CQ bit was checked by wychen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 07:21:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 07:30:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 07:30:10]

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...)

[Ted C, 2016-10-06 18:16:13]

tedchoc@chromium.org changed reviewers:
+ boliu@chromium.org

[Ted C, 2016-10-06 18:16:14]

+boliu for the web_contents_delegate_android discussion

In general this seems reasonable to me, but we should definitely use a
subpackage in content (either content/colorpicker or content/input/color).

But looking at this, I am saddened by the fact that
web_contents_delegate_android has anything related to color picker
(ColorChooserAndroid specifically).

I also think it is weird that the directory has ValidationMessageBubble, which
definitely doesn't seem consistent with the web_contents_delegate_android
component.

@boliu, what is the history of web_contents_delegate_android?  Why doesn't it
live in content/?

For the two ui-y things in web_contents_delegate_android, should they live in
content?

Looks like the color picker might live in multiple places on desktop:
content/renderer/renderer_webcolorchooser_impl.h

Then there are ones in chrome:
chrome/browser/ui/views/color_chooser_aura.h
chrome/browser/ui/cocoa/color_chooser_mac.mm

I don't know enough about the color picker to know if it can live in content/ or
if it should live in a component to be more consistent with desktop.  Maybe we
want to ping either sky@ or jam@ if none of these options are obvious.

[boliu, 2016-10-06 18:37:31]

On 2016/10/06 18:16:14, Ted C wrote:
> @boliu, what is the history of web_contents_delegate_android?  Why doesn't it
> live in content/?

Originally, web_contents_delegate_android used to live in chrome. Then webview
wanted to share that code, so it was moved to a component.
This was the first CL: https://chromiumcodereview.appspot.com/11365199

> For the two ui-y things in web_contents_delegate_android, should they live in
> content?

I'm fine where they are right now. chromecast actually doesn't depend on
web_contents_delegate_android. Which means anything that's moved from
web_contents_delegate_android back to content means it's adding more deadcode to
chromecast.

[Ted C, 2016-10-06 20:37:14]

On 2016/10/06 18:37:31, boliu wrote:
> On 2016/10/06 18:16:14, Ted C wrote:
> > @boliu, what is the history of web_contents_delegate_android?  Why doesn't
it
> > live in content/?
> 
> Originally, web_contents_delegate_android used to live in chrome. Then webview
> wanted to share that code, so it was moved to a component.
> This was the first CL: https://chromiumcodereview.appspot.com/11365199
> 
> > For the two ui-y things in web_contents_delegate_android, should they live
in
> > content?
> 
> I'm fine where they are right now. chromecast actually doesn't depend on
> web_contents_delegate_android. Which means anything that's moved from
> web_contents_delegate_android back to content means it's adding more deadcode
to
> chromecast.

Maybe the name of web_contents_delegate_android is the thing that is so off
putting to me.  The fact that any android build doesn't use it already makes
it really weird to me.  Maybe we need to come up with some new buzz word
that is the amalgamation of android_webview and chrome.  I also wonder if we
could have split out the logic bits from the delegate and made a generic
delegate that provided the necessary java hooks and didn't need to be split
off as it is.  Regardless, don't need to bring up every old discussion, so we
can focus on where the color picker should live because ui/ isn't the right
place.

I think if we want to keep web_contents_delegate_android out of content, then it
makes sense to not add the UI to content either.  So, I think moving the color
picker into web_contents_delegate_android makes the most sense, but I do think
that makes it hugely nebulous what the component actually means in it's
entirety.

Does chromecast depend on any java based UI widgets?  If not, then I think they
will constantly be picking up tons of dead code.

the ANDROID_JAVA_UI ifdef is probably what they should be using and we should
be putting stuff like this into that.

[boliu, 2016-10-06 20:41:16]

On 2016/10/06 20:37:14, Ted C wrote:
> Does chromecast depend on any java based UI widgets?  If not, then I think
they
> will constantly be picking up tons of dead code.

I haz no idea. But I assume yes, but it's just they probably don't care about
showing the color picker :)

[Ted C, 2016-10-06 22:45:27]

On 2016/10/06 20:41:16, boliu wrote:
> On 2016/10/06 20:37:14, Ted C wrote:
> > Does chromecast depend on any java based UI widgets?  If not, then I think
> they
> > will constantly be picking up tons of dead code.
> 
> I haz no idea. But I assume yes, but it's just they probably don't care about
> showing the color picker :)

ok, for now let's move the color picker to the component then since it is the
only usage of it.

[wychen, 2016-11-05 23:34:01]

Description was changed from

==========
Move Android view based ColorPicker from ui/ to content/

BUG=651988
==========

to

==========
Move Android view based ColorPicker from ui/ to components/

BUG=651988
==========

[wychen, 2016-11-05 23:37:23]

The CQ bit was checked by wychen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-05 23:37:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-06 00:24:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-06 00:24:31]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-11-06 00:41:20]

On 2016/10/06 22:45:27, Ted C wrote:
> On 2016/10/06 20:41:16, boliu wrote:
> > On 2016/10/06 20:37:14, Ted C wrote:
> > > Does chromecast depend on any java based UI widgets?  If not, then I think
> > they
> > > will constantly be picking up tons of dead code.
> > 
> > I haz no idea. But I assume yes, but it's just they probably don't care
about
> > showing the color picker :)
> 
> ok, for now let's move the color picker to the component then since it is the
> only usage of it.

It's moved to components.
Please take another look.

[Ted C, 2016-11-08 17:02:38]

lgtm! thanks

https://codereview.chromium.org/2398543003/diff/40001/components/web_contents...
File components/web_contents_delegate_android/android/java/res/values/dimens.xml
(right):

https://codereview.chromium.org/2398543003/diff/40001/components/web_contents...
components/web_contents_delegate_android/android/java/res/values/dimens.xml:2:
<!-- Copyright (c) 2016 The Chromium Authors. All rights reserved.
no (c)

[wychen, 2016-11-08 21:20:33]

wychen@chromium.org changed reviewers:
+ blundell@chromium.org

[wychen, 2016-11-08 21:20:33]

blundell@, could you take a look at the strings in components? Thanks!

https://codereview.chromium.org/2398543003/diff/40001/components/web_contents...
File components/web_contents_delegate_android/android/java/res/values/dimens.xml
(right):

https://codereview.chromium.org/2398543003/diff/40001/components/web_contents...
components/web_contents_delegate_android/android/java/res/values/dimens.xml:2:
<!-- Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/11/08 17:02:38, Ted C wrote:
> no (c)

Done. '(c)' is all over the place. Do we also want to get rid of them to keep
the style consistent?

[Ted C, 2016-11-08 21:35:16]

https://codereview.chromium.org/2398543003/diff/40001/components/web_contents...
File components/web_contents_delegate_android/android/java/res/values/dimens.xml
(right):

https://codereview.chromium.org/2398543003/diff/40001/components/web_contents...
components/web_contents_delegate_android/android/java/res/values/dimens.xml:2:
<!-- Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/11/08 21:20:33, wychen wrote:
> On 2016/11/08 17:02:38, Ted C wrote:
> > no (c)
> 
> Done. '(c)' is all over the place. Do we also want to get rid of them to keep
> the style consistent?

Nah, we should just not add new ones.

[blundell, 2016-11-09 10:29:56]

//components lgtm

https://codereview.chromium.org/2398543003/diff/60001/components/web_contents...
File components/web_contents_delegate_android_strings.grdp (right):

https://codereview.chromium.org/2398543003/diff/60001/components/web_contents...
components/web_contents_delegate_android_strings.grdp:1: <?xml version="1.0"
encoding="utf-8"?>
Add tcomponents/web_contents_delegate_android/OWNERS as per-file OWNERS for this
file in //components/OWNERS.

[wychen, 2016-11-09 22:38:14]

https://codereview.chromium.org/2398543003/diff/60001/components/web_contents...
File components/web_contents_delegate_android_strings.grdp (right):

https://codereview.chromium.org/2398543003/diff/60001/components/web_contents...
components/web_contents_delegate_android_strings.grdp:1: <?xml version="1.0"
encoding="utf-8"?>
On 2016/11/09 10:29:56, blundell wrote:
> Add tcomponents/web_contents_delegate_android/OWNERS as per-file OWNERS for
this
> file in //components/OWNERS. 

Done.

[wychen, 2016-11-09 22:38:19]

The CQ bit was checked by wychen@chromium.org

[wychen, 2016-11-09 22:38:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2398543003/#ps80001
(title: "add owners")

[commit-bot: I haz the power, 2016-11-09 22:38:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 00:05:00]

Description was changed from

==========
Move Android view based ColorPicker from ui/ to components/

BUG=651988
==========

to

==========
Move Android view based ColorPicker from ui/ to components/

BUG=651988
==========

[commit-bot: I haz the power, 2016-11-10 00:05:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-10 00:20:13]

Description was changed from

==========
Move Android view based ColorPicker from ui/ to components/

BUG=651988
==========

to

==========
Move Android view based ColorPicker from ui/ to components/

BUG=651988

Committed: https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c
Cr-Commit-Position: refs/heads/master@{#431088}
==========

[commit-bot: I haz the power, 2016-11-10 00:20:14]

Patchset 5 (id:??) landed as
https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c
Cr-Commit-Position: refs/heads/master@{#431088}