Refactor the Android port to allow access to the chrome layer.

Refactor the Android port to allow access to the chrome layer.

While in desktop chrome the main WebContentsDelegate is implemented in the
chrome layer by the Browser class, the Android port implements it in the
content layer in its ContentViewClient class. However, because of the content
layering limitations this renders the chrome layer out of reach.

This patch splits the WebContentsDelegate implementation in ContentViewClient
into a separate class named WebContentsDelegateAndroid in the first chrome browser component "web_contents_delegate_android".

Also, this patch introduces stubs for Chrome-specific and WebView-specific extensions of WebContentsDelegateAndroid in order to set the foundations for later patches.


BUG=137967
TEST=existing tests

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

[Leandro Graciá Gil, 2012-07-27 23:24:59]

joth: base/android, content/browser/android code and any further general
comments you might have, as most of the original patches in this refactoring are
yours.

jam: please verify the approach we're taking and the changes in the content and
chrome folders. Sorry for the size of the patch. This refactoring should unblock
upstreaming other features like find in page.

[Leandro Graciá Gil, 2012-07-27 23:27:33]

joth: and also chrome/browser/android. Basically any android-specific code :)

On 2012/07/27 23:24:59, Leandro Graciá Gil wrote:
> joth: base/android, content/browser/android code and any further general
> comments you might have, as most of the original patches in this refactoring
are
> yours.
> 
> jam: please verify the approach we're taking and the changes in the content
and
> chrome folders. Sorry for the size of the patch. This refactoring should
unblock
> upstreaming other features like find in page.

[jam, 2012-07-27 23:45:23]

some comments before I drill down:

WebContentsDelegateAndroid seems too generic for this. Is this basically the C++
object in the browser side that hosts a WebContents for WebView or Android
browser? So maybe depending on that, it would be clearer to call it
AndroidBrowser/BrowserAndroid or WebView? In desktop chrome, the related object
is called Browser.

If this object depends on Chrome, and the java side object is ContentVew*, it
seems that those java files don't belong in content/, but also in chrome?

What's the timeline for removing the C++ ConventViewClient class? It's very odd
that it would be in content if the WCD it uses is in chrome.

[Leandro Graciá Gil, 2012-07-28 14:35:50]

On 2012/07/27 23:45:23, John Abd-El-Malek wrote:
> some comments before I drill down:
> 
> WebContentsDelegateAndroid seems too generic for this. Is this basically the
C++
> object in the browser side that hosts a WebContents for WebView or Android
> browser? So maybe depending on that, it would be clearer to call it
> AndroidBrowser/BrowserAndroid or WebView? In desktop chrome, the related
object
> is called Browser.

I think it is, but we should double-check with Joth. He's the one who made the
main split, I have been helping him with related issues and upstreaming his
changes. Sounds reasonable to call the class something similar to Browser as its
equivalent in desktop.

> 
> If this object depends on Chrome, and the java side object is ContentVew*, it
> seems that those java files don't belong in content/, but also in chrome?

If they're still in content it's likely that they are because something in
content requires it (probably ContentViewCore). I have already moved a few files
from content to chrome downstream and I'm happy to move more if we find it
appropriate. Which specific files are you interested in? I'll take another look.

> What's the timeline for removing the C++ ConventViewClient class? It's very
odd
> that it would be in content if the WCD it uses is in chrome.

I'm afraid I don't know. Joth, can you tell us about this?

[joth, 2012-07-31 00:42:04]

On 28 July 2012 07:35, <leandrogracia@chromium.org> wrote:

> On 2012/07/27 23:45:23, John Abd-El-Malek wrote:
>
>> some comments before I drill down:
>>
>
>  WebContentsDelegateAndroid seems too generic for this. Is this basically
>> the
>>
> C++
>
>> object in the browser side that hosts a WebContents for WebView or Android
>> browser? So maybe depending on that, it would be clearer to call it
>> AndroidBrowser/BrowserAndroid or WebView? In desktop chrome, the related
>>
> object
>
>> is called Browser.
>>
>
> I think it is, but we should double-check with Joth. He's the one who made
> the
> main split, I have been helping him with related issues and upstreaming his
> changes. Sounds reasonable to call the class something similar to Browser
> as its
> equivalent in desktop.
>
>
It's not quite equivalent to browser on desktop, as we have one WCDA
instance per WebContents, whereas desktop has one "singleton-ish" instance
shared across all WebContents
On android it's more convenient to have one per WebContents as we need to
store a java weak reference to the corresponding java-side object, and this
serves that purpose. Also we don't have a convenient C++ singleton object
to hang ownership of it off of for webview, and it seems counter to current
design guidelines to introduce new singletons.
Would life be better for us if we did define an equivalent singleton for
this?

I agree the name is on the wishy-washy side, but both C++ and java halves
of this are intended for derivation by concrete classes in both app and
webview, so it's a fairly generic purpose it is serving (i.e. providing a
default native -> java bridge for methods not overridden by the embedder on
the native side) so the name needs to be fairly generic.



>
>
>  If this object depends on Chrome, and the java side object is
>> ContentVew*, it
>> seems that those java files don't belong in content/, but also in chrome?
>>
>
> If they're still in content it's likely that they are because something in
> content requires it (probably ContentViewCore). I have already moved a few
> files
> from content to chrome downstream and I'm happy to move more if we find it
> appropriate. Which specific files are you interested in? I'll take another
> look.
>
>
WCDA has its own
native<https://chromiumcodereview.appspot.com/10831060/diff/1/chrome/browser/android/web_contents_delegate_android.h>and
java<https://chromiumcodereview.appspot.com/10831060/diff/1/chrome/android/java/src/org/chromium/chrome/browser/WebContentsDelegateAndroid.java>sides.
It has no dependency on ContentView*.



>
>  What's the timeline for removing the C++ ConventViewClient class? It's
>> very
>>
> odd
>
>> that it would be in content if the WCD it uses is in chrome.
>>
>
> I'm afraid I don't know. Joth, can you tell us about this?
>
>  ContentViewClient knows nothing about WCD -- with this change it is not
using a WCD at all.
(It does indirectly receive some WebContents_Observer_ callbacks... these
will eventually be removed and then ContentViewClient can become a
java-only class, however I'm delaying doing that change right now as it
will block areas several others are working on (upstreaming ContentView)
and doesn't have any immediate pressing need like we had for WCD.)





>
>
https://chromiumcodereview.**appspot.com/10831060/<https://chromiumcodereview...
>

[jam, 2012-07-31 22:58:20]

On 2012/07/31 00:42:04, joth wrote:
> On 28 July 2012 07:35, <mailto:leandrogracia@chromium.org> wrote:
> 
> > On 2012/07/27 23:45:23, John Abd-El-Malek wrote:
> >
> >> some comments before I drill down:
> >>
> >
> >  WebContentsDelegateAndroid seems too generic for this. Is this basically
> >> the
> >>
> > C++
> >
> >> object in the browser side that hosts a WebContents for WebView or Android
> >> browser? So maybe depending on that, it would be clearer to call it
> >> AndroidBrowser/BrowserAndroid or WebView? In desktop chrome, the related
> >>
> > object
> >
> >> is called Browser.
> >>
> >
> > I think it is, but we should double-check with Joth. He's the one who made
> > the
> > main split, I have been helping him with related issues and upstreaming his
> > changes. Sounds reasonable to call the class something similar to Browser
> > as its
> > equivalent in desktop.
> >
> >
> It's not quite equivalent to browser on desktop, as we have one WCDA
> instance per WebContents, whereas desktop has one "singleton-ish" instance
> shared across all WebContents
> On android it's more convenient to have one per WebContents as we need to
> store a java weak reference to the corresponding java-side object, and this
> serves that purpose. Also we don't have a convenient C++ singleton object
> to hang ownership of it off of for webview, and it seems counter to current
> design guidelines to introduce new singletons.
> Would life be better for us if we did define an equivalent singleton for
> this?
> 
> I agree the name is on the wishy-washy side, but both C++ and java halves
> of this are intended for derivation by concrete classes in both app and
> webview, so it's a fairly generic purpose it is serving (i.e. providing a
> default native -> java bridge for methods not overridden by the embedder on
> the native side) so the name needs to be fairly generic.

ah I see. if you're going to derive from it anyways, then the name is fine.

> 
> 
> 
> >
> >
> >  If this object depends on Chrome, and the java side object is
> >> ContentVew*, it
> >> seems that those java files don't belong in content/, but also in chrome?
> >>
> >
> > If they're still in content it's likely that they are because something in
> > content requires it (probably ContentViewCore). I have already moved a few
> > files
> > from content to chrome downstream and I'm happy to move more if we find it
> > appropriate. Which specific files are you interested in? I'll take another
> > look.
> >
> >
> WCDA has its own
>
native<https://chromiumcodereview.appspot.com/10831060/diff/1/chrome/browser/android/web_contents_delegate_android.h>and
>
java<https://chromiumcodereview.appspot.com/10831060/diff/1/chrome/android/java/src/org/chromium/chrome/browser/WebContentsDelegateAndroid.java>sides.
> It has no dependency on ContentView*.

ah, my bad

> 
> 
> 
> >
> >  What's the timeline for removing the C++ ConventViewClient class? It's
> >> very
> >>
> > odd
> >
> >> that it would be in content if the WCD it uses is in chrome.
> >>
> >
> > I'm afraid I don't know. Joth, can you tell us about this?
> >
> >  ContentViewClient knows nothing about WCD -- with this change it is not
> using a WCD at all.
> (It does indirectly receive some WebContents_Observer_ callbacks... these
> will eventually be removed and then ContentViewClient can become a
> java-only class, however I'm delaying doing that change right now as it
> will block areas several others are working on (upstreaming ContentView)
> and doesn't have any immediate pressing need like we had for WCD.)
> 
> 
> 
> 
> 
> >
> >
>
https://chromiumcodereview.**appspot.com/10831060/%3Chttps://chromiumcoderevi...>
> >

[Leandro Graciá Gil, 2012-08-03 00:03:44]

Any changes I should do to the current approach before review, then? Thanks for
taking a look to the overall design.

On 2012/07/31 22:58:20, John Abd-El-Malek wrote:
> On 2012/07/31 00:42:04, joth wrote:
> > On 28 July 2012 07:35, <mailto:leandrogracia@chromium.org> wrote:
> > 
> > > On 2012/07/27 23:45:23, John Abd-El-Malek wrote:
> > >
> > >> some comments before I drill down:
> > >>
> > >
> > >  WebContentsDelegateAndroid seems too generic for this. Is this basically
> > >> the
> > >>
> > > C++
> > >
> > >> object in the browser side that hosts a WebContents for WebView or
Android
> > >> browser? So maybe depending on that, it would be clearer to call it
> > >> AndroidBrowser/BrowserAndroid or WebView? In desktop chrome, the related
> > >>
> > > object
> > >
> > >> is called Browser.
> > >>
> > >
> > > I think it is, but we should double-check with Joth. He's the one who made
> > > the
> > > main split, I have been helping him with related issues and upstreaming
his
> > > changes. Sounds reasonable to call the class something similar to Browser
> > > as its
> > > equivalent in desktop.
> > >
> > >
> > It's not quite equivalent to browser on desktop, as we have one WCDA
> > instance per WebContents, whereas desktop has one "singleton-ish" instance
> > shared across all WebContents
> > On android it's more convenient to have one per WebContents as we need to
> > store a java weak reference to the corresponding java-side object, and this
> > serves that purpose. Also we don't have a convenient C++ singleton object
> > to hang ownership of it off of for webview, and it seems counter to current
> > design guidelines to introduce new singletons.
> > Would life be better for us if we did define an equivalent singleton for
> > this?
> > 
> > I agree the name is on the wishy-washy side, but both C++ and java halves
> > of this are intended for derivation by concrete classes in both app and
> > webview, so it's a fairly generic purpose it is serving (i.e. providing a
> > default native -> java bridge for methods not overridden by the embedder on
> > the native side) so the name needs to be fairly generic.
> 
> ah I see. if you're going to derive from it anyways, then the name is fine.
> 
> > 
> > 
> > 
> > >
> > >
> > >  If this object depends on Chrome, and the java side object is
> > >> ContentVew*, it
> > >> seems that those java files don't belong in content/, but also in chrome?
> > >>
> > >
> > > If they're still in content it's likely that they are because something in
> > > content requires it (probably ContentViewCore). I have already moved a few
> > > files
> > > from content to chrome downstream and I'm happy to move more if we find it
> > > appropriate. Which specific files are you interested in? I'll take another
> > > look.
> > >
> > >
> > WCDA has its own
> >
>
native<https://chromiumcodereview.appspot.com/10831060/diff/1/chrome/browser/android/web_contents_delegate_android.h>and
> >
>
java<https://chromiumcodereview.appspot.com/10831060/diff/1/chrome/android/java/src/org/chromium/chrome/browser/WebContentsDelegateAndroid.java>sides.
> > It has no dependency on ContentView*.
> 
> ah, my bad
> 
> > 
> > 
> > 
> > >
> > >  What's the timeline for removing the C++ ConventViewClient class? It's
> > >> very
> > >>
> > > odd
> > >
> > >> that it would be in content if the WCD it uses is in chrome.
> > >>
> > >
> > > I'm afraid I don't know. Joth, can you tell us about this?
> > >
> > >  ContentViewClient knows nothing about WCD -- with this change it is not
> > using a WCD at all.
> > (It does indirectly receive some WebContents_Observer_ callbacks... these
> > will eventually be removed and then ContentViewClient can become a
> > java-only class, however I'm delaying doing that change right now as it
> > will block areas several others are working on (upstreaming ContentView)
> > and doesn't have any immediate pressing need like we had for WCD.)
> > 
> > 
> > 
> > 
> > 
> > >
> > >
> >
>
https://chromiumcodereview.**appspot.com/10831060/%253Chttps://chromiumcodere...>
> > >

[jam, 2012-08-06 18:16:58]

On 2012/08/03 00:03:44, Leandro Graciá Gil wrote:
> Any changes I should do to the current approach before review, then? Thanks
for
> taking a look to the overall design.

nope

[joth, 2012-08-07 19:42:20]

+joi
per discussion earlier, this is the patch already landed  downstream.
WebContentsDelegateAndroid is a reusable 'component' of sorts for forwarding
callbacks over to a java-side embedder, but still allowing the embedder to
override some callbacks native side if it wishes (e.g. for simpler code reuse
and efficiency).

It's used by the android chrome app and webview, so suggests it should be a
component? (It already has no other dependency on anything else in chrome/)
*however* the java bridging is also convenient to use in java-driven test code,
including in the content layer, but right now we have no way to reuse it there
so we end-up duplicating some of that; or worse, people write their new tests
for content APIs up in the chrome layer, as that gives more convenient tools
such as this for writing the test. AFAICT the most common thing the test code
wants from it is callbacks for WebContentsDelegate::LoadingStateChanged or
LoadProgressChanged to know when some test step is 'done'.

[Jói, 2012-08-08 15:19:39]

I agree it seems like WebContentsDelegateAndroid should be moved to
somewhere you can reuse it from Android code under both content/ and
chrome/.

As a first step you could write a DEPS file for chrome/browser/android
that disallows chrome/browser and allows chrome/browser/android; I see
no dependencies in my current upstream checkout that would violate
these rules.  Regardless, you should probably add a DEPS file to the
directory.

Perhaps though, there is nothing special to wait for to move
WebContentsDelegateAndroid to somewhere reusable (it doesn't have to
be done in this change though).  We just have to decide about the new
top level directory.

The rules for it are obvious enough I think; it would be a top-level
directory and code under it could depend on all the same stuff
src/chrome/ depends on (i.e. content/ and below), but nothing within
src/chrome/, but may get abstract dependencies injected for stuff that
might be implemented by src/chrome/ or by a different implementation
of a browser or app container.  It would be for components that aren't
generic enough to live in content/ but which have been removed of
concrete dependencies of chrome/-specific stuff.

Naming is tougher; we could consider e.g.
- src/browser_components
- src/chromeponents
- src/chromosomes
- src/chromebits
- src/cc or src/cbc or src/crc for something like content-based
components or Chrome components or something like that

Cheers,
Jói



On Tue, Aug 7, 2012 at 7:42 PM,  <joth@chromium.org> wrote:
> +joi
> per discussion earlier, this is the patch already landed  downstream.
> WebContentsDelegateAndroid is a reusable 'component' of sorts for forwarding
> callbacks over to a java-side embedder, but still allowing the embedder to
> override some callbacks native side if it wishes (e.g. for simpler code
> reuse
> and efficiency).
>
> It's used by the android chrome app and webview, so suggests it should be a
> component? (It already has no other dependency on anything else in chrome/)
> *however* the java bridging is also convenient to use in java-driven test
> code,
> including in the content layer, but right now we have no way to reuse it
> there
> so we end-up duplicating some of that; or worse, people write their new
> tests
> for content APIs up in the chrome layer, as that gives more convenient tools
> such as this for writing the test. AFAICT the most common thing the test
> code
> wants from it is callbacks for WebContentsDelegate::LoadingStateChanged or
> LoadProgressChanged to know when some test step is 'done'.
>
>
> http://codereview.chromium.org/10831060/

[Jói, 2012-08-08 15:28:26]

Looking at WebContentsDelegateAndroid (C++) some more, is there a
reason why it shouldn't actually just live in content/?  It seems
reasonably generic and its only chrome/ dependency seems to be on
chrome/browser/android/java_conversion_utils.h which I presume doesn't
have many dependencies.

Cheers,
Jói


On Wed, Aug 8, 2012 at 3:18 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> I agree it seems like WebContentsDelegateAndroid should be moved to
> somewhere you can reuse it from Android code under both content/ and
> chrome/.
>
> As a first step you could write a DEPS file for chrome/browser/android
> that disallows chrome/browser and allows chrome/browser/android; I see
> no dependencies in my current upstream checkout that would violate
> these rules.  Regardless, you should probably add a DEPS file to the
> directory.
>
> Perhaps though, there is nothing special to wait for to move
> WebContentsDelegateAndroid to somewhere reusable (it doesn't have to
> be done in this change though).  We just have to decide about the new
> top level directory.
>
> The rules for it are obvious enough I think; it would be a top-level
> directory and code under it could depend on all the same stuff
> src/chrome/ depends on (i.e. content/ and below), but nothing within
> src/chrome/, but may get abstract dependencies injected for stuff that
> might be implemented by src/chrome/ or by a different implementation
> of a browser or app container.  It would be for components that aren't
> generic enough to live in content/ but which have been removed of
> concrete dependencies of chrome/-specific stuff.
>
> Naming is tougher; we could consider e.g.
> - src/browser_components
> - src/chromeponents
> - src/chromosomes
> - src/chromebits
> - src/cc or src/cbc or src/crc for something like content-based
> components or Chrome components or something like that
>
> Cheers,
> Jói
>
>
>
> On Tue, Aug 7, 2012 at 7:42 PM,  <joth@chromium.org> wrote:
>> +joi
>> per discussion earlier, this is the patch already landed  downstream.
>> WebContentsDelegateAndroid is a reusable 'component' of sorts for forwarding
>> callbacks over to a java-side embedder, but still allowing the embedder to
>> override some callbacks native side if it wishes (e.g. for simpler code
>> reuse
>> and efficiency).
>>
>> It's used by the android chrome app and webview, so suggests it should be a
>> component? (It already has no other dependency on anything else in chrome/)
>> *however* the java bridging is also convenient to use in java-driven test
>> code,
>> including in the content layer, but right now we have no way to reuse it
>> there
>> so we end-up duplicating some of that; or worse, people write their new
>> tests
>> for content APIs up in the chrome layer, as that gives more convenient tools
>> such as this for writing the test. AFAICT the most common thing the test
>> code
>> wants from it is callbacks for WebContentsDelegate::LoadingStateChanged or
>> LoadProgressChanged to know when some test step is 'done'.
>>
>>
>> http://codereview.chromium.org/10831060/

[Leandro Graciá Gil, 2012-08-08 15:33:43]

There are more incoming chrome dependencies that haven't been upstreamed yet.
For example, for the find-in-page feature we need to be able to access
FindTabHelper in the chrome layer. The dependency you see in
java_conversion_utils is in fact the result of refactoring to solve some DEPS
layering violations that appeared when moving WebContentsDelegateAndroid from
content to chrome.


On 2012/08/08 15:28:26, Jói wrote:
> Looking at WebContentsDelegateAndroid (C++) some more, is there a
> reason why it shouldn't actually just live in content/?  It seems
> reasonably generic and its only chrome/ dependency seems to be on
> chrome/browser/android/java_conversion_utils.h which I presume doesn't
> have many dependencies.
> 
> Cheers,
> Jói
> 
> 
> On Wed, Aug 8, 2012 at 3:18 PM, Jói Sigurðsson <mailto:joi@chromium.org>
wrote:
> > I agree it seems like WebContentsDelegateAndroid should be moved to
> > somewhere you can reuse it from Android code under both content/ and
> > chrome/.
> >
> > As a first step you could write a DEPS file for chrome/browser/android
> > that disallows chrome/browser and allows chrome/browser/android; I see
> > no dependencies in my current upstream checkout that would violate
> > these rules.  Regardless, you should probably add a DEPS file to the
> > directory.
> >
> > Perhaps though, there is nothing special to wait for to move
> > WebContentsDelegateAndroid to somewhere reusable (it doesn't have to
> > be done in this change though).  We just have to decide about the new
> > top level directory.
> >
> > The rules for it are obvious enough I think; it would be a top-level
> > directory and code under it could depend on all the same stuff
> > src/chrome/ depends on (i.e. content/ and below), but nothing within
> > src/chrome/, but may get abstract dependencies injected for stuff that
> > might be implemented by src/chrome/ or by a different implementation
> > of a browser or app container.  It would be for components that aren't
> > generic enough to live in content/ but which have been removed of
> > concrete dependencies of chrome/-specific stuff.
> >
> > Naming is tougher; we could consider e.g.
> > - src/browser_components
> > - src/chromeponents
> > - src/chromosomes
> > - src/chromebits
> > - src/cc or src/cbc or src/crc for something like content-based
> > components or Chrome components or something like that
> >
> > Cheers,
> > Jói
> >
> >
> >
> > On Tue, Aug 7, 2012 at 7:42 PM,  <mailto:joth@chromium.org> wrote:
> >> +joi
> >> per discussion earlier, this is the patch already landed  downstream.
> >> WebContentsDelegateAndroid is a reusable 'component' of sorts for
forwarding
> >> callbacks over to a java-side embedder, but still allowing the embedder to
> >> override some callbacks native side if it wishes (e.g. for simpler code
> >> reuse
> >> and efficiency).
> >>
> >> It's used by the android chrome app and webview, so suggests it should be a
> >> component? (It already has no other dependency on anything else in chrome/)
> >> *however* the java bridging is also convenient to use in java-driven test
> >> code,
> >> including in the content layer, but right now we have no way to reuse it
> >> there
> >> so we end-up duplicating some of that; or worse, people write their new
> >> tests
> >> for content APIs up in the chrome layer, as that gives more convenient
tools
> >> such as this for writing the test. AFAICT the most common thing the test
> >> code
> >> wants from it is callbacks for WebContentsDelegate::LoadingStateChanged or
> >> LoadProgressChanged to know when some test step is 'done'.
> >>
> >>
> >> http://codereview.chromium.org/10831060/

[Leandro Graciá Gil, 2012-08-08 16:31:14]

On 2012/08/08 15:19:39, Jói wrote:
> As a first step you could write a DEPS file for chrome/browser/android
> that disallows chrome/browser and allows chrome/browser/android; I see
> no dependencies in my current upstream checkout that would violate
> these rules.  Regardless, you should probably add a DEPS file to the
> directory.

Adding chrome/browser/android but removing chrome/browser in DEPS is generating
many DEPS errors for the following dependencies upstream:
- chrome/browser/browser_process.h
- chrome/browser/profiles/profile.h
- chrome/browser/profiles/profile_manager.h
- chrome/browser/history/top_sites.h

There might be further issues with code not yet upstreamed. For example,
find-in-page will introduce at least these:
- chrome/browser/ui/find_bar/find_tab_helper.h
- chrome/browser/ui/find_bar/find_notification_details.h

I don't completely understand your proposal about DEPS, the naming and the new
folders. Could you elaborate, please?

[Leandro Graciá Gil, 2012-08-08 16:36:11]

A few extra chrome layer DEPS in the downstream chrome/browser/android code:
- chrome/browser/ui/tab_contents/tab_contents.h
- chrome/browser/history/android/sqlite_cursor.h
- chrome/browser/ui/android/chrome_http_auth_handler.h
- chrome/browser/ui/android/js_modal_dialog_android.h
- chrome/browser/bookmarks/bookmark_model.h
- chrome/browser/content_settings/host_content_settings_map.h
- chrome/browser/net/chrome_url_request_context.h
- chrome/browser/infobars/infobar_delegate.h
- chrome/browser/tab_contents/link_infobar_delegate.h


On 2012/08/08 16:31:14, Leandro Graciá Gil wrote:
> On 2012/08/08 15:19:39, Jói wrote:
> > As a first step you could write a DEPS file for chrome/browser/android
> > that disallows chrome/browser and allows chrome/browser/android; I see
> > no dependencies in my current upstream checkout that would violate
> > these rules.  Regardless, you should probably add a DEPS file to the
> > directory.
> 
> Adding chrome/browser/android but removing chrome/browser in DEPS is
generating
> many DEPS errors for the following dependencies upstream:
> - chrome/browser/browser_process.h
> - chrome/browser/profiles/profile.h
> - chrome/browser/profiles/profile_manager.h
> - chrome/browser/history/top_sites.h
> 
> There might be further issues with code not yet upstreamed. For example,
> find-in-page will introduce at least these:
> - chrome/browser/ui/find_bar/find_tab_helper.h
> - chrome/browser/ui/find_bar/find_notification_details.h
> 
> I don't completely understand your proposal about DEPS, the naming and the new
> folders. Could you elaborate, please?

[Jói, 2012-08-08 16:44:51]

For background, Jonathan had suggested that the
WebContentsDelegateAndroid didn't have any dependencies on
chrome/browser and that it would be good to be able to reuse it in
code not just in chrome/ but in app/ and content/ as well, and that
possibly it should become some kind of top-level component.  My
initial suggestion to add a DEPS file was to help ensure that
WebContentsDelegateAndroid didn't accidentally start getting further
chrome/browser/ dependencies, and when I looked at
chrome/browser/android/ in my local checkout (which is upstream) it
didn't look like any of the files had chrome/browser/ dependencies
other than chrome/browser/android/ (but I may have made a mistake with
my grep commands).  My suggestion to perhaps move it to a new
top-level directory came out of similar discussions.

For further background, I'm working on this:
http://www.chromium.org/developers/design-documents/browser-components

If there is a set of stuff from chrome/ that chrome/browser/android/
shouldn't use (long-term) then it would be good to have a DEPS file
that expresses that, but you don't have to do it in the current
change.  DEPS files can now express temporarily-allowed dependencies
(see e.g. chrome/browser/autofill/DEPS) and that feature could be used
to tag dependencies that you want to get rid of long-term.

For the current review, I'm not sure any changes are needed, so
apologies for the confusion.  Jonathan and I should have probably
continued the discussion on a separate thread rather than within the
review thread.

Cheers,
Jói




On Wed, Aug 8, 2012 at 4:31 PM,  <leandrogracia@chromium.org> wrote:
> On 2012/08/08 15:19:39, Jói wrote:
>>
>> As a first step you could write a DEPS file for chrome/browser/android
>> that disallows chrome/browser and allows chrome/browser/android; I see
>> no dependencies in my current upstream checkout that would violate
>> these rules.  Regardless, you should probably add a DEPS file to the
>> directory.
>
>
> Adding chrome/browser/android but removing chrome/browser in DEPS is
> generating
> many DEPS errors for the following dependencies upstream:
> - chrome/browser/browser_process.h
> - chrome/browser/profiles/profile.h
> - chrome/browser/profiles/profile_manager.h
> - chrome/browser/history/top_sites.h
>
> There might be further issues with code not yet upstreamed. For example,
> find-in-page will introduce at least these:
> - chrome/browser/ui/find_bar/find_tab_helper.h
> - chrome/browser/ui/find_bar/find_notification_details.h
>
> I don't completely understand your proposal about DEPS, the naming and the
> new
> folders. Could you elaborate, please?
>
> https://chromiumcodereview.appspot.com/10831060/

[Leandro Graciá Gil, 2012-08-08 16:58:35]

Thanks for the clarification. Let's have this reviewed and ready to land.

On 2012/08/08 16:44:51, Jói wrote:
> For background, Jonathan had suggested that the
> WebContentsDelegateAndroid didn't have any dependencies on
> chrome/browser and that it would be good to be able to reuse it in
> code not just in chrome/ but in app/ and content/ as well, and that
> possibly it should become some kind of top-level component.  My
> initial suggestion to add a DEPS file was to help ensure that
> WebContentsDelegateAndroid didn't accidentally start getting further
> chrome/browser/ dependencies, and when I looked at
> chrome/browser/android/ in my local checkout (which is upstream) it
> didn't look like any of the files had chrome/browser/ dependencies
> other than chrome/browser/android/ (but I may have made a mistake with
> my grep commands).  My suggestion to perhaps move it to a new
> top-level directory came out of similar discussions.
> 
> For further background, I'm working on this:
> http://www.chromium.org/developers/design-documents/browser-components
> 
> If there is a set of stuff from chrome/ that chrome/browser/android/
> shouldn't use (long-term) then it would be good to have a DEPS file
> that expresses that, but you don't have to do it in the current
> change.  DEPS files can now express temporarily-allowed dependencies
> (see e.g. chrome/browser/autofill/DEPS) and that feature could be used
> to tag dependencies that you want to get rid of long-term.
> 
> For the current review, I'm not sure any changes are needed, so
> apologies for the confusion.  Jonathan and I should have probably
> continued the discussion on a separate thread rather than within the
> review thread.
> 
> Cheers,
> Jói
> 
> 
> 
> 
> On Wed, Aug 8, 2012 at 4:31 PM,  <mailto:leandrogracia@chromium.org> wrote:
> > On 2012/08/08 15:19:39, Jói wrote:
> >>
> >> As a first step you could write a DEPS file for chrome/browser/android
> >> that disallows chrome/browser and allows chrome/browser/android; I see
> >> no dependencies in my current upstream checkout that would violate
> >> these rules.  Regardless, you should probably add a DEPS file to the
> >> directory.
> >
> >
> > Adding chrome/browser/android but removing chrome/browser in DEPS is
> > generating
> > many DEPS errors for the following dependencies upstream:
> > - chrome/browser/browser_process.h
> > - chrome/browser/profiles/profile.h
> > - chrome/browser/profiles/profile_manager.h
> > - chrome/browser/history/top_sites.h
> >
> > There might be further issues with code not yet upstreamed. For example,
> > find-in-page will introduce at least these:
> > - chrome/browser/ui/find_bar/find_tab_helper.h
> > - chrome/browser/ui/find_bar/find_notification_details.h
> >
> > I don't completely understand your proposal about DEPS, the naming and the
> > new
> > folders. Could you elaborate, please?
> >
> > https://chromiumcodereview.appspot.com/10831060/

[Leandro Graciá Gil, 2012-08-20 13:30:58]

Ok, we finally have the first browser component here. This patch splits the
WebContentsDelegate implementation out of ContentViewClient to the new browser
component and introduces stubs for both Chrome on Android and WebView sides.

It's a big patch since many files are affected and some other code need to move
together with the changes above, but please let me know if you have any
questions.

[Jói, 2012-08-20 14:02:39]

I haven't reviewed the implementation in detail yet, was looking primarily at
this from a structural perspective.

In general I think the structure looks right, although note the comment about a
.gypi file below.

Please add a chrome/browser/component/OWNERS with my address in it.

Cheers,
Jói

https://chromiumcodereview.appspot.com/10831060/diff/6006/android_webview/nat...
File android_webview/native/webview_web_contents_delegate_android.h (right):

https://chromiumcodereview.appspot.com/10831060/diff/6006/android_webview/nat...
android_webview/native/webview_web_contents_delegate_android.h:17: class
WebViewWebContentsDelegateAndroid
I think joth@ had proposed Aw as a prefix instead of WebView.

https://chromiumcodereview.appspot.com/10831060/diff/6006/android_webview/nat...
android_webview/native/webview_web_contents_delegate_android.h:18: : public
chrome::component::web_contents_delegate_android::
This namespace nesting is pretty deep.  Personally, I'd be fine with just a
single namespace per component, if any, so e.g. just
web_contents_delegate_android.

https://chromiumcodereview.appspot.com/10831060/diff/6006/chrome/browser/comp...
File chrome/browser/component/web_contents_delegate_android/DEPS (right):

https://chromiumcodereview.appspot.com/10831060/diff/6006/chrome/browser/comp...
chrome/browser/component/web_contents_delegate_android/DEPS:2:
"-chrome/browser",
please add a DEPS file at the chrome/browser/component level.  It would have the
following, meaning this line could be deleted from the present DEPS file:

include_rules = [
  "-chrome/browser",
  "+chrome/browser/api",
]

https://chromiumcodereview.appspot.com/10831060/diff/6006/chrome/chrome_brows...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10831060/diff/6006/chrome/chrome_brows...
chrome/chrome_browser.gypi:965:
'browser/component/web_contents_delegate_android/component_jni_registrar.cc',
The component should have a .gypi file within its directory that creates the
component's lib, and the 'browser' target should just depend on that.

[Leandro Graciá Gil, 2012-08-20 20:26:17]

https://chromiumcodereview.appspot.com/10831060/diff/6006/android_webview/nat...
File android_webview/native/webview_web_contents_delegate_android.h (right):

https://chromiumcodereview.appspot.com/10831060/diff/6006/android_webview/nat...
android_webview/native/webview_web_contents_delegate_android.h:17: class
WebViewWebContentsDelegateAndroid
On 2012/08/20 14:02:39, Jói wrote:
> I think joth@ had proposed Aw as a prefix instead of WebView.

Confirmed the name to use with Joth. Renaming.

https://chromiumcodereview.appspot.com/10831060/diff/6006/android_webview/nat...
android_webview/native/webview_web_contents_delegate_android.h:18: : public
chrome::component::web_contents_delegate_android::
On 2012/08/20 14:02:39, Jói wrote:
> This namespace nesting is pretty deep.  Personally, I'd be fine with just a
> single namespace per component, if any, so e.g. just
> web_contents_delegate_android.

Done.

https://chromiumcodereview.appspot.com/10831060/diff/6006/chrome/browser/comp...
File chrome/browser/component/web_contents_delegate_android/DEPS (right):

https://chromiumcodereview.appspot.com/10831060/diff/6006/chrome/browser/comp...
chrome/browser/component/web_contents_delegate_android/DEPS:2:
"-chrome/browser",
On 2012/08/20 14:02:39, Jói wrote:
> please add a DEPS file at the chrome/browser/component level.  It would have
the
> following, meaning this line could be deleted from the present DEPS file:
> 
> include_rules = [
>   "-chrome/browser",
>   "+chrome/browser/api",
> ]

Done.

https://chromiumcodereview.appspot.com/10831060/diff/6006/chrome/chrome_brows...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10831060/diff/6006/chrome/chrome_brows...
chrome/chrome_browser.gypi:965:
'browser/component/web_contents_delegate_android/component_jni_registrar.cc',
On 2012/08/20 14:02:39, Jói wrote:
> The component should have a .gypi file within its directory that creates the
> component's lib, and the 'browser' target should just depend on that.

Done. Please double-check this is right.

[Jói, 2012-08-21 12:17:07]

In general, this LGTM with some comments below.

I don't have the background needed to review the JNI bits or the
Android-specific implementation bits, so I only skimmed those.

Cheers,
Jói

https://chromiumcodereview.appspot.com/10831060/diff/16001/android_webview/ja...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java
(right):

https://chromiumcodereview.appspot.com/10831060/diff/16001/android_webview/ja...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java:14:
* These methods belong to WebView but are not shared with the Chromium Android
port.
Are you following an 80-column convention for Java files?

https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/browser/com...
File
chrome/browser/component/web_contents_delegate_android/component_jni_registrar.cc
(right):

https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/browser/com...
chrome/browser/component/web_contents_delegate_android/component_jni_registrar.cc:18:
return RegisterNativeMethods(env, kComponentRegisteredMethods,
I think our coding guide says all one line or one per line, except you can group
when related e.g. x/y coordinates or width/length, so you could group
kComponentRegisteredMethods and arraysize(kComponentRegisteredMethods).

https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/chrome_brow...
File chrome/chrome_browser_components.gyp (right):

https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/chrome_brow...
chrome/chrome_browser_components.gyp:6: {
I think this file should live at chrome/browser/component/components.gyp.

I understand that it builds the one thing under chrome/android but that seems
like it will be an exception rather than a rule?

[Leandro Graciá Gil, 2012-08-21 16:17:42]

https://chromiumcodereview.appspot.com/10831060/diff/16001/android_webview/ja...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java
(right):

https://chromiumcodereview.appspot.com/10831060/diff/16001/android_webview/ja...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java:14:
* These methods belong to WebView but are not shared with the Chromium Android
port.
On 2012/08/21 12:17:08, Jói wrote:
> Are you following an 80-column convention for Java files?

No, the Java code style guidelines use 100-column convention.

https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/browser/com...
File
chrome/browser/component/web_contents_delegate_android/component_jni_registrar.cc
(right):

https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/browser/com...
chrome/browser/component/web_contents_delegate_android/component_jni_registrar.cc:18:
return RegisterNativeMethods(env, kComponentRegisteredMethods,
On 2012/08/21 12:17:08, Jói wrote:
> I think our coding guide says all one line or one per line, except you can
group
> when related e.g. x/y coordinates or width/length, so you could group
> kComponentRegisteredMethods and arraysize(kComponentRegisteredMethods).

Done.

https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/chrome_brow...
File chrome/chrome_browser_components.gyp (right):

https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/chrome_brow...
chrome/chrome_browser_components.gyp:6: {
On 2012/08/21 12:17:08, Jói wrote:
> I think this file should live at chrome/browser/component/components.gyp.
> 
> I understand that it builds the one thing under chrome/android but that seems
> like it will be an exception rather than a rule?

I'll be moving it then. Any requirements for JNI headers will refer to
android/java/src/org/chromium/chrome/browser/component/..., but these should be
the only cases where this happens.

[Leandro Graciá Gil, 2012-08-21 16:39:58]

jam, any comments on this? Thanks!

On 2012/08/21 16:17:42, Leandro Graciá Gil wrote:
>
https://chromiumcodereview.appspot.com/10831060/diff/16001/android_webview/ja...
> File
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java
> (right):
> 
>
https://chromiumcodereview.appspot.com/10831060/diff/16001/android_webview/ja...
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java:14:
> * These methods belong to WebView but are not shared with the Chromium Android
> port.
> On 2012/08/21 12:17:08, Jói wrote:
> > Are you following an 80-column convention for Java files?
> 
> No, the Java code style guidelines use 100-column convention.
> 
>
https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/browser/com...
> File
>
chrome/browser/component/web_contents_delegate_android/component_jni_registrar.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/browser/com...
>
chrome/browser/component/web_contents_delegate_android/component_jni_registrar.cc:18:
> return RegisterNativeMethods(env, kComponentRegisteredMethods,
> On 2012/08/21 12:17:08, Jói wrote:
> > I think our coding guide says all one line or one per line, except you can
> group
> > when related e.g. x/y coordinates or width/length, so you could group
> > kComponentRegisteredMethods and arraysize(kComponentRegisteredMethods).
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/chrome_brow...
> File chrome/chrome_browser_components.gyp (right):
> 
>
https://chromiumcodereview.appspot.com/10831060/diff/16001/chrome/chrome_brow...
> chrome/chrome_browser_components.gyp:6: {
> On 2012/08/21 12:17:08, Jói wrote:
> > I think this file should live at chrome/browser/component/components.gyp.
> > 
> > I understand that it builds the one thing under chrome/android but that
seems
> > like it will be an exception rather than a rule?
> 
> I'll be moving it then. Any requirements for JNI headers will refer to
> android/java/src/org/chromium/chrome/browser/component/..., but these should
be
> the only cases where this happens.

[jam, 2012-08-21 17:27:13]

lgtm

http://codereview.chromium.org/10831060/diff/17001/chrome/browser/component/w...
File
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.h
(right):

http://codereview.chromium.org/10831060/diff/17001/chrome/browser/component/w...
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.h:23:
struct NativeWebKeyboardEvent;
nit: class then struct

[Leandro Graciá Gil, 2012-08-21 17:35:06]

https://chromiumcodereview.appspot.com/10831060/diff/17001/chrome/browser/com...
File
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/10831060/diff/17001/chrome/browser/com...
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.h:23:
struct NativeWebKeyboardEvent;
On 2012/08/21 17:27:13, John Abd-El-Malek wrote:
> nit: class then struct

Done.

[joth, 2012-08-21 17:43:58]

lgtm

[Leandro Graciá Gil, 2012-08-21 17:46:43]

Thanks for the long review and all the help to get the right approach and create
the first browser component :)

[commit-bot: I haz the power, 2012-08-21 17:49:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/10831060/8053

[commit-bot: I haz the power, 2012-08-21 17:49:21]

Presubmit check for 10831060-8053 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
 
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java,
line 8, 102 chars \
  base/android/jni_generator/jni_generator.py, line 167, 104 chars \
 
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java,
line 7, 102 chars

Presubmit checks took 3.0s to calculate.

[Leandro Graciá Gil, 2012-08-21 17:51:38]

These issues are actually on Java import lines (as if they were #includes) and
Java class names in the jni generator script. Should I land this manually?


On 2012/08/21 17:49:21, I haz the power (commit-bot) wrote:
> Presubmit check for 10831060-8053 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Warnings **
> Found lines longer than 80 characters (first 5 shown).
>  
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java,
> line 8, 102 chars \
>   base/android/jni_generator/jni_generator.py, line 167, 104 chars \
>  
>
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java,
> line 7, 102 chars
> 
> Presubmit checks took 3.0s to calculate.

[Leandro Graciá Gil, 2012-08-21 18:29:15]

Landed the patch manually. Keeping an eye on the bots.

On 2012/08/21 17:51:38, Leandro Graciá Gil wrote:
> These issues are actually on Java import lines (as if they were #includes) and
> Java class names in the jni generator script. Should I land this manually?
> 
> 
> On 2012/08/21 17:49:21, I haz the power (commit-bot) wrote:
> > Presubmit check for 10831060-8053 failed and returned exit status 1.
> > 
> > 
> > Running presubmit commit checks ...
> > 
> > ** Presubmit Warnings **
> > Found lines longer than 80 characters (first 5 shown).
> >  
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java,
> > line 8, 102 chars \
> >   base/android/jni_generator/jni_generator.py, line 167, 104 chars \
> >  
> >
>
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java,
> > line 7, 102 chars
> > 
> > Presubmit checks took 3.0s to calculate.

[jam, 2012-08-22 01:09:56]

On 2012/08/21 17:46:43, Leandro Graciá Gil wrote:
> Thanks for the long review and all the help to get the right approach and
create
> the first browser component :)

thanks for bearing through with this :)