Move autofill from ui/ to components/

Move autofill from ui/ to components/

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

[wychen, 2016-10-01 07:23:57]

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

[commit-bot: I haz the power, 2016-10-01 07:24:02]

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-01 07:27:32]

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

[commit-bot: I haz the power, 2016-10-01 07:27:32]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[wychen, 2016-10-01 07:52:35]

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

[wychen, 2016-10-01 07:52:42]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-01 07:52:43]

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-01 08:15:21]

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

[commit-bot: I haz the power, 2016-10-01 08:15:22]

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

[wychen, 2016-10-01 08:48:36]

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

[commit-bot: I haz the power, 2016-10-01 08:48:39]

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

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

[commit-bot: I haz the power, 2016-10-01 09:43:01]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-10-01 10:36:39]

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

[commit-bot: I haz the power, 2016-10-01 10:36:43]

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

[wychen, 2016-10-01 10:37:00]

Patchset #1 (id:20001) has been deleted

[wychen, 2016-10-01 10:37:12]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-10-01 10:49:13]

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

[commit-bot: I haz the power, 2016-10-01 10:49:14]

Dry run: Exceeded global retry quota

[wychen, 2016-10-01 11:41:48]

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

[commit-bot: I haz the power, 2016-10-01 11:41:55]

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-01 12:33:45]

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

[commit-bot: I haz the power, 2016-10-01 12:33:46]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-10-03 06:11:12]

Patchset #2 (id:80001) has been deleted

[wychen, 2016-10-03 06:17:35]

Description was changed from

==========
Move autofill from ui to components

BUG=
==========

to

==========
Move autofill from ui to components

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

[wychen, 2016-10-03 06:17:35]

wychen@chromium.org changed reviewers:
+ boliu@chromium.org, sky@chromium.org

[wychen, 2016-10-03 06:19:54]

PTAL

[boliu, 2016-10-03 16:59:48]

android_webview lgtm

you probably want tedchoc to review the other android parts. I don't own them

[sky, 2016-10-03 17:45:43]

Please clarify what files you need me to review.

[wychen, 2016-10-03 21:51:37]

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

[wychen, 2016-10-03 22:58:40]

wychen@chromium.org changed reviewers:
- sky@chromium.org

[wychen, 2016-10-03 22:58:40]

Ted, can you take a look?

sky@, Sorry, I was thinking about getting some comments about the big picture.
I've moved you to CC.

[Ted C, 2016-10-04 20:39:39]

overall looks awesome...one question about the dependency on ui/ res

https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
File components/autofill/android/BUILD.gn (right):

https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
components/autofill/android/BUILD.gn:11: "//ui/android:ui_java_resources",
Is this needed for dimens.xml?

Looks like some of the values are only needed by autofill, so ideally they live
here.  Ideally, we have the component entirely isolated.

[wychen, 2016-10-04 21:00:39]

Patchset #4 (id:140001) has been deleted

[wychen, 2016-10-04 21:01:03]

https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
File components/autofill/android/BUILD.gn (right):

https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
components/autofill/android/BUILD.gn:11: "//ui/android:ui_java_resources",
On 2016/10/04 20:39:39, Ted C wrote:
> Is this needed for dimens.xml?
> 
> Looks like some of the values are only needed by autofill, so ideally they
live
> here.  Ideally, we have the component entirely isolated.

I've moved them to components as well.

One downside: keyboard_accessory_height is shared by chrome layer and ui (now
components), so this is duplicated in 2 dimens.xml files.

[Ted C, 2016-10-04 21:15:55]

On 2016/10/04 21:01:03, wychen wrote:
>
https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
> File components/autofill/android/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
> components/autofill/android/BUILD.gn:11: "//ui/android:ui_java_resources",
> On 2016/10/04 20:39:39, Ted C wrote:
> > Is this needed for dimens.xml?
> > 
> > Looks like some of the values are only needed by autofill, so ideally they
> live
> > here.  Ideally, we have the component entirely isolated.
> 
> I've moved them to components as well.
> 
> One downside: keyboard_accessory_height is shared by chrome layer and ui (now
> components), so this is duplicated in 2 dimens.xml files.

Can chrome/ not depend on components/autofill/android/java/res like you had
it done in ui?  Since the values are shared, that seems like a valid usecase
this time around.

[wychen, 2016-10-04 23:26:36]

On 2016/10/04 21:15:55, Ted C wrote:
> On 2016/10/04 21:01:03, wychen wrote:
> >
>
https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
> > File components/autofill/android/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
> > components/autofill/android/BUILD.gn:11: "//ui/android:ui_java_resources",
> > On 2016/10/04 20:39:39, Ted C wrote:
> > > Is this needed for dimens.xml?
> > > 
> > > Looks like some of the values are only needed by autofill, so ideally they
> > live
> > > here.  Ideally, we have the component entirely isolated.
> > 
> > I've moved them to components as well.
> > 
> > One downside: keyboard_accessory_height is shared by chrome layer and ui
(now
> > components), so this is duplicated in 2 dimens.xml files.
> 
> Can chrome/ not depend on components/autofill/android/java/res like you had
> it done in ui?  Since the values are shared, that seems like a valid usecase
> this time around.

I'm a bit confused. chrome/ didn't depend on components/autofill/ res, right?
chrome/ only depends on components/autofill/ src, but not res.

I could put keyboard_accessory_height back to ui/ and move the others to
components/, but it seems a bit too fine-grained.
I think reverting back to patch set 3 is good enough. What do you think?

[Ted C, 2016-10-05 00:15:14]

On 2016/10/04 23:26:36, wychen wrote:
> On 2016/10/04 21:15:55, Ted C wrote:
> > On 2016/10/04 21:01:03, wychen wrote:
> > >
> >
>
https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
> > > File components/autofill/android/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
> > > components/autofill/android/BUILD.gn:11: "//ui/android:ui_java_resources",
> > > On 2016/10/04 20:39:39, Ted C wrote:
> > > > Is this needed for dimens.xml?
> > > > 
> > > > Looks like some of the values are only needed by autofill, so ideally
they
> > > live
> > > > here.  Ideally, we have the component entirely isolated.
> > > 
> > > I've moved them to components as well.
> > > 
> > > One downside: keyboard_accessory_height is shared by chrome layer and ui
> (now
> > > components), so this is duplicated in 2 dimens.xml files.
> > 
> > Can chrome/ not depend on components/autofill/android/java/res like you had
> > it done in ui?  Since the values are shared, that seems like a valid usecase
> > this time around.
> 
> I'm a bit confused. chrome/ didn't depend on components/autofill/ res, right?
> chrome/ only depends on components/autofill/ src, but not res.
> 
> I could put keyboard_accessory_height back to ui/ and move the others to
> components/, but it seems a bit too fine-grained.
> I think reverting back to patch set 3 is good enough. What do you think?

My problem with it existing in ui/ is that there are no usages of the
autofill dimens within the ui directory, so it shouldn't exist there.

If chrome/ previously used ui/res and depended on it, and we moved stuff
to components/autofill/res, then it makes sense for chrome/ to depend on
components/autofill/res as well.

Autofill should be entirely contained within the component.  Chrome can
and should depend on on all the bits it needs, which includes src/ and
res/ (assuming it works).  "Hopefully" that is a bit clearer.

[wychen, 2016-10-05 00:37:00]

Patchset #5 (id:180001) has been deleted

[wychen, 2016-10-05 01:00:08]

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

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

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

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

On 2016/10/05 00:15:14, Ted C wrote:
> On 2016/10/04 23:26:36, wychen wrote:
> > On 2016/10/04 21:15:55, Ted C wrote:
> > > On 2016/10/04 21:01:03, wychen wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
> > > > File components/autofill/android/BUILD.gn (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2382913004/diff/120001/components/autofill/an...
> > > > components/autofill/android/BUILD.gn:11:
"//ui/android:ui_java_resources",
> > > > On 2016/10/04 20:39:39, Ted C wrote:
> > > > > Is this needed for dimens.xml?
> > > > > 
> > > > > Looks like some of the values are only needed by autofill, so ideally
> they
> > > > live
> > > > > here.  Ideally, we have the component entirely isolated.
> > > > 
> > > > I've moved them to components as well.
> > > > 
> > > > One downside: keyboard_accessory_height is shared by chrome layer and ui
> > (now
> > > > components), so this is duplicated in 2 dimens.xml files.
> > > 
> > > Can chrome/ not depend on components/autofill/android/java/res like you
had
> > > it done in ui?  Since the values are shared, that seems like a valid
usecase
> > > this time around.
> > 
> > I'm a bit confused. chrome/ didn't depend on components/autofill/ res,
right?
> > chrome/ only depends on components/autofill/ src, but not res.
> > 
> > I could put keyboard_accessory_height back to ui/ and move the others to
> > components/, but it seems a bit too fine-grained.
> > I think reverting back to patch set 3 is good enough. What do you think?
> 
> My problem with it existing in ui/ is that there are no usages of the
> autofill dimens within the ui directory, so it shouldn't exist there.
> 
> If chrome/ previously used ui/res and depended on it, and we moved stuff
> to components/autofill/res, then it makes sense for chrome/ to depend on
> components/autofill/res as well.
> 
> Autofill should be entirely contained within the component.  Chrome can
> and should depend on on all the bits it needs, which includes src/ and
> res/ (assuming it works).  "Hopefully" that is a bit clearer.

Done.

I misread it as instructing me to stop letting chrome/ depend on
components/autofill/ res.
Sorry to make you explain it again.

[commit-bot: I haz the power, 2016-10-05 01:52:59]

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

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

Dry run: This issue passed the CQ dry run.

[wychen, 2016-10-05 07:32:36]

Description was changed from

==========
Move autofill from ui to components

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

to

==========
Move autofill from ui/ to components/

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

[Ted C, 2016-10-05 16:12:36]

lgtm

[wychen, 2016-10-05 20:23:41]

The CQ bit was checked by wychen@chromium.org

[wychen, 2016-10-05 20:23:42]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2382913004/#ps200001
(title: "move dimens cleanly")

[commit-bot: I haz the power, 2016-10-05 20:24:05]

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 20:34:16]

Description was changed from

==========
Move autofill from ui/ to components/

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

to

==========
Move autofill from ui/ to components/

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

[commit-bot: I haz the power, 2016-10-05 20:34:18]

Committed patchset #5 (id:200001)

[commit-bot: I haz the power, 2016-10-05 20:37:54]

Description was changed from

==========
Move autofill from ui/ to components/

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

to

==========
Move autofill from ui/ to components/

BUG=651988

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

[commit-bot: I haz the power, 2016-10-05 20:37:56]

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