Separate the logic of popup alignment and workarea handling as delegate.

Separate the logic of popup alignment and workarea handling as delegate.

MessagePopupCollection contains plenty size of conditions and ifdefs
to work properly with each type of the desktop we have, and some
logic makes side effects on another desktop. This design is unhealty
and adding more conditions sounds incorrect.

Considering this, it would be better to extract platform dependent
parts as a delegate class (PopupAlignmentDelegate) and allow
subclasses to provide platform-specific features.

This design is also beneficial for win-ash, because we had
OS_CHROMEOS layout data and logic which actually means Ash.

BUG=389656
R=stevenjb@chromium.org, dimich@chromium.org
TBR=jamescook@chromium.org, harrym@chromium.org
TEST=message_center_unittests, ash_unittests, some manual checks

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

[stevenjb, 2014-07-02 21:59:24]

Nice. Mostly nits.

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
File ash/system/web_notification/popup_alignment_delegate.cc (right):

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.cc:36: screen_(NULL),
root_window_(NULL)

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.cc:47: shelf_ = NULL;
nit: unnecessary (and inconsistent with screen_)

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.cc:51: gfx::Screen* screen,
const gfx::Display& display) {
one line per arg

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
File ash/system/web_notification/popup_alignment_delegate.h (right):

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.h:31: // The
PopupAlignmentDelegate for Ash. It needs to care of alignment of the
s/care of/handle/ (or 'take care of')

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.h:33: class ASH_EXPORT
PopupAlignmentDelegate
Can we name this AshPopupAlignmentDelegate just to be a little more clearly
separated from message_center::PopupAlignmentDelegate? It makes searches,
backtraces, etc, a little easier to follow.
It is also consistent with DesktopPopupAlignmentDelegate.

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.h:63: // Update |shelf_|
and start watching when it's set first. This should not
s/set first/first set/

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.h:64: // be done in the
constructor because shelf might not be initialized at that
s/shelf/the shelf/

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
File ui/message_center/views/popup_alignment_delegate.cc (right):

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.cc:11:
PopupAlignmentDelegate::PopupAlignmentDelegate() : collection_(NULL) {}
WS

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
File ui/message_center/views/popup_alignment_delegate.h (right):

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.h:38: // Returns true if the
toast should be aligned from top to down.
s/from top to down/top down/ (or 'from top to bottom')

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.h:41: // Returns true if the
toasts is positioned at the left side of the desktop
s/toasts is/toasts are/

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.h:47: // so that the toasts are
arranged at the right place.
s/right/correct/ (only because 'right' is ambiguous here since there are lots of
references to left/right)

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.h:48: virtual void
RecomputeAlignment(const gfx::Display& display) {}
nit: It might be better to make this a pure virtual so that each subclass has to
do something here or explicitly do nothing.

[Jun Mukai, 2014-07-07 18:12:36]

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
File ash/system/web_notification/popup_alignment_delegate.cc (right):

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.cc:36: screen_(NULL),
On 2014/07/02 21:59:23, stevenjb wrote:
> root_window_(NULL)

Done.

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.cc:47: shelf_ = NULL;
On 2014/07/02 21:59:23, stevenjb wrote:
> nit: unnecessary (and inconsistent with screen_)

Done.

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.cc:51: gfx::Screen* screen,
const gfx::Display& display) {
On 2014/07/02 21:59:23, stevenjb wrote:
> one line per arg

Done.

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
File ash/system/web_notification/popup_alignment_delegate.h (right):

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.h:31: // The
PopupAlignmentDelegate for Ash. It needs to care of alignment of the
On 2014/07/02 21:59:23, stevenjb wrote:
> s/care of/handle/ (or 'take care of')

Done.

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.h:33: class ASH_EXPORT
PopupAlignmentDelegate
On 2014/07/02 21:59:23, stevenjb wrote:
> Can we name this AshPopupAlignmentDelegate just to be a little more clearly
> separated from message_center::PopupAlignmentDelegate? It makes searches,
> backtraces, etc, a little easier to follow.
> It is also consistent with DesktopPopupAlignmentDelegate.

Done.

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.h:63: // Update |shelf_|
and start watching when it's set first. This should not
On 2014/07/02 21:59:23, stevenjb wrote:
> s/set first/first set/

Done.

https://codereview.chromium.org/369573004/diff/20001/ash/system/web_notificat...
ash/system/web_notification/popup_alignment_delegate.h:64: // be done in the
constructor because shelf might not be initialized at that
On 2014/07/02 21:59:23, stevenjb wrote:
> s/shelf/the shelf/

Done.

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
File ui/message_center/views/popup_alignment_delegate.cc (right):

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.cc:11:
PopupAlignmentDelegate::PopupAlignmentDelegate() : collection_(NULL) {}
On 2014/07/02 21:59:23, stevenjb wrote:
> WS

Done.

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
File ui/message_center/views/popup_alignment_delegate.h (right):

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.h:38: // Returns true if the
toast should be aligned from top to down.
On 2014/07/02 21:59:24, stevenjb wrote:
> s/from top to down/top down/ (or 'from top to bottom')

Done.

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.h:41: // Returns true if the
toasts is positioned at the left side of the desktop
On 2014/07/02 21:59:24, stevenjb wrote:
> s/toasts is/toasts are/

Done.

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.h:47: // so that the toasts are
arranged at the right place.
On 2014/07/02 21:59:24, stevenjb wrote:
> s/right/correct/ (only because 'right' is ambiguous here since there are lots
of
> references to left/right)

Done.

https://codereview.chromium.org/369573004/diff/20001/ui/message_center/views/...
ui/message_center/views/popup_alignment_delegate.h:48: virtual void
RecomputeAlignment(const gfx::Display& display) {}
On 2014/07/02 21:59:23, stevenjb wrote:
> nit: It might be better to make this a pure virtual so that each subclass has
to
> do something here or explicitly do nothing.

Done.

[stevenjb, 2014-07-07 18:37:45]

lgtm

[Jun Mukai, 2014-07-07 18:47:19]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-07 18:47:55]

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

[commit-bot: I haz the power, 2014-07-07 22:16:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/29997)

[commit-bot: I haz the power, 2014-07-07 22:22:00]

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

[commit-bot: I haz the power, 2014-07-07 22:22:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Jun Mukai, 2014-07-07 22:28:46]

oops, add some TBRs:
jamescook for ash/ash.gyp (adding new files)
harrym for ash/shelf/shelf_layout_manager.h (adding new FRIEND_TESTS)

[Jun Mukai, 2014-07-07 22:28:52]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-07 22:30:25]

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

[commit-bot: I haz the power, 2014-07-07 23:57:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30051)

[commit-bot: I haz the power, 2014-07-08 00:11:53]

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

[commit-bot: I haz the power, 2014-07-08 00:11:54]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Jun Mukai, 2014-07-08 18:37:56]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-08 18:40:33]

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

[Dmitry Titov, 2014-07-08 18:59:06]

Sorry for late, but LGTM!

[commit-bot: I haz the power, 2014-07-08 21:46:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30549)

[commit-bot: I haz the power, 2014-07-08 21:57:16]

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

[commit-bot: I haz the power, 2014-07-08 21:57:17]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Jun Mukai, 2014-07-08 22:18:15]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-08 22:19:58]

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

[commit-bot: I haz the power, 2014-07-09 02:07:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-09 03:12:01]

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

[commit-bot: I haz the power, 2014-07-09 03:12:02]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Jun Mukai, 2014-07-09 23:17:56]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-09 23:21:17]

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

[commit-bot: I haz the power, 2014-07-10 03:00:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-10 07:40:56]

Change committed as 282269