Move Android view based DateTimePicker from ui/ to content/

Move Android view based DateTimePicker from ui/ to content/

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

[wychen, 2016-10-04 23:19:59]

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

[commit-bot: I haz the power, 2016-10-04 23:20:30]

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-04 23:33:39]

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

[commit-bot: I haz the power, 2016-10-04 23:33:40]

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

[wychen, 2016-10-05 00:23:12]

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

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

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 00:34:01]

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

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

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

[wychen, 2016-10-05 01:28:48]

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

[wychen, 2016-10-05 01:28:49]

PTAL

[wychen, 2016-10-05 04:25:55]

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

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

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 05:17:32]

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

[commit-bot: I haz the power, 2016-10-05 05:17:33]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-10-05 07:18:52]

Description was changed from

==========
Move Android view based HTML dropdowns to content

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

to

==========
Move Android view based HTML dropdowns from ui/ to content/

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

[wychen, 2016-10-05 21:01:27]

Description was changed from

==========
Move Android view based HTML dropdowns from ui/ to content/

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

to

==========
Move Android view based datetime picker from ui/ to content/

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

[wychen, 2016-10-05 22:56:26]

Description was changed from

==========
Move Android view based datetime picker from ui/ to content/

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

to

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

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

[Ted C, 2016-10-06 00:13:29]

\o/  Awesome...couple cleanups that I think would be nice to do along the way,
but then we are good

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java
(right):

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java:5:
package org.chromium.content.browser.picker;
I think maybe content.browser.input.datetime is a clearer package name for this
stuff.  Thoughts?

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java:19:
class ChromeDatePickerDialog extends DatePickerDialog {
Let's rename this to DatePickerDialogCompat.  Chrome shouldn't have been used in
the ui/ layer.

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/picker/InputDialogContainer.java
(right):

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/picker/InputDialogContainer.java:63:
public void showPickerDialog(final int dialogType, double dialogValue,
I suspect many of these methods are visible for testing.  Now that we are/can be
in the same package between code and tests, can you do a sweep and mark things
as protected/package protected/private as needed?

@VisibleForTesting if we have to keep things public and where it shouldn't be
accessed otherwise, but I hope we can get rid of that.

[Ted C, 2016-10-06 00:14:44]

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

[Ted C, 2016-10-06 00:14:45]

+changwan for his thoughts on the datetime subpackage in input suggestion I had

[Changwan Ryu, 2016-10-06 09:11:18]

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java
(right):

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java:5:
package org.chromium.content.browser.picker;
On 2016/10/06 00:13:29, Ted C wrote:
> I think maybe content.browser.input.datetime is a clearer package name for
this
> stuff.  Thoughts?

Hmm... If this falls under content, then I prefer content.browser.input.picker
because there are other picker types and it's best they belong to the same
package.

But I'm not entirely sure if content/ is the best location to have these dialog
implementations. Sorry if I misunderstood something, but my concern is as
follows: SelectFileDialog is in ui/, (and it's not necessarily a building
block...) If we were to have the same theme implemented for all the Chrome's
dialogs in the future, it might be a bit awkward to base these off of
ui.base.DialogBase. (Yes, this scenario is weird on many levels...) Or is it the
way it should be? On the other hand, selection handles are in the content, and
selection handles still have 'UI' element such as color. I think this is a hard
problem anyways...

[Ted C, 2016-10-06 14:11:40]

On 2016/10/06 09:11:18, Changwan Ryu wrote:
>
https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java
> (right):
> 
>
https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java:5:
> package org.chromium.content.browser.picker;
> On 2016/10/06 00:13:29, Ted C wrote:
> > I think maybe content.browser.input.datetime is a clearer package name for
> this
> > stuff.  Thoughts?
> 
> Hmm... If this falls under content, then I prefer content.browser.input.picker
> because there are other picker types and it's best they belong to the same
> package.
> 
> But I'm not entirely sure if content/ is the best location to have these
dialog
> implementations. Sorry if I misunderstood something, but my concern is as
> follows: SelectFileDialog is in ui/, (and it's not necessarily a building
> block...) If we were to have the same theme implemented for all the Chrome's
> dialogs in the future, it might be a bit awkward to base these off of
> ui.base.DialogBase. (Yes, this scenario is weird on many levels...) Or is it
the
> way it should be? On the other hand, selection handles are in the content, and
> selection handles still have 'UI' element such as color. I think this is a
hard
> problem anyways...

Your description of SelectFileDialog and DialogBase is exactly the intended
point of this move.

The goal is to have ui/ contain just the basic building blocks of UI components.

Features built on top of those building blocks should reside in their respective
modules (content/, chrome/, etc...).  In the event the UI is shared across
modules, then it should live in components/ instead of ui/.

I know the color picker is used by webview and chrome, and I'm not sure if
that will work in content/.  We will have to see.  Ideally, all the html input
pickers will go together.  The color picker is also a fair amount of code, which
is why I was thinking we could have them in more isolated directories, but
i'll defer that to you.

My thinking.
input
input/datetime
input/color
input/file <maybe based on your above comment>

But it is true that each one of those are related and are ui components for
picking things (partially, this is my dislike of the package name picker as
I don't think it is very descriptive, but I shan't push that agenda any further
:-P)

[Changwan Ryu, 2016-10-07 17:02:34]

On 2016/10/06 14:11:40, Ted C wrote:
> On 2016/10/06 09:11:18, Changwan Ryu wrote:
> >
>
https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
> >
>
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java:5:
> > package org.chromium.content.browser.picker;
> > On 2016/10/06 00:13:29, Ted C wrote:
> > > I think maybe content.browser.input.datetime is a clearer package name for
> > this
> > > stuff.  Thoughts?
> > 
> > Hmm... If this falls under content, then I prefer
content.browser.input.picker
> > because there are other picker types and it's best they belong to the same
> > package.
> > 
> > But I'm not entirely sure if content/ is the best location to have these
> dialog
> > implementations. Sorry if I misunderstood something, but my concern is as
> > follows: SelectFileDialog is in ui/, (and it's not necessarily a building
> > block...) If we were to have the same theme implemented for all the Chrome's
> > dialogs in the future, it might be a bit awkward to base these off of
> > ui.base.DialogBase. (Yes, this scenario is weird on many levels...) Or is it
> the
> > way it should be? On the other hand, selection handles are in the content,
and
> > selection handles still have 'UI' element such as color. I think this is a
> hard
> > problem anyways...
> 
> Your description of SelectFileDialog and DialogBase is exactly the intended
> point of this move.
> 
> The goal is to have ui/ contain just the basic building blocks of UI
components.
> 
> Features built on top of those building blocks should reside in their
respective
> modules (content/, chrome/, etc...).  In the event the UI is shared across
> modules, then it should live in components/ instead of ui/.
> 
> I know the color picker is used by webview and chrome, and I'm not sure if
> that will work in content/.  We will have to see.  Ideally, all the html input
> pickers will go together.  The color picker is also a fair amount of code,
which
> is why I was thinking we could have them in more isolated directories, but
> i'll defer that to you.
> 
> My thinking.
> input
> input/datetime
> input/color
> input/file <maybe based on your above comment>
> 
> But it is true that each one of those are related and are ui components for
> picking things (partially, this is my dislike of the package name picker as
> I don't think it is very descriptive, but I shan't push that agenda any
further
> :-P)

Thanks for the explanation about the purpose of ui/ directory and the color
picker.
In that sense, separating datetime and color may be clearer. Also there's a file
picker which is an odd duck in this picture.
Now I think you're suggestion makes more sense. Thanks!

BTW, input/file may not make sense. datetime is used by chrome and webview, but
it seems that SelectFileDialog is used only by chrome/ directory and not by
android_webview/ nor by content/,
so it may make more sense to move it to chrome/ and not content/ when we are to
move it outside ui/.

[Ted C, 2016-10-07 17:08:09]

On 2016/10/07 17:02:34, Changwan Ryu wrote:
> On 2016/10/06 14:11:40, Ted C wrote:
> > On 2016/10/06 09:11:18, Changwan Ryu wrote:
> > >
> >
>
https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
> > > File
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java:5:
> > > package org.chromium.content.browser.picker;
> > > On 2016/10/06 00:13:29, Ted C wrote:
> > > > I think maybe content.browser.input.datetime is a clearer package name
for
> > > this
> > > > stuff.  Thoughts?
> > > 
> > > Hmm... If this falls under content, then I prefer
> content.browser.input.picker
> > > because there are other picker types and it's best they belong to the same
> > > package.
> > > 
> > > But I'm not entirely sure if content/ is the best location to have these
> > dialog
> > > implementations. Sorry if I misunderstood something, but my concern is as
> > > follows: SelectFileDialog is in ui/, (and it's not necessarily a building
> > > block...) If we were to have the same theme implemented for all the
Chrome's
> > > dialogs in the future, it might be a bit awkward to base these off of
> > > ui.base.DialogBase. (Yes, this scenario is weird on many levels...) Or is
it
> > the
> > > way it should be? On the other hand, selection handles are in the content,
> and
> > > selection handles still have 'UI' element such as color. I think this is a
> > hard
> > > problem anyways...
> > 
> > Your description of SelectFileDialog and DialogBase is exactly the intended
> > point of this move.
> > 
> > The goal is to have ui/ contain just the basic building blocks of UI
> components.
> > 
> > Features built on top of those building blocks should reside in their
> respective
> > modules (content/, chrome/, etc...).  In the event the UI is shared across
> > modules, then it should live in components/ instead of ui/.
> > 
> > I know the color picker is used by webview and chrome, and I'm not sure if
> > that will work in content/.  We will have to see.  Ideally, all the html
input
> > pickers will go together.  The color picker is also a fair amount of code,
> which
> > is why I was thinking we could have them in more isolated directories, but
> > i'll defer that to you.
> > 
> > My thinking.
> > input
> > input/datetime
> > input/color
> > input/file <maybe based on your above comment>
> > 
> > But it is true that each one of those are related and are ui components for
> > picking things (partially, this is my dislike of the package name picker as
> > I don't think it is very descriptive, but I shan't push that agenda any
> further
> > :-P)
> 
> Thanks for the explanation about the purpose of ui/ directory and the color
> picker.
> In that sense, separating datetime and color may be clearer. Also there's a
file
> picker which is an odd duck in this picture.
> Now I think you're suggestion makes more sense. Thanks!
> 
> BTW, input/file may not make sense. datetime is used by chrome and webview,
but
> it seems that SelectFileDialog is used only by chrome/ directory and not by
> android_webview/ nor by content/,
> so it may make more sense to move it to chrome/ and not content/ when we are
to
> move it outside ui/.

Good to know.  Yeah, if file is only used by chrome/ then that is where it
should
live.  It seems like color might end up in components since it is only
referenced
from one component and not content/ nor chrome/

So, it might only be input.datetime in content at all.

[wychen, 2016-11-06 00:57:28]

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java
(right):

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java:5:
package org.chromium.content.browser.picker;
Now that color picker is in components, the only picker in content is datetime
picker. It seems we can keep the package name as is, since there's nothing to
isolate from.

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/picker/ChromeDatePickerDialog.java:19:
class ChromeDatePickerDialog extends DatePickerDialog {
On 2016/10/06 00:13:29, Ted C wrote:
> Let's rename this to DatePickerDialogCompat.  Chrome shouldn't have been used
in
> the ui/ layer.

Done.

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/picker/InputDialogContainer.java
(right):

https://codereview.chromium.org/2398443002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/picker/InputDialogContainer.java:63:
public void showPickerDialog(final int dialogType, double dialogValue,
On 2016/10/06 00:13:29, Ted C wrote:
> I suspect many of these methods are visible for testing.  Now that we are/can
be
> in the same package between code and tests, can you do a sweep and mark things
> as protected/package protected/private as needed?
> 
> @VisibleForTesting if we have to keep things public and where it shouldn't be
> accessed otherwise, but I hope we can get rid of that.

Done.

[wychen, 2016-11-06 00:59:54]

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

[commit-bot: I haz the power, 2016-11-06 01:00:07]

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 01:24:03]

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

[commit-bot: I haz the power, 2016-11-06 01:24:05]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[wychen, 2016-11-07 06:09:35]

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

[commit-bot: I haz the power, 2016-11-07 06:09:47]

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

[wychen, 2016-11-07 06:09:54]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-11-07 07:22:35]

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

[commit-bot: I haz the power, 2016-11-07 07:22:35]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-11-08 01:06:43]

lgtm

[wychen, 2016-11-08 02:09:14]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2016-11-08 02:09: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-08 02:16:38]

Description was changed from

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

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

to

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

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

[commit-bot: I haz the power, 2016-11-08 02:16:39]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-11-08 03:27:11]

Description was changed from

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

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

to

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

BUG=651988

Committed: https://crrev.com/50a29dd4612b03331762028e049cfade95561966
Cr-Commit-Position: refs/heads/master@{#430479}
==========

[commit-bot: I haz the power, 2016-11-08 03:27:12]

Patchset 6 (id:??) landed as
https://crrev.com/50a29dd4612b03331762028e049cfade95561966
Cr-Commit-Position: refs/heads/master@{#430479}