Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11316153:
implement input type=color for android (Closed)
|
Created:
8 years ago by Miguel Garcia Modified:
7 years, 2 months ago CC:
joth, chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen+watch_chromium.org, keishi1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement input type=color for android
Note that the feature is still disabled in WebKit and it will remain so until we finalize the UI.
TBR=thakis
BUG=135771
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170517
Patch Set 1 #
Total comments: 1
Patch Set 2 : #Patch Set 3 : #
Total comments: 31
Patch Set 4 : Fix a FindBug error #
Total comments: 37
Patch Set 5 : #Patch Set 6 : #
Total comments: 24
Patch Set 7 : #
Total comments: 6
Patch Set 8 : #
Total comments: 9
Patch Set 9 : #
Total comments: 24
Patch Set 10 : #Patch Set 11 : #Messages
Total messages: 35 (0 generated)
https://codereview.chromium.org/11316153/diff/1/ui/android/java/src/org/chrom... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/1/ui/android/java/src/org/chrom... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:5: package org.chromium.ui; This is an initial UI heavily based on the color picker that comes as an example on the android SDK documentation. The final UI will need to wait for UX review.
Hi Miguel, thanks for the patch! I haven't thoroughly reviewed the implementation of the color picker itself as I'm not sure whether we should land it in this shape. Please let me know what you think. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:15: SkColor initial_color) : nit: the colon should be on the next line, two spaces indented, with the WebContentsObserver call being on the line below that. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:33: void ColorChooserAndroid::End() { When the WebContentsImpl gets destroyed, it will call the End() method. This could happen when, for example, we're seeing a renderer crash. It can probably also get called when a navigation event occurs. This method should probably be implemented to closer the native-side color picker even if the user hasn't made a choice yet. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:36: void ColorChooserAndroid::SetSelectedColor(SkColor color) { This will also be used for setting the current color in the picker (i.e. <input type=color value=#ff00ff />. Should this have a TODO? https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:56: } nit: empty line above here, and add a "// namespace content" comment. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:6: #define CHROME_BROWSER_COMPONENT_WEB_CONTENTS_DELEGATE_ANDROID_COLOR_CHOOSER_ANDROID_H_ These should be CONTENT_COMPONENTS_WEB_CONTENTS_DELEGATE_ANDROID_COLOR_CHOOSER_ANDROID_H_. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:32: private: nit: empty line above private:. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:40: bool RegisterColorChooserAndroid(JNIEnv* env); nit: no indenting here. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... File content/components/web_contents_delegate_android/web_contents_delegate_android.cc (right): https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/web_contents_delegate_android.cc:56: nit: only a single blank line. https://codereview.chromium.org/11316153/diff/4001/content/content_components... File content/content_components_web_contents_delegate_android.gypi (right): https://codereview.chromium.org/11316153/diff/4001/content/content_components... content/content_components_web_contents_delegate_android.gypi:31: 'components/web_contents_delegate_android/color_chooser_android.h', My alphabet lists "l" before "m", and "c" before "h" :-). https://codereview.chromium.org/11316153/diff/4001/content/public/browser/web... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/11316153/diff/4001/content/public/browser/web... content/public/browser/web_contents_delegate.h:319: // ownership of the returned pointer is transferred to the caller. nit: Capital O. https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:32: private static final int CONTAINER_R = 100; BOUNDING_BOX_EDGE maybe? https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:46: mListener = listener; Should we add a TODO about this not being the final UI? Maybe it shouldn't be checked in altogether, and we should have a stub for the ColorPickerView here? I'm going to skip thoroughly reviewing this file for now. https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:100: private int ave(int s, int d, float p) { This calculates the position between [s]tart and [d]estination for [p]osition, which seems to be [0, 1]. Should we maybe call this interpolate(int start, int end, float interpolant)? https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:165: mTrackingCenter = false; // so we draw w/o halo nit: please use whole sentences for comments. https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:201: new ColorPickerView(getContext(), listener, mInitialColor)); nit: this would fit on a single line.
Some minor nits on the title and description: Maybe a description like "Implement a color picker for Android" would be more accurate, as <input type=color> is already implemented?
thanks miguel! this will be a great "demo" on how to implement a feature in content shell... I'll be a bit late in the office and haven't finished this yet, but a few small comments and suggestions so far: https://codereview.chromium.org/11316153/diff/3006/chrome/browser/android/chr... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/11316153/diff/3006/chrome/browser/android/chr... chrome/browser/android/chrome_jni_registrar.cc:20: #include "content/components/web_contents_delegate_android//color_chooser_android.h" nit: remove extra / before color https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:27: Java_ColorChooserAndroid_openColorChooser(env, j_color_chooser_.obj()); here and 22 is crossing twice the jni boundaries to the same class in the java side.. probably best to bundle them together and call it only once.. https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:55: } nit: add a \n https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:56: } nit: // namespace content https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:5: #ifndef CHROME_BROWSER_COMPONENT_WEB_CONTENTS_DELEGATE_ANDROID_COLOR_CHOOSER_ANDROID_H_ nit: s/CHROME_BROWSER_COMPONENT/CONTENT_COMPONENTS/ https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:31: void OnColorChosen(JNIEnv* env, jobject obj, jint color); nit: order is normally: statics, ctor/dtor, non-virtuals, virtuals... move this up to 26.. https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:40: bool RegisterColorChooserAndroid(JNIEnv* env); nit: unindent https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:41: } nit: // namespace content
cool, just some more comments after a full read.. besides nits, the more important part is about decoupling the dialog / builder from the logic, so that we can test it appropriately: https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... File content/components/web_contents_delegate_android/web_contents_delegate_android.h (right): https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/web_contents_delegate_android.h:59: content::WebContents* web_contents, int color_chooser_id, nit: from the methods around here, "content::WebContents* web_contents" is normally "WebContents* source".. https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:37: private OnColorChangedListener mListener; nit: looks like 34-37 can all be made final https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:51: Shader s = new SweepGradient(0, 0, mColors, null); nit: "shader" is probably better than "s" https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:68: center_y = getHeight() / 2; I'm not super familiar here, but what would happen if the orientation changes? should we really cache these? maybe best to use canvas.getWidth() / height directly? https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:70: float r = CONTAINER_R - mPaint.getStrokeWidth()*0.5f; nit: spaces around * https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:78: int c = mCenterPaint.getColor(); nit: int color https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:100: private int ave(int s, int d, float p) { nit: maybe more explicit than "ave"? https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:104: private int interpColor(int colors[], float unit) { nit: interpret? https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:133: boolean inCenter = java.lang.Math.sqrt(x*x + y*y) <= CENTER_RADIUS; nit: spaces around "*" here and 152 https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:156: mCenterPaint.setColor(interpColor(mColors, unit)); this is the only caller to "interpColor"... it'd be easier to test if 150-155 were part of interpColor: it'd take x, y instead of unit, and it could be made static and package protected, so that it could be tested in isolation... wdyt?
https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:15: SkColor initial_color) : On 2012/11/23 17:55:48, Peter Beverloo wrote: > nit: the colon should be on the next line, two spaces indented, with the > WebContentsObserver call being on the line below that. Done. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:56: } On 2012/11/23 17:55:48, Peter Beverloo wrote: > nit: empty line above here, and add a "// namespace content" comment. Done. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:6: #define CHROME_BROWSER_COMPONENT_WEB_CONTENTS_DELEGATE_ANDROID_COLOR_CHOOSER_ANDROID_H_ On 2012/11/23 17:55:48, Peter Beverloo wrote: > These should be > CONTENT_COMPONENTS_WEB_CONTENTS_DELEGATE_ANDROID_COLOR_CHOOSER_ANDROID_H_. Done. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:32: private: On 2012/11/23 17:55:48, Peter Beverloo wrote: > nit: empty line above private:. Done. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:40: bool RegisterColorChooserAndroid(JNIEnv* env); On 2012/11/23 17:55:48, Peter Beverloo wrote: > nit: no indenting here. Done. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... File content/components/web_contents_delegate_android/web_contents_delegate_android.cc (right): https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/web_contents_delegate_android.cc:56: On 2012/11/23 17:55:48, Peter Beverloo wrote: > nit: only a single blank line. Done. https://codereview.chromium.org/11316153/diff/4001/content/content_components... File content/content_components_web_contents_delegate_android.gypi (right): https://codereview.chromium.org/11316153/diff/4001/content/content_components... content/content_components_web_contents_delegate_android.gypi:31: 'components/web_contents_delegate_android/color_chooser_android.h', On 2012/11/23 17:55:48, Peter Beverloo wrote: > My alphabet lists "l" before "m", and "c" before "h" :-). Done. https://codereview.chromium.org/11316153/diff/4001/content/public/browser/web... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/11316153/diff/4001/content/public/browser/web... content/public/browser/web_contents_delegate.h:319: // ownership of the returned pointer is transferred to the caller. On 2012/11/23 17:55:48, Peter Beverloo wrote: > nit: Capital O. Done. https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:32: private static final int CONTAINER_R = 100; On 2012/11/23 17:55:48, Peter Beverloo wrote: > BOUNDING_BOX_EDGE maybe? Done. https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:46: mListener = listener; Peter and I followed up offline and decided that the best way would be to commit this class in a different CL to make it very clear the part that is final (all gluing) and what part is temporary (pending UI review). On 2012/11/23 17:55:48, Peter Beverloo wrote: > Should we add a TODO about this not being the final UI? Maybe it shouldn't be > checked in altogether, and we should have a stub for the ColorPickerView here? > I'm going to skip thoroughly reviewing this file for now. https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:100: private int ave(int s, int d, float p) { it's actually [0,1) but yeah it's a normal interpolation, it's also called from interpColor so I am going to rename that one as well to be consistent On 2012/11/23 17:55:48, Peter Beverloo wrote: > This calculates the position between [s]tart and [d]estination for [p]osition, > which seems to be [0, 1]. Should we maybe call this interpolate(int start, int > end, float interpolant)? https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:165: mTrackingCenter = false; // so we draw w/o halo On 2012/11/23 17:55:48, Peter Beverloo wrote: > nit: please use whole sentences for comments. Done. https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:201: new ColorPickerView(getContext(), listener, mInitialColor)); On 2012/11/23 17:55:48, Peter Beverloo wrote: > nit: this would fit on a single line. Done. https://codereview.chromium.org/11316153/diff/3006/chrome/browser/android/chr... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/11316153/diff/3006/chrome/browser/android/chr... chrome/browser/android/chrome_jni_registrar.cc:20: #include "content/components/web_contents_delegate_android//color_chooser_android.h" On 2012/11/26 09:05:32, bulach wrote: > nit: remove extra / before color Done. https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:27: Java_ColorChooserAndroid_openColorChooser(env, j_color_chooser_.obj()); I really wanted to split the creation of the object with the action of displaying it. I've seen that a few times already on both WebKit and Chrome and I think it's not a great idea. In this case I can see the crossing the boundaries argument so I guess I'll go with it. On 2012/11/26 09:05:32, bulach wrote: > here and 22 is crossing twice the jni boundaries to the same class in the java > side.. probably best to bundle them together and call it only once.. https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:55: } On 2012/11/26 09:05:32, bulach wrote: > nit: add a \n Done. https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:56: } On 2012/11/26 09:05:32, bulach wrote: > nit: // namespace content Done. https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:5: #ifndef CHROME_BROWSER_COMPONENT_WEB_CONTENTS_DELEGATE_ANDROID_COLOR_CHOOSER_ANDROID_H_ On 2012/11/26 09:05:32, bulach wrote: > nit: s/CHROME_BROWSER_COMPONENT/CONTENT_COMPONENTS/ Done. https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:31: void OnColorChosen(JNIEnv* env, jobject obj, jint color); On 2012/11/26 09:05:32, bulach wrote: > nit: order is normally: statics, ctor/dtor, non-virtuals, virtuals... move this > up to 26.. Done. https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:40: bool RegisterColorChooserAndroid(JNIEnv* env); On 2012/11/26 09:05:32, bulach wrote: > nit: unindent Done. https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:41: } On 2012/11/26 09:05:32, bulach wrote: > nit: // namespace content Done.
https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:46: mListener = listener; On 2012/11/23 17:55:48, Peter Beverloo wrote: > Should we add a TODO about this not being the final UI? Maybe it shouldn't be > checked in altogether, and we should have a stub for the ColorPickerView here? > I'm going to skip thoroughly reviewing this file for now. agree on adding a TODO / ensuring the patch description is clear about this being temporray, but it'd be great to check in a working stub.. :) since this is already in, seems better to just go ahead with it and change it later to the final ui..
TODO added, also linking it to the UI bug. I agree it's nice to have a working stub checked in and it's easy to change once we get the final UI. On 2012/11/26 11:39:16, bulach wrote: > https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... > File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): > > https://codereview.chromium.org/11316153/diff/4001/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:46: mListener = > listener; > On 2012/11/23 17:55:48, Peter Beverloo wrote: > > Should we add a TODO about this not being the final UI? Maybe it shouldn't be > > checked in altogether, and we should have a stub for the ColorPickerView here? > > I'm going to skip thoroughly reviewing this file for now. > > agree on adding a TODO / ensuring the patch description is clear about this > being temporray, but it'd be great to check in a working stub.. :) since this is > already in, seems better to just go ahead with it and change it later to the > final ui..
https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... File content/components/web_contents_delegate_android/web_contents_delegate_android.h (right): https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... content/components/web_contents_delegate_android/web_contents_delegate_android.h:59: content::WebContents* web_contents, int color_chooser_id, In the .cc file sometimes is web_contents sometimes it's source :) On 2012/11/26 11:29:32, bulach wrote: > nit: from the methods around here, "content::WebContents* web_contents" is > normally "WebContents* source".. https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:37: private OnColorChangedListener mListener; On 2012/11/26 11:29:32, bulach wrote: > nit: looks like 34-37 can all be made final Done. https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:51: Shader s = new SweepGradient(0, 0, mColors, null); On 2012/11/26 11:29:32, bulach wrote: > nit: "shader" is probably better than "s" Done. https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:68: center_y = getHeight() / 2; Since the circle is fully centered it actually works fine as is. I tested it both on a table and a phone without issues. On 2012/11/26 11:29:32, bulach wrote: > I'm not super familiar here, but what would happen if the orientation changes? > should we really cache these? maybe best to use canvas.getWidth() / height > directly? https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:70: float r = CONTAINER_R - mPaint.getStrokeWidth()*0.5f; On 2012/11/26 11:29:32, bulach wrote: > nit: spaces around * Done. https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:70: float r = CONTAINER_R - mPaint.getStrokeWidth()*0.5f; On 2012/11/26 11:29:32, bulach wrote: > nit: spaces around * Done. https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:78: int c = mCenterPaint.getColor(); On 2012/11/26 11:29:32, bulach wrote: > nit: int color Done. https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:100: private int ave(int s, int d, float p) { On 2012/11/26 11:29:32, bulach wrote: > nit: maybe more explicit than "ave"? This was changed to interpolate per Peter's suggestion https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:104: private int interpColor(int colors[], float unit) { On 2012/11/26 11:29:32, bulach wrote: > nit: interpret? I changed it to interpolateColor which seems to be more accurate https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:133: boolean inCenter = java.lang.Math.sqrt(x*x + y*y) <= CENTER_RADIUS; On 2012/11/26 11:29:32, bulach wrote: > nit: spaces around "*" here and 152 Done. https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:156: mCenterPaint.setColor(interpColor(mColors, unit)); That's a great idea, done On 2012/11/26 11:29:32, bulach wrote: > this is the only caller to "interpColor"... it'd be easier to test if 150-155 > were part of interpColor: it'd take x, y instead of unit, and it could be made > static and package protected, so that it could be tested in isolation... wdyt?
Thanks for the review! Ready for another go when you have a chance. On 2012/11/26 13:25:57, Miguel Garcia wrote: > https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... > File > content/components/web_contents_delegate_android/web_contents_delegate_android.h > (right): > > https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... > content/components/web_contents_delegate_android/web_contents_delegate_android.h:59: > content::WebContents* web_contents, int color_chooser_id, > In the .cc file sometimes is web_contents sometimes it's source :) > > On 2012/11/26 11:29:32, bulach wrote: > > nit: from the methods around here, "content::WebContents* web_contents" is > > normally "WebContents* source".. > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:37: private > OnColorChangedListener mListener; > On 2012/11/26 11:29:32, bulach wrote: > > nit: looks like 34-37 can all be made final > > Done. > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:51: Shader s = new > SweepGradient(0, 0, mColors, null); > On 2012/11/26 11:29:32, bulach wrote: > > nit: "shader" is probably better than "s" > > Done. > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:68: center_y = > getHeight() / 2; > Since the circle is fully centered it actually works fine as is. I tested it > both on a table and a phone without issues. > > > On 2012/11/26 11:29:32, bulach wrote: > > I'm not super familiar here, but what would happen if the orientation changes? > > should we really cache these? maybe best to use canvas.getWidth() / height > > directly? > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:70: float r = > CONTAINER_R - mPaint.getStrokeWidth()*0.5f; > On 2012/11/26 11:29:32, bulach wrote: > > nit: spaces around * > > Done. > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:70: float r = > CONTAINER_R - mPaint.getStrokeWidth()*0.5f; > On 2012/11/26 11:29:32, bulach wrote: > > nit: spaces around * > > Done. > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:78: int c = > mCenterPaint.getColor(); > On 2012/11/26 11:29:32, bulach wrote: > > nit: int color > > Done. > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:100: private int > ave(int s, int d, float p) { > On 2012/11/26 11:29:32, bulach wrote: > > nit: maybe more explicit than "ave"? > > This was changed to interpolate per Peter's suggestion > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:104: private int > interpColor(int colors[], float unit) { > On 2012/11/26 11:29:32, bulach wrote: > > nit: interpret? > > I changed it to interpolateColor which seems to be more accurate > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:133: boolean inCenter > = java.lang.Math.sqrt(x*x + y*y) <= CENTER_RADIUS; > On 2012/11/26 11:29:32, bulach wrote: > > nit: spaces around "*" here and 152 > > Done. > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:156: > mCenterPaint.setColor(interpColor(mColors, unit)); > That's a great idea, done > > > On 2012/11/26 11:29:32, bulach wrote: > > this is the only caller to "interpColor"... it'd be easier to test if 150-155 > > were part of interpColor: it'd take x, y instead of unit, and it could be made > > static and package protected, so that it could be tested in isolation... wdyt?
Let me know if there is anything else you need here or if I can hit commit ok? On 2012/11/26 18:20:33, Miguel Garcia wrote: > Thanks for the review! > > Ready for another go when you have a chance. > > On 2012/11/26 13:25:57, Miguel Garcia wrote: > > > https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... > > File > > > content/components/web_contents_delegate_android/web_contents_delegate_android.h > > (right): > > > > > https://codereview.chromium.org/11316153/diff/3006/content/components/web_con... > > > content/components/web_contents_delegate_android/web_contents_delegate_android.h:59: > > content::WebContents* web_contents, int color_chooser_id, > > In the .cc file sometimes is web_contents sometimes it's source :) > > > > On 2012/11/26 11:29:32, bulach wrote: > > > nit: from the methods around here, "content::WebContents* web_contents" is > > > normally "WebContents* source".. > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:37: private > > OnColorChangedListener mListener; > > On 2012/11/26 11:29:32, bulach wrote: > > > nit: looks like 34-37 can all be made final > > > > Done. > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:51: Shader s = new > > SweepGradient(0, 0, mColors, null); > > On 2012/11/26 11:29:32, bulach wrote: > > > nit: "shader" is probably better than "s" > > > > Done. > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:68: center_y = > > getHeight() / 2; > > Since the circle is fully centered it actually works fine as is. I tested it > > both on a table and a phone without issues. > > > > > > On 2012/11/26 11:29:32, bulach wrote: > > > I'm not super familiar here, but what would happen if the orientation > changes? > > > should we really cache these? maybe best to use canvas.getWidth() / height > > > directly? > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:70: float r = > > CONTAINER_R - mPaint.getStrokeWidth()*0.5f; > > On 2012/11/26 11:29:32, bulach wrote: > > > nit: spaces around * > > > > Done. > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:70: float r = > > CONTAINER_R - mPaint.getStrokeWidth()*0.5f; > > On 2012/11/26 11:29:32, bulach wrote: > > > nit: spaces around * > > > > Done. > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:78: int c = > > mCenterPaint.getColor(); > > On 2012/11/26 11:29:32, bulach wrote: > > > nit: int color > > > > Done. > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:100: private int > > ave(int s, int d, float p) { > > On 2012/11/26 11:29:32, bulach wrote: > > > nit: maybe more explicit than "ave"? > > > > This was changed to interpolate per Peter's suggestion > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:104: private int > > interpColor(int colors[], float unit) { > > On 2012/11/26 11:29:32, bulach wrote: > > > nit: interpret? > > > > I changed it to interpolateColor which seems to be more accurate > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:133: boolean > inCenter > > = java.lang.Math.sqrt(x*x + y*y) <= CENTER_RADIUS; > > On 2012/11/26 11:29:32, bulach wrote: > > > nit: spaces around "*" here and 152 > > > > Done. > > > > > https://codereview.chromium.org/11316153/diff/3006/ui/android/java/src/org/ch... > > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:156: > > mCenterPaint.setColor(interpColor(mColors, unit)); > > That's a great idea, done > > > > > > On 2012/11/26 11:29:32, bulach wrote: > > > this is the only caller to "interpColor"... it'd be easier to test if > 150-155 > > > were part of interpColor: it'd take x, y instead of unit, and it could be > made > > > static and package protected, so that it could be tested in isolation... > wdyt?
Three more spacing nits and a reference to comments in PS3. The native part looks good to me otherwise, but awaiting comments. You'll need to get OWNERS approval too. https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:32: } Because of the PS3/PS4 confusion you may have missed my comments regarding this file. I'd appreciate it if you could take a look in PS3. https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:55: } // namespace content nit: two spaces before //. https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:40: bool RegisterColorChooserAndroid(JNIEnv* env); nit: newline after this. https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:41: } // namespace content nit: two spaces before // (seems to be more common).
some drive-by comments. https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:16: : ColorChooser::ColorChooser(identifier), nit: wrong indent. 4 spaces https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:19: class ColorChooserAndroid : public content::ColorChooser, class comment. Interestingly, this class isn't really intended for the component consumer to include, it's an internal implementation detail of the web_contents_delegate (cpp). Maybe we need to add a pubilc/ and/or internal/ sub-dir inside this folder? https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:20: public content::WebContentsObserver { your not actually overriding any WCO methods, which seems unusual but possibly not bad. (esp as this object has short lifetime) OTOH, you're only using this to have a WebContents* to callback on. I think a better solution would be to have the WebContents ensure it calls End() before it itself is destroyed, thereby meaning you can just hold a raw (weak) pointer to WC. But if this pattern is well established on all other platforms, then fine https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:21: nit: spurious \n https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:74: center_x = getWidth() / 2; in java, all on one line, or use braces plus wrong indent (several times in file) https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:196: mListener.colorChanged(mInitialColor); wrong indent https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:204: OnColorChangedListener listener = new OnColorChangedListener() { not clear what this anon class is for. why not just pass mListener as the second param on line 210? https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:213: setTitle("Select Color"); will need a resource :)
https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:33: void ColorChooserAndroid::End() { On 2012/11/23 17:55:48, Peter Beverloo wrote: > When the WebContentsImpl gets destroyed, it will call the End() method. This > could happen when, for example, we're seeing a renderer crash. It can probably > also get called when a navigation event occurs. This method should probably be > implemented to closer the native-side color picker even if the user hasn't made > a choice yet. Done. https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:36: void ColorChooserAndroid::SetSelectedColor(SkColor color) { This works fine as is, sinec the initial_color is passed on the create call. I think this is used on the windows implementation only since there they don't create a picker each time but reuse the one they first create due to platform restrictions. I added a comment. I will CC kent to see if he can confirm it. On 2012/11/23 17:55:48, Peter Beverloo wrote: > This will also be used for setting the current color in the picker (i.e. <input > type=color value=#ff00ff />. Should this have a TODO? https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:32: } Done, sorry about that! On 2012/11/27 17:37:23, Peter Beverloo wrote: > Because of the PS3/PS4 confusion you may have missed my comments regarding this > file. I'd appreciate it if you could take a look in PS3. https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:55: } // namespace content On 2012/11/27 17:37:23, Peter Beverloo wrote: > nit: two spaces before //. Done. https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:40: bool RegisterColorChooserAndroid(JNIEnv* env); On 2012/11/27 17:37:23, Peter Beverloo wrote: > nit: newline after this. Done. https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:41: } // namespace content On 2012/11/27 17:37:23, Peter Beverloo wrote: > nit: two spaces before // (seems to be more common). Done.
Kent, if you have a sec would you mind having a look at content/components/web_contents_delegate_android/color_chooser_android.cc ? In particular the method SetSelectedColor is left unimplemented, I think it is ok since there is no way to set the color from C++ on android (you can only do it from Java) but I wanted to be extra sure. On 2012/11/27 18:44:30, Miguel Garcia wrote: > https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... > File content/components/web_contents_delegate_android/color_chooser_android.cc > (right): > > https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.cc:33: > void ColorChooserAndroid::End() { > On 2012/11/23 17:55:48, Peter Beverloo wrote: > > When the WebContentsImpl gets destroyed, it will call the End() method. This > > could happen when, for example, we're seeing a renderer crash. It can probably > > also get called when a navigation event occurs. This method should probably be > > implemented to closer the native-side color picker even if the user hasn't > made > > a choice yet. > > Done. > > https://codereview.chromium.org/11316153/diff/4001/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.cc:36: > void ColorChooserAndroid::SetSelectedColor(SkColor color) { > This works fine as is, sinec the initial_color is passed on the create call. I > think this is used on the windows implementation only since there they don't > create a picker each time but reuse the one they first create due to platform > restrictions. I added a comment. I will CC kent to see if he can confirm it. > > > On 2012/11/23 17:55:48, Peter Beverloo wrote: > > This will also be used for setting the current color in the picker (i.e. > <input > > type=color value=#ff00ff />. Should this have a TODO? > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > File content/components/web_contents_delegate_android/color_chooser_android.cc > (right): > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.cc:32: } > Done, sorry about that! > > On 2012/11/27 17:37:23, Peter Beverloo wrote: > > Because of the PS3/PS4 confusion you may have missed my comments regarding > this > > file. I'd appreciate it if you could take a look in PS3. > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.cc:55: } > // namespace content > On 2012/11/27 17:37:23, Peter Beverloo wrote: > > nit: two spaces before //. > > Done. > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > File content/components/web_contents_delegate_android/color_chooser_android.h > (right): > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.h:40: > bool RegisterColorChooserAndroid(JNIEnv* env); > On 2012/11/27 17:37:23, Peter Beverloo wrote: > > nit: newline after this. > > Done. > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.h:41: } > // namespace content > On 2012/11/27 17:37:23, Peter Beverloo wrote: > > nit: two spaces before // (seems to be more common). > > Done.
> In particular the method SetSelectedColor is left unimplemented, I think it is > ok since there is no way to set the color from C++ on android (you can only do > it from Java) but I wanted to be extra sure. SetSelectedColor is called when a JavaScript code in a page updates the color value of input[type=color] during showing a color chooser. I think it's ok to ignore SetSelectedColor because updating the selected color in a color chooser dialog without user action might confuse a user.
https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:74: center_x = getWidth() / 2; I don't know Java style in Chromium, but the indentation of this line looks inconsistent. https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:76: center_y = getHeight() / 2; Ditto/ https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:116: unit += 1; Ditto.
https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:16: : ColorChooser::ColorChooser(identifier), On 2012/11/27 18:44:07, joth wrote: > nit: wrong indent. 4 spaces Done. https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:19: class ColorChooserAndroid : public content::ColorChooser, The gtk port declares the internal class inside the .cc file directly. I thought it'd be clearer to declare it in its own file but I can certainly move it to an internal subdir if you prefer. Can I ask to do this on a second cl? On 2012/11/27 18:44:07, joth wrote: > class comment. > > Interestingly, this class isn't really intended for the component consumer to > include, it's an internal implementation detail of the web_contents_delegate > (cpp). Maybe we need to add a pubilc/ and/or internal/ sub-dir inside this > folder? https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:20: public content::WebContentsObserver { So this is the pattern used on both the gtk and the windows port so I'd rather stick to it. On 2012/11/27 18:44:07, joth wrote: > your not actually overriding any WCO methods, which seems unusual but possibly > not bad. (esp as this object has short lifetime) > > OTOH, you're only using this to have a WebContents* to callback on. I think a > better solution would be to have the WebContents ensure it calls End() before it > itself is destroyed, thereby meaning you can just hold a raw (weak) pointer to > WC. But if this pattern is well established on all other platforms, then fine https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:21: On 2012/11/27 18:44:07, joth wrote: > nit: spurious \n Done. https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:74: center_x = getWidth() / 2; On 2012/11/27 18:44:07, joth wrote: > in java, all on one line, or use braces > > plus wrong indent (several times in file) Done. https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:196: mListener.colorChanged(mInitialColor); On 2012/11/27 18:44:07, joth wrote: > wrong indent Done. https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:204: OnColorChangedListener listener = new OnColorChangedListener() { You are totally right, removed it now. On 2012/11/27 18:44:07, joth wrote: > not clear what this anon class is for. why not just pass mListener as the second > param on line 210? https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:213: setTitle("Select Color"); indeed :) On 2012/11/27 18:44:07, joth wrote: > will need a resource :) https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:74: center_x = getWidth() / 2; On 2012/11/28 01:09:45, Kent Tamura wrote: > I don't know Java style in Chromium, but the indentation of this line looks > inconsistent. Done. https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:76: center_y = getHeight() / 2; On 2012/11/28 01:09:45, Kent Tamura wrote: > Ditto/ Done. https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:116: unit += 1; On 2012/11/28 01:09:45, Kent Tamura wrote: > Ditto. Done.
Adding joth for OWNERS of content/components/web_contents_delegate_android/ and joi for OWNERS of content/public/browser/ On 2012/11/28 11:35:44, Miguel Garcia wrote: > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > File content/components/web_contents_delegate_android/color_chooser_android.cc > (right): > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.cc:16: : > ColorChooser::ColorChooser(identifier), > On 2012/11/27 18:44:07, joth wrote: > > nit: wrong indent. 4 spaces > > Done. > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > File content/components/web_contents_delegate_android/color_chooser_android.h > (right): > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.h:19: > class ColorChooserAndroid : public content::ColorChooser, > The gtk port declares the internal class inside the .cc file directly. I thought > it'd be clearer to declare it in its own file but I can certainly move it to an > internal subdir if you prefer. Can I ask to do this on a second cl? > > On 2012/11/27 18:44:07, joth wrote: > > class comment. > > > > Interestingly, this class isn't really intended for the component consumer to > > include, it's an internal implementation detail of the web_contents_delegate > > (cpp). Maybe we need to add a pubilc/ and/or internal/ sub-dir inside this > > folder? > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.h:20: > public content::WebContentsObserver { > So this is the pattern used on both the gtk and the windows port so I'd rather > stick to it. > > > On 2012/11/27 18:44:07, joth wrote: > > your not actually overriding any WCO methods, which seems unusual but possibly > > not bad. (esp as this object has short lifetime) > > > > OTOH, you're only using this to have a WebContents* to callback on. I think a > > better solution would be to have the WebContents ensure it calls End() before > it > > itself is destroyed, thereby meaning you can just hold a raw (weak) pointer to > > WC. But if this pattern is well established on all other platforms, then fine > > https://codereview.chromium.org/11316153/diff/2003/content/components/web_con... > content/components/web_contents_delegate_android/color_chooser_android.h:21: > On 2012/11/27 18:44:07, joth wrote: > > nit: spurious \n > > Done. > > https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... > File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): > > https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:74: center_x = > getWidth() / 2; > On 2012/11/27 18:44:07, joth wrote: > > in java, all on one line, or use braces > > > > plus wrong indent (several times in file) > > Done. > > https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:196: > mListener.colorChanged(mInitialColor); > On 2012/11/27 18:44:07, joth wrote: > > wrong indent > > Done. > > https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:204: > OnColorChangedListener listener = new OnColorChangedListener() { > You are totally right, removed it now. > > On 2012/11/27 18:44:07, joth wrote: > > not clear what this anon class is for. why not just pass mListener as the > second > > param on line 210? > > https://codereview.chromium.org/11316153/diff/2003/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:213: setTitle("Select > Color"); > indeed :) > > On 2012/11/27 18:44:07, joth wrote: > > will need a resource :) > > https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... > File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): > > https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:74: center_x = > getWidth() / 2; > On 2012/11/28 01:09:45, Kent Tamura wrote: > > I don't know Java style in Chromium, but the indentation of this line looks > > inconsistent. > > Done. > > https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:76: center_y = > getHeight() / 2; > On 2012/11/28 01:09:45, Kent Tamura wrote: > > Ditto/ > > Done. > > https://codereview.chromium.org/11316153/diff/2004/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:116: unit += 1; > On 2012/11/28 01:09:45, Kent Tamura wrote: > > Ditto. > > Done.
lgtm (but not an OWNERS on all files), just one suggestion for the Context parameter and some nits: https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... content/components/web_contents_delegate_android/color_chooser_android.cc:25: content_view_core->GetJavaObject().obj(), initial_color)); looks like it's only using the ContentViewCore in java to get the application context, is that correct? if so, pass directly base::android::GetApplicationContext() from base/android/jni_android.h https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... content/components/web_contents_delegate_android/color_chooser_android.h:19: /** nit: sorry for missing this earlier... c/c++ style for comments is // rather than javadoc /** * */ https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... File content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java (right): https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:20: private final ColorPickerDialog mDialog; sorry, I should've noticed this before.. java style uses 4-space indentation, and 8 for continuation.. https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:51: ContentViewCore contentViewCore, int initialColor) { see above, just pass Context directly.. https://codereview.chromium.org/11316153/diff/11007/ui/android/java/src/org/c... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/11007/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:131: // now p is just the fractional part [0...1) and i is the index ultra-nit: comments are sentences, so upper-case Now, and end with a period.
https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... content/components/web_contents_delegate_android/color_chooser_android.cc:25: content_view_core->GetJavaObject().obj(), initial_color)); On 2012/11/28 12:13:47, bulach wrote: > looks like it's only using the ContentViewCore in java to get the application > context, is that correct? > if so, pass directly base::android::GetApplicationContext() from > base/android/jni_android.h my bad: looks like dialogs need the activity context rather than the application context... so please keep as is.
https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... content/components/web_contents_delegate_android/color_chooser_android.h:19: /** On 2012/11/28 12:13:47, bulach wrote: > nit: sorry for missing this earlier... c/c++ style for comments is // rather > than javadoc /** * */ Done. https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... File content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java (right): https://codereview.chromium.org/11316153/diff/11007/content/components/web_co... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:20: private final ColorPickerDialog mDialog; On 2012/11/28 12:13:47, bulach wrote: > sorry, I should've noticed this before.. java style uses 4-space indentation, > and 8 for continuation.. Done. https://codereview.chromium.org/11316153/diff/11007/ui/android/java/src/org/c... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/11007/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:131: // now p is just the fractional part [0...1) and i is the index On 2012/11/28 12:13:47, bulach wrote: > ultra-nit: comments are sentences, so upper-case Now, and end with a period. Done.
lgtm as well, with a few indentation nits. https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... File content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java (right): https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:26: new ColorPickerDialog.OnColorChangedListener() { nit: this is a continuation, so indentation ought to be eight spaces? https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:50: int nativeColorChooserAndroid, dito. https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:55: initialColor); dito. https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:97: CENTER_RADIUS + mCenterPaint.getStrokeWidth(), nit: eight spaces for indentation of continuations. https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:188: OnColorChangedListener listener, nit: the alignment of these parameters is inconsistent with ColorChooserAndroid.java:23, but I'm not sure which is correct. I'd be inclined to say CCA.java given that this is a continuation. https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:196: @Override nit: two spaces, should be four. https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:198: mListener.colorChanged(mInitialColor); nit: three spaces, should be four.
Joth, Joi, can you have a look and lgtm if you think we are good? On 2012/11/28 17:56:58, Peter Beverloo wrote: > lgtm as well, with a few indentation nits. > > https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... > File > content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java > (right): > > https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... > content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:26: > new ColorPickerDialog.OnColorChangedListener() { > nit: this is a continuation, so indentation ought to be eight spaces? > > https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... > content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:50: > int nativeColorChooserAndroid, > dito. > > https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... > content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:55: > initialColor); > dito. > > https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... > File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): > > https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:97: CENTER_RADIUS + > mCenterPaint.getStrokeWidth(), > nit: eight spaces for indentation of continuations. > > https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:188: > OnColorChangedListener listener, > nit: the alignment of these parameters is inconsistent with > ColorChooserAndroid.java:23, but I'm not sure which is correct. I'd be inclined > to say CCA.java given that this is a continuation. > > https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:196: @Override > nit: two spaces, should be four. > > https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... > ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:198: > mListener.colorChanged(mInitialColor); > nit: three spaces, should be four.
LGTM with Peter's nits addressed. Defer to joth@ for the final review. https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:19: just one blank line
nits + 1 minor bug otherwise lgtm, no need to wait on me for another review. https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:35: Java_ColorChooserAndroid_closeColorChooser(env, j_color_chooser_.obj()); this is racy: the java object may have been GCed between lines 33 & 35. The pattern was use almost everywhere else is: JNIEnv* env = AttachCurrentThread(); ScopedJavaLocalRef<jobject> obj = java_ref_.get(env); if (obj.is_null()) return; DoSomething(obj.get()); https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:52: nit: spurious \n https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:142: private static final float PI = 3.1415926f; nit: odd placement for static final. I think it can go inside the method you use it in https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:148: boolean inCenter = java.lang.Math.sqrt(x * x + y * y) <= CENTER_RADIUS; nit, expensive sqrt easy to avoid: inCenter = (x * x + y * y) <= (CENTER_RADIUS * CENTER_RADIUS);
https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.cc (right): https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:35: Java_ColorChooserAndroid_closeColorChooser(env, j_color_chooser_.obj()); I think that'd be true if j_color_chooser_ was a weak ref to something else but in this case it's a ScopedJavaGlobalRef (owned by this object) so it cannot be released until the C++ ColorChooserAndroid class is destroyed right? On 2012/11/29 18:11:14, joth wrote: > this is racy: the java object may have been GCed between lines 33 & 35. > > The pattern was use almost everywhere else is: > > JNIEnv* env = AttachCurrentThread(); > ScopedJavaLocalRef<jobject> obj = java_ref_.get(env); > if (obj.is_null()) > return; > > DoSomething(obj.get()); https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.cc:52: On 2012/11/29 18:11:14, joth wrote: > nit: spurious \n Done. https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... File content/components/web_contents_delegate_android/color_chooser_android.h (right): https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/color_chooser_android.h:19: On 2012/11/29 16:01:32, Jói wrote: > just one blank line Done. https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... File content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java (right): https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:26: new ColorPickerDialog.OnColorChangedListener() { On 2012/11/28 17:56:58, Peter Beverloo wrote: > nit: this is a continuation, so indentation ought to be eight spaces? Done. https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:50: int nativeColorChooserAndroid, On 2012/11/28 17:56:58, Peter Beverloo wrote: > dito. Done. https://codereview.chromium.org/11316153/diff/7017/content/components/web_con... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/ColorChooserAndroid.java:55: initialColor); On 2012/11/28 17:56:58, Peter Beverloo wrote: > dito. Done. https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/ColorPickerDialog.java (right): https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:97: CENTER_RADIUS + mCenterPaint.getStrokeWidth(), On 2012/11/28 17:56:58, Peter Beverloo wrote: > nit: eight spaces for indentation of continuations. Done. https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:142: private static final float PI = 3.1415926f; Moved it to the top of the class together with the other static members. On 2012/11/29 18:11:14, joth wrote: > nit: odd placement for static final. I think it can go inside the method you use > it in https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:148: boolean inCenter = java.lang.Math.sqrt(x * x + y * y) <= CENTER_RADIUS; On 2012/11/29 18:11:14, joth wrote: > nit, expensive sqrt easy to avoid: > inCenter = (x * x + y * y) <= (CENTER_RADIUS * CENTER_RADIUS); Done. https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:188: OnColorChangedListener listener, On 2012/11/28 17:56:58, Peter Beverloo wrote: > nit: the alignment of these parameters is inconsistent with > ColorChooserAndroid.java:23, but I'm not sure which is correct. I'd be inclined > to say CCA.java given that this is a continuation. Done. https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:196: @Override On 2012/11/28 17:56:58, Peter Beverloo wrote: > nit: two spaces, should be four. Done. https://codereview.chromium.org/11316153/diff/7017/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/ColorPickerDialog.java:198: mListener.colorChanged(mInitialColor); On 2012/11/28 17:56:58, Peter Beverloo wrote: > nit: three spaces, should be four. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miguelg@chromium.org/11316153/5024
Presubmit check for 11316153-5024 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 1.8s to calculate.
Nico: I TBRed you for the single-line file removal in chrome/chrome_browser_ui.gypi.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miguelg@chromium.org/11316153/5024
gypi file lgtm
Retried try job too often on win_rel for step(s) unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miguelg@chromium.org/11316153/5024
Message was sent while issue was closed.
Change committed as 170517 |
