Disallow display of multiple experimental.extension.popup(...) windows

Clean-up of the asynchronous behaviour of the experimental extension popup API. 

 - It was previously possible to programmatically launch two popups from the same extension.  (Simply call popup.show twice in a row, or in a callback chain.)  

 I removed this incorrect funcationality by registering ExtensionPopupHost to listen for EXTENSION_HOST_CREATED notifications.  If a popup is shown, and a new ExtensionHost is constructed of type EXTENSION_POPUP, then the presently displayed popup is dismissed. 

 - The callback function for popup.show(...) was  previously called in response to EXTENSION_POPUP_VIEW_READY, as processed in  response to a ViewHostMsg_DocumentAvailableInMainFrame message.  This message wassent after PARSING of the conent of the popup view.  Because of this behaviour, the API was difficult to use because one could not meaningfully interact with the popup page 
during the callback:  The callback would race with completion of the onload handler within the popup, so some sort of polling for onload-complete was required.
  I fixed the problem by adding new notifications and messages so that EXTENSION_POPUP_VIEW_READY is now sent only after all onload handlers have been invoked.Corresponding unit-tests have also been added.

BUG=None
TEST=ExtensionApiTest.Popup


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

[Jeff Timanus, 2010-04-26 19:41:02]

Hi Rafael, 

If you have a moment, could you please have a look at this?

Thx,

Jeff

[rafaelw1, 2010-04-26 20:32:02]

Will get this to you by tonight.

R

On Mon, Apr 26, 2010 at 12:41 PM,  <twiz@chromium.org> wrote:
> Hi Rafael,
>
> If you have a moment, could you please have a look at this?
>
> Thx,
>
> Jeff
>
> http://codereview.chromium.org/1512007/show
>

[rafaelw, 2010-04-27 03:32:10]

Also, please change the DCHECK in ExtensionProcessBindings from "is there at
least returned view" to "there is exactly 1 returned view" (for GetPopupView
impl).

http://codereview.chromium.org/1512007/diff/4001/5002
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/1512007/diff/4001/5002#newcode178
chrome/browser/extensions/extension_popup_api.cc:178: if
(Details<ExtensionHost>(popup_->host()) != details &&
nit: make checks in this order:

1) Is EXTENSION_POPUP?
2) Is this Extension?
3) Is different ExtensionHost?

http://codereview.chromium.org/1512007/diff/4001/5012
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1512007/diff/4001/5012#newcode2690
chrome/renderer/render_view.cc:2690: Send(new
ViewHostMsg_DocumentOnLoadCompletedInMainFrame(routing_id_));
Are you sure this only gets called for the main frame? I'd guess by looking at
this that it gets called for all frames (the source seems to be
Document::implicitClose which would suggest the same thing to me).

http://codereview.chromium.org/1512007/diff/4001/5001
File chrome/test/data/extensions/api_test/popup/toolband.html (right):

http://codereview.chromium.org/1512007/diff/4001/5001#newcode130
chrome/test/data/extensions/api_test/popup/toolband.html:130: // This test
ensures that for a given extension with a popup displayed,
I feel like tests that assert correct behavior implicitly, rather than through
accounting preferable.

Would the following test be simpler:

1) Open popup_a from here.
2) Popup_a opens popup_b.
3) popup_b calls getViews() and tests if he's the only popup (via page title, or
getIdentity(), etc...) and tells toolbar test to succeed or fail

?

[twiz, 2010-04-27 05:44:11]

http://codereview.chromium.org/1512007/diff/4001/5002
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/1512007/diff/4001/5002#newcode178
chrome/browser/extensions/extension_popup_api.cc:178: if
(Details<ExtensionHost>(popup_->host()) != details &&
On 2010/04/27 03:32:10, rafaelw wrote:
> nit: make checks in this order:
> 
> 1) Is EXTENSION_POPUP?
> 2) Is this Extension?
> 3) Is different ExtensionHost?

Done.

http://codereview.chromium.org/1512007/diff/4001/5012
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1512007/diff/4001/5012#newcode2690
chrome/renderer/render_view.cc:2690: Send(new
ViewHostMsg_DocumentOnLoadCompletedInMainFrame(routing_id_));
On 2010/04/27 03:32:10, rafaelw wrote:
> Are you sure this only gets called for the main frame? I'd guess by looking at
> this that it gets called for all frames (the source seems to be
> Document::implicitClose which would suggest the same thing to me).

My webkit expertise is novice at best.  Are you suggesting that nested elements
(such as iframes) will trigger multiple calls to this function?  (Once for the
iframe, and once again for the main document?)

http://codereview.chromium.org/1512007/diff/4001/5001
File chrome/test/data/extensions/api_test/popup/toolband.html (right):

http://codereview.chromium.org/1512007/diff/4001/5001#newcode130
chrome/test/data/extensions/api_test/popup/toolband.html:130: // This test
ensures that for a given extension with a popup displayed,
On 2010/04/27 03:32:10, rafaelw wrote:
> I feel like tests that assert correct behavior implicitly, rather than through
> accounting preferable.
> 
> Would the following test be simpler:
> 
> 1) Open popup_a from here.
> 2) Popup_a opens popup_b.
> 3) popup_b calls getViews() and tests if he's the only popup (via page title,
or
> getIdentity(), etc...) and tells toolbar test to succeed or fail
> 
> ?

The current design is such that popups cannot nest.  If popup A requested a
popup B, the construction of B would dismiss A, which would result in B becoming
a 'floating' window - A, the anchoring view, would have been dismissed.

(Even though now that I review the code, I don't believe that is strictly
enforced. I added a test that enforces this behaviour.)

Popup A could call back into the toolstrip to request that it launch another
popup, B, which would check that it is the only popup present, but I'm uncertain
if that would clarify the test.

I agree that the test for the closure of A is implicit in this test, but the
identity checks are very explicit during the popup callbacks below.  Given that
the closure of popup A is an implicit operation, I think it's reasonable for the
testing of it to be implicit.  Do you still strongly feel otherwise?

[rafaelw, 2010-04-27 18:11:58]

Also, please see my earlier summary comment about the DCHECK in
ExtensionProcessBindings.

http://codereview.chromium.org/1512007/diff/4001/5012
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1512007/diff/4001/5012#newcode2690
chrome/renderer/render_view.cc:2690: Send(new
ViewHostMsg_DocumentOnLoadCompletedInMainFrame(routing_id_));
I don't know what's going to happen. My guess would be that this gets called
when any frame in the current frameset finishes its onload events. I'd be
surprised if it fired twice for each time a sub-frame finishes.

I'm unfamiliar with this code path as well. You're going to need to do a little
digging.

On 2010/04/27 05:44:11, twiz wrote:
> On 2010/04/27 03:32:10, rafaelw wrote:
> > Are you sure this only gets called for the main frame? I'd guess by looking
at
> > this that it gets called for all frames (the source seems to be
> > Document::implicitClose which would suggest the same thing to me).
> 
> My webkit expertise is novice at best.  Are you suggesting that nested
elements
> (such as iframes) will trigger multiple calls to this function?  (Once for the
> iframe, and once again for the main document?)

http://codereview.chromium.org/1512007/diff/4001/5001
File chrome/test/data/extensions/api_test/popup/toolband.html (right):

http://codereview.chromium.org/1512007/diff/4001/5001#newcode130
chrome/test/data/extensions/api_test/popup/toolband.html:130: // This test
ensures that for a given extension with a popup displayed,
I wasn't suggesting nesting the popups, just having the first open the second.
The first should be able to get the relative-to DOMElement from the toolbar.
i.e.

getToolbar().getElementById('foo').

Basically, I'm objecting to using counters here. I think it's safer to call
getViews() and assert that the right number of popups are being shown. I think
this is a clearer and more direct measurement of the correct behavior. You don't
have to take the suggestion to structure the test, but please use the results of
getViews() to determine the correct state.

On 2010/04/27 05:44:11, twiz wrote:
> On 2010/04/27 03:32:10, rafaelw wrote:
> > I feel like tests that assert correct behavior implicitly, rather than
through
> > accounting preferable.
> > 
> > Would the following test be simpler:
> > 
> > 1) Open popup_a from here.
> > 2) Popup_a opens popup_b.
> > 3) popup_b calls getViews() and tests if he's the only popup (via page
title,
> or
> > getIdentity(), etc...) and tells toolbar test to succeed or fail
> > 
> > ?
> 
> The current design is such that popups cannot nest.  If popup A requested a
> popup B, the construction of B would dismiss A, which would result in B
becoming
> a 'floating' window - A, the anchoring view, would have been dismissed.
> 
> (Even though now that I review the code, I don't believe that is strictly
> enforced. I added a test that enforces this behaviour.)
> 
> Popup A could call back into the toolstrip to request that it launch another
> popup, B, which would check that it is the only popup present, but I'm
uncertain
> if that would clarify the test.
> 
> I agree that the test for the closure of A is implicit in this test, but the
> identity checks are very explicit during the popup callbacks below.  Given
that
> the closure of popup A is an implicit operation, I think it's reasonable for
the
> testing of it to be implicit.  Do you still strongly feel otherwise?
> 
>

[Jeff Timanus, 2010-04-27 22:06:00]

DCHECK changed as you suggested.

Please let me know if there are any other issues remaining with the code as-is. 
Hopefully my use of the webkit-onload callback will turn out to be correct for
this use case.  I'll report back on that later this evening.

Note that I had to hack the test_shell project to allow successful re-building
of the extensions documentation.  A DCHECK in
\webkit\tools\test_shell\simple_appcache_system.cc (370) was triggering during
execution of the build.py script.  I was able to update the extensions docs by
simply removing this DCHECK.

http://codereview.chromium.org/1512007/diff/4001/5012
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1512007/diff/4001/5012#newcode2690
chrome/renderer/render_view.cc:2690: Send(new
ViewHostMsg_DocumentOnLoadCompletedInMainFrame(routing_id_));
On 2010/04/27 18:11:58, rafaelw wrote:
> I don't know what's going to happen. My guess would be that this gets called
> when any frame in the current frameset finishes its onload events. I'd be
> surprised if it fired twice for each time a sub-frame finishes.
> 
> I'm unfamiliar with this code path as well. You're going to need to do a
little
> digging.
> 
> On 2010/04/27 05:44:11, twiz wrote:
> > On 2010/04/27 03:32:10, rafaelw wrote:
> > > Are you sure this only gets called for the main frame? I'd guess by
looking
> at
> > > this that it gets called for all frames (the source seems to be
> > > Document::implicitClose which would suggest the same thing to me).
> > 
> > My webkit expertise is novice at best.  Are you suggesting that nested
> elements
> > (such as iframes) will trigger multiple calls to this function?  (Once for
the
> > iframe, and once again for the main document?)
> 
> 

I'll triple-check this behaviour and report back later this evening with my
findings.

http://codereview.chromium.org/1512007/diff/4001/5001
File chrome/test/data/extensions/api_test/popup/toolband.html (right):

http://codereview.chromium.org/1512007/diff/4001/5001#newcode130
chrome/test/data/extensions/api_test/popup/toolband.html:130: // This test
ensures that for a given extension with a popup displayed,
On 2010/04/27 18:11:58, rafaelw wrote:
> I wasn't suggesting nesting the popups, just having the first open the second.
> The first should be able to get the relative-to DOMElement from the toolbar.
> i.e.
> 
> getToolbar().getElementById('foo').
> 
> Basically, I'm objecting to using counters here. I think it's safer to call
> getViews() and assert that the right number of popups are being shown. I think
> this is a clearer and more direct measurement of the correct behavior. You
don't
> have to take the suggestion to structure the test, but please use the results
of
> getViews() to determine the correct state.

Counters removed, and the tests now rely entirely on getViews.  Unfortunately,
'popup' wasn't a previously supported argument type to get-views, so I had to
add that to the extension_api.json file, and update the documentation.

> 
> On 2010/04/27 05:44:11, twiz wrote:
> > On 2010/04/27 03:32:10, rafaelw wrote:
> > > I feel like tests that assert correct behavior implicitly, rather than
> through
> > > accounting preferable.
> > > 
> > > Would the following test be simpler:
> > > 
> > > 1) Open popup_a from here.
> > > 2) Popup_a opens popup_b.
> > > 3) popup_b calls getViews() and tests if he's the only popup (via page
> title,
> > or
> > > getIdentity(), etc...) and tells toolbar test to succeed or fail
> > > 
> > > ?
> > 
> > The current design is such that popups cannot nest.  If popup A requested a
> > popup B, the construction of B would dismiss A, which would result in B
> becoming
> > a 'floating' window - A, the anchoring view, would have been dismissed.
> > 
> > (Even though now that I review the code, I don't believe that is strictly
> > enforced. I added a test that enforces this behaviour.)
> > 
> > Popup A could call back into the toolstrip to request that it launch another
> > popup, B, which would check that it is the only popup present, but I'm
> uncertain
> > if that would clarify the test.
> > 
> > I agree that the test for the closure of A is implicit in this test, but the
> > identity checks are very explicit during the popup callbacks below.  Given
> that
> > the closure of popup A is an implicit operation, I think it's reasonable for
> the
> > testing of it to be implicit.  Do you still strongly feel otherwise?
> > 
> > 
> 
>

[twiz, 2010-04-28 15:48:16]

http://codereview.chromium.org/1512007/diff/29003/47012
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1512007/diff/29003/47012#newcode2720
chrome/renderer/render_view.cc:2720: Send(new
ViewHostMsg_DocumentOnLoadCompletedInMainFrame(routing_id_));
You were correct:  This call back will be invoked for any frame with an onload
handler in the main frame (for example an iframe.)

I now check and only send the message if the notification corresponds to onload
in the main frame.

[Jeff Timanus, 2010-04-29 19:02:12]

+ Reviewers Erik & Aaron

Erik and Aaron, you may care to comment on some of the changes I've made here
wrt dismissal of the experimental popup API windows.

Jeff

[Erik does not do reviews, 2010-04-30 01:05:41]

I'm not going to have time for this in the next few days.

Erik


On Thu, Apr 29, 2010 at 12:02 PM,  <twiz@chromium.org> wrote:
> + Reviewers Erik & Aaron
>
> Erik and Aaron, you may care to comment on some of the changes I've made
> here
> wrt dismissal of the experimental popup API windows.
>
> Jeff
>
> http://codereview.chromium.org/1512007/show
>

[rafaelw, 2010-04-30 01:27:57]

+brettw

Brett, please just check the new call through for render_view*

lgtm w/ nits.

http://codereview.chromium.org/1512007/diff/52001/53002
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/1512007/diff/52001/53002#newcode307
chrome/browser/extensions/extension_popup_api.cc:307: // Popups may only be
displayed from TAB_CONTENTS and EXTENSION_TOOLSTRIP
nit: this should probably be the first thing that happens in this function. no
sense doing all the input validation.

http://codereview.chromium.org/1512007/diff/52001/53007
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/1512007/diff/52001/53007#newcode534
chrome/browser/renderer_host/render_view_host_delegate.h:534: // The
RenderView's main frame document element is ready, and the onload
Nit, I don't think repeating the first part of this comment from the above
comment is necessary. Just focus on the onload having been handled.

[brettw, 2010-04-30 05:08:37]

On Thu, Apr 29, 2010 at 6:27 PM,  <rafaelw@chromium.org> wrote:
> +brettw
>
> Brett, please just check the new call through for render_view*

I only looked briefly, but it looks fine to me.

Brett

[Jeff Timanus, 2010-04-30 17:11:03]

http://codereview.chromium.org/1512007/diff/52001/53002
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/1512007/diff/52001/53002#newcode307
chrome/browser/extensions/extension_popup_api.cc:307: // Popups may only be
displayed from TAB_CONTENTS and EXTENSION_TOOLSTRIP
On 2010/04/30 01:27:57, rafaelw wrote:
> nit: this should probably be the first thing that happens in this function. no
> sense doing all the input validation.

I had considered doing that too, but I didn't want to add yet another #ifdef
block at the top of the function.  (Upon further inspection, I've noticed that
render_view_host{_delegate}.h do not need to be wrapped in ifdefs, so I've made
the change you suggest.)

Done.

http://codereview.chromium.org/1512007/diff/52001/53007
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/1512007/diff/52001/53007#newcode534
chrome/browser/renderer_host/render_view_host_delegate.h:534: // The
RenderView's main frame document element is ready, and the onload
On 2010/04/30 01:27:57, rafaelw wrote:
> Nit, I don't think repeating the first part of this comment from the above
> comment is necessary. Just focus on the onload having been handled.

Done.