ShouldDisplayOverFullscreen implementation

ShouldDisplayOverFullscreen implementation

Add default ShouldDisplayOverFullscreen method to NotificationDelegate
that always returns false.
Add implementation for NotificationsApiDelegate that returns true for
ShouldDisplayOverFullscreen when the app that created the notificaiton
is displaying fullscreen content.

Note that this change does not enable this functionality.

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

[bmalcolm, 2016-09-13 17:39:21]

Description was changed from

==========
ShouldDisplayOverFullscreen implementation

Add default ShouldDisplayOverFullscreen method to NotificationDelegate
that always returns false.
Add implementation for NotificationsApiDelegate that returns true for
ShouldDisplayOverFullscreen when the app that created the notificaiton
is displaying fullscreen content.

Note that this change does not enable this functionality.

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

to

==========
ShouldDisplayOverFullscreen implementation

Add default ShouldDisplayOverFullscreen method to NotificationDelegate
that always returns false.
Add implementation for NotificationsApiDelegate that returns true for
ShouldDisplayOverFullscreen when the app that created the notificaiton
is displaying fullscreen content.

Note that this change does not enable this functionality.

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

[bmalcolm, 2016-09-13 17:39:21]

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

[bmalcolm, 2016-09-13 17:42:12]

Sorry so long between CLs. Hope you still remember what we discussed. My next CL
will hook the code in and add the logging to monitor what would happen when we
turn this code on. I may have questions... :-)

[dewittj, 2016-09-13 22:56:51]

I did not notice an l-g-t-m from API owners on apps-dev or extensions-dev.  Did
they give it to you out-of-band?

https://codereview.chromium.org/2335293002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/2335293002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/notifications_api.cc:196: bool
ShouldDisplayOverFullscreen() const override {
is it possible for two apps to have a fullscreen window?  Your tests don't seem
to cover multiple apps.

https://codereview.chromium.org/2335293002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/notifications_api.cc:200: for
(AppWindowRegistry::AppWindowList::const_iterator i = windows.begin();
for (const auto& window : windows)

https://codereview.chromium.org/2335293002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/notifications_api.cc:202:
shouldDisplay |= (*i)->IsFullscreen();
I'd prefer:
if (i->IsFullscreen())
  return true;

then shouldDisplay can disappear.

[bmalcolm, 2016-09-14 20:40:37]

On 2016/09/13 22:56:51, dewittj wrote:
> I did not notice an l-g-t-m from API owners on apps-dev or extensions-dev. 
Did
> they give it to you out-of-band?

As we discussed, no, but because we're not going to change the API now, it
didn't seem relevant.
> 
>
https://codereview.chromium.org/2335293002/diff/1/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/notifications/notifications_api.cc (right):
> 
??
>
https://codereview.chromium.org/2335293002/diff/1/chrome/browser/extensions/a...
> chrome/browser/extensions/api/notifications/notifications_api.cc:196: bool
> ShouldDisplayOverFullscreen() const override {
> is it possible for two apps to have a fullscreen window?  Your tests don't
seem
> to cover multiple apps.

Added a new multiple fullscreen test. Had to refine the in code check.

> 
>
https://codereview.chromium.org/2335293002/diff/1/chrome/browser/extensions/a...
> chrome/browser/extensions/api/notifications/notifications_api.cc:200: for
> (AppWindowRegistry::AppWindowList::const_iterator i = windows.begin();
> for (const auto& window : windows)

Done.

> 
>
https://codereview.chromium.org/2335293002/diff/1/chrome/browser/extensions/a...
> chrome/browser/extensions/api/notifications/notifications_api.cc:202:
> shouldDisplay |= (*i)->IsFullscreen();
> I'd prefer:
> if (i->IsFullscreen())
>   return true;
> 
> then shouldDisplay can disappear.

Done.

[dewittj, 2016-09-14 21:06:52]

If you look at the unified diffs for each file, you can respond to each comment
in the place that it is.  The (right): is in reference to the "side" of the
diff.

It is polite to respond to every comment using the UI rather than the "reply"
button (even if only with "Acknowledged") so that the reviewer can quickly check
that everything has been addressed.

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_api.cc:149:
profile_(profile),
Don't store profile, call GetProfile()

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_api.cc:202: if
(window->IsFullscreen() && window->GetBaseWindow()->IsActive()) {
nit: single line if blocks should not have braces

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/notifications/notifications_apitest.cc
(right):

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_apitest.cc:104: const
std::string& test_name, const std::string& window_state) {
instead of |window_state| could we have an enum class:

enum class WindowState {
  FULLSCREEN,
  NORMAL
}

then inside the function make the appropriate JSON blob.  Then the tests
wouldn't need random JSON in the body

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_apitest.cc:106: const
extensions::Extension* extension = LoadExtension(extdir);
lose the "extensions::", there's a using statement at the top

same in other places in this patch

https://codereview.chromium.org/2335293002/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/notifications/api/other_app/manifest.json
(right):

https://codereview.chromium.org/2335293002/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/notifications/api/other_app/manifest.json:2:
"name": "Packaged App - Notification Launcher",
This one doesn't actually create a notification, right?

[bmalcolm, 2016-09-14 21:56:25]

Sorry for the incorrect use of reply. I actually looked for the inline method of
responding to comments and couldn't find it.

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_api.cc:149:
profile_(profile),
On 2016/09/14 21:06:51, dewittj wrote:
> Don't store profile, call GetProfile()

GetProfile isn't available in NotificationsApiDelegate or NotificationDelegate -
it is available in NotificationsApiFunction, which is passing it in to the
NotificationsApiDelegate c'tor. I don't see a way to not store it unless I store
the AppWindowRegistry in the c'tor instead. 

Am I missing something?

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_api.cc:202: if
(window->IsFullscreen() && window->GetBaseWindow()->IsActive()) {
On 2016/09/14 21:06:51, dewittj wrote:
> nit: single line if blocks should not have braces

Done.

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/notifications/notifications_apitest.cc
(right):

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_apitest.cc:104: const
std::string& test_name, const std::string& window_state) {
On 2016/09/14 21:06:51, dewittj wrote:
> instead of |window_state| could we have an enum class:
> 
> enum class WindowState {
>   FULLSCREEN,
>   NORMAL
> }
> 
> then inside the function make the appropriate JSON blob.  Then the tests
> wouldn't need random JSON in the body

Done.

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_apitest.cc:106: const
extensions::Extension* extension = LoadExtension(extdir);
On 2016/09/14 21:06:51, dewittj wrote:
> lose the "extensions::", there's a using statement at the top
> 
> same in other places in this patch

Done.

https://codereview.chromium.org/2335293002/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/notifications/api/other_app/manifest.json
(right):

https://codereview.chromium.org/2335293002/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/notifications/api/other_app/manifest.json:2:
"name": "Packaged App - Notification Launcher",
On 2016/09/14 21:06:52, dewittj wrote:
> This one doesn't actually create a notification, right?

Correct. Fixed.

[dewittj, 2016-09-14 23:09:47]

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/2335293002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_api.cc:149:
profile_(profile),
On 2016/09/14 21:56:24, bmalcolm wrote:
> On 2016/09/14 21:06:51, dewittj wrote:
> > Don't store profile, call GetProfile()
> 
> GetProfile isn't available in NotificationsApiDelegate or NotificationDelegate
-
> it is available in NotificationsApiFunction, which is passing it in to the
> NotificationsApiDelegate c'tor. I don't see a way to not store it unless I
store
> the AppWindowRegistry in the c'tor instead. 
> 
> Am I missing something?

You're right.  One way to do it is to call api_function_->GetProfile()

But I'm beginning to worry about lifetimes.  In particular, Profile* can be
destroyed if the profile is incognito.  Holding a bare pointer could cause
crashes if the profile is destroyed.

Looks like tons of other notification delegates hold on to Profile* so it's
probably fine.

https://codereview.chromium.org/2335293002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/notifications/notifications_apitest.cc
(right):

https://codereview.chromium.org/2335293002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_apitest.cc:116:
kWindowStateStrings[static_cast<int>(window_state)]);
Sorry to be picky here, but since there's only 2 let's just do a switch
statement rather than a lookup table and a cast.

[bmalcolm, 2016-09-15 16:25:52]

I went ahead and removed the profile_ member variable since it's available from
api_function_.

https://codereview.chromium.org/2335293002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/notifications/notifications_apitest.cc
(right):

https://codereview.chromium.org/2335293002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_apitest.cc:116:
kWindowStateStrings[static_cast<int>(window_state)]);
On 2016/09/14 23:09:47, dewittj wrote:
> Sorry to be picky here, but since there's only 2 let's just do a switch
> statement rather than a lookup table and a cast.

Done.

[dewittj, 2016-09-15 16:45:56]

lgtm

[bmalcolm, 2016-09-15 16:47:34]

The CQ bit was checked by bmalcolm@chromium.org

[commit-bot: I haz the power, 2016-09-15 16:47:56]

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-15 18:02:40]

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

[commit-bot: I haz the power, 2016-09-15 18:02:41]

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-15 18:49:23]

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

[commit-bot: I haz the power, 2016-09-15 18:49:56]

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-15 20:17:08]

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

[commit-bot: I haz the power, 2016-09-15 20:17:09]

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-15 21:45:09]

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

[commit-bot: I haz the power, 2016-09-15 21:45:58]

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-15 23:17:04]

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

[commit-bot: I haz the power, 2016-09-15 23:17:05]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[bmalcolm, 2016-09-16 15:38:04]

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

[commit-bot: I haz the power, 2016-09-16 15:38:21]

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-16 16:26:48]

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

[commit-bot: I haz the power, 2016-09-16 16:27:03]

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-16 17:00:04]

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

[commit-bot: I haz the power, 2016-09-16 17:00:06]

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-16 17:47:46]

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

[commit-bot: I haz the power, 2016-09-16 17:48:20]

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-16 18:51:13]

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

[commit-bot: I haz the power, 2016-09-16 18:51:37]

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-16 18:54:51]

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

[commit-bot: I haz the power, 2016-09-16 18:54:52]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bmalcolm, 2016-09-16 19:14:22]

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

[commit-bot: I haz the power, 2016-09-16 19:14: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-16 20:13:11]

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

[commit-bot: I haz the power, 2016-09-16 20:13:12]

Dry run: This issue passed the CQ dry run.

[bmalcolm, 2016-09-16 20:26:38]

PTAL

I had to modify the unit tests (but not the code) to run on OSX. I followed the
pattern I found here
(https://cs.chromium.org/chromium/src/chrome/browser/apps/app_window_interacti...)
to make the Fullscreen test pass on OSX, but I can't make the multi-app in
fullscreen test pass, as the fake fullscreen mode crashes when the second app is
made fullscreen.

[dewittj, 2016-09-19 16:44:54]

still lgtm

[bmalcolm, 2016-09-19 18:19:04]

The CQ bit was checked by bmalcolm@chromium.org

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

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-19 19:42:43]

Description was changed from

==========
ShouldDisplayOverFullscreen implementation

Add default ShouldDisplayOverFullscreen method to NotificationDelegate
that always returns false.
Add implementation for NotificationsApiDelegate that returns true for
ShouldDisplayOverFullscreen when the app that created the notificaiton
is displaying fullscreen content.

Note that this change does not enable this functionality.

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

to

==========
ShouldDisplayOverFullscreen implementation

Add default ShouldDisplayOverFullscreen method to NotificationDelegate
that always returns false.
Add implementation for NotificationsApiDelegate that returns true for
ShouldDisplayOverFullscreen when the app that created the notificaiton
is displaying fullscreen content.

Note that this change does not enable this functionality.

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

[commit-bot: I haz the power, 2016-09-19 19:42:45]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-09-19 21:03:10]

Description was changed from

==========
ShouldDisplayOverFullscreen implementation

Add default ShouldDisplayOverFullscreen method to NotificationDelegate
that always returns false.
Add implementation for NotificationsApiDelegate that returns true for
ShouldDisplayOverFullscreen when the app that created the notificaiton
is displaying fullscreen content.

Note that this change does not enable this functionality.

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

to

==========
ShouldDisplayOverFullscreen implementation

Add default ShouldDisplayOverFullscreen method to NotificationDelegate
that always returns false.
Add implementation for NotificationsApiDelegate that returns true for
ShouldDisplayOverFullscreen when the app that created the notificaiton
is displaying fullscreen content.

Note that this change does not enable this functionality.

BUG=481318

Committed: https://crrev.com/4607efe4c6936c028c7fa4af04a0460312bd7b61
Cr-Commit-Position: refs/heads/master@{#419533}
==========

[commit-bot: I haz the power, 2016-09-19 21:03:11]

Patchset 12 (id:??) landed as
https://crrev.com/4607efe4c6936c028c7fa4af04a0460312bd7b61
Cr-Commit-Position: refs/heads/master@{#419533}