Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10825155:
Build target for Android WebView. (Closed)
|
Created:
8 years, 4 months ago by Torne Modified:
8 years, 4 months ago Reviewers:
mnaganov (inactive), Jói, erikwright (departed), joth, mkosiba (inactive), benm (inactive), Steve Block CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, andreip3000, erikwright (departed) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBuild target for Android WebView.
This creates a "libwebview" build target for the JNI code to power the
Android WebView. This is unfinished and can't run without additional
code that has not yet been upstreamed, but it compiles and links
successfully.
In its present state the WebViewMainDelegate reuses Chrome
implementations of other content client interfaces. This is a temporary
state of affairs until necessary refactorings can be performed.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150780
Patch Set 1 #
Total comments: 11
Patch Set 2 : Shuffle files about a bit and address comments #
Total comments: 9
Patch Set 3 : joth's comments #Patch Set 4 : Implement a ContentMainDelegate #
Total comments: 6
Patch Set 5 : Remove chrome/ changes #
Messages
Total messages: 25 (0 generated)
Very first version; it links but I haven't even tried to run it yet. We need to upstream TabAndroid before there is any real chance that this will work at all. I've added Jói because he asked, I don't think it's worth bugging the other chrome-team folks just yet (maybe the next version or two).
Initial round of comments. Cheers, Jói https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/androi... File android_webview/android_webview.gyp (right): https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/androi... android_webview/android_webview.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. 2012 https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/androi... android_webview/android_webview.gyp:43: 'lib/webview_entry_point.cc', should these sources be in dll/ since they depend on Chrome? https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/DEPS File android_webview/lib/DEPS (right): https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/DE... android_webview/lib/DEPS:1: include_rules = [ Could you add the following to this DEPS file, and any others created under android_webview, to help make sure us Browser Components folks are aware of added dependencies and can help break them ASAP? # Please add joi@ or erikwright@ as reviewers for any changes # to DEPS files under android_webview/. https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/DE... android_webview/lib/DEPS:5: "!chrome/app/chrome_main_delegate.h" I believe the plan was for all the temporary dependencies on Chrome to be in android_webview/dll, and none in lib. Did I misunderstand? https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/we... File android_webview/lib/webview_main_delegate.h (right): https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/we... android_webview/lib/webview_main_delegate.h:18: // Android WebView override of ChromeMainDelegate. I think there should be a comment here, for the benefit of people reading the code, that this is temporary and what the intent is (separate implementation rather than inherited, reusing components rather than directly reusing code). https://chromiumcodereview.appspot.com/10825155/diff/1/chrome/common/chrome_p... File chrome/common/chrome_paths_android.cc (right): https://chromiumcodereview.appspot.com/10825155/diff/1/chrome/common/chrome_p... chrome/common/chrome_paths_android.cc:16: This blank line looks like it was introduced accidentally.
New version uploaded. Files have moved around a bit (sorry). https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/androi... File android_webview/android_webview.gyp (right): https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/androi... android_webview/android_webview.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2012/08/02 15:20:00, Jói wrote: > 2012 Done. https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/DEPS File android_webview/lib/DEPS (right): https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/DE... android_webview/lib/DEPS:1: include_rules = [ On 2012/08/02 15:20:00, Jói wrote: > Could you add the following to this DEPS file, and any others created under > android_webview, to help make sure us Browser Components folks are aware of > added dependencies and can help break them ASAP? > > # Please add joi@ or erikwright@ as reviewers for any changes > # to DEPS files under android_webview/. Done. https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/DE... android_webview/lib/DEPS:5: "!chrome/app/chrome_main_delegate.h" On 2012/08/02 15:20:00, Jói wrote: > I believe the plan was for all the temporary dependencies on Chrome to be in > android_webview/dll, and none in lib. Did I misunderstand? Sorry, I arrived at a different structure separately, you didn't misunderstand, I just wasn't in that conversation. :) I've discussed the names with joth, and the "dll" vs "lib" names don't seem ideal; it looks like we want to go with the library main code to be in "lib/main" (with "lib" allowing deps on chrome) and the normal webview implementation code to be in "native" and "java" and "test" and so on. I've shuffled the code slightly more in the next version to match, and I've added more comments/explanation to DEPS here, and also add a DEPS in android_webview itself, to make this clearer. Using "lib" for this is briefly confusing to the people who heard the earlier suggestion, but in the long term I think it makes more sense. https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/we... File android_webview/lib/webview_main_delegate.h (right): https://chromiumcodereview.appspot.com/10825155/diff/1/android_webview/lib/we... android_webview/lib/webview_main_delegate.h:18: // Android WebView override of ChromeMainDelegate. On 2012/08/02 15:20:00, Jói wrote: > I think there should be a comment here, for the benefit of people reading the > code, that this is temporary and what the intent is (separate implementation > rather than inherited, reusing components rather than directly reusing code). Done. https://chromiumcodereview.appspot.com/10825155/diff/1/chrome/common/chrome_p... File chrome/common/chrome_paths_android.cc (right): https://chromiumcodereview.appspot.com/10825155/diff/1/chrome/common/chrome_p... chrome/common/chrome_paths_android.cc:16: On 2012/08/02 15:20:00, Jói wrote: > This blank line looks like it was introduced accidentally. Done.
LGTM from my point of view.
thanks for this. https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/OWNERS File android_webview/OWNERS (right): https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/OWN... android_webview/OWNERS:2: set noparent until we're up and running smoothly lets minimize the top level OWNERS to ensure we keep in loop on how the overall structure evolves. me and you and steve will (soon) give excellent time zone coverage :) We can add much larger OWNERS files in java javatests & native. (Hence idea to put them all in one subfolder..) https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/lib... File android_webview/lib/android_webview.gyp (right): https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/lib... android_webview/lib/android_webview.gyp:19: '../../chrome/chrome.gyp:utility', none of these chrome/ references are inspected by DEPS? (or below) https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/lib... File android_webview/lib/main/webview_entry_point.cc (right): https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/lib... android_webview/lib/main/webview_entry_point.cc:12: // We'll do most of the the initialization later in the main thread. remove the last two lines about threads. We may eventually do that dance, but certainly not yet. (I assume??) https://chromiumcodereview.appspot.com/10825155/diff/6001/chrome/renderer/ext... File chrome/renderer/extensions/extension_dispatcher.cc (right): https://chromiumcodereview.appspot.com/10825155/diff/6001/chrome/renderer/ext... chrome/renderer/extensions/extension_dispatcher.cc:626: #if !defined(OS_ANDROID) this is slightly misleading, as none of these extensions are supported on android. Is there a useful comment to add? can we ifdef out the whole method?
https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/OWNERS File android_webview/OWNERS (right): https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/OWN... android_webview/OWNERS:2: set noparent On 2012/08/03 15:20:11, joth wrote: > until we're up and running smoothly lets minimize the top level OWNERS to ensure > we keep in loop on how the overall structure evolves. me and you and steve will > (soon) give excellent time zone coverage :) > > We can add much larger OWNERS files in java javatests & native. (Hence idea to > put them all in one subfolder..) or... we put another 'set noparent' into a smaller OWNERS in lib/ ?
https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/OWNERS File android_webview/OWNERS (right): https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/OWN... android_webview/OWNERS:2: set noparent On 2012/08/03 15:20:11, joth wrote: > until we're up and running smoothly lets minimize the top level OWNERS to ensure > we keep in loop on how the overall structure evolves. me and you and steve will > (soon) give excellent time zone coverage :) > > We can add much larger OWNERS files in java javatests & native. (Hence idea to > put them all in one subfolder..) Done. https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/lib... File android_webview/lib/android_webview.gyp (right): https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/lib... android_webview/lib/android_webview.gyp:19: '../../chrome/chrome.gyp:utility', On 2012/08/03 15:20:11, joth wrote: > none of these chrome/ references are inspected by DEPS? (or below) > DEPS only checks header inclusion, not linkage. https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/lib... File android_webview/lib/main/webview_entry_point.cc (right): https://chromiumcodereview.appspot.com/10825155/diff/6001/android_webview/lib... android_webview/lib/main/webview_entry_point.cc:12: // We'll do most of the the initialization later in the main thread. On 2012/08/03 15:20:11, joth wrote: > remove the last two lines about threads. We may eventually do that dance, but > certainly not yet. (I assume??) I think we are currently loading the library synchronously, so this isn't loading in a separate thread, but the main initialisation is still done separately in LibraryLoadedOnMainThread(). I'll update the comments. https://chromiumcodereview.appspot.com/10825155/diff/6001/chrome/renderer/ext... File chrome/renderer/extensions/extension_dispatcher.cc (right): https://chromiumcodereview.appspot.com/10825155/diff/6001/chrome/renderer/ext... chrome/renderer/extensions/extension_dispatcher.cc:626: #if !defined(OS_ANDROID) On 2012/08/03 15:20:11, joth wrote: > this is slightly misleading, as none of these extensions are supported on > android. > Is there a useful comment to add? can we ifdef out the whole method? I expect this entire file won't be compiled at all and extensions will be disabled at a higher level. This diff hunk is just copied from our downstream diff; I don't intend to submit it.
LGTM. Didn't review the chrome/ changes as I gather they will appear elsewhere.
lgtm
Hi all, I've implemented a ContentMainDelegate specific to the WebView (which still reuses chrome/ code to implement the individual content client interfaces for now). This simplifies the CL significantly, requiring less changes to existing code to make it build. I've also been able to test this implementation by cherrypicking it into our downstream tree and building it against our current Java implementation and it's able to render basic webpages successfully, though plenty of functionality is missing. I've updated the CL description accordingly, and I am now considering that this may be ready to submit (if the relevant people approve). The one remaining patch to chrome/ is currently required to satisfy link dependencies that are not trivial to stub out. If this is a problem, I can try and find the relevant people to talk to about how to get these features to "work" better from our downstream fork..
http://codereview.chromium.org/10825155/diff/13001/android_webview/lib/main/w... File android_webview/lib/main/webview_stubs.cc (right): http://codereview.chromium.org/10825155/diff/13001/android_webview/lib/main/w... android_webview/lib/main/webview_stubs.cc:9: // They will be removed once real implementations are written/upstreamed, or For my enlightenment: Does this mean the previous implementation is currently #ifdef'ed out under chrome/ ? http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... File chrome/renderer/chrome_content_renderer_client.cc (right): http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:356: #if defined(OS_ANDROID) It seems like you can avoid this #if and the one below by writing a subclass of ChromeContentRendererClient and instantiating that instead in WebViewMainDelegate.
http://codereview.chromium.org/10825155/diff/13001/android_webview/lib/main/w... File android_webview/lib/main/webview_stubs.cc (right): http://codereview.chromium.org/10825155/diff/13001/android_webview/lib/main/w... android_webview/lib/main/webview_stubs.cc:9: // They will be removed once real implementations are written/upstreamed, or On 2012/08/07 13:49:47, Jói wrote: > For my enlightenment: Does this mean the previous implementation is currently > #ifdef'ed out under chrome/ ? There's currently no real implementation of TabAndroid in the upstream tree at all; just stubs for test purposes. The real implementation only exists in the downstream fork and that version is unlikely to be suitable for WebView's purposes; we will probably write our own implementation from scratch, it's just not yet been necessary. AutofillExternalDelegate::Create is already ifdef'ed for OS_ANDROID because there is, again, a separate downstream implementation that hasn't been upstreamed yet, and currently the upstream tree only has a stub test version for Android. http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... File chrome/renderer/chrome_content_renderer_client.cc (right): http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:356: #if defined(OS_ANDROID) On 2012/08/07 13:49:47, Jói wrote: > It seems like you can avoid this #if and the one below by writing a subclass of > ChromeContentRendererClient and instantiating that instead in > WebViewMainDelegate. These things need to be disabled for the regular Chrome for Android build as well, which doesn't support prerendering or extensions either. It's not specific to WebView; these things simply aren't implemented for Android (we don't build a PrerenderWebMediaPlayer). For WebView specifically, writing our own version of the class is exactly the intention. However, this is likely to require copy+pasting the implementations of many of the other functions defined here, since there's no alternative implementation to use. Future CLs will implement our own versions of these classes as we work out exactly which functionality is required.
Regarding ChromeContentRendererClient: Even if those two functions need to be disabled for Chrome for Android, it still seems cleaner to introduce a subclass (e.g. ClankContentRendererClient), disable the two virtual methods there, and leave the rest unmodified (no copy and paste, just regular polymorphic behavior). If you do it this way, then for WebView there are zero #if statements instead of two, since you have your own MainDelegate, and for Chrome for Android there is a single #if (where the ClankContentRendererClient gets instantiated instead of the ChromeContentRendererClient). I'd say, always prefer fewer #if statements when there is a clean way to have fewer. Cheers, Jói On Tue, Aug 7, 2012 at 1:58 PM, <torne@chromium.org> wrote: > > http://codereview.chromium.org/10825155/diff/13001/android_webview/lib/main/w... > File android_webview/lib/main/webview_stubs.cc (right): > > http://codereview.chromium.org/10825155/diff/13001/android_webview/lib/main/w... > android_webview/lib/main/webview_stubs.cc:9: // They will be removed > once real implementations are written/upstreamed, or > On 2012/08/07 13:49:47, Jói wrote: >> >> For my enlightenment: Does this mean the previous implementation is > > currently >> >> #ifdef'ed out under chrome/ ? > > > There's currently no real implementation of TabAndroid in the upstream > tree at all; just stubs for test purposes. The real implementation only > exists in the downstream fork and that version is unlikely to be > suitable for WebView's purposes; we will probably write our own > implementation from scratch, it's just not yet been necessary. > > AutofillExternalDelegate::Create is already ifdef'ed for OS_ANDROID > because there is, again, a separate downstream implementation that > hasn't been upstreamed yet, and currently the upstream tree only has a > stub test version for Android. > > > http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... > File chrome/renderer/chrome_content_renderer_client.cc (right): > > http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... > chrome/renderer/chrome_content_renderer_client.cc:356: #if > defined(OS_ANDROID) > On 2012/08/07 13:49:47, Jói wrote: >> >> It seems like you can avoid this #if and the one below by writing a > > subclass of >> >> ChromeContentRendererClient and instantiating that instead in >> WebViewMainDelegate. > > > These things need to be disabled for the regular Chrome for Android > build as well, which doesn't support prerendering or extensions either. > It's not specific to WebView; these things simply aren't implemented for > Android (we don't build a PrerenderWebMediaPlayer). > > For WebView specifically, writing our own version of the class is > exactly the intention. However, this is likely to require copy+pasting > the implementations of many of the other functions defined here, since > there's no alternative implementation to use. Future CLs will implement > our own versions of these classes as we work out exactly which > functionality is required. > > http://codereview.chromium.org/10825155/
On 2012/08/07 14:03:03, Jói wrote: > Regarding ChromeContentRendererClient: Even if those two functions > need to be disabled for Chrome for Android, it still seems cleaner to > introduce a subclass (e.g. ClankContentRendererClient), disable the > two virtual methods there, and leave the rest unmodified (no copy and > paste, just regular polymorphic behavior). If you do it this way, > then for WebView there are zero #if statements instead of two, since > you have your own MainDelegate, and for Chrome for Android there is a > single #if (where the ClankContentRendererClient gets instantiated > instead of the ChromeContentRendererClient). I'd say, always prefer > fewer #if statements when there is a clean way to have fewer. Unfortunately that doesn't solve the problem; the parent implementation still tries to call a function that's not implemented, and this is retained via the vtables of the classes. It won't link unless the missing things exist. I could implement stubs but this is nontrivial for constructors of classes with virtual methods (since the vtable for *that* class must be present, and thus you have to implement everything referenced).
On 2012/08/07 14:05:37, Torne wrote: > On 2012/08/07 14:03:03, Jói wrote: > > Regarding ChromeContentRendererClient: Even if those two functions > > need to be disabled for Chrome for Android, it still seems cleaner to > > introduce a subclass (e.g. ClankContentRendererClient), disable the > > two virtual methods there, and leave the rest unmodified (no copy and > > paste, just regular polymorphic behavior). If you do it this way, > > then for WebView there are zero #if statements instead of two, since > > you have your own MainDelegate, and for Chrome for Android there is a > > single #if (where the ClankContentRendererClient gets instantiated > > instead of the ChromeContentRendererClient). I'd say, always prefer > > fewer #if statements when there is a clean way to have fewer. > > Unfortunately that doesn't solve the problem; the parent implementation still > tries to call a function that's not implemented, and this is retained via the > vtables of the classes. It won't link unless the missing things exist. I could > implement stubs but this is nontrivial for constructors of classes with virtual > methods (since the vtable for *that* class must be present, and thus you have to > implement everything referenced). Also, stub implementations are a maintenance burden, as they must be kept in sync with the interface of the real implementation.
http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... File chrome/renderer/chrome_content_renderer_client.cc (right): http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:356: #if defined(OS_ANDROID) On 2012/08/07 13:58:09, Torne wrote: > On 2012/08/07 13:49:47, Jói wrote: > > It seems like you can avoid this #if and the one below by writing a subclass > of > > ChromeContentRendererClient and instantiating that instead in > > WebViewMainDelegate. > > These things need to be disabled for the regular Chrome for Android build as > well, which doesn't support prerendering or extensions either. It's not specific > to WebView; these things simply aren't implemented for Android (we don't build a > PrerenderWebMediaPlayer). > > For WebView specifically, writing our own version of the class is exactly the > intention. However, this is likely to require copy+pasting the implementations > of many of the other functions defined here, since there's no alternative > implementation to use. Future CLs will implement our own versions of these > classes as we work out exactly which functionality is required. Thanks for the explanation Torne, that makes sense. In this case, one approach would be to pull everything used by both Clank and desktop-Chrome into a CommonContentRendererClient that implements these two methods by returning NULL, with a subclass ChromeContentRendererClient that implements them "for real". Whether this should be done or #if statements should be used kind of depends on what end state we are aiming for. You talked about having your own implementation of ContentRendererClient, but you also said you might copy+paste a lot from the ChromeContentRendererClient implementation. If it's more like the latter, then possibly the common base class approach (i.e. a CommonContentRendererClient) is viable. If it's more like the former, then I guess the current set of #if statements is fine for now, but obviously once you have the separate implementation then if it's doing the "same thing" more or less, then the code ideally shouldn't be copied and pasted but rather reused - but we can figure out how to do that at the right time.
On 2012/08/07 14:17:31, Jói wrote: > http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... > File chrome/renderer/chrome_content_renderer_client.cc (right): > > http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... > chrome/renderer/chrome_content_renderer_client.cc:356: #if defined(OS_ANDROID) > On 2012/08/07 13:58:09, Torne wrote: > > On 2012/08/07 13:49:47, Jói wrote: > > > It seems like you can avoid this #if and the one below by writing a subclass > > of > > > ChromeContentRendererClient and instantiating that instead in > > > WebViewMainDelegate. > > > > These things need to be disabled for the regular Chrome for Android build as > > well, which doesn't support prerendering or extensions either. It's not > specific > > to WebView; these things simply aren't implemented for Android (we don't build > a > > PrerenderWebMediaPlayer). > > > > For WebView specifically, writing our own version of the class is exactly the > > intention. However, this is likely to require copy+pasting the implementations > > of many of the other functions defined here, since there's no alternative > > implementation to use. Future CLs will implement our own versions of these > > classes as we work out exactly which functionality is required. > > Thanks for the explanation Torne, that makes sense. > > In this case, one approach would be to pull everything used by both Clank and > desktop-Chrome into a CommonContentRendererClient that implements these two > methods by returning NULL, with a subclass ChromeContentRendererClient that > implements them "for real". > > Whether this should be done or #if statements should be used kind of depends on > what end state we are aiming for. You talked about having your own > implementation of ContentRendererClient, but you also said you might copy+paste > a lot from the ChromeContentRendererClient implementation. If it's more like > the latter, then possibly the common base class approach (i.e. a > CommonContentRendererClient) is viable. If it's more like the former, then I > guess the current set of #if statements is fine for now, but obviously once you > have the separate implementation then if it's doing the "same thing" more or > less, then the code ideally shouldn't be copied and pasted but rather reused - > but we can figure out how to do that at the right time. Right, that's the idea. The way I implemented the WebViewMainDelegate here was to copy the entire ChromeMainDelegate implementation, remove all the parts not appropriate for Android, then examine what was left and remove the parts that WebView doesn't need. The final result has very little code in common with ChromeMainDelegate. I expect to follow much the same process with the other client interfaces: copy the implementation, strip it down to the minimum that still serves our purposes, and then see what's left. If there is still significant code from the original this is a good argument for a refactoring that makes it possible to share that code, but I don't intend to refactor the code until after it's been shown that the currently-shared code actually should be shared in the long term, and I don't intend to land any CLs that contain any significant amount of duplicate code (the intermediate states will be in my personal tree) :) The #if statements let me leave ContentRendererClient until later, which is my preference for now; I intend to focus on the other parts first as I believe the renderer support code is where we will want to share the most and thus the other parts have lower-hanging fruit.
OK, in this case LGTM. On Tue, Aug 7, 2012 at 2:35 PM, <torne@chromium.org> wrote: > On 2012/08/07 14:17:31, Jói wrote: > > http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... >> >> File chrome/renderer/chrome_content_renderer_client.cc (right): > > > > http://codereview.chromium.org/10825155/diff/13001/chrome/renderer/chrome_con... >> >> chrome/renderer/chrome_content_renderer_client.cc:356: #if >> defined(OS_ANDROID) >> On 2012/08/07 13:58:09, Torne wrote: >> > On 2012/08/07 13:49:47, Jói wrote: >> > > It seems like you can avoid this #if and the one below by writing a > > subclass >> >> > of >> > > ChromeContentRendererClient and instantiating that instead in >> > > WebViewMainDelegate. >> > >> > These things need to be disabled for the regular Chrome for Android >> > build as >> > well, which doesn't support prerendering or extensions either. It's not >> specific >> > to WebView; these things simply aren't implemented for Android (we don't > > build >> >> a >> > PrerenderWebMediaPlayer). >> > >> > For WebView specifically, writing our own version of the class is >> > exactly > > the >> >> > intention. However, this is likely to require copy+pasting the > > implementations >> >> > of many of the other functions defined here, since there's no >> > alternative >> > implementation to use. Future CLs will implement our own versions of >> > these >> > classes as we work out exactly which functionality is required. > > >> Thanks for the explanation Torne, that makes sense. > > >> In this case, one approach would be to pull everything used by both Clank >> and >> desktop-Chrome into a CommonContentRendererClient that implements these >> two >> methods by returning NULL, with a subclass ChromeContentRendererClient >> that >> implements them "for real". > > >> Whether this should be done or #if statements should be used kind of >> depends > > on >> >> what end state we are aiming for. You talked about having your own >> implementation of ContentRendererClient, but you also said you might > > copy+paste >> >> a lot from the ChromeContentRendererClient implementation. If it's more >> like >> the latter, then possibly the common base class approach (i.e. a >> CommonContentRendererClient) is viable. If it's more like the former, >> then I >> guess the current set of #if statements is fine for now, but obviously >> once > > you >> >> have the separate implementation then if it's doing the "same thing" more >> or >> less, then the code ideally shouldn't be copied and pasted but rather >> reused - >> but we can figure out how to do that at the right time. > > > Right, that's the idea. The way I implemented the WebViewMainDelegate here > was > to copy the entire ChromeMainDelegate implementation, remove all the parts > not > appropriate for Android, then examine what was left and remove the parts > that > WebView doesn't need. The final result has very little code in common with > ChromeMainDelegate. I expect to follow much the same process with the other > client interfaces: copy the implementation, strip it down to the minimum > that > still serves our purposes, and then see what's left. If there is still > significant code from the original this is a good argument for a refactoring > that makes it possible to share that code, but I don't intend to refactor > the > code until after it's been shown that the currently-shared code actually > should > be shared in the long term, and I don't intend to land any CLs that contain > any > significant amount of duplicate code (the intermediate states will be in my > personal tree) :) > > The #if statements let me leave ContentRendererClient until later, which is > my > preference for now; I intend to focus on the other parts first as I believe > the > renderer support code is where we will want to share the most and thus the > other > parts have lower-hanging fruit. > > http://codereview.chromium.org/10825155/
OK, on further thought and comparison with android downstream I'm gonna revisit the chrome/ changes and see if I can drop the ifdefs entirely rather than introducing them temporarily; I thought of a way to do it that should be nicer. Sorry for the back-and-forth on this change!
One quick comment re. TabAndroid I agree stub versions of this is best way to deal with this for now. http://codereview.chromium.org/10825155/diff/13001/android_webview/lib/main/w... File android_webview/lib/main/webview_stubs.cc (right): http://codereview.chromium.org/10825155/diff/13001/android_webview/lib/main/w... android_webview/lib/main/webview_stubs.cc:9: // They will be removed once real implementations are written/upstreamed, or On 2012/08/07 13:58:09, Torne wrote: > On 2012/08/07 13:49:47, Jói wrote: > > For my enlightenment: Does this mean the previous implementation is currently > > #ifdef'ed out under chrome/ ? > > There's currently no real implementation of TabAndroid in the upstream tree at > all; just stubs for test purposes. The real implementation only exists in the > downstream fork and that version is unlikely to be suitable for WebView's > purposes; we will probably write our own implementation from scratch, it's just > not yet been necessary. > Actually it turns out that http auth dialogs are currently the only thing in TabAndroid we would actually want for WebView, and even those it's easier to cut into that functionality at the Content API rather than try and reuse the chrome layer implementation (that adds a bunch of password autofill complexity we don't want). So my aim now is never to make a TabAndroid subclass for webview. The eventual aim should be not to compile in and code that expects to look up a TabAndroid*, or if we find some that is needed, refactor it to use a dedicated FooFeature delegate rather than the catch-all TabAndroid escape hatch. > AutofillExternalDelegate::Create is already ifdef'ed for OS_ANDROID because > there is, again, a separate downstream implementation that hasn't been > upstreamed yet, and currently the upstream tree only has a stub test version for > Android.
OK, I've removed the chrome/ changes from this CL entirely, since they're not really related to this (they're just part of android upstreaming) and uploaded two separate CLs to tackle those problems in a slightly different way: http://codereview.chromium.org/10834226/ http://codereview.chromium.org/10855047/ I'll wait for the relevant reviewers to approve those, then land this as well unless there are objections.
On 2012/08/08 12:34:09, Torne wrote: > OK, I've removed the chrome/ changes from this CL entirely, since they're not > really related to this (they're just part of android upstreaming) and uploaded > two separate CLs to tackle those problems in a slightly different way: > > http://codereview.chromium.org/10834226/ > http://codereview.chromium.org/10855047/ > > I'll wait for the relevant reviewers to approve those, then land this as well > unless there are objections. One of those CLs has now landed; the other will be a little longer. I'm going to land this anyway, since it doesn't get built by default. We need the directory to exist (with OWNERS/DEPS/etc) so that we can start upstreaming other CLs into it with the actual webview implementation code.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/torne@chromium.org/10825155/4004
Presubmit check for 10825155-4004 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit Warnings **
You added one or more #includes of files that are temporarily
allowed but being removed. Can you avoid introducing the
#include? See relevant DEPS file(s) for details and contacts.
android_webview/lib/main/webview_main_delegate.cc
Illegal include: "chrome/browser/chrome_content_browser_client.h"
Because of "!chrome/browser/chrome_content_browser_client.h" from
android_webview/lib's include_rules. \
android_webview/lib/main/webview_main_delegate.cc
Illegal include: "chrome/common/chrome_paths.h"
Because of "!chrome/common/chrome_paths.h" from android_webview/lib's
include_rules. \
android_webview/lib/main/webview_main_delegate.cc
Illegal include: "chrome/renderer/chrome_content_renderer_client.h"
Because of "!chrome/renderer/chrome_content_renderer_client.h" from
android_webview/lib's include_rules. \
android_webview/lib/main/webview_main_delegate.h
Illegal include: "chrome/common/chrome_content_client.h"
Because of "!chrome/common/chrome_content_client.h" from
android_webview/lib's include_rules. \
android_webview/lib/main/webview_stubs.cc
Illegal include: "chrome/browser/android/tab_android.h"
Because of "!chrome/browser/android/tab_android.h" from
android_webview/lib's include_rules. \
android_webview/lib/main/webview_stubs.cc
Illegal include: "chrome/browser/autofill/autofill_external_delegate.h"
Because of "!chrome/browser/autofill/autofill_external_delegate.h" from
android_webview/lib's include_rules.
Presubmit checks took 1.6s to calculate.
On 2012/08/09 10:48:41, I haz the power (commit-bot) wrote: > Presubmit check for 10825155-4004 failed and returned exit status 1. > > > Running presubmit commit checks ... > > ** Presubmit Messages ** > If this change has an associated bug, add BUG=[bug number]. > > ** Presubmit Warnings ** > You added one or more #includes of files that are temporarily > allowed but being removed. Can you avoid introducing the > #include? See relevant DEPS file(s) for details and contacts. > android_webview/lib/main/webview_main_delegate.cc > Illegal include: "chrome/browser/chrome_content_browser_client.h" > Because of "!chrome/browser/chrome_content_browser_client.h" from > android_webview/lib's include_rules. \ > android_webview/lib/main/webview_main_delegate.cc > Illegal include: "chrome/common/chrome_paths.h" > Because of "!chrome/common/chrome_paths.h" from android_webview/lib's > include_rules. \ > android_webview/lib/main/webview_main_delegate.cc > Illegal include: "chrome/renderer/chrome_content_renderer_client.h" > Because of "!chrome/renderer/chrome_content_renderer_client.h" from > android_webview/lib's include_rules. \ > android_webview/lib/main/webview_main_delegate.h > Illegal include: "chrome/common/chrome_content_client.h" > Because of "!chrome/common/chrome_content_client.h" from > android_webview/lib's include_rules. \ > android_webview/lib/main/webview_stubs.cc > Illegal include: "chrome/browser/android/tab_android.h" > Because of "!chrome/browser/android/tab_android.h" from > android_webview/lib's include_rules. \ > android_webview/lib/main/webview_stubs.cc > Illegal include: "chrome/browser/autofill/autofill_external_delegate.h" > Because of "!chrome/browser/autofill/autofill_external_delegate.h" from > android_webview/lib's include_rules. > > Presubmit checks took 1.6s to calculate. Doh, commitbot doesn't like presubmit warnings. Landing manually. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
