Switch Android code to use ExecuteJavascriptInWebFrameCallbackResult.

Switch Android code to use ExecuteJavascriptInWebFrameCallbackResult.

BUG=168169
TEST=should all pass

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

Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=175491

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

[joth, 2013-01-05 00:54:42]

Nice :-)

https://codereview.chromium.org/11788005/diff/3009/content/browser/android/co...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/11788005/diff/3009/content/browser/android/co...
content/browser/android/content_view_core_impl.cc:1226: base::Unretained(this),
make JavaScriptResultCallback into a static (e.g. file local anonymous
namespace) function, and then you can avoid sending "this" to it at all.

https://codereview.chromium.org/11788005/diff/3009/content/browser/android/co...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/11788005/diff/3009/content/browser/android/co...
content/browser/android/content_view_core_impl.h:269: // Notification that
Javascript execution is complete.
probably should be in  "Other private methods and data" section below

https://codereview.chromium.org/11788005/diff/3009/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11788005/diff/3009/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1992:
private void onEvaluateJavaScriptResult(String jsonResult, Message message) {
declare this as static, and then you can skip resolving the passing the
obj.obj() second param in the native side when calling through to here

https://codereview.chromium.org/11788005/diff/3009/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2501:
private native void nativeEvaluateJavaScript(String script, Message message);
huh. I think this was pretty old code :)

If you pass 'int nativeContentViewCoreImpl' as the first param, like all the
other methods here, then you can delete the native side static
EvaluateJavaScript() function. ("int native<ClassName>" is magic word that tells
the jni generator to auto-generate the static method that forwards the call into
the class of <ClassName> type, having cast that int to be a pointer to that
type).

https://codereview.chromium.org/11788005/diff/3009/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java
(right):

https://codereview.chromium.org/11788005/diff/3009/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:99:
notifyCalled(msg.obj);
will need to cast:
notifyCalled((String) msg.obj);

https://codereview.chromium.org/11788005/diff/3009/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java
(right):

https://codereview.chromium.org/11788005/diff/3009/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java:43:
@Override
remove override. we should remove this method altogether (maybe in a follow up)

[Avi (use Gerrit), 2013-01-05 01:03:34]

I uploaded this snapshot as a 'work in progress', so you caught a few broken
things that were already caught. When I get this passing tests, I will send it
out for review.

In the meanwhile, I will take into account your feedback.

https://codereview.chromium.org/11788005/diff/3009/content/browser/android/co...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/11788005/diff/3009/content/browser/android/co...
content/browser/android/content_view_core_impl.cc:1226: base::Unretained(this),
On 2013/01/05 00:54:42, joth wrote:
> make JavaScriptResultCallback into a static (e.g. file local anonymous
> namespace) function, and then you can avoid sending "this" to it at all.

JavaScriptResultCallback accesses java_ref_, so I can't split it off into a
standalone function...

(edit) oh, I see.

https://codereview.chromium.org/11788005/diff/3009/content/browser/android/co...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/11788005/diff/3009/content/browser/android/co...
content/browser/android/content_view_core_impl.h:269: // Notification that
Javascript execution is complete.
On 2013/01/05 00:54:42, joth wrote:
> probably should be in  "Other private methods and data" section below

Done.

https://codereview.chromium.org/11788005/diff/3009/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java
(right):

https://codereview.chromium.org/11788005/diff/3009/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:99:
notifyCalled(msg.obj);
On 2013/01/05 00:54:42, joth wrote:
> will need to cast:
> notifyCalled((String) msg.obj);

Already fixed in my local copy.

https://codereview.chromium.org/11788005/diff/3009/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java
(right):

https://codereview.chromium.org/11788005/diff/3009/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java:43:
@Override
This was intended as a snapshot of progress. This method is already gone in my
local copy.

[Avi (use Gerrit), 2013-01-05 01:05:47]

https://codereview.chromium.org/11788005/diff/3009/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11788005/diff/3009/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2501:
private native void nativeEvaluateJavaScript(String script, Message message);
That was the last user of ContentViewCore::GetNativeContentViewCore in the file.
Do I delete that function too, or is that still needed?

[Avi (use Gerrit), 2013-01-05 20:47:49]

For future reference, I'm getting errors in FindBugs:

--------------------------------------------------------------------------------
H C BC: Impossible cast from String to
org.chromium.android_webview.AwContents$DownloadInfo in
org.chromium.android_webview.AwContents$ClientCallbackHandler.handleMessage(Message)
 At AwContents.java
H C BC: Impossible cast from String to
org.chromium.content.browser.ContentViewCore in
org.chromium.android_webview.AwContentsClient$WebContentsDelegateAdapter$1.handleMessage(Message)
 At AwContentsClient.java
H C BC: Impossible cast from String to
org.chromium.content.common.CleanupReference in
org.chromium.content.common.CleanupReference$2.handleMessage(Message)  At
CleanupReference.java
--------------------------------------------------------------------------------

Where FindBugs is determining that every Message everywhere now has a String as
the Message.obj.

[Avi (use Gerrit), 2013-01-05 21:57:08]

Woot!

To Jonathan for review since he so aggressively grabbed it, Nilesh FYI.

https://codereview.chromium.org/11788005/diff/12003/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1055:
*                result will be json encoded in the Message's {@link
Message#obj} field.
Did I get the javadoc right?

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:96:
Message message = Message.obtain(new Handler() {
Do I need a special Looper? Right now it all fires through the same thread that
the callback happens on. I think that's the same thread as the previous version
of the code.

[mkosiba (inactive), 2013-01-07 10:42:09]

Me like too.

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:96:
Message message = Message.obtain(new Handler() {
On 2013/01/05 21:57:09, Avi wrote:
> Do I need a special Looper? Right now it all fires through the same thread
that
> the callback happens on. I think that's the same thread as the previous
version
> of the code.

The CallbackHelper should be threadsafe so you don't need a special looper.

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java:42:
*/
why did you leave the comment?

[Avi (use Gerrit), 2013-01-07 15:10:26]

Thanks!

As I mentioned to joth in email, there's probably more cleanup to do here.  The
fact that there's one OnEvaluateJavaScriptResultHelper and it lives in the test
content view client is now a historical accident. Places that need to
synchronously call out to Javascript should have the equivalent in a local
object rather than having one central one. But I'll leave that fun to someone
else.

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java:42:
*/
It said "methods" so I thought that it was referring to both following methods.
Does it not?

[mkosiba (inactive), 2013-01-07 15:37:11]

It shouldn't be too hard to get rid of it - there are only few places that use
the OnEvaluateJavaScriptResultHelper directly. I could do the cleanup after this
CL lands.

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java:42:
*/
On 2013/01/07 15:10:26, Avi wrote:
> It said "methods" so I thought that it was referring to both following
methods.
> Does it not?

yes it does. my mistake.

[nilesh, 2013-01-07 18:46:03]

https://codereview.chromium.org/11788005/diff/12003/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:428:
* TODO(nileshagrawal): Remove this method from the public interface.
Please remove to this TODO.

[Avi (use Gerrit), 2013-01-07 18:50:53]

https://codereview.chromium.org/11788005/diff/12003/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:428:
* TODO(nileshagrawal): Remove this method from the public interface.
On 2013/01/07 18:46:03, nileshagrawal1 wrote:
> Please remove to this TODO.

OK.

[joth, 2013-01-07 19:43:27]

lgtm

https://codereview.chromium.org/11788005/diff/12003/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1055:
*                result will be json encoded in the Message's {@link
Message#obj} field.
On 2013/01/05 21:57:09, Avi wrote:
> Did I get the javadoc right?

lgtm

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java
(right):

https://codereview.chromium.org/11788005/diff/12003/content/public/test/andro...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:96:
Message message = Message.obtain(new Handler() {
On 2013/01/07 10:42:09, Martin Kosiba wrote:
> On 2013/01/05 21:57:09, Avi wrote:
> > Do I need a special Looper? Right now it all fires through the same thread
> that
> > the callback happens on. I think that's the same thread as the previous
> version
> > of the code.
> 
> The CallbackHelper should be threadsafe so you don't need a special looper.

This isn't using a 'special' looper though? it is just creating a handler on the
default (main thread) looper. This is fine as we need some handler to attach the
message to, the handler needs some looper, and its not generally guaranteed the
current thread will have a looper (Looper.myLooper() may be null) so using main
looper is the simplest choice.

but, yes, more cleanup could be made here to remove it from this class
altogether.

[Avi (use Gerrit), 2013-01-07 19:47:44]

The notification is coming back on the main thread, so
base::android::AttachCurrentThread() should get that, and there should (should
there?) be a Looper on the main thread.

[commit-bot: I haz the power, 2013-01-07 19:52:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11788005/17014

[joth, 2013-01-07 20:03:30]

On 2013/01/07 19:47:44, Avi wrote:
> The notification is coming back on the main thread, so
> base::android::AttachCurrentThread() should get that, and there should (should
> there?) be a Looper on the main thread.

True, although it actually it doesn't matter which thread the callback happens
on (i.e. that would be fine happening on any thread, with or without looper).
Message.sendToTaget() causes the message to be run on the handler/looper/thread
the message was bound to (e.g. passed to obtain()). i.e. the thread
evaluateJavaScript() was originally called on.. which is also the UI thread. so
it's all good.

[Avi (use Gerrit), 2013-01-07 20:09:21]

Slick!

[commit-bot: I haz the power, 2013-01-07 21:57:40]

Retried try job too often on win_aura for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-07 21:59:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11788005/17014

[commit-bot: I haz the power, 2013-01-08 01:08:11]

Retried try job too often on win_aura for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-08 03:07:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11788005/17014

[commit-bot: I haz the power, 2013-01-08 06:09:44]

Change committed as 175489

[Avi (use Gerrit), 2013-01-08 15:42:44]

This fails miserably on real bots. Yay trybots?

https://codereview.chromium.org/11778034/

Will update and try again.

[commit-bot: I haz the power, 2013-01-08 16:21:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11788005/26001

[commit-bot: I haz the power, 2013-01-08 18:40:31]

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

[Avi (use Gerrit), 2013-01-13 18:55:30]

Turns out this is not true for the JavascriptAppModalDialog tests; if the main
looper isn't specified, the message simply doesn't fire and all the tests time
out. I've had enough of Message though, so I'm switching to a simpler callback.
New version soon.

[Avi (use Gerrit), 2013-01-13 20:43:30]

OK, try this new version and let me know what you think.

[joth, 2013-01-14 17:21:28]

lgtm.

sorry about the Message headaches. Agree the synchronous callback i/f seems fine
here and avoids stuff going awol when the handler is null. (this is a useful
thing to know that I'd not come across before).

https://codereview.chromium.org/11788005/diff/48001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11788005/diff/48001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1111:
*                result will be json encoded in the parameter.
nit: add that the callback will be run on the main thread. (it is called on
chrome's UI thread, right?)

https://codereview.chromium.org/11788005/diff/48001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1116:
JavaScriptCallback callback) throws IllegalStateException {
nit: in java paramls always indents by 2 stops (8 spaces) rather than align with
paren.
(and line 2054)

[Avi (use Gerrit), 2013-01-14 17:28:54]

Re Message, yeah... I should have gone with this method from day one. Oh well.

https://codereview.chromium.org/11788005/diff/48001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11788005/diff/48001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1111:
*                result will be json encoded in the parameter.
On 2013/01/14 17:21:28, joth wrote:
> nit: add that the callback will be run on the main thread. (it is called on
> chrome's UI thread, right?)

Yep. Chrome's UI thread == main thread?

https://codereview.chromium.org/11788005/diff/48001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1116:
JavaScriptCallback callback) throws IllegalStateException {
On 2013/01/14 17:21:28, joth wrote:
> nit: in java paramls always indents by 2 stops (8 spaces) rather than align
with
> paren.
> (and line 2054)

Done.

[joth, 2013-01-14 17:37:15]

lgtm

On 2013/01/14 17:28:54, Avi wrote:
> Re Message, yeah... I should have gone with this method from day one. Oh well.
> 
>
https://codereview.chromium.org/11788005/diff/48001/content/public/android/ja...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/11788005/diff/48001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1111:
> *                result will be json encoded in the parameter.
> On 2013/01/14 17:21:28, joth wrote:
> > nit: add that the callback will be run on the main thread. (it is called on
> > chrome's UI thread, right?)
> 
> Yep. Chrome's UI thread == main thread?
> 

Correct.

>
https://codereview.chromium.org/11788005/diff/48001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1116:
> JavaScriptCallback callback) throws IllegalStateException {
> On 2013/01/14 17:21:28, joth wrote:
> > nit: in java paramls always indents by 2 stops (8 spaces) rather than align
> with
> > paren.
> > (and line 2054)
> 
> Done.

[Avi (use Gerrit), 2013-01-14 17:45:29]

On 2013/01/14 17:21:28, joth wrote:
> Agree the synchronous callback i/f seems fine
> here and avoids stuff going awol when the handler is null.

I have no idea why things didn't work. The Handler wasn't null, since I
subclassed it. It seems to have been a Looper problem, but the docs say that if
the current thread didn't have a Looper it would throw (which didn't seem to
happen). All I know are my traces, where a correct execution would look like:

16003:  02-27 21:47:29.968  8288  8288 W avilog  :
OnEvaluateJavaScriptResultHelper.evaluateJavaScript
16003:  02-27 21:47:29.968  8288  8288 W avilog  :
ContentViewCore.evaluateJavaScript
16003:  02-27 21:47:29.968  8288  8301 W avilog  :
OnEvaluateJavaScriptResultHelper.waitUntilHasValue()
16003:  02-27 21:47:30.039  8288  8288 W avilog  :
ContentViewCore.onEvaluateJavaScriptResult
16003:  02-27 21:47:30.039  8288  8288 W avilog  :
OnEvaluateJavaScriptResultHelper.evaluateJavaScript.handleMessage
16003:  02-27 21:47:30.039  8288  8288 W avilog  :
OnEvaluateJavaScriptResultHelper.notifyCalled
16003:  02-27 21:47:30.039  8288  8301 W avilog  :
OnEvaluateJavaScriptResultHelper.getJsonResultAndClear

and a failure would be:

16003:  02-27 21:50:48.554 11579 11592 W avilog  :
OnEvaluateJavaScriptResultHelper.evaluateJavaScript
16003:  02-27 21:50:48.554 11579 11592 W avilog  :
ContentViewCore.evaluateJavaScript (with message) 
16003:  02-27 21:50:48.765 11579 11592 W avilog  :
OnEvaluateJavaScriptResultHelper.waitUntilHasValue()
16003:  02-27 21:50:48.781 11579 11579 W avilog  :
ContentViewCore.onEvaluateJavaScriptResult
// ... um... where'd the message execution go? .. and 5s later...
16003:  02-27 21:50:53.875 11579 11592 I TestRunner: failed:
testPromptModalDialog(org.chromium.chrome.browser.test.ModalDialogTest)
16003:  02-27 21:50:53.875 11579 11592 I TestRunner: ----- begin exception -----
16003:  02-27 21:50:53.875 11579 11592 I TestRunner: 
16003:  02-27 21:50:53.875 11579 11592 I TestRunner:
java.util.concurrent.TimeoutException: waitUntilCriteria timed out!

[commit-bot: I haz the power, 2013-01-14 17:49:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11788005/41004

[commit-bot: I haz the power, 2013-01-15 01:32:18]

Change committed as 176773