Observes work area change and auto-hide for notifications.

Observes work area change and auto-hide for notifications.

BUG=249509
R=dewittj@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212431

[dewittj, 2013-07-16 21:31:32]

lgtm with nit

https://codereview.chromium.org/19291004/diff/1/ash/system/web_notification/w...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/19291004/diff/1/ash/system/web_notification/w...
ash/system/web_notification/web_notification_tray.cc:252:
popup_collection_.reset();
work_area_observer_.reset() here?

https://codereview.chromium.org/19291004/diff/1/ash/system/web_notification/w...
ash/system/web_notification/web_notification_tray.cc:345:
work_area_observer_.reset();
I don't love how the work_area_observer_ needs to be manually managed at the
same time as popup_collection_ but it seems correct at this time.

https://codereview.chromium.org/19291004/diff/1/ui/message_center/views/messa...
File ui/message_center/views/message_popup_collection.h (right):

https://codereview.chromium.org/19291004/diff/1/ui/message_center/views/messa...
ui/message_center/views/message_popup_collection.h:82: void SetWorkArea(const
gfx::Rect& work_area);
Comment here

[Jun Mukai, 2013-07-16 22:49:42]

https://codereview.chromium.org/19291004/diff/1/ash/system/web_notification/w...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/19291004/diff/1/ash/system/web_notification/w...
ash/system/web_notification/web_notification_tray.cc:252:
popup_collection_.reset();
On 2013/07/16 21:31:33, dewittj wrote:
> work_area_observer_.reset() here?

Done.

https://codereview.chromium.org/19291004/diff/1/ash/system/web_notification/w...
ash/system/web_notification/web_notification_tray.cc:345:
work_area_observer_.reset();
On 2013/07/16 21:31:33, dewittj wrote:
> I don't love how the work_area_observer_ needs to be manually managed at the
> same time as popup_collection_ but it seems correct at this time.

we can disable work_area_observer_ or something, but just removing observer
itself would be good, since this doesn't do anything if there is not
popup_collection_.

https://codereview.chromium.org/19291004/diff/1/ui/message_center/views/messa...
File ui/message_center/views/message_popup_collection.h (right):

https://codereview.chromium.org/19291004/diff/1/ui/message_center/views/messa...
ui/message_center/views/message_popup_collection.h:82: void SetWorkArea(const
gfx::Rect& work_area);
On 2013/07/16 21:31:33, dewittj wrote:
> Comment here

Done.

[commit-bot: I haz the power, 2013-07-16 23:15:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/19291004/7001

[commit-bot: I haz the power, 2013-07-17 03:01:54]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Jun Mukai, 2013-07-17 03:24:32]

jamescook, jennyz, please review this:
jamescook: ash/shelf
jennyz: ash/system

also cc-ing harrym for the change in ash/shelf

[jennyz, 2013-07-17 17:00:36]

lgtm for ash/system.

[James Cook, 2013-07-17 21:14:14]

LGTM with one nit and one optional suggestion

https://codereview.chromium.org/19291004/diff/7001/ash/shelf/shelf_layout_man...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/19291004/diff/7001/ash/shelf/shelf_layout_man...
ash/shelf/shelf_layout_manager.h:77: static int GetPreferredShelfSize();
nit: Add a comment indicating that this is the shelf size when shown.

Also, should kShelfSize be removed from this header, to force callers to use
GetPreferredShelfSize()?  Or is it needed somewhere, like in tests?

https://codereview.chromium.org/19291004/diff/7001/ash/system/web_notificatio...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/19291004/diff/7001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:69: // and notifies to
MessagePopupCollection.
nit: either "notifies MessagePopupCollection" or "sends a notification to
MessagePopupCollection". Gosh, English is a weird language.

[Jun Mukai, 2013-07-18 17:55:43]

https://codereview.chromium.org/19291004/diff/7001/ash/shelf/shelf_layout_man...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/19291004/diff/7001/ash/shelf/shelf_layout_man...
ash/shelf/shelf_layout_manager.h:77: static int GetPreferredShelfSize();
On 2013/07/17 21:14:14, James Cook (Chromium) wrote:
> nit: Add a comment indicating that this is the shelf size when shown.
> 
> Also, should kShelfSize be removed from this header, to force callers to use
> GetPreferredShelfSize()?  Or is it needed somewhere, like in tests?

comment added.
Some code uses kShelfSize, but it would be because this code is slightly new? 
It seems better to be done in another CL.

https://codereview.chromium.org/19291004/diff/7001/ash/system/web_notificatio...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/19291004/diff/7001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:69: // and notifies to
MessagePopupCollection.
On 2013/07/17 21:14:14, James Cook (Chromium) wrote:
> nit: either "notifies MessagePopupCollection" or "sends a notification to
> MessagePopupCollection". Gosh, English is a weird language.

Done.

[commit-bot: I haz the power, 2013-07-18 17:56:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/19291004/20001

[commit-bot: I haz the power, 2013-07-18 21:26:47]

Change committed as 212431