Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9192008:
Hook up ContentViewCore.add/removeJavascriptInterface() (Closed)
|
Created:
8 years, 11 months ago by Steve Block Modified:
8 years, 4 months ago CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, ajwong+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHook up ContentViewCore.add/removeJavascriptInterface()
The entry point to the Java Bridge is JavaBridgeDispatcherHostManager. This is
not part of the content API so is available only on WebContentsImpl, not
WebContents. We therefore modify ContentViewCoreImpl to store and use
WebContentsImpl*, rather than WebContents*. It's safe for ContentViewCoreImpl
to cast the WebContents* it receives in its constructor to WebContentsImpl*
because WebContentsImpl is the only concrete implementation of the WebContents
interface.
BUG=110637
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148951
Patch Set 1 #Patch Set 2 : Keep impl out of public/ dir #Patch Set 3 : Hook up to ContentView #
Total comments: 6
Patch Set 4 : Add to ContentViewCore only #Patch Set 5 : Modify ContentViewCoreImpl to use WebContentsImpl #
Total comments: 12
Patch Set 6 : Addressed comments #Patch Set 7 : Try again #
Messages
Total messages: 22 (0 generated)
avi is OOO
see http://www.chromium.org/developers/content-module/content-api and other APIs in that directory. Content API, like WebKit API uses pure virtual interfaces and we don't put implementations in the public dir (except for trivial constructors or automatic registration/unregistration of observer interfaces). where is the android code that uses this? part of the reason why i was ok with this java code landing in content is that at least we avoided exposing the npobject code from content. now that we'd have to expose a java api, it might be cleaner to make an api around npboject code instead and move the java code out.
> see http://www.chromium.org/developers/content-module/content-api and other APIs > in that directory. Content API, like WebKit API uses pure virtual interfaces and > we don't put implementations in the public dir OK, got it. > where is the android code that uses this? Currently, JavaBridgeDispatcherHostManager is made available via a getter on TabContents. On Android, we access this from our chrome/ layer (not yet upstreamed). This layer uses JavaBoundObject to convert from a Java object to an NPObject* from a Java object. JavaBoundObject currently lives in content/, but should probably be moved to chrome/ > part of the reason why i was ok with > this java code landing in content is that at least we avoided exposing the > npobject code from content. now that we'd have to expose a java api, it might be > cleaner to make an api around npboject code instead and move the java code out. Alternatively, I could re-work the API slightly to avoid exposing NPObject*. If the conversion from Java object to NPObject* were done in the content/ implementation, we could expose JNIEnv* and jobject instead. Alternatively, if exposing types from base/ is OK, we could expose base::android::JavaRef. WDYT?
Sorry for the very delayed reply John. The structure of the Android code has changed a little since I sent the first patchset but the basic requirement is still the same. I've uploaded a new patchset which hopefully uses the content API correctly. Regarding exposing NPObject object code in the content API, this change exposes only the standard NPObject type. I'm not sure if exposing a standard 'third-party' type is OK, or if it should be wrapped with a new content API type? As an alternative, I could easily change the signature of 'void JavaBridgeDispatcherHostManager::AddNamedObject(const string16& name, NPObject* object)' to 'void JavaBridgeDispatcherHostManager::AddNamedObject(const string16& name, const base::android::JavaRef<jobject>& object)'. The code that converts from base::android::JavaRef to NPObject is already in content/ and is currently used from within content/. I assume it's OK for types from base to feature in the content API? I can update the patchset to reflect this if it would make things more clear.
one thing that's not clear to me yet is where will the android code that uses this live? if in content, then we shouldn't need to change the public API. if it's in /chrome then we will. On 2012/05/15 11:25:27, Steve Block wrote: > Sorry for the very delayed reply John. > > The structure of the Android code has changed a little since I sent the first > patchset but the basic requirement is still the same. I've uploaded a new > patchset which hopefully uses the content API correctly. > > Regarding exposing NPObject object code in the content API, this change exposes > only the standard NPObject type. I'm not sure if exposing a standard > 'third-party' type is OK, or if it should be wrapped with a new content API > type? to be clearer, I was referring to the npobject supporting code that's in content. > > As an alternative, I could easily change the signature of 'void > JavaBridgeDispatcherHostManager::AddNamedObject(const string16& name, NPObject* > object)' to 'void JavaBridgeDispatcherHostManager::AddNamedObject(const > string16& name, const base::android::JavaRef<jobject>& object)'. The code that > converts from base::android::JavaRef to NPObject is already in content/ and is > currently used from within content/. I assume it's OK for types from base to > feature in the content API? I can update the patchset to reflect this if it > would make things more clear.
Joth, would you mind taking a look at this? I don't think it's impacted by your plans in http://code.google.com/p/chromium/issues/detail?id=137967, but wanted to check.
RE crbug.com/137967 ContentViewClient refactor, yes this is fine as it doesn't touch that class. RE. the overall problem this is solving -- making addJaveBridge part of public API so ContentView can call it -- this also does seem to be an example of how we've made a rod for our own backs by trying to define a re-usable public Java API as part of the content layer. Right now, ContentView.java is basically a wrapper class that takes a [native] WebContents as input, and makes it easier to call methods on it from Java. But as it takes the abstract WebContents base class as constructor param (it has to, as this is in the public API) it means internally it is limited to only using the public API to WebContents (or, it must do messy down-casting). Thus by limiting it to only the public C++ API, to all intents and purposes we've pushed java ContentView "above" the public API boundary, even though it is defined on the content/ folder. The alternative is we have ContentView construct WebContentImpl itself internally. This means the client must pass in all the WebContents dependencies: currently 4 C++ objects (but that could change...), the pointers to which all must be constructed native side, marshaled to java just to go through the c'tor call chain and pop back to native to be used -- all of which is non-typesafe. Maybe we could make a WebContentsDependencies container struct to carry those pointers about, but that's adds another object ownership dance to go through (e.g. defining the lifetime of that struct and things it points to across the "client native->client java->content java->content native" boundaries in a clear way) http://codereview.chromium.org/9192008/diff/10002/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): http://codereview.chromium.org/9192008/diff/10002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentView.java:496: * make sure none of your allowed methods allow JavaScript to get access to a nit: I'd word this more directly: In addition, ensure none of your [public] methods only return other objects designed to receive calls from untrusted code, and never return a raw Object instance. although, this is moot with the comment below. http://codereview.chromium.org/9192008/diff/10002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentView.java:508: * JavaScript. I was talking with dtrainer yesterday and he plans to rework this to be 'require annotation on methods' rather than allow inherited methods. http://codereview.chromium.org/9192008/diff/10002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentView.java:520: mContentViewCore.removeJavascriptInterface(name); Note ContentView has a getContentViewCore method anyway, so for new things that really are non-view related I'd prefer not duplicating the API at the ContentView layer.
http://codereview.chromium.org/9192008/diff/10002/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): http://codereview.chromium.org/9192008/diff/10002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentView.java:496: * make sure none of your allowed methods allow JavaScript to get access to a On 2012/07/20 17:33:53, joth wrote: > nit: I'd word this more directly: In addition, ensure none of your [public] > methods only return other objects designed to receive calls from untrusted code, > and never return a raw Object instance. > > although, this is moot with the comment below. Done. http://codereview.chromium.org/9192008/diff/10002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentView.java:508: * JavaScript. Yes, he said this to me. Sounds good. http://codereview.chromium.org/9192008/diff/10002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentView.java:520: mContentViewCore.removeJavascriptInterface(name); On 2012/07/20 17:33:53, joth wrote: > Note ContentView has a getContentViewCore method anyway, so for new things that > really are non-view related I'd prefer not duplicating the API at the > ContentView layer. Done.
John, this is ready for you to take another look. Hopefully this will make more sense now that ContentView(Core) is upstreamed.
On 2012/07/23 13:25:52, Steve Block wrote: > John, this is ready for you to take another look. Hopefully this will make more > sense now that ContentView(Core) is upstreamed. If this new functionality is only used within src/content (i.e. content_view_core_impl.cc), then it doesn't belong on the Content API in src/content/public. That is only for stuff that an embedder of Content needs. If it's used internally inside content, you can add this on WebContentsImpl directly, and the JavaBridgeDispatcherHostManager can be a class in content/browser/android
On 2012/07/25 01:51:51, John Abd-El-Malek wrote: > On 2012/07/23 13:25:52, Steve Block wrote: > > John, this is ready for you to take another look. Hopefully this will make > more > > sense now that ContentView(Core) is upstreamed. > > If this new functionality is only used within src/content (i.e. > content_view_core_impl.cc), then it doesn't belong on the Content API in > src/content/public. That is only for stuff that an embedder of Content needs. If > it's used internally inside content, you can add this on WebContentsImpl > directly, and the JavaBridgeDispatcherHostManager can be a class in > content/browser/android This will require JavaBridgeDispatcherHostManager to make the assumption it can down-cast the WebContents* it is passed to be WebContentsImpl*. Is this reasonable?
On 2012/07/25 20:22:58, joth wrote: > On 2012/07/25 01:51:51, John Abd-El-Malek wrote: > > On 2012/07/23 13:25:52, Steve Block wrote: > > > John, this is ready for you to take another look. Hopefully this will make > > more > > > sense now that ContentView(Core) is upstreamed. > > > > If this new functionality is only used within src/content (i.e. > > content_view_core_impl.cc), then it doesn't belong on the Content API in > > src/content/public. That is only for stuff that an embedder of Content needs. > If > > it's used internally inside content, you can add this on WebContentsImpl > > directly, and the JavaBridgeDispatcherHostManager can be a class in > > content/browser/android > > This will require JavaBridgeDispatcherHostManager to make the assumption it can > down-cast the WebContents* it is passed to be WebContentsImpl*. Is this > reasonable? yes, just like in WebKit API, where casting interface pointers from the embedder to concrete objects is the norm
> If it's used internally inside content, you can add this on > WebContentsImpl directly OK, got it. I'll prepare a patch to add JavaBridgeDispatcherHostManager to WebContentsImpl directly. > and the JavaBridgeDispatcherHostManager can be a class in > content/browser/android Is there a particular reason for moving this? It seems like such a move is unrelated to this change? In any case, if we're to move this class, we should probably move all of content/browser/renderer_host/java. > This will require JavaBridgeDispatcherHostManager to make the assumption it > can down-cast the WebContents* it is passed to be WebContentsImpl*. Actually it's Android's ContentViewCoreImpl that will have to make that assumption, in order to get at the JavaBridgeDispatcherHostManager. JavaBridgeDispatcherHostManager only uses WebContents to allow it to implement WebContentsObserver.
On 2012/07/26 17:18:32, Steve Block wrote: > > If it's used internally inside content, you can add this on > > WebContentsImpl directly > OK, got it. I'll prepare a patch to add JavaBridgeDispatcherHostManager to > WebContentsImpl directly. > > > and the JavaBridgeDispatcherHostManager can be a class in > > content/browser/android > Is there a particular reason for moving this? It seems like such a move is > unrelated to this change? In any case, if we're to move this class, we should > probably move all of content/browser/renderer_host/java. > +1 for renaming this folder one way or another: everywhere else, 'java' means the folder contains .java files (only). content/browser/android could make sense for all of that code. We didn't have that location back when this was started :-) Agree that seems worth splitting that to its own CL though. > > This will require JavaBridgeDispatcherHostManager to make the assumption it > > can down-cast the WebContents* it is passed to be WebContentsImpl*. > Actually it's Android's ContentViewCoreImpl that will have to make that > assumption, in order to get at the JavaBridgeDispatcherHostManager. > JavaBridgeDispatcherHostManager only uses WebContents to allow it to implement > WebContentsObserver.
I've updated the patch to use the new approach. Will move Java Bridge code to a new directory in a separate patch, as discussed.
http://codereview.chromium.org/9192008/diff/28002/content/public/browser/andr... File content/public/browser/android/content_view_core.h (right): http://codereview.chromium.org/9192008/diff/28002/content/public/browser/andr... content/public/browser/android/content_view_core.h:40: // -------------------------------------------------------------------------- does anyone know why these are declared as public virtual methods in the base class? It seems the way Java calls to native should be an internal detail, and they could be private non-virtual in the concrete class.
http://codereview.chromium.org/9192008/diff/28002/content/public/browser/andr... File content/public/browser/android/content_view_core.h (right): http://codereview.chromium.org/9192008/diff/28002/content/public/browser/andr... content/public/browser/android/content_view_core.h:40: // -------------------------------------------------------------------------- On 2012/07/27 16:59:30, joth wrote: > does anyone know why these are declared as public virtual methods in the base > class? It seems the way Java calls to native should be an internal detail, and > they could be private non-virtual in the concrete class. I think when were originally defining the content_view_core.h downstream, we copied all the methods over without filtering. In many cases they shouldn't be in the public API (many of the java methods are examples of that), so we can probably just remove these.
nits! http://codereview.chromium.org/9192008/diff/28002/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): http://codereview.chromium.org/9192008/diff/28002/content/browser/android/con... content/browser/android/content_view_core_impl.cc:26: using base::android::ConvertJavaStringToUTF16; move under AttachCurrentThread http://codereview.chromium.org/9192008/diff/28002/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): http://codereview.chromium.org/9192008/diff/28002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:751: * This method injects the supplied Java object into the WebView. The should WebView be ContentViewCore? same for other mentions http://codereview.chromium.org/9192008/diff/28002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:756: * <pre> webView.addJavascriptInterface(new Object(), "injectedObject"); same, should these webView's be renamed. http://codereview.chromium.org/9192008/diff/28002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:778: * @param object the Java object to inject into the WebView's JavaScript It looks like most other javadocs in this file follow the format of Capitalizing the first word and ending with a period. Both are valid by the style guide though, so only a minor nit.
lgtm to be clear, moving the file from one place in the internal content directory to another is fine. my point was really that it shouldn't be in content/public http://codereview.chromium.org/9192008/diff/28002/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): http://codereview.chromium.org/9192008/diff/28002/content/browser/android/con... content/browser/android/content_view_core_impl.cc:71: : web_contents_impl_(static_cast<WebContentsImpl*>(web_contents)), nit: just keep it web_contents_, even if it is a WebContentsImpl
http://codereview.chromium.org/9192008/diff/28002/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): http://codereview.chromium.org/9192008/diff/28002/content/browser/android/con... content/browser/android/content_view_core_impl.cc:26: using base::android::ConvertJavaStringToUTF16; On 2012/07/27 18:09:04, Ted C wrote: > move under AttachCurrentThread Done. http://codereview.chromium.org/9192008/diff/28002/content/browser/android/con... content/browser/android/content_view_core_impl.cc:71: : web_contents_impl_(static_cast<WebContentsImpl*>(web_contents)), On 2012/07/27 23:29:57, John Abd-El-Malek wrote: > nit: just keep it web_contents_, even if it is a WebContentsImpl Done. http://codereview.chromium.org/9192008/diff/28002/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): http://codereview.chromium.org/9192008/diff/28002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:751: * This method injects the supplied Java object into the WebView. The Yes, well spotted, I just copied this from the WebView docs. Done. http://codereview.chromium.org/9192008/diff/28002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:778: * @param object the Java object to inject into the WebView's JavaScript On 2012/07/27 18:09:04, Ted C wrote: > It looks like most other javadocs in this file follow the format of Capitalizing > the first word and ending with a period. > > Both are valid by the style guide though, so only a minor nit. Done. http://codereview.chromium.org/9192008/diff/28002/content/public/browser/andr... File content/public/browser/android/content_view_core.h (right): http://codereview.chromium.org/9192008/diff/28002/content/public/browser/andr... content/public/browser/android/content_view_core.h:40: // -------------------------------------------------------------------------- On 2012/07/27 17:11:47, Ted C wrote: > On 2012/07/27 16:59:30, joth wrote: > > does anyone know why these are declared as public virtual methods in the base > > class? It seems the way Java calls to native should be an internal detail, and > > they could be private non-virtual in the concrete class. > > I think when were originally defining the content_view_core.h downstream, we > copied all the methods over without filtering. In many cases they shouldn't be > in the public API (many of the java methods are examples of that), so we can > probably just remove these. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/steveblock@chromium.org/9192008/26005 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
