Suppress JavaScript dialogs when a tab puts itself into fullscreen.

chrome/browser/ui/browser.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/browser.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <string>
@@ skipping 1188 matching lines @@
 
   // The detached bookmark bar has appearance of floating above the
   // web-contents. This does not play nicely with overscroll navigation
   // gestures. So disable overscroll navigation when the bookmark bar is in the
   // detached state and the overscroll effect moves the layers.
   if (value == "1" && bookmark_bar_state_ == BookmarkBar::DETACHED)
     return false;
   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.
   Profile* profile = Profile::FromBrowserContext(source->GetBrowserContext());
   if (!profile || !source->GetController().GetLastCommittedEntry())
     return false;
   GURL committed_url(source->GetController().GetLastCommittedEntry()->GetURL());
   return search::IsNTPURL(committed_url, profile);
 }
 
@@ skipping 1383 matching lines @@
   if (contents && !allow_js_access) {
     contents->web_contents()->GetController().LoadURL(
         target_url,
         content::Referrer(),
         ui::PAGE_TRANSITION_LINK,
         std::string());  // No extra headers.
   }
 
   return contents != NULL;
 }