Push API: Don't require notification if tab is visible.

chrome/browser/services/gcm/push_messaging_service_impl.cc

 // Copyright 2014 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/services/gcm/push_messaging_service_impl.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 15 matching lines @@
 #include "chrome/grit/generated_resources.h"
 #include "components/content_settings/core/common/permission_request_id.h"
 #include "components/gcm_driver/gcm_driver.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/platform_notification_data.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/l10n/l10n_util.h"
 
+#if defined(OS_ANDROID)
+#include "chrome/browser/ui/android/tab_model/tab_model.h"
+#include "chrome/browser/ui/android/tab_model/tab_model_list.h"
+#else
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_iterator.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#endif
+
 namespace gcm {
 
 namespace {
 const int kMaxRegistrations = 1000000;
 
 blink::WebPushPermissionStatus ToPushPermission(ContentSetting setting) {
   switch (setting) {
     case CONTENT_SETTING_ALLOW:
       return blink::WebPushPermissionStatusGranted;
     case CONTENT_SETTING_BLOCK:
@@ skipping 187 matching lines @@
       PlatformNotificationServiceImpl::GetInstance();
   // Can't use g_browser_process->notification_ui_manager(), since the test uses
   // PlatformNotificationServiceImpl::SetNotificationUIManagerForTesting.
   // TODO(peter): Remove the need to use both APIs here once Notification.get()
   // is supported.
   int notification_count = notification_service->GetNotificationUIManager()->
       GetAllIdsByProfileAndSourceOrigin(profile_, application_id.origin).size();
   if (notification_count > 0)
     return;
 
+  // Sites with a currently visible tab don't need to show notifications.
+#if defined(OS_ANDROID)
+  for (TabModel* tab_model : TabModelList) {
+    Profile* profile = tab_model->GetProfile();
+    content::WebContents* active_web_contents =
+        tab_model->GetActiveWebContents();
+#else
+  for (chrome::BrowserIterator it; !it.done(); it.Next()) {
+    Profile* profile = it->profile();
+    content::WebContents* active_web_contents =
+        it->tab_strip_model()->GetActiveWebContents();
+#endif
+
+    // Don't leak information from other profiles.
+    if (!profile || profile->IsOffTheRecord() ||
+        !profile->IsSameProfile(profile_)) {

[mlamouri (slow - plz ping), 2015/02/02 12:36:57]

These checks do not really match the comment. Only one does.

What about doing the sanity check first:
if (!profile || !active_web_contents)
  continue;

Then, you could check that we are not leaking information:
if (!profile->IsSameProfile(profile_))
  continue;

Regarding the last check, I'm not sure why it's needed. I assume it's
arbitrary/temporary. If that's the case, could you point to a bug and add a
TODO?

[johnme, 2015/02/02 15:58:36]

Done (I changed it to profile_!=profile, since IsSameProfile considers incognito
and non-incognito to be the same which is inappropriate here, but pointer
equality avoids that problem).

+      continue;
+    }
+
+    // Ignore minimized windows etc.
+    if (!active_web_contents)

[mlamouri (slow - plz ping), 2015/02/02 12:36:58]

Again, the check doesn't match the comment. Could you merge that with the check
of |profile| being not null.

[johnme, 2015/02/02 15:58:36]

Done (split this out).

+      continue;
+    switch (active_web_contents->GetMainFrame()->GetVisibilityState()) {
+     case blink::WebPageVisibilityStateHidden:
+     case blink::WebPageVisibilityStatePrerender:
+      continue;
+     case blink::WebPageVisibilityStateVisible:
+      break;
+    }
+
+    // Use the visible URL since that's the one the user is aware of (and it
+    // doesn't matter whether the page loaded successfully).
+    const GURL& active_url = active_web_contents->GetVisibleURL();
+
+    // Allow https://foo.example.com Service Worker to not show notification if
+    // an https://bar.example.com tab is visible (and hence might conceivably
+    // be showing UI in response to the push message); but http:// doesn't count
+    // as even with navigator.connect the Service Worker can't talk to it.

[mlamouri (slow - plz ping), 2015/02/02 12:36:57]

I wouldn't mention navigator.connect in the comments given that it's not
something we ship (so there is a chance that the comment becomes irrelevant).

[johnme, 2015/02/02 15:58:36]

Chrome already implements most of navigator.connect (see
content/{child,browser}/navigator_connect etc), so it seems reasonable to refer
to it in comments. Did a minor rewording for clarity though.

[mlamouri (slow - plz ping), 2015/02/02 16:23:35]

Chrome implementing it doesn't mean it will ship and will stay a thing.
WebIntents has been removed for example. I do not mind if you want to keep that
though.

+    if (application_id.origin.scheme() != active_url.scheme())
+      continue;
+    if (net::registry_controlled_domains::SameDomainOrHost(
+        application_id.origin, active_url,
+        net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) {
+      return;
+    }
+  }
+
   // If we haven't returned yet, the site failed to show a notification, so we
   // will show a generic notification. See https://crbug.com/437277
   // TODO(johnme): The generic notification should probably automatically close
   // itself when the next push message arrives?
   content::PlatformNotificationData notification_data;
   // TODO(johnme): Switch to FormatOriginForDisplay from crbug.com/402698
   notification_data.title = l10n_util::GetStringFUTF16(
       IDS_PUSH_MESSAGING_GENERIC_NOTIFICATION_TITLE,
       base::UTF8ToUTF16(application_id.origin.host()));
   notification_data.direction =
@@ skipping 248 matching lines @@
 bool PushMessagingServiceImpl::HasPermission(const GURL& origin) {
   gcm::PushMessagingPermissionContext* permission_context =
       gcm::PushMessagingPermissionContextFactory::GetForProfile(profile_);
   DCHECK(permission_context);
 
   return permission_context->GetPermissionStatus(origin, origin) ==
       CONTENT_SETTING_ALLOW;
 }
 
 }  // namespace gcm