Implement DocuementHasImages

Implement DocuementHasImages

Requires new plumbing to get custom android_webview
layer IPC messages a flowing.

BUG=


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

[Torne, 2012-08-29 10:34:29]

https://chromiumcodereview.appspot.com/10890024/diff/1/android_webview/aw_com...
File android_webview/aw_common.gypi (left):

https://chromiumcodereview.appspot.com/10890024/diff/1/android_webview/aw_com...
android_webview/aw_common.gypi:7: 'sources': [
Let's not add any more of these include files in this form; it's weird and
doesn't match what other chrome components do. I'll refactor the gyp files we
already have and send you a cl.

https://chromiumcodereview.appspot.com/10890024/diff/1/android_webview/lib/ma...
File android_webview/lib/main/webview_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/10890024/diff/1/android_webview/lib/ma...
android_webview/lib/main/webview_main_delegate.cc:22: class AwRendererClient :
public chrome::ChromeContentRendererClient {
AwContentRendererClient for consistency

https://chromiumcodereview.appspot.com/10890024/diff/1/android_webview/render...
File android_webview/renderer/DEPS (right):

https://chromiumcodereview.appspot.com/10890024/diff/1/android_webview/render...
android_webview/renderer/DEPS:2: "+third_party/WebKit/Source/WebKit/chromium",
Rather than add a new deps file here, I'd just put this rule in the one a level
above?

[joth, 2012-08-29 22:28:05]

thanks for the quick feedback on this.

http://codereview.chromium.org/10890024/diff/1/android_webview/aw_common.gypi
File android_webview/aw_common.gypi (left):

http://codereview.chromium.org/10890024/diff/1/android_webview/aw_common.gypi...
android_webview/aw_common.gypi:7: 'sources': [
On 2012/08/29 10:34:29, Torne wrote:
> Let's not add any more of these include files in this form; it's weird and
> doesn't match what other chrome components do. I'll refactor the gyp files we
> already have and send you a cl.

Thanks. I sent your CL to CQ so I can rebase onto it..

http://codereview.chromium.org/10890024/diff/1/android_webview/lib/main/webvi...
File android_webview/lib/main/webview_main_delegate.cc (right):

http://codereview.chromium.org/10890024/diff/1/android_webview/lib/main/webvi...
android_webview/lib/main/webview_main_delegate.cc:22: class AwRendererClient :
public chrome::ChromeContentRendererClient {
On 2012/08/29 10:34:29, Torne wrote:
> AwContentRendererClient for consistency

Yeah. We do have AwMainDelegate (no Content) now, but that's how Chrome does it
so agree lets keep in the same pattern as that.

http://codereview.chromium.org/10890024/diff/1/android_webview/renderer/DEPS
File android_webview/renderer/DEPS (right):

http://codereview.chromium.org/10890024/diff/1/android_webview/renderer/DEPS#...
android_webview/renderer/DEPS:2: "+third_party/WebKit/Source/WebKit/chromium",
On 2012/08/29 10:34:29, Torne wrote:
> Rather than add a new deps file here, I'd just put this rule in the one a
level
> above?
yeah that is how content and chrome do it.

I kind of liked the idea that we explicitly don't allow webkit code to be called
on browser side, especially as we're running single process only at the moment,
we might get away with calling some webkit API that would break in multiprocess.
mode.

(likewise we could have rules in browser/ and render/ to exclude depending on
each other. or do we get that automatically?)

[joth, 2012-08-30 02:29:15]

This is like almost ready to go; hitting some unrelated problems in trying to
run tests - if necessary I can enable the test in a follow-up, as I want to get
the plumbing of this one in place soon to avoid conflicts with others who need
some of these common pieces.
Thanks

[joth, 2012-08-30 02:58:57]

Test now passing so 'good to go'

Suggested bits for focus on:-

torne: gyp files, general file layout
leandro: IPC and how AwContents calls through to RenderView
mkosiba: test

thanks

[mkosiba (inactive), 2012-08-30 09:43:13]

http://codereview.chromium.org/10890024/diff/1020/android_webview/browser/ren...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

http://codereview.chromium.org/10890024/diff/1020/android_webview/browser/ren...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:26: static int
next_id = 1;
ugh... please make the function explicitly non-threadsafe with a DCHECK (more to
document the behavior than anything).

http://codereview.chromium.org/10890024/diff/1020/android_webview/browser/ren...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:50: if
(!handled) handled = WebContentsObserver::OnMessageReceived(message);
uber-nit; maybe a newline after ) ?

http://codereview.chromium.org/10890024/diff/1020/android_webview/browser/ren...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:62: }
I'd say we should DCHECK that a match is always found, unless you've found a
valid scenario where this is not the case?

http://codereview.chromium.org/10890024/diff/1020/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java
(right):

http://codereview.chromium.org/10890024/diff/1020/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:28:
private static final long TEST_TIMEOUT = 20000L;
maybe add this to the AndroidWebViewTestBase. I think it'd be better if we had
one central knob.

http://codereview.chromium.org/10890024/diff/1020/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:32:
private AwContents createContents(Context context) {
since you're already here.. :) please use createAwTestContainerView instead of
this.

http://codereview.chromium.org/10890024/diff/1020/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:71:
@SmallTest
feature annotation

http://codereview.chromium.org/10890024/diff/1020/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:82:
AwContents awContents = awContentsRef.get();
remove lines 73-82, replace with a call to
createAwTestContainerViewOnMainSync(mContentsClient)

[Torne, 2012-08-30 13:15:52]

gyp/layout LGTM, no other comments

[benm (inactive), 2012-08-30 13:30:45]

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/bro...
File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right):

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/bro...
android_webview/browser/renderer_host/aw_render_view_host_ext.h:12: // Provides
a convenience methods around a RenderViewHost for sending android
nix first 'a'?

for sending android ...?

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/ren...
File android_webview/renderer/aw_render_view_ext.cc (right):

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/ren...
android_webview/renderer/aw_render_view_ext.cc:42: 
extra new line?

[joth, 2012-08-30 17:26:53]

thanks

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/bro...
File android_webview/browser/renderer_host/aw_render_view_host_ext.cc (right):

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/bro...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:26: static int
next_id = 1;
On 2012/08/30 09:43:13, Martin Kosiba wrote:
> ugh... please make the function explicitly non-threadsafe with a DCHECK (more
to
> document the behavior than anything). 

Yeah the hidden static int isn't cutest thing. (I blame the code I was copying
http://codereview.chromium.org/10553014/ :-) but it is the prevailing pattern
for this I believe.


of course the map<> is also totally non-thread safe too. So yes, added
NonThreadSafe base class and sprinkled some DCHECKs around.

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/bro...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:50: if
(!handled) handled = WebContentsObserver::OnMessageReceived(message);
On 2012/08/30 09:43:13, Martin Kosiba wrote:
> uber-nit; maybe a newline after ) ?

You're right. 
Actually I can tertiary it..
  return handled ? true : WebContentsObserver::OnMessageReceived(message);

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/bro...
android_webview/browser/renderer_host/aw_render_view_host_ext.cc:62: }
On 2012/08/30 09:43:13, Martin Kosiba wrote:
> I'd say we should DCHECK that a match is always found, unless you've found a
> valid scenario where this is not the case?

A good question.
in general chrome code treats anything coming from the render process as slighty
suspect, and by far the most common pattern is to just disregard unexpected
message (Postel's law). 

Adding DCHECK sounds a good balance, but quickly starts to go right against
http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
(i.e. it's creating making a non-testable code path in debug builds)

So that suggests correctly handling it with a call to
web_content()->GetRenderProcessHost()->ReceivedBadMessage() -- but it seems
*extremely* rare for anyone to call that. (and, it's just a round about way to
commit suicide in single process mode).


DLOG(WARNING) ?

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/bro...
File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right):

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/bro...
android_webview/browser/renderer_host/aw_render_view_host_ext.h:12: // Provides
a convenience methods around a RenderViewHost for sending android
On 2012/08/30 13:30:46, benm wrote:
> nix first 'a'?
> 
> for sending android ...?

Done.

// Provides RenderViewHost wrapper functionality for sending WebView-specific
// IPC messages to the renderer and from there to WebKit.

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/jav...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java
(right):

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/jav...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:28:
private static final long TEST_TIMEOUT = 20000L;
On 2012/08/30 09:43:13, Martin Kosiba wrote:
> maybe add this to the AndroidWebViewTestBase. I think it'd be better if we had
> one central knob.

Done.
(actually switched to WAIT_TIMEOUT_SECONDS that already lives there)

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/jav...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:32:
private AwContents createContents(Context context) {
On 2012/08/30 09:43:13, Martin Kosiba wrote:
> since you're already here.. :) please use createAwTestContainerView instead of
> this.

Done.

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/jav...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:71:
@SmallTest
On 2012/08/30 09:43:13, Martin Kosiba wrote:
> feature annotation

Done.

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/jav...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:82:
AwContents awContents = awContentsRef.get();
On 2012/08/30 09:43:13, Martin Kosiba wrote:
> remove lines 73-82, replace with a call to
> createAwTestContainerViewOnMainSync(mContentsClient)

Brill, thanks. I thought that must already exist somewhere.

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/jav...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:124:
}
test reduced to 81 lines :-)

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/ren...
File android_webview/renderer/aw_render_view_ext.cc (right):

https://chromiumcodereview.appspot.com/10890024/diff/1020/android_webview/ren...
android_webview/renderer/aw_render_view_ext.cc:42: 
On 2012/08/30 13:30:46, benm wrote:
> extra new line?

Done.

[joth, 2012-08-30 23:32:52]

+jam, could you rubberstamp ipc/* and if you have time also look over the
common/* and render_view*.* to check my new IPC plumbing layer meets current
best practice?

Thanks!

[jam, 2012-08-31 02:18:37]

lgtm

[mkosiba (inactive), 2012-08-31 09:29:35]

LGTM

https://chromiumcodereview.appspot.com/10890024/diff/19019/android_webview/na...
File android_webview/native/aw_contents.cc (right):

https://chromiumcodereview.appspot.com/10890024/diff/19019/android_webview/na...
android_webview/native/aw_contents.cc:103: base::Owned(new
ScopedJavaGlobalRef<jobject>(
ok, this just looks like ScopedJavaGlobalRef is missing a constructor. up to you
if you want to make that part of this change, but I don't think the compiler is
smart enough to optimize away the ScopedJavaLocalRef..

[joth, 2012-08-31 22:13:39]

On 2012/08/31 09:29:35, Martin Kosiba wrote:
> LGTM
> 
>
https://chromiumcodereview.appspot.com/10890024/diff/19019/android_webview/na...
> File android_webview/native/aw_contents.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10890024/diff/19019/android_webview/na...
> android_webview/native/aw_contents.cc:103: base::Owned(new
> ScopedJavaGlobalRef<jobject>(
> ok, this just looks like ScopedJavaGlobalRef is missing a constructor. up to
you
> if you want to make that part of this change, but I don't think the compiler
is
> smart enough to optimize away the ScopedJavaLocalRef..

It's intentional that it doesn't have this c'tor: for ScopedLocalRef the
|jobject| c'tor "takes ownership" of the passed in (local) jobject, but for
ScopedGlobalRef it's not obvious what this would mean, and easy for people to
assume the wrong thing.
Some static factory methods would help, but they wouldn't really help here as
for normal use they would return the new ScopedGlobalRef by value whereas here
we (unusually) want it to be stack-based.
I can do it another way though -- construct an empty GlobalRef and call Reset()
on it, and then I can use base::Passed() instead.

yeah, other than "scoped_ptr<ScopedJavaGlobalRef<jobject> >" being rather a
mouthful, that works really well (and removes need for special base::Owned
comment on the callback method)

[commit-bot: I haz the power, 2012-08-31 22:32:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/10890024/19019

[commit-bot: I haz the power, 2012-09-01 01:33:06]

Change committed as 154564