Suppress JavaScript dialogs when a tab puts itself into fullscreen.

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 39519544c8df67a06252595604997d394b10d618..403711908cc6ff130510f078621e48fee6f43ee0 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -1206,6 +1206,14 @@ bool Browser::CanOverscrollContent() const {
   return true;
 }
 
+bool Browser::ShouldSuppressDialogs(WebContents* source, bool before_unload) {
+  if (before_unload)
+    return false;
+
+  return exclusive_access_manager_->fullscreen_controller()

[Matt Giuca, 2016/08/02 01:42:20]

optionally add a comment:
// Do not show dialogs if the browser is in fullscreen.

[Avi (use Gerrit), 2016/08/02 03:53:30]

That seems obvious enough from the code, though.

[Matt Giuca, 2016/08/02 04:23:50]

Acknowledged, but see below because it's going to be less clear now.

+      ->IsControllerInitiatedFullscreen();

[Matt Giuca, 2016/08/02 03:34:56]

Actually, I thought some more about this. I think we don't want this suppressed
in user-entered fullscreen (e.g., F11), only in site-entered fullscreen (HTML5
API).

Rationale is that sites have no control over the user-entered fullscreen so this
would immediately break all sites using alerts, if the user decides to use
fullscreen (e.g., using Reitveld in fullscreen, press Delete on a Patch Set and
there will be no confirm prompt, just silent failure). Whereas if we only block
prompts on HTML5 fullscreen, it only breaks a fraction of sites. Also there is
no spoofing possibility for user fullscreen.

So change IsControllerInitiatedFullscreen to IsTabFullscreen. See:
https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren...

(The name IsTabFullscreen is bad; I'll change it but after your CL to avoid
conflicts.)

[Avi (use Gerrit), 2016/08/02 03:53:30]

That is a very reasonable approach, and I was thinking about our possibilities
here, but got lost in the details of the fullscreen options.

First, I'd recommend you go ahead and rename it, and I'll rebase.

Second, this really helps us make the case publicly.

Do the renaming tomorrow morning and let me know the new name, and I'll adjust
this.

[Matt Giuca, 2016/08/02 04:23:50]

Actually, I started doing this but I discovered that FullscreenController has a
fairly well-established terminology for "tab fullscreen" versus "browser
fullscreen" which I don't think we want to change:

https://cs.chromium.org/chromium/src/chrome/browser/ui/exclusive_access/fulls...

"Tab fullscreen" refers to any renderer-initiated fullscreen (Flash or HTML5
API) so renaming it to "IsHTMLFullscreen" would become a misnomer. Also the
terminology "Tab" is internally consistent throughout the many methods of
FullscreenController. So let's just keep it as-is. Maybe you can add a comment
here saying exactly what modes this captures.

(I'm not sure we need to worry about Flash fullscreen because we shouldn't be
getting alert() popups during fullscreen Flash, but if we did, I think it would
be safe to suppress them.)

[Avi (use Gerrit), 2016/08/02 04:27:54]

In that case, agreed, I'll clarify. How about:

// In the case where fullscreen was triggered by the web page
// ("tab fullscreen"), do not allow dialogs. This does not affect
// the case of user-initiated fullscreen ("browser fullscreen").

Is that accurate? (I'll switch to IsTabFullscreen with the next patchset too.)

[Matt Giuca, 2016/08/02 04:28:52]

SGTM.

+}
+
 bool Browser::ShouldPreserveAbortedURLs(WebContents* source) {
   // Allow failed URLs to stick around in the omnibox on the NTP, but not when
   // other pages have committed.