Upstreaming SelectFileDialog for Android

Upstreaming SelectFileDialog for Android

Upstreaming the Select File Dialog and its dependencies needed for
Chrome on Android

BUG=116131
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157424

[aurimas (slooooooooow), 2012-09-06 23:14:46]

Hey Ted and David,

Could you take a look at my CL?

Thanks,
Aurimas

[David Trainor- moved to gerrit, 2012-09-10 20:51:05]

https://chromiumcodereview.appspot.com/10916160/diff/5001/content/browser/and...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/10916160/diff/5001/content/browser/and...
content/browser/android/content_view_core_impl.cc:91: window_android_ =
window_android;
Put this up with java_ref_, just make sure you do it in the same order as the
member variables are listed in the header file.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
File ui/android/java/src/org/chromium/ui/SelectFileDialog.java (right):

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/SelectFileDialog.java:115: "Unable to
complete previous operation due to low memory")) {
Do we need this error message at the other window.showIntent messages above?  If
so, pull it out to a constant and put a TODO stating that when AppResources is
moved to base we can use that instead to support internationalization.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/SelectFileDialog.java:187:
window.showError("Failed to open selected file");
This will also have to be internationalized.  We should either pull it from
native (I assume there is some string for this) or use AppResources when that is
moved to base.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
File ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java (right):

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:93: return
showIntent(intent, callback, mActivity.getString(errorId));
What does getString do if you pass it an invalid resource id?  If it throws an
exception should we handle that?

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:139: HashMap<Integer,
String> intentErrors = (HashMap<Integer, String>) errors;
Can get rid of intentErrors, just do mIntentErrors = (HashMap<Integer, String>)
errors;

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:149:
Toast.makeText(mActivity, error, Toast.LENGTH_SHORT).show();
What if error is null?

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:157:
showError(mActivity.getString(errorId));
getString check again?  Not sure what happens if it isn't a valid resource id.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:164: protected void
showCallbackNonExistent(String error) {
I know I'm responsible for this name, but put Error at the end or something. 
Sorry!

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:183: IntentCallback
callback = mOutstandingIntents.get(requestCode);
Remove the id from the mIntentErrors here... if we successfully process the
intent then we will leave the id in the map forever.

[aurimas (slooooooooow), 2012-09-10 23:48:47]

Fixed all the David's nits except for one in NativeWindow.java to do with
@SuppressWarnings. See the comment below.

https://chromiumcodereview.appspot.com/10916160/diff/5001/content/browser/and...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/10916160/diff/5001/content/browser/and...
content/browser/android/content_view_core_impl.cc:91: window_android_ =
window_android;
On 2012/09/10 20:51:05, dtrainor wrote:
> Put this up with java_ref_, just make sure you do it in the same order as the
> member variables are listed in the header file.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
File ui/android/java/src/org/chromium/ui/SelectFileDialog.java (right):

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/SelectFileDialog.java:115: "Unable to
complete previous operation due to low memory")) {
On 2012/09/10 20:51:05, dtrainor wrote:
> Do we need this error message at the other window.showIntent messages above? 
If
> so, pull it out to a constant and put a TODO stating that when AppResources is
> moved to base we can use that instead to support internationalization.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/SelectFileDialog.java:187:
window.showError("Failed to open selected file");
On 2012/09/10 20:51:05, dtrainor wrote:
> This will also have to be internationalized.  We should either pull it from
> native (I assume there is some string for this) or use AppResources when that
is
> moved to base.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
File ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java (right):

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:93: return
showIntent(intent, callback, mActivity.getString(errorId));
On 2012/09/10 20:51:05, dtrainor wrote:
> What does getString do if you pass it an invalid resource id?  If it throws an
> exception should we handle that?

Done.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:139: HashMap<Integer,
String> intentErrors = (HashMap<Integer, String>) errors;
On 2012/09/10 20:51:05, dtrainor wrote:
> Can get rid of intentErrors, just do mIntentErrors = (HashMap<Integer,
String>)
> errors;
I tried doing that, but then the @SuppressWarnings does not work. Not sure what
is up with that.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:149:
Toast.makeText(mActivity, error, Toast.LENGTH_SHORT).show();
On 2012/09/10 20:51:05, dtrainor wrote:
> What if error is null?

Done.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:157:
showError(mActivity.getString(errorId));
On 2012/09/10 20:51:05, dtrainor wrote:
> getString check again?  Not sure what happens if it isn't a valid resource id.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:164: protected void
showCallbackNonExistent(String error) {
On 2012/09/10 20:51:05, dtrainor wrote:
> I know I'm responsible for this name, but put Error at the end or something. 
> Sorry!

Done.

https://chromiumcodereview.appspot.com/10916160/diff/5001/ui/android/java/src...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:183: IntentCallback
callback = mOutstandingIntents.get(requestCode);
On 2012/09/10 20:51:05, dtrainor wrote:
> Remove the id from the mIntentErrors here... if we successfully process the
> intent then we will leave the id in the map forever.

Done.

[aurimas_google.com, 2012-09-11 15:29:30]

Hello Elliot,

I was you worked on moving the SelectFileDialog to ui/base/dialogs/. I am adding
this functionality for Android. Do you mind looking over this CL?

Thanks,
Aurimas

[Elliot Glaysher, 2012-09-11 16:51:46]

The parts ui/base/dialogs/ look like they're set up properly, but I'm not
comfortable reviewing JNI code.

limited lgtm only for the select_file_dialog_android.cc and related files.

http://codereview.chromium.org/10916160/diff/10001/ui/base/dialogs/android/se...
File ui/base/dialogs/android/select_file_dialog_android.cc (right):

http://codereview.chromium.org/10916160/diff/10001/ui/base/dialogs/android/se...
ui/base/dialogs/android/select_file_dialog_android.cc:18: namespace ui {
blank line under namespace

http://codereview.chromium.org/10916160/diff/10001/ui/base/dialogs/android/se...
ui/base/dialogs/android/select_file_dialog_android.cc:108: }
blank line between } and }

[aurimas_google.com, 2012-09-12 02:49:18]

Hey Yaron,

Could you take a look at this CL? Especially the JNI stuff.

Thanks,
Aurimas

http://codereview.chromium.org/10916160/diff/10001/ui/base/dialogs/android/se...
File ui/base/dialogs/android/select_file_dialog_android.cc (right):

http://codereview.chromium.org/10916160/diff/10001/ui/base/dialogs/android/se...
ui/base/dialogs/android/select_file_dialog_android.cc:18: namespace ui {
On 2012/09/11 16:51:47, Elliot Glaysher wrote:
> blank line under namespace

Done.

http://codereview.chromium.org/10916160/diff/10001/ui/base/dialogs/android/se...
ui/base/dialogs/android/select_file_dialog_android.cc:108: }
On 2012/09/11 16:51:47, Elliot Glaysher wrote:
> blank line between } and }

Done.

[Yaron, 2012-09-12 19:14:45]

Seems reasonable but I'd like to see this downstream too/first. We can't vet
that this is fully functional upstream. You'll also want ui OWNER to look at
this.

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/DEPS
File content/DEPS (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/DEPS#newco...
content/DEPS:50: "+ui/android",
You probably want this above the comment.

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/browser/an...
File content/browser/android/content_view_core_impl.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/browser/an...
content/browser/android/content_view_core_impl.h:220: WindowAndroid*
window_android_;
Add comment.

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:104:
public static ContentView newInstance(Context context, int nativeWebContents,
Pretty sure you need to update webview to deal with this.

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/android/java/sr...
File ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/android/java/sr...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:22: */
@JNINamespace("ui")

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/base/dialogs/an...
File ui/base/dialogs/android/select_file_dialog_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/base/dialogs/an...
ui/base/dialogs/android/select_file_dialog_android.h:60: bool
RegisterSelectFileDialog(JNIEnv*);
Nit: make this a static member of the above class

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/android/win...
File ui/gfx/android/window_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/android/win...
ui/gfx/android/window_android.h:21: JavaObject* java_object_;
I think this should be a JavaObjectWeakGlobalRef

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/android/win...
ui/gfx/android/window_android.h:24: bool RegisterWindowAndroid(JNIEnv* env);
Nit: make static member of above

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/native_widg...
File ui/gfx/native_widget_types.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/native_widg...
ui/gfx/native_widget_types.h:114: class WindowAndroid;
This should be in the #ifdef above.
Also I think you can remove ContentViewCore

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/native_widg...
ui/gfx/native_widget_types.h:149: typedef content::ContentViewCore* NativeView;
Should this not also be WindowAndroid?

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/ui.gyp
File ui/ui.gyp (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/ui.gyp#newcode144
ui/ui.gyp:144: 'base/dialogs/android/select_file_dialog_android.cc',
You don't really need the /android/ dir. This can sit alongside the other ports
in base/dialogs/

[aurimas (slooooooooow), 2012-09-13 15:37:14]

Hello Scott,

I'm adding code to UI so I was wondering if you could take a look at my CL?

Thanks,
Aurimas

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/DEPS
File content/DEPS (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/DEPS#newco...
content/DEPS:50: "+ui/android",
On 2012/09/12 19:14:45, Yaron wrote:
> You probably want this above the comment.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/browser/an...
File content/browser/android/content_view_core_impl.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/content/browser/an...
content/browser/android/content_view_core_impl.h:220: WindowAndroid*
window_android_;
On 2012/09/12 19:14:45, Yaron wrote:
> Add comment.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/android/java/sr...
File ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/android/java/sr...
ui/android/java/src/org/chromium/ui/gfx/NativeWindow.java:22: */
On 2012/09/12 19:14:45, Yaron wrote:
> @JNINamespace("ui")

Done.

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/base/dialogs/an...
File ui/base/dialogs/android/select_file_dialog_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/base/dialogs/an...
ui/base/dialogs/android/select_file_dialog_android.h:60: bool
RegisterSelectFileDialog(JNIEnv*);
On 2012/09/12 19:14:45, Yaron wrote:
> Nit: make this a static member of the above class

Done.

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/android/win...
File ui/gfx/android/window_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/android/win...
ui/gfx/android/window_android.h:21: JavaObject* java_object_;
On 2012/09/12 19:14:45, Yaron wrote:
> I think this should be a JavaObjectWeakGlobalRef

Is there an example somewhere? I followed content_view_core as example for this.

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/android/win...
ui/gfx/android/window_android.h:24: bool RegisterWindowAndroid(JNIEnv* env);
On 2012/09/12 19:14:45, Yaron wrote:
> Nit: make static member of above

Done.

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/native_widg...
File ui/gfx/native_widget_types.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/native_widg...
ui/gfx/native_widget_types.h:114: class WindowAndroid;
On 2012/09/12 19:14:45, Yaron wrote:
> This should be in the #ifdef above.
> Also I think you can remove ContentViewCore

Done.

https://chromiumcodereview.appspot.com/10916160/diff/13001/ui/gfx/native_widg...
ui/gfx/native_widget_types.h:149: typedef content::ContentViewCore* NativeView;
On 2012/09/12 19:14:45, Yaron wrote:
> Should this not also be WindowAndroid?

I talked to dtrainor and he said not at this point. It might change later on
with another CL.

[sky, 2012-09-13 16:24:03]

I did not look at chrome or content. Do you have reviewers for those
directories?

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
File ui/base/dialogs/select_file_dialog_android.cc (right):

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.cc:32:
SelectFileDialogImpl::SelectFileDialogImpl(Listener* listener,
order of methods should match header.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.cc:67: listener_ = 0;
NULL

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
File ui/base/dialogs/select_file_dialog_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.h:21: void OnFileSelected(JNIEnv*,
jobject, jstring filepath);
Why do these need to be public?

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.h:21: void OnFileSelected(JNIEnv*,
jobject, jstring filepath);
all method parameters should have names and the names in the
definition/declaration should match.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.h:47: explicit
SelectFileDialogImpl(Listener* listener,
no explicit and indentation is off.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.h:54: bool is_running_;
add description.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.h:55: };
DISALLOW_...

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
File ui/gfx/android/window_android.cc (right):

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.cc:19: ScopedJavaLocalRef<jobject>
WindowAndroid(JNIEnv* env) {
Generally structs shouldn't have methods.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.cc:33: java_object_->obj_ = 0;
NULL

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.cc:39: delete this;
If this is the only way to delte 'this' then make the destructor private.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
File ui/gfx/android/window_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:13: class WindowAndroid{
WindowAndroid{ -> WindowAndroid {

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:16: virtual ~WindowAndroid();
Why virtual?

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:17: void Destroy(JNIEnv* env, jobject obj);
newline between 16/17

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:25: JavaObject* java_object_;
Why don't you declare JavaObject by value?

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:26: };
DISALLOW_..

[aurimas (slooooooooow), 2012-09-14 00:06:58]

Hello Antoine,

Could you take a look at my CL? In particular the code under content/.

Thanks,
Aurimas

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
File ui/base/dialogs/select_file_dialog_android.cc (right):

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.cc:32:
SelectFileDialogImpl::SelectFileDialogImpl(Listener* listener,
On 2012/09/13 16:24:03, sky wrote:
> order of methods should match header.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.cc:67: listener_ = 0;
On 2012/09/13 16:24:03, sky wrote:
> NULL

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
File ui/base/dialogs/select_file_dialog_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.h:47: explicit
SelectFileDialogImpl(Listener* listener,
On 2012/09/13 16:24:04, sky wrote:
> no explicit and indentation is off.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.h:55: };
On 2012/09/13 16:24:04, sky wrote:
> DISALLOW_...

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
File ui/gfx/android/window_android.cc (right):

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.cc:19: ScopedJavaLocalRef<jobject>
WindowAndroid(JNIEnv* env) {
On 2012/09/13 16:24:04, sky wrote:
> Generally structs shouldn't have methods.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.cc:33: java_object_->obj_ = 0;
On 2012/09/13 16:24:04, sky wrote:
> NULL

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.cc:39: delete this;
On 2012/09/13 16:24:04, sky wrote:
> If this is the only way to delte 'this' then make the destructor private.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
File ui/gfx/android/window_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:13: class WindowAndroid{
On 2012/09/13 16:24:04, sky wrote:
> WindowAndroid{ -> WindowAndroid {

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:16: virtual ~WindowAndroid();
On 2012/09/13 16:24:04, sky wrote:
> Why virtual?

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:17: void Destroy(JNIEnv* env, jobject obj);
On 2012/09/13 16:24:04, sky wrote:
> newline between 16/17

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:25: JavaObject* java_object_;
On 2012/09/13 16:24:04, sky wrote:
> Why don't you declare JavaObject by value?

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/gfx/android/win...
ui/gfx/android/window_android.h:26: };
On 2012/09/13 16:24:04, sky wrote:
> DISALLOW_..

Done.

[piman, 2012-09-14 00:21:03]

At a high level it looks reasonable.

We should be as minimalist as possible when adding DEPS. If we can restrict
ui/android to the browser directories that would be best. Intuitively it should
be possible.

Also +jam if he has some comments about this.

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/DEPS
File content/DEPS (right):

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/DEPS#newco...
content/DEPS:49: "+ui/android",
Should this be only allowed in content/browser?

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/content.gyp
File content/content.gyp (right):

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/content.gy...
content/content.gyp:285: '../ui/ui.gyp:ui_java',
nit: indent

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/content.gy...
content/content.gyp:292: '../ui/ui.gyp:ui_java',
nit: indent

[aurimas (slooooooooow), 2012-09-14 18:24:00]

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/DEPS
File content/DEPS (right):

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/DEPS#newco...
content/DEPS:49: "+ui/android",
On 2012/09/14 00:21:03, piman wrote:
> Should this be only allowed in content/browser?

Done.

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/content.gyp
File content/content.gyp (right):

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/content.gy...
content/content.gyp:285: '../ui/ui.gyp:ui_java',
On 2012/09/14 00:21:03, piman wrote:
> nit: indent

Done.

https://chromiumcodereview.appspot.com/10916160/diff/14004/content/content.gy...
content/content.gyp:292: '../ui/ui.gyp:ui_java',
On 2012/09/14 00:21:03, piman wrote:
> nit: indent

Done.

[piman, 2012-09-14 18:24:52]

lgtm

[aurimas (slooooooooow), 2012-09-14 20:19:18]

Hey Nico,

Do you mind taking a look at this CL? I need an LGTM from a chrome/ owner.

Aurimas

[Nico, 2012-09-15 13:11:48]

chrome lgtm. you probably need a content/ owner too.

https://chromiumcodereview.appspot.com/10916160/diff/6036/chrome/browser/file...
File chrome/browser/file_select_helper.cc (right):

https://chromiumcodereview.appspot.com/10916160/diff/6036/chrome/browser/file...
chrome/browser/file_select_helper.cc:414: // allows to pass MIME types and the
capture value to the SelectFile method.
useless comment, remove

[aurimas (slooooooooow), 2012-09-17 16:56:44]

Hello Scott,

I got LGTMs from owners of content/ and chrome/. Could I get one from you to
cover ui/ if everything looks ok?

Aurimas

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
File ui/base/dialogs/select_file_dialog_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.h:21: void OnFileSelected(JNIEnv*,
jobject, jstring filepath);
On 2012/09/13 16:24:03, sky wrote:
> all method parameters should have names and the names in the
> definition/declaration should match.

Done.

https://chromiumcodereview.appspot.com/10916160/diff/15003/ui/base/dialogs/se...
ui/base/dialogs/select_file_dialog_android.h:21: void OnFileSelected(JNIEnv*,
jobject, jstring filepath);
On 2012/09/13 16:24:03, sky wrote:
> Why do these need to be public?

They have to be public so java could access them through jni.

https://chromiumcodereview.appspot.com/10916160/diff/6036/chrome/browser/file...
File chrome/browser/file_select_helper.cc (right):

https://chromiumcodereview.appspot.com/10916160/diff/6036/chrome/browser/file...
chrome/browser/file_select_helper.cc:414: // allows to pass MIME types and the
capture value to the SelectFile method.
On 2012/09/15 13:11:49, Nico wrote:
> useless comment, remove

Done.

[aurimas (slooooooooow), 2012-09-17 16:59:15]

Hey Jay,

Would you mind looking at my base/android changes and giving me an LGTM?

Thanks,
Aurimas

[Jay Civelli, 2012-09-17 17:33:20]

lgtm

[Ted C, 2012-09-17 18:40:16]

Couple little things...sorry for being late to the party

https://chromiumcodereview.appspot.com/10916160/diff/4070/content/shell/andro...
File content/shell/android/java/content_shell_apk.xml (right):

https://chromiumcodereview.appspot.com/10916160/diff/4070/content/shell/andro...
content/shell/android/java/content_shell_apk.xml:32: <pathelement
location="${PRODUCT_DIR}/lib.java/chromium_ui.jar"/>
Why do you need to specify this one any different?  It should be in the
INPUT_JAR_PATHS instead.

https://chromiumcodereview.appspot.com/10916160/diff/4070/ui/base/dialogs/sel...
File ui/base/dialogs/select_file_dialog_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/4070/ui/base/dialogs/sel...
ui/base/dialogs/select_file_dialog_android.h:16: class SelectFileDialogImpl :
public ui::SelectFileDialog {
Hmm...the other platforms have the Impl definition in the .cc.  Do we do it
differently for JNI needs or something?

https://chromiumcodereview.appspot.com/10916160/diff/4070/ui/gfx/android/wind...
File ui/gfx/android/window_android.h (right):

https://chromiumcodereview.appspot.com/10916160/diff/4070/ui/gfx/android/wind...
ui/gfx/android/window_android.h:14: class WindowAndroid {
Maybe a simple class doc (should we say that it is the activity window and is
more constrained than other platforms).

[jam, 2012-09-17 19:59:55]

content lgtm, just one nit

https://codereview.chromium.org/10916160/diff/4070/content/app/DEPS
File content/app/DEPS (right):

https://codereview.chromium.org/10916160/diff/4070/content/app/DEPS#newcode4
content/app/DEPS:4: "+ui/android",
nit: seems like this better belongs in content/DEPS beside the other ui/
includes?

[sky, 2012-09-17 20:53:44]

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
File ui/base/dialogs/select_file_dialog_android.cc (right):

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.cc:26:
SelectFileDialogImpl::SelectFileDialogImpl(Listener* listener,
Remember, order of implementation needs to match order of header.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
File ui/base/dialogs/select_file_dialog_android.h (right):

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:18: static SelectFileDialogImpl*
Create(Listener* listener,
wrap listener to next line so that it aligns with indentation of policy (you
have it write on line 32).

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:21: void OnFileSelected(JNIEnv*
env, jobject javaObject, jstring filepath);
java_object

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:22: void OnFileNotSelected(JNIEnv*
env, jobject javaObject);
java_object

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:32: ui::SelectFileDialog::Type,
remember, all parameters need names.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:41: static bool
RegisterSelectFileDialog(JNIEnv*);
parameter needs a name.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:53: bool is_running_;
Add description.

https://codereview.chromium.org/10916160/diff/4070/ui/gfx/android/window_andr...
File ui/gfx/android/window_android.cc (right):

https://codereview.chromium.org/10916160/diff/4070/ui/gfx/android/window_andr...
ui/gfx/android/window_android.cc:21: void WindowAndroid::Destroy(JNIEnv* env,
jobject obj) {
I'm not familiar with the java side, but how come it can't 'delete this'
directly?

[aurimas (slooooooooow), 2012-09-17 22:20:47]

https://codereview.chromium.org/10916160/diff/4070/content/app/DEPS
File content/app/DEPS (right):

https://codereview.chromium.org/10916160/diff/4070/content/app/DEPS#newcode4
content/app/DEPS:4: "+ui/android",
On 2012/09/17 19:59:55, John Abd-El-Malek wrote:
> nit: seems like this better belongs in content/DEPS beside the other ui/
> includes?

Done.

https://codereview.chromium.org/10916160/diff/4070/content/shell/android/java...
File content/shell/android/java/content_shell_apk.xml (right):

https://codereview.chromium.org/10916160/diff/4070/content/shell/android/java...
content/shell/android/java/content_shell_apk.xml:32: <pathelement
location="${PRODUCT_DIR}/lib.java/chromium_ui.jar"/>
On 2012/09/17 18:40:16, Ted C wrote:
> Why do you need to specify this one any different?  It should be in the
> INPUT_JAR_PATHS instead.

Done.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
File ui/base/dialogs/select_file_dialog_android.cc (right):

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.cc:26:
SelectFileDialogImpl::SelectFileDialogImpl(Listener* listener,
On 2012/09/17 20:53:44, sky wrote:
> Remember, order of implementation needs to match order of header.

Done.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
File ui/base/dialogs/select_file_dialog_android.h (right):

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:18: static SelectFileDialogImpl*
Create(Listener* listener,
On 2012/09/17 20:53:44, sky wrote:
> wrap listener to next line so that it aligns with indentation of policy (you
> have it write on line 32).

Done.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:21: void OnFileSelected(JNIEnv*
env, jobject javaObject, jstring filepath);
On 2012/09/17 20:53:44, sky wrote:
> java_object

Done.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:22: void OnFileNotSelected(JNIEnv*
env, jobject javaObject);
On 2012/09/17 20:53:44, sky wrote:
> java_object

Done.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:32: ui::SelectFileDialog::Type,
On 2012/09/17 20:53:44, sky wrote:
> remember, all parameters need names.

Done.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:41: static bool
RegisterSelectFileDialog(JNIEnv*);
On 2012/09/17 20:53:44, sky wrote:
> parameter needs a name.

Done.

https://codereview.chromium.org/10916160/diff/4070/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_android.h:53: bool is_running_;
On 2012/09/17 20:53:44, sky wrote:
> Add description.

Done.

https://codereview.chromium.org/10916160/diff/4070/ui/gfx/android/window_andr...
File ui/gfx/android/window_android.cc (right):

https://codereview.chromium.org/10916160/diff/4070/ui/gfx/android/window_andr...
ui/gfx/android/window_android.cc:21: void WindowAndroid::Destroy(JNIEnv* env,
jobject obj) {
On 2012/09/17 20:53:44, sky wrote:
> I'm not familiar with the java side, but how come it can't 'delete this'
> directly?

Sadly it cannot do it directly, java has to call the native destroy function.

https://codereview.chromium.org/10916160/diff/4070/ui/gfx/android/window_andr...
File ui/gfx/android/window_android.h (right):

https://codereview.chromium.org/10916160/diff/4070/ui/gfx/android/window_andr...
ui/gfx/android/window_android.h:14: class WindowAndroid {
On 2012/09/17 18:40:16, Ted C wrote:
> Maybe a simple class doc (should we say that it is the activity window and is
> more constrained than other platforms).

Done.

[sky, 2012-09-17 22:41:17]

LGTM

[Ted C, 2012-09-17 23:15:46]

lgtm

[David Trainor- moved to gerrit, 2012-09-17 23:17:12]

lgtm

[commit-bot: I haz the power, 2012-09-17 23:18:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/10916160/11042

[commit-bot: I haz the power, 2012-09-17 23:38:48]

Retried try job too often for step(s) compile

[commit-bot: I haz the power, 2012-09-18 00:21:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/10916160/11042

[commit-bot: I haz the power, 2012-09-18 00:41:10]

Retried try job too often for step(s) interactive_ui_tests, jingle_unittests,
gpu_unittests, base_unittests, sync_integration_tests, sql_unittests,
chrome_frame_unittests, content_unittests, safe_browsing_tests, ipc_tests,
mini_installer_test, cacheinvalidation_unittests, remoting_unittests,
browser_tests, net_unittests, installer_util_unittests, chrome_frame_net_tests,
check_deps, crypto_unittests, unit_tests, nacl_integration, media_unittests,
content_browsertests, printing_unittests, sync_unit_tests

[commit-bot: I haz the power, 2012-09-18 01:01:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/10916160/11042

[commit-bot: I haz the power, 2012-09-18 01:15:55]

Retried try job too often for step(s) compile

[commit-bot: I haz the power, 2012-09-18 15:52:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/10916160/11042

[commit-bot: I haz the power, 2012-09-18 17:23:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/10916160/20006

[commit-bot: I haz the power, 2012-09-18 20:19:10]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-18 20:30:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/10916160/20006

[commit-bot: I haz the power, 2012-09-18 21:47:58]

Change committed as 157424