[Android] Upstream Modal Dialogs functionality

[Android] Upstream Modal Dialogs functionality

This is used for managing alert/confirm/prompt dialogs. There are downstream tests that can't be upstreamed yet until we have a test shell set up for chromium code.


BUG=138483
TBR=jam@chromium.org for content/public/browser. Should we have a content/public/browser/android/OWNERS?

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

[Yaron, 2012-08-15 04:37:14]

ben: chrome/chrome_browser.gypi and
chrome/browser/ui/app_modal_dialogs/javascript_dialog_creator.cc
joth: the rest (i.e. everything android)

[joth, 2012-08-15 06:11:24]

I'm not going to have chance to do a detailed review for a couple days, as I'm
still trying to rework the webview layout that is blocking us from making the
sort of improvements described below!
If this is in the way of a lot of other upstreaming, then I personally have no
problem with you pushing ahead on this and we'll rework it later to meet the
chrome-less webview needs.

http://codereview.chromium.org/10823340/diff/2001/content/browser/android/con...
File content/browser/android/content_view_core_impl.cc (right):

http://codereview.chromium.org/10823340/diff/2001/content/browser/android/con...
content/browser/android/content_view_core_impl.cc:164: return
!content_view_client_->OnJsModalDialog(type,
this is another example of adding an awkward API on content that's only purpose
is to call right back out to the client again.

added some comments on crbug.com/138483 about it.

http://codereview.chromium.org/10823340/diff/2001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
(right):

http://codereview.chromium.org/10823340/diff/2001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:215:
public boolean  onJsAlert(String url, String Message, JsResultReceiver receiver)
{
maybe we mark all these @deprecated, with comment linking bug 138483 (or a new
dedicated bug) and pointing out these methods will be going away so don't write
new tests that rely on them.

[Yaron, 2012-08-20 22:54:41]

Ok, removed all the webview code as that's being sorted out and implemented
under the android_webview path. The core chromium flow is equivalent to other
ports and this just provides the implementation of that.

[Ben Goodger (Google), 2012-08-20 22:56:10]

lgtm for the gyp stuff

[Yaron, 2012-08-24 20:29:18]

benm/joth: friendly ping

[joth, 2012-08-24 23:09:56]

lgtm % nits.

http://codereview.chromium.org/10823340/diff/6001/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
(right):

http://codereview.chromium.org/10823340/diff/6001/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:1:
// Copyright (c) 2010 The Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/10823340/diff/6001/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:30:
boolean shouldShowSuppressCheckBox) {
nit: wrong indent (through out file)

http://codereview.chromium.org/10823340/diff/6001/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:72:
// code is moved into the framework.
not relevant any more?

http://codereview.chromium.org/10823340/diff/6001/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:146:
}
AIUI (from reading the webview equiv) an alert dialog should not have a cancel
button?

http://codereview.chromium.org/10823340/diff/6001/chrome/browser/ui/android/j...
File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right):

http://codereview.chromium.org/10823340/diff/6001/chrome/browser/ui/android/j...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:30: dialog,
parent_window);
wrong indent

http://codereview.chromium.org/10823340/diff/6001/chrome/browser/ui/android/j...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:38:
parent_jobject_weak_ref(env, parent->GetJavaObject().obj()) {
parent_jobject_weak_ref_ (..needs trailing underscore)

[Yaron, 2012-08-28 20:26:40]

thanks for the reviews. sending to cq

http://codereview.chromium.org/10823340/diff/6001/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
(right):

http://codereview.chromium.org/10823340/diff/6001/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:30:
boolean shouldShowSuppressCheckBox) {
On 2012/08/24 23:09:56, joth wrote:
> nit: wrong indent (through out file)

Done.

http://codereview.chromium.org/10823340/diff/6001/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:72:
// code is moved into the framework.
On 2012/08/24 23:09:56, joth wrote:
> not relevant any more?

Done.

http://codereview.chromium.org/10823340/diff/6001/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:146:
}
On 2012/08/24 23:09:56, joth wrote:
> AIUI (from reading the webview equiv) an alert dialog should not have a cancel
> button?

It doesn't. This is common code shared by all dialogs. If you look at the
JavascriptAppAlertDialog subclass, it calls this  |prepare| and then hides the
cancel button.

http://codereview.chromium.org/10823340/diff/6001/chrome/browser/ui/android/j...
File chrome/browser/ui/android/javascript_app_modal_dialog_android.cc (right):

http://codereview.chromium.org/10823340/diff/6001/chrome/browser/ui/android/j...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:30: dialog,
parent_window);
On 2012/08/24 23:09:56, joth wrote:
> wrong indent

Done.

http://codereview.chromium.org/10823340/diff/6001/chrome/browser/ui/android/j...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:30: dialog,
parent_window);
On 2012/08/24 23:09:56, joth wrote:
> wrong indent

Done.

http://codereview.chromium.org/10823340/diff/6001/chrome/browser/ui/android/j...
chrome/browser/ui/android/javascript_app_modal_dialog_android.cc:38:
parent_jobject_weak_ref(env, parent->GetJavaObject().obj()) {
On 2012/08/24 23:09:56, joth wrote:
> parent_jobject_weak_ref_ (..needs trailing underscore)

Done.

[commit-bot: I haz the power, 2012-08-28 20:28:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10823340/23002

[commit-bot: I haz the power, 2012-08-28 20:28:53]

Presubmit check for 10823340-23002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/browser

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Yaron, 2012-08-28 20:35:59]

tbr jam for content/public/browser/android change.

[commit-bot: I haz the power, 2012-08-28 20:36:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10823340/23002

[jam, 2012-08-28 21:53:22]

lgtm

[commit-bot: I haz the power, 2012-08-28 22:08:16]

Try job failure for 10823340-23002 (retry) on mac_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-28 22:37:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10823340/23002

[commit-bot: I haz the power, 2012-08-29 00:10:34]

Try job failure for 10823340-23002 (retry) on mac_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Yaron, 2012-08-29 01:01:11]

Added missing DEPS change so that chrome java code can depend on
content/public/java code. trying to submit again.

[commit-bot: I haz the power, 2012-08-29 01:02:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10823340/17011

[commit-bot: I haz the power, 2012-08-29 05:41:55]

Failed to apply patch for chrome/browser/android/chrome_jni_registrar.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/android/chrome_jni_registrar.cc
Hunk #1 FAILED at 10.
Hunk #2 succeeded at 18 with fuzz 2 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/android/chrome_jni_registrar.cc.rej

[commit-bot: I haz the power, 2012-08-29 17:43:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10823340/23006

[commit-bot: I haz the power, 2012-08-29 23:44:45]

Try job failure for 10823340-23006 (retry) (retry) (retry) on win_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...