Fix the problem that notifications and panels overlap.

Fix the problem that notifications and panels overlap.

The workaround is to move all notifications on top of the panels that could overlap with.

BUG=103052
TEST=new test


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109588

[Dmitry Titov, 2011-11-09 03:09:57]

Cool. Some notes below.

I understand we are not doing the low-left corner... Do you think it won't be a
problem? In any case, I would rather get this in and then add low-left corner if
it will indicate itself as a problem. I think very few people would ever change
their notifications to be in low-left corner...

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.cc:404: int
BalloonCollectionImpl::Layout::GetPanelHeightBase() const {
If we add a method on PanelManager to pull this height from there, then we don't
need to expose 'panels' and 'horizontal_spacing' from PanelManager. It seems to
be a bit cleaner then.

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
File chrome/browser/notifications/balloon_collection_impl.h (right):

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.h:144: int
GetPanelHeightBase() const;
Not clear why "Base". Maybe GetPositionAbovePanels()?

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.h:155: // The height of
right-most panels we're currently using as a base to allign
allign->align

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.h:156: // those
virtically-from-bottom-right balloons to prevent them from being
virtically->vertically
May be change wording of this comment to something like:
The height that guarantees that the notificaitons shown in the low-right or
low-left corner of the screen will go above currently shown panels and will not
be obscured by them.

http://codereview.chromium.org/8503037/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8503037/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:1429: 
extra line

[Dmitry Titov, 2011-11-09 03:10:35]

LGTM if all above are addressed and try bots are clear.

[jianli, 2011-11-09 19:04:40]

Added the support for balloons shown on bottom-left corner.

Uploaded a new patch and will land it once the trybots pass.

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.cc:404: int
BalloonCollectionImpl::Layout::GetPanelHeightBase() const {
On 2011/11/09 03:09:57, Dmitry Titov wrote:
> If we add a method on PanelManager to pull this height from there, then we
don't
> need to expose 'panels' and 'horizontal_spacing' from PanelManager. It seems
to
> be a bit cleaner then.

Indeed we don't need to expose 'horizontal_spacing'. I think  the logic here to
compute the offset is specific to notifications itself. I rather keep it here
since moving to PanelManager to have this specific method for notification
sounds also not clean enough. Now we only need to expose 'panels' getter.

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
File chrome/browser/notifications/balloon_collection_impl.h (right):

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.h:144: int
GetPanelHeightBase() const;
On 2011/11/09 03:09:57, Dmitry Titov wrote:
> Not clear why "Base". Maybe GetPositionAbovePanels()?
> 

Renamed to ComputeOffsetsToMoveAbovePanels. Also changed the variable name to
bottom_left_offset_to_move_above_panels_ and
bottom_right_offset_to_move_above_panels_.

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.h:155: // The height of
right-most panels we're currently using as a base to allign
On 2011/11/09 03:09:57, Dmitry Titov wrote:
> allign->align

Removed with the comment change as suggested below.

http://codereview.chromium.org/8503037/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.h:156: // those
virtically-from-bottom-right balloons to prevent them from being
On 2011/11/09 03:09:57, Dmitry Titov wrote:
> virtically->vertically
> May be change wording of this comment to something like:
> The height that guarantees that the notificaitons shown in the low-right or
> low-left corner of the screen will go above currently shown panels and will
not
> be obscured by them.
> 

Done.

http://codereview.chromium.org/8503037/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8503037/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:1429: 
On 2011/11/09 03:09:57, Dmitry Titov wrote:
> extra line

Done.

[Dmitry Titov, 2011-11-09 19:55:47]

still LGTM

[jennb, 2011-11-09 21:44:33]

Drive by...

Make sure the bug report notes that this is a temporary hack.

The coupling between Panels and Balloon Notifications as implemented here is
uncomfortably tight.

http://codereview.chromium.org/8503037/diff/4001/chrome/browser/ui/panels/bas...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/8503037/diff/4001/chrome/browser/ui/panels/bas...
chrome/browser/ui/panels/base_panel_browser_test.cc:181: if
(!testing_work_area_.IsEmpty())
This comment makes me worry that tests are affecting each other. How about
getting rid of set_testing_work_area() and have the subclass override
testing_work_area() instead? Then change line 182 to
panel_manager->SetWorkAreaForTestign(testing_work_area());

http://codereview.chromium.org/8503037/diff/4001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8503037/diff/4001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:1638: params.notification_id = 0;
This is done in StandardTestNotification() already.

http://codereview.chromium.org/8503037/diff/4001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:1654: Panel* panel2 =
CreatePanelWithBounds("Panel2", gfx::Rect(0, 0, 200, 400));
Does this assume notification balloons are wider than 200? Can we check for the
assumption?

http://codereview.chromium.org/8503037/diff/4001/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8503037/diff/4001/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:953: // Used only in coordination w/
notifications.
s/notifications/balloon notifications?  Given that these are all notifications
in this file, the word 'notifications' alone in the comments may not be
interpreted correctly by the reader.

[jianli, 2011-11-10 02:10:29]

dimich, can you take a look again with new patch? I fixed the issue for MacOSX.

[Dmitry Titov, 2011-11-10 18:35:45]

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.cc:184: closed_browser =
browser;
Lets add a notification when panel is removed from the PanelManager and then use
that one here. This will allow to remove the closed_browser parameter.

[jennb, 2011-11-10 19:17:13]

I think there are problems with the way notifications are used in this patch.
See comments below for more details.

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.cc:44: registrar_.Add(this,
chrome::NOTIFICATION_BROWSER_WINDOW_READY,
Instead of these browser notifications, perhaps better to have a PANEL
notifications sent by the panel manager after removing a panel? Then you don't
have to track if a browser is closing or not. Also, window_ready is sent before
panel manager has added the new panel to its list.

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.cc:187: case
chrome::NOTIFICATION_PANEL_CHANGED_BOUNDS:
Should you check the panel bounds here and only do work if the panel bounds are
within the balloon area?

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
File chrome/browser/notifications/balloon_collection_impl.h (right):

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.h:140: // Returns true if
there is change to offsets. Passing |closed_browser|
s/Passing/Pass in

?

(Irrelevant if you change to using a new panel_closed notification.)

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.h:159: // The offsets that
guarantees that the notificaitons shown in the
typo: notifications

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
File chrome/browser/notifications/balloon_collection_mac.mm (right):

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_mac.mm:38: gfx::Point
BalloonCollectionImpl::Layout::GetLayoutOrigin() const {
Comments in the code here make the code hard to read. Can you explain once at
the start of this function? Also, seems like a mistake to say TOP_LEFT is same
as lower-right?

[jianli, 2011-11-11 00:38:29]

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.cc:44: registrar_.Add(this,
chrome::NOTIFICATION_BROWSER_WINDOW_READY,
On 2011/11/10 19:17:13, jennb wrote:
> Instead of these browser notifications, perhaps better to have a PANEL
> notifications sent by the panel manager after removing a panel? Then you don't
> have to track if a browser is closing or not. Also, window_ready is sent
before
> panel manager has added the new panel to its list.

Done.

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.cc:184: closed_browser =
browser;
On 2011/11/10 18:35:45, Dmitry Titov wrote:
> Lets add a notification when panel is removed from the PanelManager and then
use
> that one here. This will allow to remove the closed_browser parameter.

Done.

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.cc:187: case
chrome::NOTIFICATION_PANEL_CHANGED_BOUNDS:
On 2011/11/10 19:17:13, jennb wrote:
> Should you check the panel bounds here and only do work if the panel bounds
are
> within the balloon area?

Done.

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
File chrome/browser/notifications/balloon_collection_impl.h (right):

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.h:140: // Returns true if
there is change to offsets. Passing |closed_browser|
On 2011/11/10 19:17:13, jennb wrote:
> s/Passing/Pass in
> 
> ?
> 
> (Irrelevant if you change to using a new panel_closed notification.)

Done.

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_impl.h:159: // The offsets that
guarantees that the notificaitons shown in the
On 2011/11/10 19:17:13, jennb wrote:
> typo: notifications

Done.

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
File chrome/browser/notifications/balloon_collection_mac.mm (right):

http://codereview.chromium.org/8503037/diff/4003/chrome/browser/notifications...
chrome/browser/notifications/balloon_collection_mac.mm:38: gfx::Point
BalloonCollectionImpl::Layout::GetLayoutOrigin() const {
On 2011/11/10 19:17:13, jennb wrote:
> Comments in the code here make the code hard to read. Can you explain once at
> the start of this function? Also, seems like a mistake to say TOP_LEFT is same
> as lower-right?

Done.

[Dmitry Titov, 2011-11-11 02:03:36]

LGTM

[commit-bot: I haz the power, 2011-11-11 03:40:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/8503037/10003

[commit-bot: I haz the power, 2011-11-11 04:49:25]

Change committed as 109588

[jennb, 2011-11-11 23:02:43]

LGTM

Belated feedback. I like the improvements. One FYI and one that should be fixed
to avoid possibility of flaky test.

http://codereview.chromium.org/8503037/diff/10003/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/8503037/diff/10003/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_impl.cc:419: if
((*iter)->GetBounds().x() >= work_area_.x() + max_balloon_width())
FYI, until your overflow changes are in, this will cause balloons to be affected
by tall overflow panels appearing offscreen.

No need to fix now as you're almost done with overflow changes.

http://codereview.chromium.org/8503037/diff/10003/chrome/browser/ui/panels/ba...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/8503037/diff/10003/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.cc:196:
ui_test_utils::WindowedNotificationObserver signal(
Should create the signal first, then do the 'if (ExistsPanel...)' to avoid
possibility that panel is created after the check and before the signal is
created.