Implement form resubmission warning for Android.

Implement form resubmission warning for Android.

This patch implements WebContentsDelegateAndroid.showRepostFormWarningDialog()
providing POST resubmission warning on Android.

BUG=154198
TEST=included

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

[ppi, 2013-10-17 18:03:34]

Could you guys have a look?

I assume we want to have test coverage for that - any advice on testing
certainly welcome.

[David Trainor- moved to gerrit, 2013-10-17 18:13:11]

lgtm.  Not sure how to test this.  You *could* make a test in ChromiumTestShell
that does the form submission/page loading using a test page, but that might be
too large of a test for this.

[joth, 2013-10-17 19:53:20]

lgtm

https://codereview.chromium.org/27461003/diff/3001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/27461003/diff/3001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:119:
contentViewCore.cancelPendingReload();
note there's a TODO over here
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
to remove cancelPendingReload and instead supply a dedicated interface in
showRepostFormWarningDialog, instead of passing ContentViewCore.

If you think that makes your code / testing any easier over here, then feel free
to tackle that one too :-)

https://codereview.chromium.org/27461003/diff/3001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:127: Activity
activity = (Activity)mContext;
this is weird -- TabBase can only be used with an activity context so why not
make mContext be of type activity?

But then on line 184 I see:   TODO(dtrainor): Only store application context
here.

your addition here is making that TODO increasingly difficult to implement. I'm
not familiar with the wider interactions to suggest any answers.

[ppi, 2013-10-18 18:03:35]

Thanks!

In patch set 3 I added integration tests - hopefully this is not too much of an
overkill.

I introduced a bit of ad hoc logic in TestShellTab - I assume we can get rid of
this by exposing onDidStartLoad in TabObserver?

Could you guys have another look?

https://codereview.chromium.org/27461003/diff/3001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/27461003/diff/3001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:119:
contentViewCore.cancelPendingReload();
On 2013/10/17 19:53:20, joth wrote:
> note there's a TODO over here
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
> to remove cancelPendingReload and instead supply a dedicated interface in
> showRepostFormWarningDialog, instead of passing ContentViewCore.
> 
> If you think that makes your code / testing any easier over here, then feel
free
> to tackle that one too :-)

Thanks for the pointer, but I'd take the liberty of considering this optional :)
- tests added in patch set 3 are quite agnostic of such a change.

https://codereview.chromium.org/27461003/diff/3001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:127: Activity
activity = (Activity)mContext;
On 2013/10/17 19:53:20, joth wrote:
> this is weird -- TabBase can only be used with an activity context so why not
> make mContext be of type activity?
> 
> But then on line 184 I see:   TODO(dtrainor): Only store application context
> here.
> 
> your addition here is making that TODO increasingly difficult to implement.
I'm
> not familiar with the wider interactions to suggest any answers.

Good point, but it seems like we don't have much choice - I understand that we
need an activity context to display the dialog.

[ppi, 2013-10-21 18:57:08]

Good morning everyone!

I discovered CallbackHelpers and rewrote the tests eliminating the need for the
ad-hoc logic in TestShellTab.

I would appreciate if you could have a look at the testing part in patch set 5 -
in particular I'm curious how I could (if I should?) avoid waiting for the
timeout in testFormResubmissionCancel.

[David Trainor- moved to gerrit, 2013-10-22 00:51:43]

lgtm

https://codereview.chromium.org/27461003/diff/360001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/RepostFormWarningTest.java
(right):

https://codereview.chromium.org/27461003/diff/360001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/RepostFormWarningTest.java:53:
RepostFormWarningDialog.getCurrentDialog() == null);
assertNull(...)

https://codereview.chromium.org/27461003/diff/360001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/RepostFormWarningTest.java:66:
getInstrumentation().waitForIdleSync();
You might be able to swap these out with waitForActiveShellToBeDoneLoading(). 
Not sure if it's exactly what you want though.

https://codereview.chromium.org/27461003/diff/360001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/RepostFormWarningTest.java:68:
assertTrue("Form resubmission warning not shown upon reload", dialog != null);
assertNotNull("...", dialog)?  Same with others.

[ppi, 2013-10-22 11:55:40]

Thanks, David!

https://codereview.chromium.org/27461003/diff/360001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/RepostFormWarningTest.java
(right):

https://codereview.chromium.org/27461003/diff/360001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/RepostFormWarningTest.java:53:
RepostFormWarningDialog.getCurrentDialog() == null);
On 2013/10/22 00:51:43, David Trainor wrote:
> assertNull(...)

Done.

https://codereview.chromium.org/27461003/diff/360001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/RepostFormWarningTest.java:66:
getInstrumentation().waitForIdleSync();
On 2013/10/22 00:51:43, David Trainor wrote:
> You might be able to swap these out with waitForActiveShellToBeDoneLoading(). 
> Not sure if it's exactly what you want though.

Thanks - I think in this case we specifically want waitForIdleSync() -
waitForActiveShellToBeDoneLoading() could be called before the tab gets the call
notifying about started load and return immediately (I saw it acting racy
locally).

https://codereview.chromium.org/27461003/diff/360001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/RepostFormWarningTest.java:68:
assertTrue("Form resubmission warning not shown upon reload", dialog != null);
On 2013/10/22 00:51:43, David Trainor wrote:
> assertNotNull("...", dialog)?  Same with others.

Done.

[commit-bot: I haz the power, 2013-10-22 11:56:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/27461003/490001

[commit-bot: I haz the power, 2013-10-22 13:01:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/27461003/710001

[commit-bot: I haz the power, 2013-10-22 19:58:22]

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

[commit-bot: I haz the power, 2013-10-22 20:32:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/27461003/710001

[commit-bot: I haz the power, 2013-10-22 22:17:13]

Change committed as 230226