Implement AwContentsClient and remove dependency on chrome/

Implement AwContentsClient and remove dependency on chrome/

We are not depending on much functionality from ContentClient yet, so most
methods are either no-op, left unimplemented with a TODO, or copied straight
from the ChromeContentsClient implementation.

BUG=


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

[boliu, 2012-10-08 22:43:42]

This was surprisingly painless in terms of getting things working again. I ran
through all the upstream aw instrumentation tests locally and did not appear to
break anything. Still need to try downstream and cts tests.

General questions:

Should I open bugs for each TODO?

There are methods that are no-op, which we don't need override. But overriding
makes it clear that we are choosing to have the method no-op. Everyone ok with
this?

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

http://codereview.chromium.org/11017024/diff/1/android_webview/DEPS#newcode18
android_webview/DEPS:18: "+ui/base",
ui/base for grd resources, webkit for user agent

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
File android_webview/common/aw_content_client.cc (right):

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:52: // TODO(boliu): Should we return
a ChromeWebView/<version> or similar?
This is not breaking any tests because webview uses the override user agent all
the time? Need to double check how the override ua is generated.

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
File android_webview/common/aw_content_client.h (right):

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.h:21: 
This unintentionally left as private, but hey, nothing is breaking...

http://codereview.chromium.org/11017024/diff/1/android_webview/lib/DEPS
File android_webview/lib/DEPS (left):

http://codereview.chromium.org/11017024/diff/1/android_webview/lib/DEPS#oldco...
android_webview/lib/DEPS:29:
"!chrome/browser/ui/app_modal_dialogs/javascript_dialog_creator.h",
This was left overs. Not really related to this patch.

http://codereview.chromium.org/11017024/diff/1/android_webview/lib/main/aw_ma...
File android_webview/lib/main/aw_main_delegate.h (right):

http://codereview.chromium.org/11017024/diff/1/android_webview/lib/main/aw_ma...
android_webview/lib/main/aw_main_delegate.h:10: #include
"android_webview/common/aw_content_client.h"
Doh! Forgot to sort.

[boliu, 2012-10-09 01:03:33]

Downstream instrumentation tests are ok. And cts tests *seem* to be ok, but I'm
hitting that activity resolver problem, so it might be hiding potential
problems.

[joth, 2012-10-09 02:00:47]

+mnaganov to comment on the UA changes.

Thanks for getting stuck into this! In general, I think dropping chrome/
dependencies (while keeping tests passing), and then incrementally adding back
in bits we might need at later date as we discover it will be easiest path for
us to understand where we are.

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
File android_webview/common/aw_content_client.cc (right):

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:22: 
on the general point - it's by design that the content-API design that callback
interface base classes like ContentClient may only provide _empty_
implementations of methods - just as a convenience. So no, we shouldn't need to
explicitly override for the "intentional no-op" cases

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:37: // asset files here?
assets etc, get bundled under the file: scheme so nothing to do here for them.
content: is a valid possibility here though.  I'm not entirely clear on the
use-case for savable_schemes. 'standard' schemes modifies GURL::IsStandard()
behavior, e.g. so now GetOrigin() on a gurl ==
"content://com.company.app.table/foo/bar/4" would give "com.company.app.table"
as the origin. Note the surprising big-endian domain ordering! 
So I'd prefer to stay on the side of doing nothing here speculatively: leave it
until we have a specific need.

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:47: return false;
for thoroughness, we make an effort to implement this correctly regardless of
current single-process-ness. So in this case I'd keep the method here but with a
comment saying 'currently none of the android_webview specific IPC messages need
to be processed for swapped out renderers.
(we could delegate this to a (static) method on AwRenderViewHostExt which
returns false.... but that's not consistent with other platforms and needless
plumbing at this point so inclined not to over engineer that at this point)

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:52: // TODO(boliu): Should we return
a ChromeWebView/<version> or similar?

certainly not ChromeWebiew -  see b/6381306 for long version.

I think just remove this method  (inherit empty base version) and GetUserAgent,
but mnaganov to have final say on that.
Otherwise, we should strictly return "Version" here for
BuildUserAgentFromProduct to do the right thing.

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
File android_webview/common/aw_content_client.h (right):

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.h:21: 
On 2012/10/08 22:43:43, boliu wrote:
> This unintentionally left as private, but hey, nothing is breaking...

:)

yeah general style is to make them public anyway as it gives false sense of
privacy to 'hide' public methods in subclasses.

[mnaganov (inactive), 2012-10-09 08:57:20]

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
File android_webview/common/aw_content_client.cc (right):

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:37: // asset files here?
On 2012/10/09 02:00:47, joth wrote:
> assets etc, get bundled under the file: scheme so nothing to do here for them.
> content: is a valid possibility here though.  I'm not entirely clear on the
> use-case for savable_schemes. 'standard' schemes modifies GURL::IsStandard()
> behavior, e.g. so now GetOrigin() on a gurl ==
> "content://com.company.app.table/foo/bar/4" would give "com.company.app.table"
> as the origin. Note the surprising big-endian domain ordering! 
> So I'd prefer to stay on the side of doing nothing here speculatively: leave
it
> until we have a specific need.

I agree, I don't see any clear benefit of having content: as a "standard
scheme."

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:52: // TODO(boliu): Should we return
a ChromeWebView/<version> or similar?
On 2012/10/09 02:00:47, joth wrote:
> 
> certainly not ChromeWebiew -  see b/6381306 for long version.
> 
> I think just remove this method  (inherit empty base version) and
GetUserAgent,
> but mnaganov to have final say on that.
> Otherwise, we should strictly return "Version" here for
> BuildUserAgentFromProduct to do the right thing.
> 

In our implementation, we always supply user agent string from settings (it's in
ContentViewCore::loadUrl), so the value provided here will never be used.

So, yes, leave the default empty implementation for now.

[Torne, 2012-10-09 11:07:38]

Second all the others' comments, other than that it looks pretty good. Glad this
one was not so hard :)

[boliu, 2012-10-09 18:01:47]

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
File android_webview/common/aw_content_client.cc (right):

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:22: 
On 2012/10/09 02:00:47, joth wrote:
> on the general point - it's by design that the content-API design that
callback
> interface base classes like ContentClient may only provide _empty_
> implementations of methods - just as a convenience. So no, we shouldn't need
to
> explicitly override for the "intentional no-op" cases

Removes all no-op or default behavior methods from this class (including ones we
don't think we need right now).

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:37: // asset files here?
On 2012/10/09 08:57:20, Mikhail Naganov (Chromium) wrote:
> On 2012/10/09 02:00:47, joth wrote:
> > assets etc, get bundled under the file: scheme so nothing to do here for
them.
> > content: is a valid possibility here though.  I'm not entirely clear on the
> > use-case for savable_schemes. 'standard' schemes modifies GURL::IsStandard()
> > behavior, e.g. so now GetOrigin() on a gurl ==
> > "content://com.company.app.table/foo/bar/4" would give
"com.company.app.table"
> > as the origin. Note the surprising big-endian domain ordering! 
> > So I'd prefer to stay on the side of doing nothing here speculatively: leave
> it
> > until we have a specific need.
> 
> I agree, I don't see any clear benefit of having content: as a "standard
> scheme."

No-op, so removing.

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:47: return false;
On 2012/10/09 02:00:47, joth wrote:
> for thoroughness, we make an effort to implement this correctly regardless of
> current single-process-ness. So in this case I'd keep the method here but with
a
> comment saying 'currently none of the android_webview specific IPC messages
need
> to be processed for swapped out renderers.
> (we could delegate this to a (static) method on AwRenderViewHostExt which
> returns false.... but that's not consistent with other platforms and needless
> plumbing at this point so inclined not to over engineer that at this point)
> 

So this is only for embedder (ie aw) messages, and we definitely don't need to
handle anything yet. Removing.

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:52: // TODO(boliu): Should we return
a ChromeWebView/<version> or similar?
On 2012/10/09 08:57:20, Mikhail Naganov (Chromium) wrote:
> On 2012/10/09 02:00:47, joth wrote:
> > 
> > certainly not ChromeWebiew -  see b/6381306 for long version.
> > 
> > I think just remove this method  (inherit empty base version) and
> GetUserAgent,
> > but mnaganov to have final say on that.
> > Otherwise, we should strictly return "Version" here for
> > BuildUserAgentFromProduct to do the right thing.
> > 
> 
> In our implementation, we always supply user agent string from settings (it's
in
> ContentViewCore::loadUrl), so the value provided here will never be used.
> 
> So, yes, leave the default empty implementation for now.

I see Version/4.0 is hardcoded in ContentSettings::GetDefaultUserAgent. I'm
making that method call here so we have one consistent place for managing
*default* user agent.

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
File android_webview/common/aw_content_client.h (right):

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.h:21: 
On 2012/10/09 02:00:47, joth wrote:
> On 2012/10/08 22:43:43, boliu wrote:
> > This unintentionally left as private, but hey, nothing is breaking...
> 
> :)
> 
> yeah general style is to make them public anyway as it gives false sense of
> privacy to 'hide' public methods in subclasses.

Done.

[joth, 2012-10-09 19:53:37]

lgtm

https://codereview.chromium.org/11017024/diff/3003/android_webview/common/aw_...
File android_webview/common/aw_content_client.cc (right):

https://codereview.chromium.org/11017024/diff/3003/android_webview/common/aw_...
android_webview/common/aw_content_client.cc:20: return
webkit_glue::BuildUserAgentFromProduct(GetProduct());
Mikhail and I were actually agreeing ot remove these methods... However I like
the idea of this being here rather than in the GetDefaultUserAgent imp, so lets
go with this.

aside - tracing through the glue layer and into ContentViewCore.loadURL() I
can't actually see where we hook the ('override') UA out of the settings
instance and pass down into the load. When Martin has added AwContents.loadURL
that will be the obvious place to do that.
Also having  a sane default set here then it gives us the option in future to
only supply the override to the NavigationEntry if the embedder has actually set
an override...

https://codereview.chromium.org/11017024/diff/3003/android_webview/lib/main/a...
File android_webview/lib/main/aw_main_delegate.h (right):

https://codereview.chromium.org/11017024/diff/3003/android_webview/lib/main/a...
android_webview/lib/main/aw_main_delegate.h:10: #include
"android_webview/common/aw_content_client.h"
sort me

[boliu, 2012-10-09 20:05:54]

http://codereview.chromium.org/11017024/diff/3003/android_webview/common/aw_c...
File android_webview/common/aw_content_client.cc (right):

http://codereview.chromium.org/11017024/diff/3003/android_webview/common/aw_c...
android_webview/common/aw_content_client.cc:20: return
webkit_glue::BuildUserAgentFromProduct(GetProduct());
On 2012/10/09 19:53:37, joth wrote:
> aside - tracing through the glue layer and into ContentViewCore.loadURL() I
> can't actually see where we hook the ('override') UA out of the settings
> instance and pass down into the load. When Martin has added AwContents.loadURL
> that will be the obvious place to do that.
> Also having  a sane default set here then it gives us the option in future to
> only supply the override to the NavigationEntry if the embedder has actually
set
> an override...

The UA override logic is all very complicated.....

First, for webview, we are setting the *override* ua every time the ua in
ContentSettings change. This is done by calling
ContentViewCore.setAllUserAgentOverridesInHistory in ContentViewCore.initialize
and every time ContentSettings ua changes, so this ensures for webview, the
override is always the correct ua we want.

Then in ContentViewCore.loadUrl, we set the request to use the override ua.

Now the path is used to get the *default* ua, when the webview client calls
setUserAgentString with null.

I think I got the details right here, but of course Mikhail can correct me if
anything is wrong.

[joth, 2012-10-09 20:25:20]

On 9 October 2012 13:05, <boliu@chromium.org> wrote:

>
> http://codereview.chromium.**org/11017024/diff/3003/**
>
android_webview/common/aw_**content_client.cc<http://codereview.chromium.org/11017024/diff/3003/android_webview/common/aw_content_client.cc>
> File android_webview/common/aw_**content_client.cc (right):
>
> http://codereview.chromium.**org/11017024/diff/3003/**
>
android_webview/common/aw_**content_client.cc#newcode20<http://codereview.chromium.org/11017024/diff/3003/android_webview/common/aw_content_client.cc#newcode20>
>
> android_webview/common/aw_**content_client.cc:20: return
> webkit_glue::**BuildUserAgentFromProduct(**GetProduct());
> On 2012/10/09 19:53:37, joth wrote:
>
>> aside - tracing through the glue layer and into
>>
> ContentViewCore.loadURL() I
>
>> can't actually see where we hook the ('override') UA out of the
>>
> settings
>
>> instance and pass down into the load. When Martin has added
>>
> AwContents.loadURL
>
>> that will be the obvious place to do that.
>> Also having  a sane default set here then it gives us the option in
>>
> future to
>
>> only supply the override to the NavigationEntry if the embedder has
>>
> actually set
>
>> an override...
>>
>
> The UA override logic is all very complicated.....
>
> First, for webview, we are setting the *override* ua every time the ua
> in ContentSettings change. This is done by calling
> ContentViewCore.**setAllUserAgentOverridesInHist**ory in
> ContentViewCore.initialize and every time ContentSettings ua changes, so
> this ensures for webview, the override is always the correct ua we want.
>
> Then in ContentViewCore.loadUrl, we set the request to use the override
> ua.
>
>
Ah, the "isPersonalityView()" fudge. Got it!
There we are ... another reason to have AwContents.loadUrl()




> Now the path is used to get the *default* ua, when the webview client
> calls setUserAgentString with null.
>
> I think I got the details right here, but of course Mikhail can correct
> me if anything is wrong.
>
>
http://codereview.chromium.**org/11017024/<http://codereview.chromium.org/110...
>

[commit-bot: I haz the power, 2012-10-09 20:34:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11017024/13001

[joth, 2012-10-09 21:33:32]

belated +joi to double check the DEPS changes.

[commit-bot: I haz the power, 2012-10-09 22:40:40]

List of reviewers changed. joi@chromium.org did a drive-by without LGTM'ing!

[boliu, 2012-10-09 23:57:16]

joth: Slipping in your style suggestions from downstream while waiting for Joi
to review DEPS changes.

Removed unused forward declaration and renamed var name.

[mnaganov (inactive), 2012-10-10 08:51:45]

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
File android_webview/common/aw_content_client.cc (right):

http://codereview.chromium.org/11017024/diff/1/android_webview/common/aw_cont...
android_webview/common/aw_content_client.cc:52: // TODO(boliu): Should we return
a ChromeWebView/<version> or similar?
On 2012/10/09 18:01:47, boliu wrote:
> On 2012/10/09 08:57:20, Mikhail Naganov (Chromium) wrote:
> > On 2012/10/09 02:00:47, joth wrote:
> > > 
> > > certainly not ChromeWebiew -  see b/6381306 for long version.
> > > 
> > > I think just remove this method  (inherit empty base version) and
> > GetUserAgent,
> > > but mnaganov to have final say on that.
> > > Otherwise, we should strictly return "Version" here for
> > > BuildUserAgentFromProduct to do the right thing.
> > > 
> > 
> > In our implementation, we always supply user agent string from settings
(it's
> in
> > ContentViewCore::loadUrl), so the value provided here will never be used.
> > 
> > So, yes, leave the default empty implementation for now.
> 
> I see Version/4.0 is hardcoded in ContentSettings::GetDefaultUserAgent. I'm
> making that method call here so we have one consistent place for managing
> *default* user agent.

Looks fine, thanks!

[Jói, 2012-10-10 09:47:37]

DEPS LGTM

[commit-bot: I haz the power, 2012-10-10 13:59:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11017024/2004

[commit-bot: I haz the power, 2012-10-10 16:01:48]

Change committed as 161120