Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11348075:
[Android WebView] AwContentsClient.shouldCreate window callback part 2. (Closed)
|
Created:
8 years, 1 month ago by benm (inactive) Modified:
8 years ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android WebView] AwContentsClient.shouldCreate window callback part 2.
Part 1: https://chromiumcodereview.appspot.com/11362183/
The second part of this change implements the "true" path,
i.e. the user provides us with a new AwContents to host
the popup window. This arrives asynchronously.
While we are waiting for the reply from the user, we defer
loading of the popup contents using a resource throttle.
When we receive the callback we replace the ContentViewCore that
is contained in the new AwContents with one that wraps the WebContents
for the popup. This triggers an update to the AwIoThreadClient for
that AwContents, which resumes the deferred load for the popup.
Android only change and android bots green
NOTRY=TRUE
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170721
Patch Set 1 #Patch Set 2 : fix clang #
Total comments: 44
Patch Set 3 : address joth's comments. #Patch Set 4 : Address Marcin's comments. #
Total comments: 13
Patch Set 5 : rebase and refactor for joth's comments #Patch Set 6 : #Patch Set 7 : #
Total comments: 16
Patch Set 8 : fix comments #Patch Set 9 : fix spurious includes. #
Messages
Total messages: 17 (0 generated)
ptal joth! Probably needs some tidying up but hopefully the general approach is sound!
this looks good :) prefer this design to re-wiring the ContentViewCore internals. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... File android_webview/browser/aw_contents_io_thread_client.h (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... android_webview/browser/aw_contents_io_thread_client.h:46: class ReadyObserver { docs https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:90: // is used to host a pop up window. looks like the sort of comment that may go out of date? https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:371: // wrap it and then swap it . spurious space https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:373: ContentViewCore cvc = new ContentViewCore(mContainerView.getContext(), nit: cvc is odd name. if you want a short name, viewCore or contentCore or core are good with me :-) https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:377: mIsAccessFromFileUrlsGrantedByDefault); interestingly, we don't need to retain mIsAccessFromFileUrlsGranted across to the next settings instance, as this will immediately get overritten with the initFrom(oldSettings) below. So I'd be tempted to pass false here and just comment it. (and avoid the need for this member) https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:389: nativeReInit(mNativeAwContents, newWebContentsPtr); consider is we can factor any common init out from here and c'tor? https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:394: mContentViewCore.onSizeChanged(mContainerView.getWidth(), mContainerView.getHeight(), 0, 0); nit: could skip this if the width ==0 && height == 0 https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_contents.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:103: void AwContents::ReInit(JNIEnv* env, jobject obj, jint new_wc) { InstallWebContents or SetWebContents ? https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:105: web_contents_.reset(wc); |wc| seems unneeded here? just do the cast in the reset() https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:110: render_view_host_ext_.reset(new AwRenderViewHostExt(web_contents_.get())); factor out a method for all this and share with constructor? or.... just delete it from the c'tor and have the java side call both methods: mNativeAwContents = nativeInit(); nativeInstallWebContents(mNativeAwContents, nativeWebContents) https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:111: web_contents_->WasShown(); seems odd to tuck a visibility state change into an 'init' method. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:393: void AwContents::SetPendingWebContentsForPopup(content::WebContents* pending) { consider passing: scoped_ptr<WebContents> pending_content https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:395: DCHECK(!pending_contents_); I'd be tempted to do: if (pending_contents_) { LOG(WARNING) << "blocking window popup as previous popup still pending"; delete pending; } https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_contents.h (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.h:100: return reinterpret_cast<jint>(pending_contents_.release()); put in .cc ? https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_contents_io_thread_client_impl.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents_io_thread_client_impl.cc:96: base::AutoLock lock(map_lock_); move down to 105 https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents_io_thread_client_impl.cc:106: // we should now notify. AIUI AddObserver() and Insert(() will both only be called on IO thread. so perhaps this race couldn't happen? (and, the observer list need not be threadsafe). BUT we should add dchecks, to be sure. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_web_contents_delegate.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_web_contents_delegate.cc:83: // a callback from them later with an AwContents to use to displau typo: dispau
https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... File android_webview/browser/aw_contents_io_thread_client.h (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... android_webview/browser/aw_contents_io_thread_client.h:50: I think the usual pattern is to declare ReadyObserver as friend and make these methods private. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:130: if (!resumed_ && new_child_id == child_id_ && new_route_id == route_id_) { it seems like this is a good place to run all of the logic from MaybeCancelResourceThrottle::WillStartRequest. I don't have a good name for the resulting class though. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_contents_io_thread_client_impl.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents_io_thread_client_impl.cc:96: base::AutoLock lock(map_lock_); On 2012/11/16 06:24:31, joth wrote: > move down to 105 I'd prefer not to. Moving the lock would make it possible for 2 threads to new up the ObserverList twice, so we'd have to DCHECK that this is called from a specific thread. I think having some methods on this class being threadsafe, while others being XXX-thread only makes the API confusing.
thanks joth! https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... File android_webview/browser/aw_contents_io_thread_client.h (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... android_webview/browser/aw_contents_io_thread_client.h:46: class ReadyObserver { On 2012/11/16 06:24:31, joth wrote: > docs Done. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:90: // is used to host a pop up window. Fair enough, removed. Especially as we don't need the bool anymore. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:371: // wrap it and then swap it . On 2012/11/16 06:24:31, joth wrote: > spurious space Done. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:373: ContentViewCore cvc = new ContentViewCore(mContainerView.getContext(), Went for newCore :) https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:377: mIsAccessFromFileUrlsGrantedByDefault); sg. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:389: nativeReInit(mNativeAwContents, newWebContentsPtr); I don't think there's that much code we can share on this side, as the ordering is a bit different. In the ctor, we must new a ContentViewCore before running the AwContents init, and we need to have run AwContents init to get the web contents. When doing the popup, we don't need the native AwContents init inbetween the CVC ctor and initialize call. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:394: mContentViewCore.onSizeChanged(mContainerView.getWidth(), mContainerView.getHeight(), 0, 0); On 2012/11/16 06:24:31, joth wrote: > nit: could skip this if the width ==0 && height == 0 Done. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_contents.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:103: void AwContents::ReInit(JNIEnv* env, jobject obj, jint new_wc) { On 2012/11/16 06:24:31, joth wrote: > InstallWebContents or SetWebContents ? Done. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:105: web_contents_.reset(wc); sg. Left over from when I was doing other things in this function I think. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:110: render_view_host_ext_.reset(new AwRenderViewHostExt(web_contents_.get())); On 2012/11/16 06:24:31, joth wrote: > factor out a method for all this and share with constructor? > > or.... just delete it from the c'tor and have the java side call both methods: > mNativeAwContents = nativeInit(); > nativeInstallWebContents(mNativeAwContents, nativeWebContents) > > Done. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:111: web_contents_->WasShown(); mm, yeah. Kind of made sense while this is a "init the popup window" method, but agree bit dodgier when factored out into a general setWebContents method. I think we can probably trigger it with a ContentViewCore call in AwContents.java. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:393: void AwContents::SetPendingWebContentsForPopup(content::WebContents* pending) { On 2012/11/16 06:24:31, joth wrote: > consider passing: scoped_ptr<WebContents> pending_content Done. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.cc:395: DCHECK(!pending_contents_); On 2012/11/16 06:24:31, joth wrote: > I'd be tempted to do: > if (pending_contents_) { > LOG(WARNING) << "blocking window popup as previous popup still pending"; > delete pending; > } Done. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_contents.h (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents.h:100: return reinterpret_cast<jint>(pending_contents_.release()); On 2012/11/16 06:24:31, joth wrote: > put in .cc ? Done. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_contents_io_thread_client_impl.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents_io_thread_client_impl.cc:96: base::AutoLock lock(map_lock_); On 2012/11/16 06:24:31, joth wrote: > move down to 105 Done. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents_io_thread_client_impl.cc:106: // we should now notify. AddObserver will be coming from the IO thread, but calls to insert may come on the UI thread, so I think it needs to stay. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_web_contents_delegate.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_web_contents_delegate.cc:83: // a callback from them later with an AwContents to use to displau On 2012/11/16 06:24:31, joth wrote: > typo: dispau Done.
thanks martin! https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... File android_webview/browser/aw_contents_io_thread_client.h (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... android_webview/browser/aw_contents_io_thread_client.h:50: Do you have a precedent? I had a quick look at some other users and couldn't see that pattern being used. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:130: if (!resumed_ && new_child_id == child_id_ && new_route_id == route_id_) { IoThreadClientThrottle maybe? With a class comment saying something along the lines of "defers until the IO client is ready, then will potentially cancel the request depending on the result of client callbacks"? https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_contents_io_thread_client_impl.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents_io_thread_client_impl.cc:96: base::AutoLock lock(map_lock_); As in, Martin makes a good point, so leaving the lock in place.
https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... File android_webview/browser/aw_contents_io_thread_client.h (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... android_webview/browser/aw_contents_io_thread_client.h:50: On 2012/11/16 12:36:31, benm wrote: > Do you have a precedent? I had a quick look at some other users and couldn't see > that pattern being used. I got the idea from WebContentsObserver being friends with WebContentsImpl, but you're right - it doesn't look like it's used consistently everywhere.
https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... File android_webview/browser/aw_contents_io_thread_client.h (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/bro... android_webview/browser/aw_contents_io_thread_client.h:50: OK, thanks. think i will leave as is then.
https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwContents.java:389: nativeReInit(mNativeAwContents, newWebContentsPtr); On 2012/11/16 12:29:09, benm wrote: > I don't think there's that much code we can share on this side, as the ordering > is a bit different. In the ctor, we must new a ContentViewCore before running > the AwContents init, But this is just an accidental dependency. We should be explicitly calling AndroidBrowserProcess.initContentViewProcess() somewhere (e.g. the AwContentsFactory class I have a TODO to create :-), For now in AwContents c'tor would be fine. Then we can order these two methods anyway it makes sense to. https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... File android_webview/native/aw_contents_io_thread_client_impl.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/5001/android_webview/nat... android_webview/native/aw_contents_io_thread_client_impl.cc:96: base::AutoLock lock(map_lock_); On 2012/11/16 12:28:32, Martin Kosiba wrote: > On 2012/11/16 06:24:31, joth wrote: > > move down to 105 > > I'd prefer not to. Moving the lock would make it possible for 2 threads to new > up the ObserverList twice, so we'd have to DCHECK that this is called from a > specific thread. > > I think having some methods on this class being threadsafe, while others being > XXX-thread only makes the API confusing. It's not obvious ready_observers_ is intended to be guarded by map_lock_, partly due to the name of map_lock_, and partly because it's not covered by the lock in RemoveObserver() and partly because usage of the observer list itself is inherently thread safe (just not assignment to the ref pointer). How about using map_lock_ consistently in all methods, rename it to lock_, and then you can use a plain old ObserverList, no need for it to do internal locking too. Ah hang on, the ThreadsafeObserverList also provides us with async callbacks on the thread the client originally registered on. That's pretty important... probably worth a note on line 68 that this is why we use the "ThreadSafe" version. (although, see my other comments on the callback flow) https://chromiumcodereview.appspot.com/11348075/diff/12001/android_webview/br... File android_webview/browser/aw_contents_io_thread_client.h (right): https://chromiumcodereview.appspot.com/11348075/diff/12001/android_webview/br... android_webview/browser/aw_contents_io_thread_client.h:51: virtual void OnIoThreadClientReady(int child_id, int route_id) = 0; protected virtual d'tor. https://chromiumcodereview.appspot.com/11348075/diff/12001/android_webview/br... android_webview/browser/aw_contents_io_thread_client.h:57: static void RemoveReadyObserver(ReadyObserver* obs); document which thread these must be called on, and which thread you'll receive the callback on. https://chromiumcodereview.appspot.com/11348075/diff/12001/android_webview/br... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/12001/android_webview/br... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:98: } add: AwContentsIoThreadClient::RemoveReadyObserver(this); added_observer_ = false; (maybe then you can remove |handled_| -- not sure) https://chromiumcodereview.appspot.com/11348075/diff/12001/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://chromiumcodereview.appspot.com/11348075/diff/12001/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContents.java:399: mContentViewCore.onShow(); 1) I *think* this needs to be conditional on whether the container view is visible right now 2) Should probably plumb this via AwContents.updateVisiblityState() 3) AIUI this call matches the new_contents->WasHidden(); call in AwWebContentsDelegate::AddNewContents (c++ side). The asymmetry bugs me, but no specific suggestion for improvement other than add a comment in both places referring to each other. https://chromiumcodereview.appspot.com/11348075/diff/12001/android_webview/na... File android_webview/native/aw_contents_io_thread_client_impl.cc (right): https://chromiumcodereview.appspot.com/11348075/diff/12001/android_webview/na... android_webview/native/aw_contents_io_thread_client_impl.cc:112: } thinking about it, rather than run through the entire list here I think it would be clearer to just require the client to register its observer *before* it calls AwContentsIoThreadClient::FromID. or even nicer would be to have the client provide the process/view ID it wants to observe and then we only notify them for that single client getting registered (either immediately, or asynchronously, depending on if it's already in the map or not). We'd then just pass up the scoped_ptr<AwContentsIoThreadClient> as the OnClientReady function param. .... I'm now also debating in my mind if the ReadyObserver is needed at all. A different approach would be to make AwResourceDispatcherHostDelegate hold a list of deferred 'waiting for client' throttles, and then in AwContents::SetIoThreadClient() we could poke AwResourceDispatcherHostDelegate and have it walk that list and see if any deferred throttles can now make progress. (it's easy to get process & routing ID from WebContents, and then AwResourceDispatcherHostDelegate would internally post a task to IO thread in order to walk the list) Benefit of this approach is it keeps this "map" class simpler (more like a dumb container, as it's name suggests), and reduces coupling between the throttles and the map. WDYT?
FYI http://codereview.chromium.org/11348075/diff/12001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): http://codereview.chromium.org/11348075/diff/12001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:398: } In my patch, I've had to add nativeDidInitializeContentViewCore() -- you'll need to call that here too. http://codereview.chromium.org/11418025/diff/2002/android_webview/java/src/or...
thanks joth, new iteration soon https://codereview.chromium.org/11348075/diff/12001/android_webview/browser/a... File android_webview/browser/aw_contents_io_thread_client.h (right): https://codereview.chromium.org/11348075/diff/12001/android_webview/browser/a... android_webview/browser/aw_contents_io_thread_client.h:51: virtual void OnIoThreadClientReady(int child_id, int route_id) = 0; On 2012/11/16 21:52:57, joth wrote: > protected virtual d'tor. Done. https://codereview.chromium.org/11348075/diff/12001/android_webview/browser/a... android_webview/browser/aw_contents_io_thread_client.h:57: static void RemoveReadyObserver(ReadyObserver* obs); Would do, but I think I'm going to go with your suggestion of keeping the map "dumb" and have AwContents call into the AwResourceDispatcherHostDelegate. For completeness though, you can call on any thread and you will be called back on the thread that you call on. https://codereview.chromium.org/11348075/diff/12001/android_webview/browser/r... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/11348075/diff/12001/android_webview/browser/r... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:98: } This should all go away and become simpler in next patch. https://codereview.chromium.org/11348075/diff/12001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11348075/diff/12001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:398: } On 2012/11/17 01:38:21, joth wrote: > In my patch, I've had to add nativeDidInitializeContentViewCore() -- you'll > need to call that here too. > > http://codereview.chromium.org/11418025/diff/2002/android_webview/java/src/or... Done. https://codereview.chromium.org/11348075/diff/12001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:399: mContentViewCore.onShow(); On 2012/11/16 21:52:57, joth wrote: > 1) I *think* this needs to be conditional on whether the container view is > visible right now Seems reasonable. > 2) Should probably plumb this via AwContents.updateVisiblityState() You mean generally, when we make the WebView visible we should make sure to call mContentViewCore.onShow() ? i.e. to cover the case that the embedder creates the pop up webview hidden and then later makes it VISIBLE? > 3) AIUI this call matches the new_contents->WasHidden(); call in > AwWebContentsDelegate::AddNewContents (c++ side). The asymmetry bugs me, but no > specific suggestion for improvement other than add a comment in both places > referring to each other. Will add a comment. https://codereview.chromium.org/11348075/diff/12001/android_webview/native/aw... File android_webview/native/aw_contents_io_thread_client_impl.cc (right): https://codereview.chromium.org/11348075/diff/12001/android_webview/native/aw... android_webview/native/aw_contents_io_thread_client_impl.cc:112: } On 2012/11/16 21:52:57, joth wrote: > thinking about it, rather than run through the entire list here I think it would > be clearer to just require the client to register its observer *before* it calls > AwContentsIoThreadClient::FromID. > > or even nicer would be to have the client provide the process/view ID it wants > to observe and then we only notify them for that single client getting > registered (either immediately, or asynchronously, depending on if it's already > in the map or not). We'd then just pass up the > scoped_ptr<AwContentsIoThreadClient> as the OnClientReady function param. > > .... > I'm now also debating in my mind if the ReadyObserver is needed at all. A > different approach would be to make AwResourceDispatcherHostDelegate hold a list > of deferred 'waiting for client' throttles, and then in > AwContents::SetIoThreadClient() we could poke AwResourceDispatcherHostDelegate > and have it walk that list and see if any deferred throttles can now make > progress. (it's easy to get process & routing ID from WebContents, and then > AwResourceDispatcherHostDelegate would internally post a task to IO thread in > order to walk the list) How would we poke the AwResourceDispatcherHostDelegate? I don't see a way of getting to it other than making the list static and having a static IoThreadClientReady function that posts the task to the IO thread and resumes the throttle if appropriate. Is the Delegate a singleton? (I guess so, as the ResourceDispatcherHost appears to be). > Benefit of this approach is it keeps this "map" class simpler (more like a dumb > container, as it's name suggests), and reduces coupling between the throttles > and the map. > WDYT? >
https://codereview.chromium.org/11348075/diff/12001/android_webview/native/aw... File android_webview/native/aw_contents_io_thread_client_impl.cc (right): https://codereview.chromium.org/11348075/diff/12001/android_webview/native/aw... android_webview/native/aw_contents_io_thread_client_impl.cc:112: } On 2012/11/28 20:00:05, benm wrote: > On 2012/11/16 21:52:57, joth wrote: > > thinking about it, rather than run through the entire list here I think it > would > > be clearer to just require the client to register its observer *before* it > calls > > AwContentsIoThreadClient::FromID. > > > > or even nicer would be to have the client provide the process/view ID it wants > > to observe and then we only notify them for that single client getting > > registered (either immediately, or asynchronously, depending on if it's > already > > in the map or not). We'd then just pass up the > > scoped_ptr<AwContentsIoThreadClient> as the OnClientReady function param. > > > > .... > > I'm now also debating in my mind if the ReadyObserver is needed at all. A > > different approach would be to make AwResourceDispatcherHostDelegate hold a > list > > of deferred 'waiting for client' throttles, and then in > > AwContents::SetIoThreadClient() we could poke AwResourceDispatcherHostDelegate > > and have it walk that list and see if any deferred throttles can now make > > progress. (it's easy to get process & routing ID from WebContents, and then > > AwResourceDispatcherHostDelegate would internally post a task to IO thread in > > order to walk the list) > > How would we poke the AwResourceDispatcherHostDelegate? AwResourceDispatcherHostDelegate is indeed a singleton (see base::LazyInstance... g_webview_resource_dispatcher_host_delegate) so we'd add a static method on AwResourceDispatcherHostDelegate that would walk it's list. But yeah it's not nice to rely on the singleton nature more than we have to, so keeping direct observer coupling as you have it may be the best option. > I don't see a way of > getting to it other than making the list static and having a static > IoThreadClientReady function that posts the task to the IO thread and resumes > the throttle if appropriate. Is the Delegate a singleton? (I guess so, as the > ResourceDispatcherHost appears to be). > > > Benefit of this approach is it keeps this "map" class simpler (more like a > dumb > > container, as it's name suggests), and reduces coupling between the throttles > > and the map. > > WDYT? > > > > >
Bots are green, ptal.
aside from thread safety issue with the map, just non-functional nits. (I'm OOO tomorrow so feel free to TBR) https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:56: IoThreadClientThrottle::~IoThreadClientThrottle() { It maybe best to do the map.erase(this) call from here. That will ensure the map is not left with stale pointer however this thing gets deleted. (although, need to take care about thread safety. I think that map should only be touched on IO thread, so should be fine. DCHECKs will help :) https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:248: pending_throttles_->insert( does this get run on IO thread? we're searching and erasing on UI thread above. hmm. I think we can do all access to it on IO thread, if we just have OnIoThreadClientReady call itself straight back on the IO thread. https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h:27: class IoThreadClientThrottle I think you can just forward declare this class here? leave definition in the cc https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h:93: static PendingThrottleMap* pending_throttles_; no need for this to be static, make it a straight member (not pointer) and then access via g_webview_resource_dispatcher_host_delegate.pending_throttles_ (in most cases, have static Foo() call into a non-static private Foo() will avoid the need to explicitly get at this member via the singleton) https://codereview.chromium.org/11348075/diff/1029/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:686: } I think it clearer if we factor both these into updateVisibilityChanged. Also, we may send incorrect onShow() calls e.g. if a window becomes visible doesn't automatically mean the view is (and vice versa). if we ContentViewCoew only cares about the View (not window) visibility, then we can just delete this block here. https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... android_webview/native/aw_contents.cc:131: SetWebContents(dependency_factory->CreateWebContents(private_browsing)); I think it will be clearer to move all this to java. Suggest: TODO(joth): rather than create and set the WebContents here, expose the factory method to java side and have that orchestrate the construction order https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... android_webview/native/aw_contents.cc:532: return; This will delete |pending|. Snag is both AwWebContentsDelegate and WebContentsImpl may call methods on it after this method returns. So I think we need MessageLoop::current()->DeleteSoon(pending.release()); https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... File android_webview/native/aw_web_contents_delegate.cc (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... android_webview/native/aw_web_contents_delegate.cc:95: delete new_contents; again, MessageLoop DeleteSoon(new_contents)
thanks joth! https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:56: IoThreadClientThrottle::~IoThreadClientThrottle() { On 2012/11/29 21:02:33, joth wrote: > It maybe best to do the map.erase(this) call from here. That will ensure the map > is not left with stale pointer however this thing gets deleted. > (although, need to take care about thread safety. I think that map should only > be touched on IO thread, so should be fine. DCHECKs will help :) Done. https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:248: pending_throttles_->insert( On 2012/11/29 21:02:33, joth wrote: > does this get run on IO thread? we're searching and erasing on UI thread above. > > hmm. I think we can do all access to it on IO thread, if we just have > OnIoThreadClientReady call itself straight back on the IO thread. Done. https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h:27: class IoThreadClientThrottle On 2012/11/29 21:02:33, joth wrote: > I think you can just forward declare this class here? leave definition in the cc Done. https://codereview.chromium.org/11348075/diff/1029/android_webview/browser/re... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h:93: static PendingThrottleMap* pending_throttles_; On 2012/11/29 21:02:33, joth wrote: > no need for this to be static, make it a straight member (not pointer) and then > access via g_webview_resource_dispatcher_host_delegate.pending_throttles_ > > (in most cases, have static Foo() call into a non-static private Foo() will > avoid the need to explicitly get at this member via the singleton) Done. https://codereview.chromium.org/11348075/diff/1029/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:686: } Good point about window becoming visible not necessarily meaning view is visible. https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... android_webview/native/aw_contents.cc:131: SetWebContents(dependency_factory->CreateWebContents(private_browsing)); On 2012/11/29 21:02:33, joth wrote: > I think it will be clearer to move all this to java. Suggest: > > TODO(joth): rather than create and set the WebContents here, expose the factory > method to java side and have that orchestrate the construction order Done. https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... android_webview/native/aw_contents.cc:532: return; Good point. https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... File android_webview/native/aw_web_contents_delegate.cc (right): https://codereview.chromium.org/11348075/diff/1029/android_webview/native/aw_... android_webview/native/aw_web_contents_delegate.cc:95: delete new_contents; On 2012/11/29 21:02:33, joth wrote: > again, MessageLoop DeleteSoon(new_contents) Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benm@chromium.org/11348075/6013
Message was sent while issue was closed.
Change committed as 170721 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
