Refactoring JavaScript modal dialog.

Refactoring JavaScript modal dialog.

Small refactoring of the JavaScript modal dialogs so they use the system class
more, and some minor changes for testing.

BUG=156745
TEST=Navigate to http://www.w3schools.com/js/js_popup.asp and try all the alert
     box examples.
TBR=ben


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

[Yaron, 2012-12-12 00:35:44]

https://codereview.chromium.org/11555004/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
(right):

https://codereview.chromium.org/11555004/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:129:
public static JavascriptAppPromptDialog getCurrentDialog() {
*ForTest? I know it has the @VisibleForTesting annotation but that can apply
strictly for visibility whereas this accessor is only meant for test purposes.

https://codereview.chromium.org/11555004/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:138:
public AlertDialog getDialog() {
*ForTest

https://codereview.chromium.org/11555004/diff/1/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/11555004/diff/1/chrome/chrome.gyp#newcode1075
chrome/chrome.gyp:1075: '../third_party/guava/guava.gyp:guava_javalib',
How is this related?

[Jay Civelli, 2012-12-12 17:54:41]

https://codereview.chromium.org/11555004/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
(right):

https://codereview.chromium.org/11555004/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:129:
public static JavascriptAppPromptDialog getCurrentDialog() {
On 2012/12/12 00:35:44, Yaron wrote:
> *ForTest? I know it has the @VisibleForTesting annotation but that can apply
> strictly for visibility whereas this accessor is only meant for test purposes.

Done.

https://codereview.chromium.org/11555004/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:138:
public AlertDialog getDialog() {
On 2012/12/12 00:35:44, Yaron wrote:
> *ForTest

Done.

https://codereview.chromium.org/11555004/diff/1/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/11555004/diff/1/chrome/chrome.gyp#newcode1075
chrome/chrome.gyp:1075: '../third_party/guava/guava.gyp:guava_javalib',
On 2012/12/12 00:35:44, Yaron wrote:
> How is this related?
This is needed to bring the VisibleForTesting annotation to Chrome Java.

[Yaron, 2012-12-12 18:47:07]

lgtm

https://codereview.chromium.org/11555004/diff/6001/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
(right):

https://codereview.chromium.org/11555004/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:172:
TextView messageView = (TextView)
layout.findViewById(R.id.js_modal_dialog_message);
Could you also replace this with a call to setMessage above (like you did with
setTitle) or that makes it so you can't reach it from the test?

[Jay Civelli, 2012-12-12 21:12:58]

On 2012/12/12 18:47:07, Yaron wrote:
> lgtm
> 
>
https://codereview.chromium.org/11555004/diff/6001/chrome/android/java/src/or...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java
> (right):
> 
>
https://codereview.chromium.org/11555004/diff/6001/chrome/android/java/src/or...
>
chrome/android/java/src/org/chromium/chrome/browser/JavascriptAppModalDialog.java:172:
> TextView messageView = (TextView)
> layout.findViewById(R.id.js_modal_dialog_message);
> Could you also replace this with a call to setMessage above (like you did with
> setTitle) or that makes it so you can't reach it from the test?
Yes, great suggestion!
Changed the test not to test the message anymore, I guess that not useful a test
and it is more beneficial to have that code simpler.

[commit-bot: I haz the power, 2012-12-13 18:14:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/11555004/8008

[commit-bot: I haz the power, 2012-12-13 18:14:23]

Presubmit check for 11555004-8008 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[Jay Civelli, 2012-12-13 18:19:00]

Ben,
TBRing you to appease presubmit.
(need LGTM for chrome.gyp in which I added new Java dependency that only affects
Android)

[commit-bot: I haz the power, 2012-12-13 18:19:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/11555004/8008

[Ben Goodger (Google), 2012-12-13 21:43:15]

lgtm

[commit-bot: I haz the power, 2012-12-13 21:45:00]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for dinner.
Please email commit-bot@chromium.org with the CL url.

[commit-bot: I haz the power, 2012-12-13 23:51:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/11555004/8008

[commit-bot: I haz the power, 2012-12-14 00:41:00]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for dinner.
Please email commit-bot@chromium.org with the CL url.

[commit-bot: I haz the power, 2012-12-14 13:55:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/11555004/8008

[commit-bot: I haz the power, 2012-12-14 18:08:01]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2012-12-17 17:32:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/11555004/23003

[commit-bot: I haz the power, 2012-12-17 19:55:40]

Retried try job too often on linux_chromeos for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-17 20:53:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/11555004/23003

[commit-bot: I haz the power, 2012-12-18 00:01:39]

Change committed as 173582