Dismiss browser plugin modal dialogs when the embedder needs to.

Dismiss browser plugin modal dialogs when the embedder needs to.

Test from wjmaclean@.  PDF simply shows an alert dialog using script.

BUG=482380
TEST=See bug for repro steps.

Committed: https://crrev.com/89a0f782193755ad7a0b93c58dbcc1b96528405f
Cr-Commit-Position: refs/heads/master@{#331584}

[Charlie Reis, 2015-05-20 23:38:06]

Patchset #1 (id:1) has been deleted

[Charlie Reis, 2015-05-20 23:40:37]

Patchset #1 (id:20001) has been deleted

[Charlie Reis, 2015-05-20 23:49:18]

creis@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[Charlie Reis, 2015-05-20 23:49:19]

wjmaclean: Here's a draft of what we were discussing.  Is it similar to what you
were envisioning?

Would you be able to help write a test for this?  I've seen tests of dialogs in
chrome/ using AppModalDialogQueue (e.g.,
https://codereview.chromium.org/256233002), but I'm not sure how to test the
BrowserPlugin part of it.

Feel free to either take this over in a new patch or send a test for me to
include in here.  Thanks!

[Fady Samuel, 2015-05-21 00:12:19]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-05-21 00:12:20]

browser_plugin cannot be tested from content currently, because it assumes it
has a BrowserPluginGuestDelegate and BrowserPluginDelegate. Currently, those
implementations are GuestViewBase, and GuestViewContainer, respectively by
tomorrow will all be in components.

This particular bug is probably not testable with a <webview>. I would speak to
paulmeyer@ about writing a MIMEHandlerView test as he spent a bunch of time
writing one recently.

[Charlie Reis, 2015-05-21 23:24:19]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2015-05-21 23:24:20]

Nasko, can I get your thoughts on this while James and Paul discuss how we can
add a test?

I'm not thrilled that the outer embedder WebContents has to relay this message
to all the guests, but they maintain separate dialog state and weren't realizing
that they needed to dismiss their dialogs when an interstitial is shown.

[Fady Samuel, 2015-05-22 13:00:50]

A possible alternative (maybe cleaner?) approach is to do it in the
OwnerContentsObserver in GuestViewBase:

https://code.google.com/p/chromium/codesearch#chromium/src/components/guest_v...

OwnerContentsObserver::DidAttachInterstitialPage could call
guest_->web_contents()->GetJavaScriptDialogManager()->CancelActiveAndPendingDialogs().

Note: Not all Guestviews have a dialog manager I think so we'd need to do a null
check.

I feel like the observer pattern is a little cleaner than a ForEachGuest.

[nasko, 2015-05-22 14:20:51]

On 2015/05/22 13:00:50, Fady Samuel wrote:
> A possible alternative (maybe cleaner?) approach is to do it in the
> OwnerContentsObserver in GuestViewBase:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/guest_v...
> 
> OwnerContentsObserver::DidAttachInterstitialPage could call
>
guest_->web_contents()->GetJavaScriptDialogManager()->CancelActiveAndPendingDialogs().
> 
> Note: Not all Guestviews have a dialog manager I think so we'd need to do a
null
> check.
> 
> I feel like the observer pattern is a little cleaner than a ForEachGuest.

While I like the observer patter a bit better, I think it is better to leave
this responsibility to content/ as we will likely have to handle the same
scenario with inner/outer WebContents as we move over to oopif.

[Charlie Reis, 2015-05-22 17:21:04]

On 2015/05/22 14:20:51, nasko wrote:
> On 2015/05/22 13:00:50, Fady Samuel wrote:
> > A possible alternative (maybe cleaner?) approach is to do it in the
> > OwnerContentsObserver in GuestViewBase:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/guest_v...
> > 
> > OwnerContentsObserver::DidAttachInterstitialPage could call
> >
>
guest_->web_contents()->GetJavaScriptDialogManager()->CancelActiveAndPendingDialogs().
> > 
> > Note: Not all Guestviews have a dialog manager I think so we'd need to do a
> null
> > check.
> > 
> > I feel like the observer pattern is a little cleaner than a ForEachGuest.
> 
> While I like the observer patter a bit better, I think it is better to leave
> this responsibility to content/ as we will likely have to handle the same
> scenario with inner/outer WebContents as we move over to oopif.

I can see the benefits of both approaches.

It seems unfortunate for an outer WebContents to have to remember to tell its
inner WebContents(es) about events, and an observer at the webview level could
handle all the current places we call CancelActiveAndPendingDialogs (i.e.,
DidAttachInterstitialPage, DidNavigateAnyFrame, and RenderViewTerminated).

However, it would be very easy to forget to update the observer if we later had
a bug requiring us to add another CancelActiveAndPendingDialogs call to
WebContents.  In the current approach, that's handled for free (at least for
dialogs).

Neither approach helps much with catching similar bugs in the future for other
types of state in WebContents.

For this, I'm inclined to leave the fix in content/, since we're adding first
class support for inner/outer WebContents as Nasko mentions.  Thanks for the
suggestion, though!

[Charlie Reis, 2015-05-26 21:28:07]

Thanks for the test, wjmaclean!  I've added it to this CL.

Nasko and Fady, PTAL.

[nasko, 2015-05-26 22:39:50]

content/ LGTM

[Charlie Reis, 2015-05-26 22:51:12]

creis@chromium.org changed reviewers:
+ sky@chromium.org

[Charlie Reis, 2015-05-26 22:51:13]

Thanks!  I had to move the PDF from chrome/test/data/pdf/ to chrome/test/data/
since the PDFExtensionTest iterates over all PDFs in the former directory and
fails when I add this new one.  (Sigh.)

+sky for chrome/ owners.

[Fady Samuel, 2015-05-26 23:05:41]

browser_plugin lgtm

[sky, 2015-05-27 15:38:50]

LGTM

[Charlie Reis, 2015-05-27 16:08:06]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-05-27 16:08:07]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1150843002/#ps80001
(title: "Move test file")

[commit-bot: I haz the power, 2015-05-27 16:08:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150843002/80001

[commit-bot: I haz the power, 2015-05-27 16:13:32]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-05-27 16:14:27]

Patchset 3 (id:??) landed as
https://crrev.com/89a0f782193755ad7a0b93c58dbcc1b96528405f
Cr-Commit-Position: refs/heads/master@{#331584}