New issue
Advanced search Search tips

Issue 651988 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: ----



Sign in to add a comment

Clean up ui/ directory on Android

Project Member Reported by tedc...@chromium.org, Sep 30 2016

Issue description

The ui/ directory should only contain the fundamental building blocks of UI components.  It should not contain the UI features that are composed of those building blocks.

For cases where the UI is shared between WebView and Chrome, we should make them a component.

For cases where it is only used by one or the other, it should be moved to the respective directory.

Some examples that are the wrong layering:

1.) Autofill
Current location:
  ui/android/java/src/org/chromium/ui/autofill
Desired location:
  components/autofill/android

2.) Android view based HTML dropdowns:
Current location:
  ui/android/java/src/org/chromium/ui/picker/
Desired location:
  potentially to content, potentially to a component and extracted from content

3.) Android view based Color picker:
Current location:
  ui/android/java/src/org/chromium/ui/*Color*.java
Desired location:
  Most likely same as #2

We should use this as an opportunity to remove any classes directly in the root ui package for android (i.e. ui/android/java/src/org/chromium/ui).  We should make the layering look like the C++ ui code.  Then we can enforce ui DEPS correctly.
 
Cc: boliu@chromium.org sky@chromium.org
Owner: wychen@chromium.org
Status: Assigned (was: Available)
wychen@, can you take a look at this as part of the larger android layering refactoring.

boliu@ and sky@ -- I believe this is along the lines of what we discussed last week.  Feel free to drop off, but wanted to give you a chance to shout if I'm way off base.
#1 is here:
https://codereview.chromium.org/2382913004/

Regarding #2, if we move it to content, does it make sense to change the package name to org.chromium.content.browser.picker for #2, and org.chromium.content.browser for #3?
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d3e2b1609216768c8a50373fdae6d507dd5f7286

commit d3e2b1609216768c8a50373fdae6d507dd5f7286
Author: wychen <wychen@chromium.org>
Date: Wed Oct 05 20:33:58 2016

Move autofill from ui/ to components/

BUG= 651988 

Review-Url: https://codereview.chromium.org/2382913004
Cr-Commit-Position: refs/heads/master@{#423276}

[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/android_webview/BUILD.gn
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/android_webview/java/DEPS
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/android_webview/java/src/org/chromium/android_webview/AwAutofillClient.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/BUILD.gn
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/java/DEPS
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/javatests/DEPS
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
[add] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/BUILD.gn
[add] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/OWNERS
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/res/drawable/autofill_chip_inset.xml
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/res/layout/autofill_keyboard_accessory_icon.xml
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/res/layout/autofill_keyboard_accessory_item.xml
[add] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/res/values/dimens.xml
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/src/org/chromium/components/autofill/AutofillDelegate.java
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/ui/android/BUILD.gn
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/ui/android/java/res/values/dimens.xml

4.) Some other folders without JNI
Current location:
  ui/android/java/src/org/chromium/ui/{interpolators, text, widget}
Desired location:
  chrome

5.) Dropdown
Current location:
  ui/android/java/src/org/chromium/ui/Dropdown*.java
Desired location:
  content

6.) gl
Current location:
  ui/android/java/src/org/chromium/ui/gl and ui/gl
Desired location:
  content (used by gpu/ and media/)

7.) gfx
Current location:
  ui/android/java/src/org/chromium/ui/gfx and ui/gfx
Desired location:
  components (used by webview/ and blimp/, and chrome/)

Look good?
Actually, I "think" most of those are ok to live in UI.  We mainly want to remove any feature specific code.  The ones you listed seem to be pretty generic UI constructs, which are valid for UI.

We want UI to be the base layer containing reusable UI components, but as soon as it is feature specific, then it is something we don't want in ui.

For an example in the initial discussion, I think one of the desktop specific pieces that was highlighted was ui/file_manager.  That isn't really a core piece of UI, but something that is built upon many other underlying things.

From a quick glance, the directories you listed seem to be pretty basic to me.
Got it. I'll leave them as is. Somehow I thought we want to make ui/android/java/src/org/chromium/ui empty.

#2: https://codereview.chromium.org/2398443002/
#3: https://codereview.chromium.org/2398543003/

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d3e2b1609216768c8a50373fdae6d507dd5f7286

commit d3e2b1609216768c8a50373fdae6d507dd5f7286
Author: wychen <wychen@chromium.org>
Date: Wed Oct 05 20:33:58 2016

Move autofill from ui/ to components/

BUG= 651988 

Review-Url: https://codereview.chromium.org/2382913004
Cr-Commit-Position: refs/heads/master@{#423276}

[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/android_webview/BUILD.gn
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/android_webview/java/DEPS
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/android_webview/java/src/org/chromium/android_webview/AwAutofillClient.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/BUILD.gn
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/java/DEPS
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/javatests/DEPS
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTest.java
[add] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/BUILD.gn
[add] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/OWNERS
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/res/drawable/autofill_chip_inset.xml
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/res/layout/autofill_keyboard_accessory_icon.xml
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/res/layout/autofill_keyboard_accessory_item.xml
[add] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/res/values/dimens.xml
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/src/org/chromium/components/autofill/AutofillDelegate.java
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java
[rename] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/ui/android/BUILD.gn
[modify] https://crrev.com/d3e2b1609216768c8a50373fdae6d507dd5f7286/ui/android/java/res/values/dimens.xml

Comment 9 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/50a29dd4612b03331762028e049cfade95561966

commit 50a29dd4612b03331762028e049cfade95561966
Author: wychen <wychen@chromium.org>
Date: Tue Nov 08 02:16:07 2016

Move Android view based DateTimePicker from ui/ to content/

BUG= 651988 

Review-Url: https://codereview.chromium.org/2398443002
Cr-Commit-Position: refs/heads/master@{#430479}

[modify] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/BUILD.gn
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/res/layout-land/date_time_picker_dialog.xml
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/res/layout/date_time_picker_dialog.xml
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/res/layout/date_time_suggestion.xml
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/res/layout/multi_field_time_picker_dialog.xml
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/res/layout/two_field_date_picker.xml
[modify] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
[modify] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/DateDialogNormalizer.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/DatePickerDialogCompat.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/DateTimePickerDialog.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/DateTimeSuggestion.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/DateTimeSuggestionListAdapter.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/InputDialogContainer.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/MonthPicker.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/MonthPickerDialog.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/MultiFieldTimePickerDialog.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/TwoFieldDatePicker.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/TwoFieldDatePickerDialog.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/WeekPicker.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/java/src/org/chromium/content/browser/picker/WeekPickerDialog.java
[modify] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/javatests/src/org/chromium/content/browser/input/InputDialogContainerTest.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/javatests/src/org/chromium/content/browser/picker/DateTimePickerDialogTest.java
[rename] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/content/public/android/junit/src/org/chromium/content/browser/picker/DateDialogNormalizerTest.java
[modify] https://crrev.com/50a29dd4612b03331762028e049cfade95561966/ui/android/BUILD.gn

Cc: tedc...@chromium.org
Based on the discussion at https://codereview.chromium.org/2398443002/#msg21, it looks like we also want to move SelectFileDialog out of ui/ and maybe to chrome/.

However, ui/shell_dialogs/ contains file dialog for many platforms (win, mac, android, linux). Leaving SelectFileDialog there might fit other platforms better. What do you think?
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b387f6562f8dd96fb410bfee6efa0ce720a0b27c

commit b387f6562f8dd96fb410bfee6efa0ce720a0b27c
Author: wychen <wychen@chromium.org>
Date: Thu Nov 10 00:04:17 2016

Move Android view based ColorPicker from ui/ to components/

BUG= 651988 

Review-Url: https://codereview.chromium.org/2398543003
Cr-Commit-Position: refs/heads/master@{#431088}

[modify] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/OWNERS
[modify] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/components_strings.grd
[modify] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/BUILD.gn
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/drawable-hdpi/color_picker_advanced_select_handle.png
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/drawable-mdpi/color_picker_advanced_select_handle.png
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/drawable-xhdpi/color_picker_advanced_select_handle.png
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/drawable/color_button_background.xml
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/drawable/color_picker_border.xml
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/layout/color_picker_advanced_component.xml
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/layout/color_picker_dialog_content.xml
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/layout/color_picker_dialog_title.xml
[add] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/values/colors.xml
[add] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/res/values/dimens.xml
[modify] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorPickerAdvanced.java
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorPickerAdvancedComponent.java
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorPickerDialog.java
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorPickerMoreButton.java
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorPickerSimple.java
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorSuggestion.java
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorSuggestionListAdapter.java
[rename] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/OnColorChangedListener.java
[add] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/components/web_contents_delegate_android_strings.grdp
[modify] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/ui/android/BUILD.gn
[modify] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/ui/android/java/res/values/colors.xml
[modify] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/ui/android/java/res/values/dimens.xml
[modify] https://crrev.com/b387f6562f8dd96fb410bfee6efa0ce720a0b27c/ui/android/java/strings/android_ui_strings.grd

Status: Fixed (was: Assigned)

Sign in to add a comment