Add ShouldDisplayOverFullscreen support to web notifications.

Add ShouldDisplayOverFullscreen support to web notifications.

Added WebNotificationDelegate base class for common functionality of
NotificationObjectProxy and PersistentNotificationDelegate.

Note that this change does not enable this functionality.

BUG=481318
Committed: https://crrev.com/f8395c5644114d234614184748f27eae4934aa76
Cr-Commit-Position: refs/heads/master@{#421963}

[bmalcolm, 2016-09-27 16:53:29]

bmalcolm@chromium.org changed reviewers:
+ dewittj@chromium.org

[bmalcolm, 2016-09-27 16:53:29]

I really appreciate how you've allowed yourself to be drafted into this.

[bmalcolm, 2016-09-27 17:12:11]

The CQ bit was checked by bmalcolm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 17:12:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 17:12:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 17:12:48]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[bmalcolm, 2016-09-27 17:13:20]

The CQ bit was unchecked by bmalcolm@chromium.org

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

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

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

This looks great, thank you once more for doing it!

Sorry if I'm gate crashing. Mostly nits from my end, modulo the null check.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_object_proxy.h (right):

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_object_proxy.h:13: #include
"url/gurl.h"
nit: forward declare

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/persistent_notification_delegate.cc (right):

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/persistent_notification_delegate.cc:14: #include
"content/public/browser/web_contents.h"
nit: these includes probably aren't needed for this file?

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/persistent_notification_delegate.cc:16: using
content::WebContents;
unused

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/web_notification_delegate.cc (right):

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:45: if
(browser->profile() == profile) {
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.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:48:
DCHECK(active_contents);
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...

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:50: //  (a) the active
tab in the browser is the source of the notification.
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.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:56: return true;
micro nit: brackets for multi-line if statements.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/web_notification_delegate.h (right):

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.h:20: // notifications is
found here.
nit: This may feel rather counter-intuitive today, but would you mind adding the
following TODO since we'd like to drop the delegates for web notifications
altogether?

// TODO(peter, crbug.com/596161): Migrate this functionality offered by the
// delegate to the NotificationDisplayService.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.h:23:
WebNotificationDelegate(content::BrowserContext* browser_context,
nit: maybe make this protected too to avoid anyone instantiating this class by
accident?

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.h:26: 
micro nit:

// NotificationDelegate implementation.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.h:41: GURL origin_;
nit: DISALLOW_COPY_AND_ASSIGN(WebNotificationDelegate);

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

Thanks for jumping in Peter! I appreciate the set of eyes.

As a git-noob, I've had some struggles getting the last cl out of this one, so I
apologize for all of the interim patches and I hope that didn't mess with the
comments.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_object_proxy.h (right):

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_object_proxy.h:13: #include
"url/gurl.h"
On 2016/09/27 17:17:36, Peter Beverloo wrote:
> nit: forward declare

Done. Also fixed it in persistent_notification_delegate.h

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/persistent_notification_delegate.cc (right):

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/persistent_notification_delegate.cc:14: #include
"content/public/browser/web_contents.h"
On 2016/09/27 17:17:36, Peter Beverloo wrote:
> nit: these includes probably aren't needed for this file? 

Earlier design. Removed.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/persistent_notification_delegate.cc:16: using
content::WebContents;
On 2016/09/27 17:17:36, Peter Beverloo wrote:
> unused

Removed.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/web_notification_delegate.cc (right):

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:45: if
(browser->profile() == profile) {
On 2016/09/27 17:17:36, Peter Beverloo wrote:
> 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.

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.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:48:
DCHECK(active_contents);
On 2016/09/27 17:17:37, Peter Beverloo wrote:
> 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...

Done.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:50: //  (a) the active
tab in the browser is the source of the notification.
On 2016/09/27 17:17:37, Peter Beverloo wrote:
> 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.

Done.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:56: return true;
On 2016/09/27 17:17:37, Peter Beverloo wrote:
> micro nit: brackets for multi-line if statements.

Done.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/web_notification_delegate.h (right):

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.h:20: // notifications is
found here.
On 2016/09/27 17:17:37, Peter Beverloo wrote:
> nit: This may feel rather counter-intuitive today, but would you mind adding
the
> following TODO since we'd like to drop the delegates for web notifications
> altogether?
> 
> // TODO(peter, crbug.com/596161): Migrate this functionality offered by the
> // delegate to the NotificationDisplayService.

Done.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.h:23:
WebNotificationDelegate(content::BrowserContext* browser_context,
On 2016/09/27 17:17:37, Peter Beverloo wrote:
> nit: maybe make this protected too to avoid anyone instantiating this class by
> accident?

Done.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.h:26: 
On 2016/09/27 17:17:37, Peter Beverloo wrote:
> micro nit:
> 
> // NotificationDelegate implementation.

Done.

https://codereview.chromium.org/2377553003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.h:41: GURL origin_;
On 2016/09/27 17:17:37, Peter Beverloo wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(WebNotificationDelegate);

Done.

[dewittj, 2016-09-28 02:42:39]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 02:42:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 03:08:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 03:08:13]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Peter Beverloo, 2016-09-28 12:23:13]

Thank you! I see test failures on Mac and Android.

https://codereview.chromium.org/2377553003/diff/140001/chrome/browser/notific...
File chrome/browser/notifications/notification_interactive_uitest.cc (right):

https://codereview.chromium.org/2377553003/diff/140001/chrome/browser/notific...
chrome/browser/notifications/notification_interactive_uitest.cc:913:
ui::test::ScopedFakeNSWindowFullscreen fake_fullscreen;
Looks like the Mac test is failing because transitioning the
ScopedFakeNSWindowFullscreen is asynchronous.

One work-around I see in other tests is by having the following two lines at the
end of the test's body:

  // Let the message loop run so that |fake_fullscreen| finishes transition.
  content::RunAllPendingInMessageLoop();

(That unfortunately still wouldn't explain the EXPECT failure on line 933 :/)

https://codereview.chromium.org/2377553003/diff/140001/chrome/browser/notific...
File chrome/browser/notifications/web_notification_delegate.cc (right):

https://codereview.chromium.org/2377553003/diff/140001/chrome/browser/notific...
chrome/browser/notifications/web_notification_delegate.cc:39: Profile* profile =
Profile::FromBrowserContext(browser_context_);
Oops, sorry that I missed this - this code can't be used on Android, because
Android uses it own (Java) variants of the TabStripModel.

But that's OK, because the system UI there will decide whether to display the
notification or not, so we can just hoist lines 39-63 in an #if
!defined(OS_ANDROID) block.

[bmalcolm, 2016-09-28 18:28:47]

The CQ bit was checked by bmalcolm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 18:29:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 18:29:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 18:29:14]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[bmalcolm, 2016-09-28 18:30:09]

PTAL - unfortunately I don't have permissions to do a TryBot dry run, so I can't
validate these fixes.

In the past, Justin has LGTM'd and then I've iterated until I've gotten the unit
tests working, and then PTAL'd one last time.

B.

https://codereview.chromium.org/2377553003/diff/140001/chrome/browser/notific...
File chrome/browser/notifications/notification_interactive_uitest.cc (right):

https://codereview.chromium.org/2377553003/diff/140001/chrome/browser/notific...
chrome/browser/notifications/notification_interactive_uitest.cc:913:
ui::test::ScopedFakeNSWindowFullscreen fake_fullscreen;
On 2016/09/28 12:23:13, Peter Beverloo wrote:
> Looks like the Mac test is failing because transitioning the
> ScopedFakeNSWindowFullscreen is asynchronous.
> 
> One work-around I see in other tests is by having the following two lines at
the
> end of the test's body:
> 
>   // Let the message loop run so that |fake_fullscreen| finishes transition.
>   content::RunAllPendingInMessageLoop();
> 
> (That unfortunately still wouldn't explain the EXPECT failure on line 933 :/)

I think both are probably related - on the Mac we haven't finished transitioning
to fullscreen so the expectation fails.

I've borrowed some code from the AppWindowInteractiveUITest
(https://cs.chromium.org/chromium/src/chrome/browser/apps/app_window_interacti...)
that is trying to avoid the same problem - pumping the message loop until the
window is in the desired state. Let's try that.

https://codereview.chromium.org/2377553003/diff/140001/chrome/browser/notific...
File chrome/browser/notifications/web_notification_delegate.cc (right):

https://codereview.chromium.org/2377553003/diff/140001/chrome/browser/notific...
chrome/browser/notifications/web_notification_delegate.cc:39: Profile* profile =
Profile::FromBrowserContext(browser_context_);
On 2016/09/28 12:23:13, Peter Beverloo wrote:
> Oops, sorry that I missed this - this code can't be used on Android, because
> Android uses it own (Java) variants of the TabStripModel.
> 
> But that's OK, because the system UI there will decide whether to display the
> notification or not, so we can just hoist lines 39-63 in an #if
> !defined(OS_ANDROID) block.

Done.

[dewittj, 2016-09-28 19:42:42]

The CQ bit was checked by dewittj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 19:43:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dewittj, 2016-09-28 20:01:54]

mostly lg, thanks!

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
File
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
(right):

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:564:
IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
The tests and utilities are essentially copies between this and the other
interactive_uitests file.  Any refactoring you could do?

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:601:
EXPECT_TRUE(notification.delegate()->ShouldDisplayOverFullscreen());
does this fail on android?

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/web_notification_delegate.cc (right):

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
chrome/browser/notifications/web_notification_delegate.cc:39: #if
!defined(OS_ANDROID)
This is probably fine for now, but eventually we'll need to test on Windows and
Mac versions as well, once we move to native NC for some versions of those
platforms.

[bmalcolm, 2016-09-28 20:30:09]

The CQ bit was unchecked by bmalcolm@chromium.org

[bmalcolm, 2016-09-28 20:30:35]

The CQ bit was checked by bmalcolm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 20:31:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 20:31:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 20:31:04]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[bmalcolm, 2016-09-29 16:53:12]

The CQ bit was checked by bmalcolm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 16:53:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bmalcolm, 2016-09-29 17:46:12]

The CQ bit was checked by bmalcolm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 17:46:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 19:07:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 19:07:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bmalcolm, 2016-09-29 19:22:08]

The CQ bit was checked by bmalcolm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 19:22:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bmalcolm, 2016-09-29 19:46:50]

The CQ bit was checked by bmalcolm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 19:47:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bmalcolm, 2016-09-29 20:16:08]

The CQ bit was checked by bmalcolm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 20:16:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bmalcolm, 2016-09-29 20:53:14]

PTAL. Looks like all of the unit tests now pass.

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
File
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
(right):

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:564:
IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
On 2016/09/28 20:01:54, dewittj wrote:
> The tests and utilities are essentially copies between this and the other
> interactive_uitests file.  Any refactoring you could do?

I've moved the utilities into the common utilities file, but I'm not sure what I
can do to make the tests common, since they're using different underlying test
infrastructure.

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:601:
EXPECT_TRUE(notification.delegate()->ShouldDisplayOverFullscreen());
On 2016/09/28 20:01:53, dewittj wrote:
> does this fail on android?

No it passes. Probably because these tests don't get run on Android?

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/web_notification_delegate.cc (right):

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
chrome/browser/notifications/web_notification_delegate.cc:39: #if
!defined(OS_ANDROID)
On 2016/09/28 20:01:54, dewittj wrote:
> This is probably fine for now, but eventually we'll need to test on Windows
and
> Mac versions as well, once we move to native NC for some versions of those
> platforms.

Acknowledged.

[dewittj, 2016-09-29 20:57:38]

lgtm, you may want to wait for Peter's final comments

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
File
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
(right):

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:601:
EXPECT_TRUE(notification.delegate()->ShouldDisplayOverFullscreen());
On 2016/09/29 20:53:14, bmalcolm wrote:
> On 2016/09/28 20:01:53, dewittj wrote:
> > does this fail on android?
> 
> No it passes. Probably because these tests don't get run on Android?

There's a TODO in chrome/test/BUILD.gn to get these running on Android.  I
suppose Peter will figure it out when the TODO is taken care of.

[commit-bot: I haz the power, 2016-09-29 21:09:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 21:09:52]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-09-29 21:30:52]

lgtm!

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
File
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
(right):

https://codereview.chromium.org/2377553003/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:601:
EXPECT_TRUE(notification.delegate()->ShouldDisplayOverFullscreen());
On 2016/09/29 20:57:38, dewittj wrote:
> On 2016/09/29 20:53:14, bmalcolm wrote:
> > On 2016/09/28 20:01:53, dewittj wrote:
> > > does this fail on android?
> > 
> > No it passes. Probably because these tests don't get run on Android?
> 
> There's a TODO in chrome/test/BUILD.gn to get these running on Android.  I
> suppose Peter will figure it out when the TODO is taken care of.

I think the answer has been "soon" for as long as I can remember for browser
tests, but these are interactive UI tests, so I wouldn't hold my breath.

We have good coverage of notifications on Android with a series of
Instrumentation Tests, which are tests written in Java, so it's not a huge
priority.

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
File chrome/browser/notifications/notification_test_util.cc (right):

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/notification_test_util.cc:11: 
micro nit: no double blank lines

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/notification_test_util.cc:152:
content::RunAllPendingInMessageLoop();
nit: curly brackets for multi-line statements

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
File chrome/browser/notifications/notification_test_util.h (right):

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/notification_test_util.h:81: explicit
FullscreenStateWaiter(Browser* browser, bool desired_state);
nit: no need to make this explicit (we only do that for single-argument
constructors)

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/notification_test_util.h:91: 
micro nit: no double blank lines

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
File chrome/browser/notifications/web_notification_delegate.h (right):

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/web_notification_delegate.h:49: 
micro nit: no double blank lines

[Peter Beverloo, 2016-09-29 21:32:04]

> PTAL - unfortunately I don't have permissions to do a TryBot dry run, so I
> can't validate these fixes.

Out of interest, why not? I'd be most happy to request rights for you if you'd
like?

[chromium-reviews, 2016-09-29 21:33:13]

I have it now. Justin pointed me in the right direction - requested &
granted.

On Thu, Sep 29, 2016 at 2:32 PM, <peter@chromium.org> wrote:

> > PTAL - unfortunately I don't have permissions to do a TryBot dry run, so
> I
> > can't validate these fixes.
>
> Out of interest, why not? I'd be most happy to request rights for you if
> you'd
> like?
>
> https://codereview.chromium.org/2377553003/
>



-- 
B.
Brian Malcolm | Software Engineer | bmalcolm@google.com | 425-654-4660

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bmalcolm, 2016-09-29 21:37:13]

Thanks both. Next CL (hopefully last) will wire up FullscreenNotificationBlocker
with experiment flags & UMA.

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
File chrome/browser/notifications/notification_test_util.cc (right):

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/notification_test_util.cc:11: 
On 2016/09/29 21:30:51, Peter Beverloo wrote:
> micro nit: no double blank lines

Done.

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/notification_test_util.cc:152:
content::RunAllPendingInMessageLoop();
On 2016/09/29 21:30:51, Peter Beverloo wrote:
> nit: curly brackets for multi-line statements

Done.

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
File chrome/browser/notifications/notification_test_util.h (right):

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/notification_test_util.h:81: explicit
FullscreenStateWaiter(Browser* browser, bool desired_state);
On 2016/09/29 21:30:52, Peter Beverloo wrote:
> nit: no need to make this explicit (we only do that for single-argument
> constructors)

Done.

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/notification_test_util.h:91: 
On 2016/09/29 21:30:52, Peter Beverloo wrote:
> micro nit: no double blank lines

Done.

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
File chrome/browser/notifications/web_notification_delegate.h (right):

https://codereview.chromium.org/2377553003/diff/240001/chrome/browser/notific...
chrome/browser/notifications/web_notification_delegate.h:49: 
On 2016/09/29 21:30:52, Peter Beverloo wrote:
> micro nit: no double blank lines

Done.

[bmalcolm, 2016-09-29 21:37:26]

The CQ bit was checked by bmalcolm@chromium.org

[bmalcolm, 2016-09-29 21:37:26]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2377553003/#ps260001
(title: "Final formatting fixes")

[commit-bot: I haz the power, 2016-09-29 21:38:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 22:49:33]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-09-29 22:52:54]

Description was changed from

==========
Add ShouldDisplayOverFullscreen support to web notifications.

Added WebNotificationDelegate base class for common functionality of
NotificationObjectProxy and PersistentNotificationDelegate.

Note that this change does not enable this functionality.

BUG=481318
==========

to

==========
Add ShouldDisplayOverFullscreen support to web notifications.

Added WebNotificationDelegate base class for common functionality of
NotificationObjectProxy and PersistentNotificationDelegate.

Note that this change does not enable this functionality.

BUG=481318

Committed: https://crrev.com/f8395c5644114d234614184748f27eae4934aa76
Cr-Commit-Position: refs/heads/master@{#421963}
==========

[commit-bot: I haz the power, 2016-09-29 22:52:55]

Patchset 14 (id:??) landed as
https://crrev.com/f8395c5644114d234614184748f27eae4934aa76
Cr-Commit-Position: refs/heads/master@{#421963}