Add support for color input datalist on Android

Add support for color input datalist on Android

We pass the list of suggestions when opening the color chooser.
We also pass the suggestion labels but we aren't using them yet. We should use them in the future to improve accessibility.

The suggestions are arranged in a grid with four columns.

BUG=242455
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237890

[keishi, 2013-08-19 13:52:55]

Blink side patch is being reviewed here:
https://codereview.chromium.org/23236002/

[Miguel Garcia, 2013-08-19 14:43:27]

Thanks for doing this!

Some general comments, let me know what you think ok?
Also depending on the resolution on the maximum number of colors we might want
to relax the 4 column display, especially on large table screens?

https://codereview.chromium.org/23026006/diff/5030/apps/shell_window.h
File apps/shell_window.h (right):

https://codereview.chromium.org/23026006/diff/5030/apps/shell_window.h#newcod...
apps/shell_window.h:261: content::WebContents* web_contents,
same thing about sending color suggestions and labels than on the webkit patch.
If you decide to apply it there let's be consistent.

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
(right):

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java:25:
Context context, int initialColor, int[] suggestions, String[] suggestionLabels)
{
there is a cost (albeit small) in JNIfying the array of strings for suggestions
so I'd prefer that they are not added until we really need them. You can send
them all the way from Blink to the browser but not to java if that's ok.

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
File components/web_contents_delegate_android/color_chooser_android.cc (right):

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
components/web_contents_delegate_android/color_chooser_android.cc:18: const
std::vector<SkColor>& suggestions,
Can you rename "suggestions" to "suggested_colors" through the whole pipeline.
It will make it more readable.

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
components/web_contents_delegate_android/color_chooser_android.cc:33:
env->ReleaseIntArrayElements(suggestions_array.obj(),
Could you upgrade jni_array.cc/h with a ToJavaArrayOfInts method? We have bytes
and longs there already and it'd be nice to do a one off for ints here. You can
also test the new method in jni_array_unittest.cc

https://codereview.chromium.org/23026006/diff/5030/content/child/runtime_feat...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/23026006/diff/5030/content/child/runtime_feat...
content/child/runtime_features.cc:41:
WebRuntimeFeatures::enableDataListElement(true);
since enabling is the default shall we simply remove this line?
If we really need it at least change the comment in line #40 :)

https://codereview.chromium.org/23026006/diff/5030/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/23026006/diff/5030/content/common/view_messag...
content/common/view_messages.h:1969: std::vector<string16> /* suggestion_labels
*/)
there might have some security issues with sending a vector of strings on an
IPC, especially if they are not going to be used yet. Please add Chris Palmer or
someone from the security team for this.

https://codereview.chromium.org/23026006/diff/5030/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23026006/diff/5030/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2535: const
WebKit::WebVector<WebKit::WebString>& suggestionLabels) {
suggestion_labels

https://codereview.chromium.org/23026006/diff/5030/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2545: suggestionColors,
suggestion_colors

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/res/layout...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/res/layout...
ui/android/java/res/layout/color_picker_dialog_content.xml:22: <ScrollView
I adding newt for the UX changes ok?

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:71: public void
init(int[] colors,
I think, somewhere in the whole stack, preferably before we convert it to a bare
array that needs to be mapped into memory. We should implement a limit on the
number of suggestions. Is there precedent for this on desktop?

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:137: for (int i = 0;
i < mColors.length; ++i) {
can you calculate the number of cells and rows at the beginning and store it in
a local variable for readbility?

[newt (away), 2013-08-19 23:18:19]

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.h
(right):

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.h:60:
const std::vector<string16>& suggestion_values) OVERRIDE;
"suggestion_labels"

https://codereview.chromium.org/23026006/diff/5030/content/public/browser/web...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/23026006/diff/5030/content/public/browser/web...
content/public/browser/web_contents_delegate.cc:148: WebContents* web_contents,
indented too far

https://codereview.chromium.org/23026006/diff/5030/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23026006/diff/5030/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2538: std::vector<SkColor>
suggestionColors;
be consistent with variable naming. You could use something like
"skcolor_suggestions" and "string16_suggestion_labels", but don't use camel
case.

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/res/layout...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/res/layout...
ui/android/java/res/layout/color_picker_dialog_content.xml:22: <ScrollView
Nested ScrollViews are bug-prone and cause weird behavior in Android. It's also
a strange user experience. How does it look if you leave this as a FrameLayout?

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:40: * @param color
The initial color to set.
update javadoc

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:23: private static
final int MINIMUM_CELL_HEIGHT = 50;
MINIMUM_CELL_HEIGHT should be measured in DP, and probably should be called
MINIMUM_CELL_HEIGHT_DP for clarity. You can convert DP to pixels like so:

    int minimumCellHeight = Math.round(MINIMUM_CELL_HEIGHT_DP *
getResources().getDisplayMetrics().density);

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:63: * Initializes the
listener and precalculates the grid and color positions.
explain what happens if colors is empty. Also, I'd prefer passing null (instead
of an empty array) to signify "use default colors".

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:79: if (colors.length
== 0)
need curly braces

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:95: int rowCount =
(int)Math.ceil((double)mColors.length / COLUMN_COUNT);
Calculate mRowCount only once, and store it as a member variable. Also, this can
be done with integer arithmetic:

    mRowCount = (mColors.length + COLUMN_COUNT - 1) / COLUMN_COUNT;

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:109: int rowCount =
(int)Math.ceil((double)mColors.length / COLUMN_COUNT);
use mRowCount here and below.

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:144: if (i *
COLUMN_COUNT >= mBounds.length)
this condition can never be true.

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:197: int rowCount =
(int)Math.ceil((double)mColors.length / COLUMN_COUNT);
I don't see why you need to add/change lines 197-199 and 205-209. It should work
fine without those changes right?

If your goal is to use the value of GRID_LINE_WIDTH, you could change lines
178-182 to be:

    int left = j * (width + GRID_LINE_WIDTH) / COLUMN_COUNT + GRID_LINE_WIDTH;
    int right = (j + 1) * (width + GRID_LINE_WIDTH) / COLUMN_COUNT - 2 *
GRID_LINE_WIDTH;
    ...

But again, I'd prefer integer arithmetic over floating point division and
casting.

Also, could you move the calculations that use "i" but not "j" into the outer
loop?

[keishi, 2013-08-26 05:28:53]

https://codereview.chromium.org/23026006/diff/5030/apps/shell_window.h
File apps/shell_window.h (right):

https://codereview.chromium.org/23026006/diff/5030/apps/shell_window.h#newcod...
apps/shell_window.h:261: content::WebContents* web_contents,
On 2013/08/19 14:43:27, Miguel Garcia wrote:
> same thing about sending color suggestions and labels than on the webkit
patch.
> If you decide to apply it there let's be consistent.

Done.

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
(right):

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java:25:
Context context, int initialColor, int[] suggestions, String[] suggestionLabels)
{
On 2013/08/19 14:43:27, Miguel Garcia wrote:
> there is a cost (albeit small) in JNIfying the array of strings for
suggestions
> so I'd prefer that they are not added until we really need them. You can send
> them all the way from Blink to the browser but not to java if that's ok.

I reimplemented the simple color chooser so I can use the labels as the content
description.

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
File components/web_contents_delegate_android/color_chooser_android.cc (right):

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
components/web_contents_delegate_android/color_chooser_android.cc:18: const
std::vector<SkColor>& suggestions,
On 2013/08/19 14:43:27, Miguel Garcia wrote:
> Can you rename "suggestions" to "suggested_colors" through the whole pipeline.
> It will make it more readable.

I changed it to a vector of ColorSugestion struct so I kept the name
"suggestions".

https://codereview.chromium.org/23026006/diff/5030/components/web_contents_de...
components/web_contents_delegate_android/color_chooser_android.cc:33:
env->ReleaseIntArrayElements(suggestions_array.obj(),
On 2013/08/19 14:43:27, Miguel Garcia wrote:
> Could you upgrade jni_array.cc/h with a ToJavaArrayOfInts method? We have
bytes
> and longs there already and it'd be nice to do a one off for ints here. You
can
> also test the new method in jni_array_unittest.cc

I changed it to an array of ColorSuggestion objects so no longer relevant.

https://codereview.chromium.org/23026006/diff/5030/content/child/runtime_feat...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/23026006/diff/5030/content/child/runtime_feat...
content/child/runtime_features.cc:41:
WebRuntimeFeatures::enableDataListElement(true);
On 2013/08/19 14:43:27, Miguel Garcia wrote:
> since enabling is the default shall we simply remove this line?
> If we really need it at least change the comment in line #40 :)

Done. Removed.

https://codereview.chromium.org/23026006/diff/5030/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/23026006/diff/5030/content/common/view_messag...
content/common/view_messages.h:1969: std::vector<string16> /* suggestion_labels
*/)
On 2013/08/19 14:43:27, Miguel Garcia wrote:
> there might have some security issues with sending a vector of strings on an
> IPC, especially if they are not going to be used yet. Please add Chris Palmer
or
> someone from the security team for this.

Done.

https://codereview.chromium.org/23026006/diff/5030/content/public/browser/web...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/23026006/diff/5030/content/public/browser/web...
content/public/browser/web_contents_delegate.cc:148: WebContents* web_contents,
On 2013/08/19 23:18:19, newt wrote:
> indented too far

Done.

https://codereview.chromium.org/23026006/diff/5030/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23026006/diff/5030/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2538: std::vector<SkColor>
suggestionColors;
On 2013/08/19 23:18:19, newt wrote:
> be consistent with variable naming. You could use something like
> "skcolor_suggestions" and "string16_suggestion_labels", but don't use camel
> case.

Done.

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/res/layout...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/res/layout...
ui/android/java/res/layout/color_picker_dialog_content.xml:22: <ScrollView
On 2013/08/19 23:18:19, newt wrote:
> Nested ScrollViews are bug-prone and cause weird behavior in Android. It's
also
> a strange user experience. How does it look if you leave this as a
FrameLayout?

I changed ColorPickerSimple to a GridView so we no longer have nested scroll
views.

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:40: * @param color
The initial color to set.
On 2013/08/19 23:18:19, newt wrote:
> update javadoc

Done.

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:63: * Initializes the
listener and precalculates the grid and color positions.
On 2013/08/19 23:18:19, newt wrote:
> explain what happens if colors is empty. Also, I'd prefer passing null
(instead
> of an empty array) to signify "use default colors".

Changed to use default colors when passed null.

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:71: public void
init(int[] colors,
On 2013/08/19 14:43:27, Miguel Garcia wrote:
> I think, somewhere in the whole stack, preferably before we convert it to a
bare
> array that needs to be mapped into memory. We should implement a limit on the
> number of suggestions. Is there precedent for this on desktop?

No. I checked the limits for <input type=text> suggestions on other browsers. IE
only shows 16 suggestions. And Firefox has no limit.

I added a 10000 limit in the Blink patch.

[keishi, 2013-08-26 05:30:14]

palmer@ could you review the IPC changes in view_messages.h. Thanks,

[newt (away), 2013-08-26 20:23:19]

https://codereview.chromium.org/23026006/diff/28001/components/web_contents_d...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
(right):

https://codereview.chromium.org/23026006/diff/28001/components/web_contents_d...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java:68:
* @param name Name of the suggestion.
these params need updating

https://codereview.chromium.org/23026006/diff/28001/content/public/common/col...
File content/public/common/color_suggestion.h (right):

https://codereview.chromium.org/23026006/diff/28001/content/public/common/col...
content/public/common/color_suggestion.h:21: ColorSuggestion() { }
is it OK to put this constructor definition in the header file?

https://codereview.chromium.org/23026006/diff/28001/content/public/common/col...
content/public/common/color_suggestion.h:22: ColorSuggestion(const
WebKit::WebColorSuggestion& suggestion);
should be "explicit"

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/drawa...
File ui/android/java/res/drawable/color_button_border.xml (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/drawa...
ui/android/java/res/drawable/color_button_border.xml:1: <?xml version="1.0"
encoding="utf-8"?>
copyright

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
File ui/android/java/res/layout/color_button.xml (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_button.xml:1: <?xml version="1.0"
encoding="utf-8"?>
copyright

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_button.xml:3: <LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android"
don't use a nested layout just to create a background effect. layout groups are
expensive to create and draw, and this layout will be inflated many times. you
can use a single View with a ShapeDrawable or a LayerDrawable as a background
instead.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_button.xml:10: android:background="#ff0000"
unused background attribute

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:24: <LinearLayout
you can remove this LinearLayout. The parent RelativeLayout can lay out the
views below.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:33:
android:layout_weight="1"
layout_weight="1" doesn't do anything if the parent has
layout_height="wrap_content". I think you can just remove the layout_weight
attribute.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:37:
android:stretchMode="spacingWidth"
how does this look? I'd expect "columnWidth" to look better. can you upload
screenshots to the bug at a few different widths?

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:39:
android:horizontalSpacing="8dp"
this is a lot more spacing than we had before. why the increase?

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/value...
File ui/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/value...
ui/android/java/res/values/colors.xml:9: <drawable
name="color_button_border_pressed_color">#00000000</drawable>
shouldn't these be colors (not drawables)?

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/value...
ui/android/java/res/values/colors.xml:10: <drawable
name="color_button_border_color">#B0B0B0</drawable>
duplicate of color_picker_border_color on line 8

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:27: private
OnColorChangedListener mOnColorTouchedListener;
might as well change this name to "mOnColorChangedListener"

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:90: ColorGridAdapter
adapter = (ColorGridAdapter) getAdapter();
you could use getItemAtPosition() here

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorSuggestion.java (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorSuggestion.java:8: * Color suggestion
container used to store information for each color button.
I'd mention this is for the color picker UI

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorSuggestion.java:21: public
ColorSuggestion(int valueAsColor, String valueAsText, String label) {
what's the difference between "valueAsText" and "label"? Could you make it clear
in the javadoc?

Perhaps the member variables could be named something like: mColor, mLabel, mId?

[palmer, 2013-08-26 20:38:00]

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorSuggestion.java (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorSuggestion.java:18: * @param
valueAsText The suggested color value as a string.
Is this the *name* of the color, or a CSS value like "#FFFFFFFF"? We should not
send such strings over IPC because inevitably they will get interpolated into
CSS.

Also, it's just redundant to pass the color both as an int and as a CSS value
string. Just use the int.

[Miguel Garcia, 2013-08-27 14:31:59]

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:71: public void
init(int[] colors,
Do we really need 10K? Can we settle for 1K colors? I can't conveive a scenario
where more than 1K datalist element would be required and mapping 10K structs
would be expensive.

On 2013/08/26 05:28:54, keishi1 wrote:
> On 2013/08/19 14:43:27, Miguel Garcia wrote:
> > I think, somewhere in the whole stack, preferably before we convert it to a
> bare
> > array that needs to be mapped into memory. We should implement a limit on
the
> > number of suggestions. Is there precedent for this on desktop?
> 
> No. I checked the limits for <input type=text> suggestions on other browsers.
IE
> only shows 16 suggestions. And Firefox has no limit.
> 
> I added a 10000 limit in the Blink patch.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:39: private static
final String[] DEFAULT_COLOR_LABELS = { "Red",
how does this work with internationalization?

[newt (away), 2013-08-27 16:05:28]

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:71: public void
init(int[] colors,
On 2013/08/27 14:32:00, Miguel Garcia wrote:
> Do we really need 10K? Can we settle for 1K colors? I can't conveive a
scenario
> where more than 1K datalist element would be required and mapping 10K structs
> would be expensive.
> 
> On 2013/08/26 05:28:54, keishi1 wrote:
> > On 2013/08/19 14:43:27, Miguel Garcia wrote:
> > > I think, somewhere in the whole stack, preferably before we convert it to
a
> > bare
> > > array that needs to be mapped into memory. We should implement a limit on
> the
> > > number of suggestions. Is there precedent for this on desktop?
> > 
> > No. I checked the limits for <input type=text> suggestions on other
browsers.
> IE
> > only shows 16 suggestions. And Firefox has no limit.
> > 
> > I added a 10000 limit in the Blink patch.
> 

Should we limit the total amount of data sent over IPC (e.g. 1024 strings, max
length 1024 chars each) so we don't cause an out of memory error in the browser
process?

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:39: private static
final String[] DEFAULT_COLOR_LABELS = { "Red",
On 2013/08/27 14:32:00, Miguel Garcia wrote:
> how does this work with internationalization?

How are these strings used? If these are user-facing then they should be
properly localized. You can add strings to android_ui_strings.grd and reference
them here using getContext().getString(R.string.red).

[keishi, 2013-08-29 03:40:47]

Default colors:
https://docs.google.com/file/d/0BwRi59l_ri74eER5N1ZDSl9NZDQ/edit?usp=sharing

Portrait (dialog height grows to fit all the color buttons):
https://docs.google.com/file/d/0BwRi59l_ri74UmI1amRfQVZxWG8/edit?usp=sharing

Landscape (dialog height will grow to fill the screen and then starts to
scroll):
https://docs.google.com/file/d/0BwRi59l_ri74T1dPd1M2blhPXzg/edit?usp=sharing

https://codereview.chromium.org/23026006/diff/28001/components/web_contents_d...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java
(right):

https://codereview.chromium.org/23026006/diff/28001/components/web_contents_d...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/ColorChooserAndroid.java:68:
* @param name Name of the suggestion.
On 2013/08/26 20:23:20, newt wrote:
> these params need updating

Done.

https://codereview.chromium.org/23026006/diff/28001/content/public/common/col...
File content/public/common/color_suggestion.h (right):

https://codereview.chromium.org/23026006/diff/28001/content/public/common/col...
content/public/common/color_suggestion.h:22: ColorSuggestion(const
WebKit::WebColorSuggestion& suggestion);
On 2013/08/26 20:23:20, newt wrote:
> should be "explicit"

Done.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
File ui/android/java/res/layout/color_button.xml (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_button.xml:3: <LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android"
On 2013/08/26 20:23:20, newt wrote:
> don't use a nested layout just to create a background effect. layout groups
are
> expensive to create and draw, and this layout will be inflated many times. you
> can use a single View with a ShapeDrawable or a LayerDrawable as a background
> instead.

Done.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:24: <LinearLayout
On 2013/08/26 20:23:20, newt wrote:
> you can remove this LinearLayout. The parent RelativeLayout can lay out the
> views below.

Done.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:33:
android:layout_weight="1"
On 2013/08/26 20:23:20, newt wrote:
> layout_weight="1" doesn't do anything if the parent has
> layout_height="wrap_content". I think you can just remove the layout_weight
> attribute.

Done.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:37:
android:stretchMode="spacingWidth"
On 2013/08/26 20:23:20, newt wrote:
> how does this look? I'd expect "columnWidth" to look better. can you upload
> screenshots to the bug at a few different widths?

I wasn't sure how to use GridView to achieve the current design with gray and
white borders so I decided to change the design slightly and used square color
buttons. By using spacingWidth the color buttons are always square and the extra
space is divided equally among the space between two columns.

Here are the screen shots.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:39:
android:horizontalSpacing="8dp"
On 2013/08/26 20:23:20, newt wrote:
> this is a lot more spacing than we had before. why the increase?

I added some space between buttons so they are easier to hit.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/value...
File ui/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/res/value...
ui/android/java/res/values/colors.xml:10: <drawable
name="color_button_border_color">#B0B0B0</drawable>
On 2013/08/26 20:23:20, newt wrote:
> duplicate of color_picker_border_color on line 8

Done.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:27: private
OnColorChangedListener mOnColorTouchedListener;
On 2013/08/26 20:23:20, newt wrote:
> might as well change this name to "mOnColorChangedListener"

Done.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:39: private static
final String[] DEFAULT_COLOR_LABELS = { "Red",
On 2013/08/27 16:05:29, newt wrote:
> On 2013/08/27 14:32:00, Miguel Garcia wrote:
> > how does this work with internationalization?
> 
> How are these strings used? If these are user-facing then they should be
> properly localized. You can add strings to android_ui_strings.grd and
reference
> them here using getContext().getString(R.string.red).

Yes these are user facing when using TalkBack. I added them to
android_ui_strings.grd

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:90: ColorGridAdapter
adapter = (ColorGridAdapter) getAdapter();
On 2013/08/26 20:23:20, newt wrote:
> you could use getItemAtPosition() here

Done.

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorSuggestion.java (right):

https://codereview.chromium.org/23026006/diff/28001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorSuggestion.java:8: * Color suggestion
container used to store information for each color button.
On 2013/08/26 20:23:20, newt wrote:
> I'd mention this is for the color picker UI

Done.

[keishi, 2013-08-29 04:18:43]

I removed valueAsText.

I intended valueAsText to contain the color converted to string by Blink. I
added it because I thought maybe in the future we might want to use HTML color
names instead of the hex codes.
It won't work well with localization and stuff so I'm removing it.

[keishi, 2013-08-29 05:04:10]

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/5030/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:71: public void
init(int[] colors,
On 2013/08/27 16:05:29, newt wrote:
> On 2013/08/27 14:32:00, Miguel Garcia wrote:
> > Do we really need 10K? Can we settle for 1K colors? I can't conveive a
> scenario
> > where more than 1K datalist element would be required and mapping 10K
structs
> > would be expensive.
> > 
> > On 2013/08/26 05:28:54, keishi1 wrote:
> > > On 2013/08/19 14:43:27, Miguel Garcia wrote:
> > > > I think, somewhere in the whole stack, preferably before we convert it
to
> a
> > > bare
> > > > array that needs to be mapped into memory. We should implement a limit
on
> > the
> > > > number of suggestions. Is there precedent for this on desktop?
> > > 
> > > No. I checked the limits for <input type=text> suggestions on other
> browsers.
> > IE
> > > only shows 16 suggestions. And Firefox has no limit.
> > > 
> > > I added a 10000 limit in the Blink patch.
> > 
> 
> Should we limit the total amount of data sent over IPC (e.g. 1024 strings, max
> length 1024 chars each) so we don't cause an out of memory error in the
browser
> process?

Added the 1024 char limit in Blink. I wonder if its the best way to handle the
IPC size limit.

https://codereview.chromium.org/23026006/diff/28001/content/public/common/col...
File content/public/common/color_suggestion.h (right):

https://codereview.chromium.org/23026006/diff/28001/content/public/common/col...
content/public/common/color_suggestion.h:21: ColorSuggestion() { }
On 2013/08/26 20:23:20, newt wrote:
> is it OK to put this constructor definition in the header file?

Moved to cc file.

[newt (away), 2013-08-29 05:35:33]

Thanks for working on this!

A few more nits, and:
 - could you upload screenshots to the bug?
 - it seems like you might have forgotten to upload color_button_background.xml

https://codereview.chromium.org/23026006/diff/70001/components/web_contents_d...
File components/web_contents_delegate_android/color_chooser_android.cc (right):

https://codereview.chromium.org/23026006/diff/70001/components/web_contents_d...
components/web_contents_delegate_android/color_chooser_android.cc:37:
ScopedJavaLocalRef<jstring> label = ConvertUTF16ToJavaString(
since labels will often be empty, I'd prefer to use NULL to signify "no label"
instead of creating a bunch of empty strings

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/res/drawa...
File ui/android/java/res/drawable/color_button_border.xml (right):

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/res/drawa...
ui/android/java/res/drawable/color_button_border.xml:9: <item
android:state_pressed="true">
you can actually just add "android:color" as an attribute to <item>

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/res/layou...
File ui/android/java/res/layout/color_button.xml (right):

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/res/layou...
ui/android/java/res/layout/color_button.xml:9:
android:background="@drawable/color_button_background"
where is color_button_background defined? I can't find it...

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/res/layou...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:28:
android:layout_weight="1"
remove layout_weight="1"... that'll make this view expand as large as possible,
but we don't want this view to expand beyond the size of its content

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:32:
android:stretchMode="spacingWidth"
I can't see any screenshots. Could you upload them to the bug please:
https://code.google.com/p/chromium/issues/detail?id=242455

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorGridAdapter.java (right):

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorGridAdapter.java:33:
layout.setLayoutParams(new GridView.LayoutParams(60, 60));
you're actually respecifying the width and height of the GridView children here,
and using px instead of dp. Instead, remove this setLayoutParams line and change
the previous line to:

    layout = inflater.inflate(R.layout.color_button, parent, false);

that should respect the layout params in the color_button.xml file.

Or you could delete color_button.xml and just use "new View()" here. Then you'll
have to set the layout params manually, and be sure to convert from dp to px.

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorGridAdapter.java:36: LayerDrawable
background = (LayerDrawable)layout.getBackground();
space before "layout"

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorGridAdapter.java:38: (GradientDrawable)
background.findDrawableByLayerId(R.id.color_button_swatch);
where is color_button_swatch defined?

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorGridAdapter.java:41: if
(description.isEmpty())
need curly braces in Java. also, could description potentially be null?

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorSuggestion.java (right):

https://codereview.chromium.org/23026006/diff/70001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorSuggestion.java:13: final String
mLabel;
is this guaranteed non-null? if so, mention that here.

[palmer, 2013-08-29 19:05:41]

lgtm

https://codereview.chromium.org/23026006/diff/95001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ColorSuggestion.java (right):

https://codereview.chromium.org/23026006/diff/95001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ColorSuggestion.java:9: * the simple color
picker..
Nit: End sentence with one period.

[keishi, 2013-09-04 13:26:15]

Maybe the change in appearance is making it hard to review.

I tried to achieve the current UI design using GridView here:
https://codereview.chromium.org/23593006/
Here is the screenshot:
https://docs.google.com/file/d/0BwRi59l_ri74QTJDdHZVbHpYYzg/edit?usp=sharing
I wasn't able to get rid of the extra space on the right and I set the
background to gray so the extra space in the bottom row is gray.

I also tried to implement the circle design from the new Google Calendar app
here: https://codereview.chromium.org/23691036
Here is the screen shot:
https://docs.google.com/file/d/0BwRi59l_ri74MGVkdVZCUUxwOHM/edit?usp=sharing

After all this I still think this patch is best.

[newt (away), 2013-09-04 17:42:59]

Thanks for the screenshots. Does this screenshot
https://docs.google.com/a/google.com/file/d/0BwRi59l_ri74QTJDdHZVbHpYYzg/edit
show what this current CL looks like? In any case, can you please please upload
a screenshot of this current CL to http://crbug.com/242455 ?

I think this current CL is the way to go. I'm just waiting on responses to the
comments on patch set 10. Thanks!

https://codereview.chromium.org/23026006/diff/82001/ui/android/java/res/drawa...
File ui/android/java/res/drawable/color_button_background.xml (right):

https://codereview.chromium.org/23026006/diff/82001/ui/android/java/res/drawa...
ui/android/java/res/drawable/color_button_background.xml:15:
android:drawable="@drawable/color_button_swatch"/>
you should be able to inline color_button_swatch.xml here (you can see how
<shape> elements can live inside <item> elements here:
http://stackoverflow.com/questions/8780119/android-button-with-two-background...)

https://codereview.chromium.org/23026006/diff/82001/ui/android/java/res/layou...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/82001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:31:
android:stretchMode="spacingWidth"
does changing this to android:stretchMode="columnWidth" solve the issue with
extra blank space between the columns?

[keishi, 2013-09-06 13:09:41]

On 2013/09/04 17:42:59, newt wrote:
> Thanks for the screenshots. Does this screenshot
> https://docs.google.com/a/google.com/file/d/0BwRi59l_ri74QTJDdHZVbHpYYzg/edit
> show what this current CL looks like? In any case, can you please please
upload
> a screenshot of this current CL to http://crbug.com/242455 ?
> 
> I think this current CL is the way to go. I'm just waiting on responses to the
> comments on patch set 10. Thanks!

No here are the screenshots for this patch. This design also suffers from extra
space on the right side but it isn't that noticeable.

Default colors:
https://docs.google.com/file/d/0BwRi59l_ri74eER5N1ZDSl9NZDQ/edit?usp=sharing

Portrait (dialog height grows to fit all the color buttons):
https://docs.google.com/file/d/0BwRi59l_ri74UmI1amRfQVZxWG8/edit?usp=sharing

Landscape (dialog height will grow to fill the screen and then starts to
scroll):
https://docs.google.com/file/d/0BwRi59l_ri74T1dPd1M2blhPXzg/edit?usp=sharing

https://codereview.chromium.org/23026006/diff/82001/ui/android/java/res/drawa...
File ui/android/java/res/drawable/color_button_background.xml (right):

https://codereview.chromium.org/23026006/diff/82001/ui/android/java/res/drawa...
ui/android/java/res/drawable/color_button_background.xml:15:
android:drawable="@drawable/color_button_swatch"/>
On 2013/09/04 17:42:59, newt wrote:
> you should be able to inline color_button_swatch.xml here (you can see how
> <shape> elements can live inside <item> elements here:
>
http://stackoverflow.com/questions/8780119/android-button-with-two-background...)

Done.

https://codereview.chromium.org/23026006/diff/82001/ui/android/java/res/layou...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/82001/ui/android/java/res/layou...
ui/android/java/res/layout/color_picker_dialog_content.xml:31:
android:stretchMode="spacingWidth"
On 2013/09/04 17:42:59, newt wrote:
> does changing this to android:stretchMode="columnWidth" solve the issue with
> extra blank space between the columns?

No. According to the source for GridView, it uses an integer for
mHorizontalSpacing and mColumnWidth. The truncation error when calculating them
accumulates on the right side. So no matter what stretch mode you use there will
be extra space on the right side.

[newt (away), 2013-09-06 21:04:31]

looks good, with one comment, and I think you should run the design changes past
ainslie@, who mocked up the original the color picker design.

https://codereview.chromium.org/23026006/diff/111001/ui/android/java/res/draw...
File ui/android/java/res/drawable/color_button_swatch.xml (right):

https://codereview.chromium.org/23026006/diff/111001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_swatch.xml:1: <?xml version="1.0"
encoding="utf-8"?>
you can delete this now, right?

[keishi, 2013-09-12 12:17:16]

I was able to create something close to the current color picker by using
ListView instead of the GridView.

I have made sure it works well with RTL locales and TalkBack. I also fixed the
double width top border of the more button.

Here are the screenshots from the new patch.

10 suggestions
https://docs.google.com/file/d/0BwRi59l_ri74b0FBalJ5UlZsa0k/edit?usp=sharing

Many suggestions
https://docs.google.com/file/d/0BwRi59l_ri74SE5zblcxNE9iVFE/edit?usp=sharing

RTL langauge
https://docs.google.com/file/d/0BwRi59l_ri74bGtxWnVGNWhyc2M/edit?usp=sharing

[keishi, 2013-10-01 14:19:40]

On 2013/09/12 12:17:16, keishi1 wrote:
> I was able to create something close to the current color picker by using
> ListView instead of the GridView.
> 
> I have made sure it works well with RTL locales and TalkBack. I also fixed the
> double width top border of the more button.
> 
> Here are the screenshots from the new patch.
> 
> 10 suggestions
> https://docs.google.com/file/d/0BwRi59l_ri74b0FBalJ5UlZsa0k/edit?usp=sharing
> 
> Many suggestions
> https://docs.google.com/file/d/0BwRi59l_ri74SE5zblcxNE9iVFE/edit?usp=sharing
> 
> RTL langauge
> https://docs.google.com/file/d/0BwRi59l_ri74bGtxWnVGNWhyc2M/edit?usp=sharing

It has been a while. Could you please take a look again. Thanks.

[newt (away), 2013-10-09 06:07:02]

Thanks, keishi, I think this is a pretty good approach. I have some comments
inline.

Additionally, I think you can greatly simplify the color_button_background xml
files. Instead of drawing the border as part of color_button_background, you can
add layout_marginEnd="1dp" to the first three Views in color_button_row.xml and
set the background of the LinearLayout in color_button_row.xml to
"@color/color_picker_border_color". Then you should be able to delete
drawable-ldrtl/color_button_background.xml and
drawable/last_color_button_background.xml. Finally,
drawable/color_button_background.xml reduces to:

<shape android:shape="rectangle">
    <stroke android:width="1dp"
android:color="@color/color_picker_background_color" />
    <solid android:color="<swatch color>"/>
</shape>

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
File ui/android/java/res/drawable/color_button_background.xml (right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:9: 
remove stray empty lines. this should look cleaner.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:10: <item
android:state_enabled="false">
don't need a selector or this first item. as per the comment in
ColorSuggestionListAdapter, you should just hide the color button instead of
disabling it

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:21: <item
android:top="0dp"
top, right, left, and bottom are optional. you can leave them out when they're
just 0dp

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:43: <item
android:id="@+id/color_button_swatch"
put each attribute on its own line:

    <item
        android:id="..."
        android:top="..."
        ...

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
File ui/android/java/res/layout/color_button_row.xml (right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
ui/android/java/res/layout/color_button_row.xml:1: <?xml version="1.0"
encoding="utf-8"?>
I'd prefer if this were replaced with a Java class that extends LinearLayout and
inserts the child Views programmatically. This will let you control the number
of color buttons with a runtime parameter (instead of fixing it to 4)

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
ui/android/java/res/layout/color_button_row.xml:9:
android:orientation="horizontal"
unindent by 4

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
ui/android/java/res/layout/color_button_row.xml:16:
android:layout_width="wrap_content"
layout_width should be 0dp for all of these color_button views.
layout_weight="1" will then ensure that each view gets 25% of the available
width

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
ui/android/java/res/layout/color_picker_dialog_content.xml:16:
android:layout_height="wrap_content">
what does the advanced picker look like on a small screen in landscape after
changing the layout_height to wrap_content? Do things look OK, or does the
dialog go off the edge of the screen?

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
ui/android/java/res/layout/color_picker_dialog_content.xml:33:
android:layout_weight="1"
don't need layout_weight, right?

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:76: suggestions[i] =
nit: I'd wrap this after "new ColorSuggestion(", e.g.:

    suggestions[i] = new ColorSuggestion(
            DEFAULT_COLORS[i],
getContext().getString(DEFAULT_COLOR_LABEL_IDS[i]));

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:19: public
class ColorSuggestionListAdapter extends BaseAdapter implements
View.OnClickListener {
javadoc for all public classes and methods (except @Override methods)

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:25: super();
BaseAdapter doesn't define a constructor, so this isn't technically needed

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:34: private
void setupColorButton(View button, int index) {
"setup" -> "setUp"

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:36:
button.setTag(new Integer(-1));
I'd just set the tag to null

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:39:
button.setImportantForAccessibility(View.IMPORTANT_FOR_ACCESSIBILITY_NO);
View.IMPORTANT_FOR_ACCESSIBILITY_NO was added in Android API level 16, and we
have to support back to level 14. I'd just remove this line. Instead:

    button.setVisibility(View.INVISIBLE)

(and you can remove calls to button.setEnabled())

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:42:
button.setTag(new Integer(index));
why not just set the tag to the ColorSuggestion?

    button.setTag(mSuggestions[index]);

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:51: if
(description.isEmpty()) {
can you be sure that description is never null? if not, you can use
TextUtils.isEmpty(description)

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:61: if
(mListener == null)
need curly braces for if-else in Java, unless it's a single line if statement:

    if (mListener == null) return;

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:63: Integer
index = (Integer) v.getTag();
it's probably good to safeguard against null tags here

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:72: if
(convertView == null) {
Ideally, we'd be able to configure the number of colors in a row at runtime.

An idea: You could delete color_button_row.xml and create color_buttom.xml
describing a single color button. Then create a LinearLayout programmatically
here, and inflate and add N color buttons here.

If not, you should at least replace "4" throughout this file with a meaningful
constant like "COLORS_PER_ROW".

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:73:
LayoutInflater inflater =
easier: LayoutInflater inflater = LayoutInflater.from(mContext);

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:91: return
mSuggestions[position * 4];
I'd prefer returning null here -- that makes it clear that we're not actually
using this

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:96: return
(int) Math.ceil(mSuggestions.length / 4d);
return (mSuggestions.length + 3) / 4;

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/OnColorSuggestionClickListener.java
(right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/OnColorSuggestionClickListener.java:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
This interface could live inside ColorSuggestionListAdapter, which conceptually
owns this interface.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/OnColorSuggestionClickListener.java:3: //
found in the LICENSE file.
newline after copyright

[keishi, 2013-10-15 16:44:01]

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
File ui/android/java/res/drawable/color_button_background.xml (right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:9: 
On 2013/10/09 06:07:03, newt wrote:
> remove stray empty lines. this should look cleaner.

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:10: <item
android:state_enabled="false">
On 2013/10/09 06:07:03, newt wrote:
> don't need a selector or this first item. as per the comment in
> ColorSuggestionListAdapter, you should just hide the color button instead of
> disabling it

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:21: <item
android:top="0dp"
On 2013/10/09 06:07:03, newt wrote:
> top, right, left, and bottom are optional. you can leave them out when they're
> just 0dp

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:43: <item
android:id="@+id/color_button_swatch"
On 2013/10/09 06:07:03, newt wrote:
> put each attribute on its own line:
> 
>     <item
>         android:id="..."
>         android:top="..."
>         ...

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
File ui/android/java/res/layout/color_button_row.xml (right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
ui/android/java/res/layout/color_button_row.xml:9:
android:orientation="horizontal"
On 2013/10/09 06:07:03, newt wrote:
> unindent by 4

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
ui/android/java/res/layout/color_button_row.xml:16:
android:layout_width="wrap_content"
On 2013/10/09 06:07:03, newt wrote:
> layout_width should be 0dp for all of these color_button views.
> layout_weight="1" will then ensure that each view gets 25% of the available
> width

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
ui/android/java/res/layout/color_picker_dialog_content.xml:16:
android:layout_height="wrap_content">
On 2013/10/09 06:07:03, newt wrote:
> what does the advanced picker look like on a small screen in landscape after
> changing the layout_height to wrap_content? Do things look OK, or does the
> dialog go off the edge of the screen?

I only have my Nexus 7 so I wasn't able to test with a small screen.
ColorPickerSimple doesn't make the dialog go off the edge so I'm hoping it will
be ok.
I'll try to test it somehow.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/res/layo...
ui/android/java/res/layout/color_picker_dialog_content.xml:33:
android:layout_weight="1"
On 2013/10/09 06:07:03, newt wrote:
> don't need layout_weight, right?

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/ColorPickerSimple.java (right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorPickerSimple.java:76: suggestions[i] =
On 2013/10/09 06:07:03, newt wrote:
> nit: I'd wrap this after "new ColorSuggestion(", e.g.:
> 
>     suggestions[i] = new ColorSuggestion(
>             DEFAULT_COLORS[i],
> getContext().getString(DEFAULT_COLOR_LABEL_IDS[i]));

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:19: public
class ColorSuggestionListAdapter extends BaseAdapter implements
View.OnClickListener {
On 2013/10/09 06:07:03, newt wrote:
> javadoc for all public classes and methods (except @Override methods)

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:25: super();
On 2013/10/09 06:07:03, newt wrote:
> BaseAdapter doesn't define a constructor, so this isn't technically needed

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:34: private
void setupColorButton(View button, int index) {
On 2013/10/09 06:07:03, newt wrote:
> "setup" -> "setUp"

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:36:
button.setTag(new Integer(-1));
On 2013/10/09 06:07:03, newt wrote:
> I'd just set the tag to null

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:39:
button.setImportantForAccessibility(View.IMPORTANT_FOR_ACCESSIBILITY_NO);
On 2013/10/09 06:07:03, newt wrote:
> View.IMPORTANT_FOR_ACCESSIBILITY_NO was added in Android API level 16, and we
> have to support back to level 14. I'd just remove this line. Instead:
> 
>     button.setVisibility(View.INVISIBLE)
> 
> (and you can remove calls to button.setEnabled())

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:42:
button.setTag(new Integer(index));
On 2013/10/09 06:07:03, newt wrote:
> why not just set the tag to the ColorSuggestion?
> 
>     button.setTag(mSuggestions[index]);

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:51: if
(description.isEmpty()) {
On 2013/10/09 06:07:03, newt wrote:
> can you be sure that description is never null? if not, you can use
> TextUtils.isEmpty(description)

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:61: if
(mListener == null)
On 2013/10/09 06:07:03, newt wrote:
> need curly braces for if-else in Java, unless it's a single line if statement:
> 
>     if (mListener == null) return;

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:63: Integer
index = (Integer) v.getTag();
On 2013/10/09 06:07:03, newt wrote:
> it's probably good to safeguard against null tags here

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:72: if
(convertView == null) {
On 2013/10/09 06:07:03, newt wrote:
> Ideally, we'd be able to configure the number of colors in a row at runtime.
> 
> An idea: You could delete color_button_row.xml and create color_buttom.xml
> describing a single color button. Then create a LinearLayout programmatically
> here, and inflate and add N color buttons here.
> 
> If not, you should at least replace "4" throughout this file with a meaningful
> constant like "COLORS_PER_ROW".

I tried color_buttom.xml but the attributes like layout_width and layout_height
were being ignored (couldn't figure out why), so I changed it to code.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:73:
LayoutInflater inflater =
On 2013/10/09 06:07:03, newt wrote:
> easier: LayoutInflater inflater = LayoutInflater.from(mContext);

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:91: return
mSuggestions[position * 4];
On 2013/10/09 06:07:03, newt wrote:
> I'd prefer returning null here -- that makes it clear that we're not actually
> using this

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:96: return
(int) Math.ceil(mSuggestions.length / 4d);
On 2013/10/09 06:07:03, newt wrote:
> return (mSuggestions.length + 3) / 4;

Done.

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/OnColorSuggestionClickListener.java
(right):

https://codereview.chromium.org/23026006/diff/134001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/OnColorSuggestionClickListener.java:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2013/10/09 06:07:03, newt wrote:
> This interface could live inside ColorSuggestionListAdapter, which
conceptually
> owns this interface.

Done.

[newt (away), 2013-10-15 18:00:14]

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/draw...
File ui/android/java/res/drawable/color_button_background.xml (right):

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:8: <layer-list
xmlns:android="http://schemas.android.com/apk/res/android">
this is currently a bit broken in rtl languages. suggestion:

1. Replace this file with:

<shape android:shape="rectangle">
    <stroke android:width="1px"
android:color="@color/color_picker_background_color" />
    <solid android:color="#FF0000"/>
</shape>

2. Draw the color_picker_border_color as the background of the LinearLayout
containing each row of color buttons: In ColorSuggestionListAdapter, for each
LinearLayout you create, set the background to color_picker_border_color, and
for each button (except the last in each row), use
ApiCompatibilityUtils.setMarginEnd() to give it a 1px end margin.

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/layo...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/layo...
ui/android/java/res/layout/color_picker_dialog_content.xml:16:
android:layout_height="wrap_content">
Repeating this so it doesn't get lost: what does the advanced picker look like
on a small screen in landscape after
changing the layout_height to wrap_content? Do things look OK, or does the
dialog go off the edge of the screen?

You could test this by increasing the side of the advanced color picker (make
the rows tall, or the font big, etc).

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/layo...
ui/android/java/res/layout/color_picker_dialog_content.xml:34:
android:layout_marginBottom="-1px"
why add paddingBottom and negative marginBottom? seems like you could remove
both of these attributes. (if not, layout_marginBottom should be at the top
after layout_height)

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:21: public
class ColorSuggestionListAdapter extends BaseAdapter implements
View.OnClickListener {
still need javadoc for the class

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:96: if
(convertView == null) {
nit: convertView -> layout

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:101:
LayoutInflater inflater = LayoutInflater.from(mContext);
inflater is unused

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:104:
button.setLayoutParams(new android.widget.LinearLayout.LayoutParams(
to make the different types of layout params clear, write
AbsListView.LayoutParams and LinearLayout.LayoutParams instead of importing
AbsListView.LayoutParams.

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:106: (int)
mContext.getResources().getDimension(R.dimen.color_button_height),
nit: get this value once outside of the loop

[keishi, 2013-10-16 04:47:22]

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/draw...
File ui/android/java/res/drawable/color_button_background.xml (right):

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/draw...
ui/android/java/res/drawable/color_button_background.xml:8: <layer-list
xmlns:android="http://schemas.android.com/apk/res/android">
On 2013/10/15 18:00:15, newt wrote:
> this is currently a bit broken in rtl languages. suggestion:
> 
> 1. Replace this file with:
> 
> <shape android:shape="rectangle">
>     <stroke android:width="1px"
> android:color="@color/color_picker_background_color" />
>     <solid android:color="#FF0000"/>
> </shape>
> 
> 2. Draw the color_picker_border_color as the background of the LinearLayout
> containing each row of color buttons: In ColorSuggestionListAdapter, for each
> LinearLayout you create, set the background to color_picker_border_color, and
> for each button (except the last in each row), use
> ApiCompatibilityUtils.setMarginEnd() to give it a 1px end margin.

The number of suggestions might not be a multiple of COLORS_PER_ROW, so if we
set the background to color_picker_border_color the left over space in the last
row will be gray.

I used a negative margin start to overlap the borders.

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/layo...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/layo...
ui/android/java/res/layout/color_picker_dialog_content.xml:16:
android:layout_height="wrap_content">
On 2013/10/15 18:00:15, newt wrote:
> Repeating this so it doesn't get lost: what does the advanced picker look like
> on a small screen in landscape after
> changing the layout_height to wrap_content? Do things look OK, or does the
> dialog go off the edge of the screen?
> 
> You could test this by increasing the side of the advanced color picker (make
> the rows tall, or the font big, etc).

Great idea. I tested this and it worked properly. The scroll view was as tall as
it could be and didn't go off the edge.

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/layo...
ui/android/java/res/layout/color_picker_dialog_content.xml:34:
android:layout_marginBottom="-1px"
On 2013/10/15 18:00:15, newt wrote:
> why add paddingBottom and negative marginBottom? seems like you could remove
> both of these attributes. (if not, layout_marginBottom should be at the top
> after layout_height)

Done.

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:21: public
class ColorSuggestionListAdapter extends BaseAdapter implements
View.OnClickListener {
On 2013/10/15 18:00:15, newt wrote:
> still need javadoc for the class

Done.

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:96: if
(convertView == null) {
On 2013/10/15 18:00:15, newt wrote:
> nit: convertView -> layout

Done.

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:101:
LayoutInflater inflater = LayoutInflater.from(mContext);
On 2013/10/15 18:00:15, newt wrote:
> inflater is unused

Done.

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:104:
button.setLayoutParams(new android.widget.LinearLayout.LayoutParams(
On 2013/10/15 18:00:15, newt wrote:
> to make the different types of layout params clear, write
> AbsListView.LayoutParams and LinearLayout.LayoutParams instead of importing
> AbsListView.LayoutParams.

Done.

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/ColorSuggestionListAdapter.java:106: (int)
mContext.getResources().getDimension(R.dimen.color_button_height),
On 2013/10/15 18:00:15, newt wrote:
> nit: get this value once outside of the loop

Done.

[newt (away), 2013-10-18 22:26:58]

keishi, did you mean to upload another patch set?

[keishi, 2013-10-21 17:26:58]

On 2013/10/18 22:26:58, newt wrote:
> keishi, did you mean to upload another patch set?

I'm sorry here is the new patch set.

[newt (away), 2013-10-22 04:04:47]

LGTM!

Thanks so much Keishi. Looking forward to seeing this in action :)

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/layo...
File ui/android/java/res/layout/color_picker_dialog_content.xml (right):

https://codereview.chromium.org/23026006/diff/144001/ui/android/java/res/layo...
ui/android/java/res/layout/color_picker_dialog_content.xml:16:
android:layout_height="wrap_content">
On 2013/10/16 04:47:23, keishi1 wrote:
> On 2013/10/15 18:00:15, newt wrote:
> > Repeating this so it doesn't get lost: what does the advanced picker look
like
> > on a small screen in landscape after
> > changing the layout_height to wrap_content? Do things look OK, or does the
> > dialog go off the edge of the screen?
> > 
> > You could test this by increasing the side of the advanced color picker
(make
> > the rows tall, or the font big, etc).
> 
> Great idea. I tested this and it worked properly. The scroll view was as tall
as
> it could be and didn't go off the edge.

Great. Thanks for checking.

[Miguel Garcia, 2013-11-06 17:43:24]

lgtm

[keishi, 2013-11-07 12:47:02]

+joth Could you review the change to ColorChooserAndroid.java?
+primiano Could you review the change to R.java?
+ben Could you review the changes to the files below. We will be passing a list
of suggestions when opening the color chooser, but we will only use it on
android.

apps/shell_window.cc
apps/shell_window.h
chrome/browser/devtools/devtools_window.cc
chrome/browser/devtools/devtools_window.h
chrome/browser/extensions/extension_host.cc
chrome/browser/extensions/extension_host.h
chrome/browser/ui/browser.cc
chrome/browser/ui/browser.h
chrome/browser/ui/views/external_tab_container_win.cc
chrome/browser/ui/views/external_tab_container_win.h
content/browser/web_contents/web_contents_impl.cc
content/browser/web_contents/web_contents_impl.h

+jam could you review the changes to the files below. We want to pass a list of
suggestions from the renderer to the dialog in the browser process.
color_suggestion.h defines a new struct that contains the information for one
suggestion.
content/browser/web_contents/web_contents_impl.cc
content/browser/web_contents/web_contents_impl.h
content/common/view_messages.h
content/content_common.gypi
content/public/browser/web_contents_delegate.cc
content/public/browser/web_contents_delegate.h
content/public/common/color_suggestion.cc
content/public/common/color_suggestion.h
content/renderer/render_view_impl.cc
content/renderer/render_view_impl.h
content/renderer/renderer_webcolorchooser_impl.cc
content/renderer/renderer_webcolorchooser_impl.h

Background:
Datalist suggestions are defined by the web author like this.
<input type=color list=usa>
<datalist id=usa>
<option label="stripe">#ff0000</option>
<option>#ffffff</option>
<option>#0000ff</option>
</datalist>

[Primiano Tucci (use gerrit), 2013-11-07 18:09:23]

LGTM. Thanks for the cc.

[jam, 2013-11-07 18:51:55]

lgtm

https://codereview.chromium.org/23026006/diff/275001/content/public/common/co...
File content/public/common/color_suggestion.h (right):

https://codereview.chromium.org/23026006/diff/275001/content/public/common/co...
content/public/common/color_suggestion.h:8: #include <vector>
nit: not needed

https://codereview.chromium.org/23026006/diff/275001/content/public/common/co...
content/public/common/color_suggestion.h:20: 
nti: extra line

https://codereview.chromium.org/23026006/diff/275001/content/public/common/co...
content/public/common/color_suggestion.h:22: explicit ColorSuggestion(const
WebKit::WebColorSuggestion& suggestion);
can you inline both of these constructors and then we don't need the .cc file?

https://codereview.chromium.org/23026006/diff/275001/content/renderer/rendere...
File content/renderer/renderer_webcolorchooser_impl.h (right):

https://codereview.chromium.org/23026006/diff/275001/content/renderer/rendere...
content/renderer/renderer_webcolorchooser_impl.h:8: #include <vector>
nit: blank line after this per style guide

[keishi, 2013-11-13 10:48:11]

https://codereview.chromium.org/23026006/diff/275001/content/public/common/co...
File content/public/common/color_suggestion.h (right):

https://codereview.chromium.org/23026006/diff/275001/content/public/common/co...
content/public/common/color_suggestion.h:8: #include <vector>
On 2013/11/07 18:51:57, jam wrote:
> nit: not needed

Done.

https://codereview.chromium.org/23026006/diff/275001/content/public/common/co...
content/public/common/color_suggestion.h:20: 
On 2013/11/07 18:51:57, jam wrote:
> nti: extra line

Done.

https://codereview.chromium.org/23026006/diff/275001/content/public/common/co...
content/public/common/color_suggestion.h:22: explicit ColorSuggestion(const
WebKit::WebColorSuggestion& suggestion);
On 2013/11/07 18:51:57, jam wrote:
> can you inline both of these constructors and then we don't need the .cc file?

I tried to do this but couldn't. When I had to change the include statements in
WebColorSuggestion.h to absolute paths, I needed to add blink as a dependency to
a bunch of targets. I had to move the #include for WebColorSuggestion.h to the
cc file so I only have to add the dependency to blink to content_common.gypi

https://codereview.chromium.org/23026006/diff/275001/content/renderer/rendere...
File content/renderer/renderer_webcolorchooser_impl.h (right):

https://codereview.chromium.org/23026006/diff/275001/content/renderer/rendere...
content/renderer/renderer_webcolorchooser_impl.h:8: #include <vector>
On 2013/11/07 18:51:57, jam wrote:
> nit: blank line after this per style guide

Done.

[keishi, 2013-11-20 11:20:38]

joth@ ben@ Please take a look. Thanks.

[Ben Goodger (Google), 2013-11-20 18:45:39]

On 2013/11/20 11:20:38, keishi1 wrote:
> joth@ ben@ Please take a look. Thanks.

Which files do you want me to review?

[joth, 2013-11-20 18:57:26]

On 2013/11/07 12:47:02, keishi1 wrote:
> +joth Could you review the change to ColorChooserAndroid.java?

components/web_contents_delegate_android lgtm

> +primiano Could you review the change to R.java?
> +ben Could you review the changes to the files below. We will be passing a
list
> of suggestions when opening the color chooser, but we will only use it on
> android.
> 
> apps/shell_window.cc
> apps/shell_window.h
> chrome/browser/devtools/devtools_window.cc
> chrome/browser/devtools/devtools_window.h
> chrome/browser/extensions/extension_host.cc
> chrome/browser/extensions/extension_host.h
> chrome/browser/ui/browser.cc
> chrome/browser/ui/browser.h
> chrome/browser/ui/views/external_tab_container_win.cc
> chrome/browser/ui/views/external_tab_container_win.h
> content/browser/web_contents/web_contents_impl.cc
> content/browser/web_contents/web_contents_impl.h
> 
> +jam could you review the changes to the files below. We want to pass a list
of
> suggestions from the renderer to the dialog in the browser process.
> color_suggestion.h defines a new struct that contains the information for one
> suggestion.
> content/browser/web_contents/web_contents_impl.cc
> content/browser/web_contents/web_contents_impl.h
> content/common/view_messages.h
> content/content_common.gypi
> content/public/browser/web_contents_delegate.cc
> content/public/browser/web_contents_delegate.h
> content/public/common/color_suggestion.cc
> content/public/common/color_suggestion.h
> content/renderer/render_view_impl.cc
> content/renderer/render_view_impl.h
> content/renderer/renderer_webcolorchooser_impl.cc
> content/renderer/renderer_webcolorchooser_impl.h
> 
> Background:
> Datalist suggestions are defined by the web author like this.
> <input type=color list=usa>
> <datalist id=usa>
> <option label="stripe">#ff0000</option>
> <option>#ffffff</option>
> <option>#0000ff</option>
> </datalist>

[Ben Goodger (Google), 2013-11-20 21:44:53]

lgtm for those files.

FTR, I wish we would invent some way of isolating features like this from the
core "multiprocess view" interface. It's insane that every time we want to tweak
parameters to one of these functions we have to wire it up through so many
layers.

[commit-bot: I haz the power, 2013-11-21 13:38:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/565001

[commit-bot: I haz the power, 2013-11-21 13:57:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[keishi, 2013-11-21 14:07:26]

kenrb@ Hi. Could your review the change to view_messages.h? Thanks.

[kenrb, 2013-11-22 20:37:48]

lgtm

[commit-bot: I haz the power, 2013-11-25 03:37:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/565001

[commit-bot: I haz the power, 2013-11-25 03:55:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-25 06:00:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/565001

[commit-bot: I haz the power, 2013-11-25 06:15:29]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-25 07:33:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/565001

[commit-bot: I haz the power, 2013-11-25 07:49:41]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-25 11:45:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/565001

[commit-bot: I haz the power, 2013-11-25 11:59:37]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-26 02:03:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/565001

[commit-bot: I haz the power, 2013-11-26 02:18:43]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-27 07:08:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/565001

[commit-bot: I haz the power, 2013-11-27 07:22:13]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-29 02:47:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/935001

[commit-bot: I haz the power, 2013-11-29 03:14:52]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-11-29 05:10:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/955001

[commit-bot: I haz the power, 2013-11-29 05:34:53]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-11-29 06:33:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/23026006/955001

[commit-bot: I haz the power, 2013-11-29 06:34:10]

Change committed as 237890