Add ShouldDisplayOverFullscreen support to web notifications.

chrome/browser/notifications/web_notification_delegate.cc

+// Copyright 2016 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/notifications/web_notification_delegate.h"
+
+#include "chrome/browser/notifications/notification_common.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/exclusive_access/exclusive_access_context.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "content/public/browser/web_contents.h"
+
+WebNotificationDelegate::WebNotificationDelegate(
+    content::BrowserContext* browser_context,
+    const std::string& notification_id,
+    const GURL& origin)
+    : browser_context_(browser_context),
+      notification_id_(notification_id),
+      origin_(origin) {}
+
+WebNotificationDelegate::~WebNotificationDelegate() {}
+
+std::string WebNotificationDelegate::id() const {
+  return notification_id_;
+}
+
+void WebNotificationDelegate::SettingsClick() {
+  NotificationCommon::OpenNotificationSettings(browser_context_);
+}
+
+bool WebNotificationDelegate::ShouldDisplaySettingsButton() {
+  return true;
+}
+
+bool WebNotificationDelegate::ShouldDisplayOverFullscreen() const {
+  Profile* profile = Profile::FromBrowserContext(browser_context_);
+  DCHECK(profile);
+  // Check to see if this notification comes from a webpage that is displaying
+  // fullscreen content.
+  for (auto* browser : *BrowserList::GetInstance()) {
+    // Only consider the browsers for the profile that created the notification
+    if (browser->profile() == profile) {

[Peter Beverloo, 2016/09/27 17:17:36]

micro nit: you could check whether the |browser|'s profile is *not* equal to
|profile| and "continue" to the next iteration in the loop instead, that saves a
level of indentation.

We used to prefer this in Blink, I don't think Chromium has guidelines on it.
Entirely up to you.

[bmalcolm, 2016/09/27 20:34:36]

I normally choose that pattern when there are several conditions that may cause
a continuation, but in this case because there was a single case, I thought the
code was slightly more readable without the continue.  But it's not a big deal,
changed since there's a slight preference the other way.

+      const content::WebContents* active_contents =
+          browser->tab_strip_model()->GetActiveWebContents();
+      DCHECK(active_contents);

[Peter Beverloo, 2016/09/27 17:17:37]

The documentation says that this may be NULL, and most non-test code has a NULL
check in place. We should do the same.

https://cs.chromium.org/chromium/src/chrome/browser/ui/tabs/tab_strip_model.h...

[bmalcolm, 2016/09/27 20:34:36]

Done.

+      // Check to see if
+      //  (a) the active tab in the browser is the source of the notification.

[Peter Beverloo, 2016/09/27 17:17:37]

I'd rephrase this, since it may not be. We don't have a definite way to
associate notifications with their source, since the source (in 99% of cases,
ironically) is not a page.

What about:

  //  (a) the active tab in the browser shares its origin with the notification.

[bmalcolm, 2016/09/27 20:34:36]

Done.

+      //  (b) the browser is fullscreen
+      //  (c) the browser has focus.
+      if (active_contents->GetURL().GetOrigin() == origin_ &&
+          browser->exclusive_access_manager()->context()->IsFullscreen() &&
+          browser->window()->IsActive())
+        return true;

[Peter Beverloo, 2016/09/27 17:17:37]

micro nit: brackets for multi-line if statements.

[bmalcolm, 2016/09/27 20:34:36]

Done.

+    }
+  }
+
+  return false;
+}