position the balloons after closing in a way that will keep the next one's cl...

position the balloons after closing in a way that will keep the next one's close button under your mouse; don't reposition them to the normal place until the mouse has left the balloon collection

BUG=47333
TEST=make notifications of different sizes, try to close them all; the X should remain under your mouse

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

[Andrew T Wilson (Slow), 2010-07-14 20:28:23]

Looks mostly good - just a few comments.

http://codereview.chromium.org/2915003/diff/7001/8004
File chrome/browser/notifications/balloon_collection.cc (right):

http://codereview.chromium.org/2915003/diff/7001/8004#newcode29
chrome/browser/notifications/balloon_collection.cc:29: const bool kUseOffsets =
false;
I'd use #define/#ifdef USE_OFFSETS instead. It seems weird to have code compiled
on the Mac that won't ever be executed.

http://codereview.chromium.org/2915003/diff/7001/8004#newcode169
chrome/browser/notifications/balloon_collection.cc:169:
MessageLoopForUI::current()->RemoveObserver(this);
This would be a good place to use that common #define, so the platform logic is
in a single place above. And maybe have a NOTREACHED statement on other
platforms?

http://codereview.chromium.org/2915003/diff/7001/8004#newcode182
chrome/browser/notifications/balloon_collection.cc:182:
(*it)->set_offset(gfx::Point(0, 0));
Nit: should remove unnecessary braces around for-loop body?

http://codereview.chromium.org/2915003/diff/7001/8005
File chrome/browser/notifications/balloon_collection_impl.h (right):

http://codereview.chromium.org/2915003/diff/7001/8005#newcode141
chrome/browser/notifications/balloon_collection_impl.h:141: void
RemoveMessageLoopObserver();
I'm wondering if we should only declare these APIs for WIN/LINUX, rather than
declaring them but leaving their bodies blank (or leaving them undefined) on
MAC?

http://codereview.chromium.org/2915003/diff/7001/8006
File chrome/browser/notifications/balloon_collection_linux.cc (right):

http://codereview.chromium.org/2915003/diff/7001/8006#newcode45
chrome/browser/notifications/balloon_collection_linux.cc:45: if
(balloons_.size() == 0)
Nit: Also consider balloons_.empty() - possibly more readable, but it's your
call.

http://codereview.chromium.org/2915003/diff/7001/8006#newcode48
chrome/browser/notifications/balloon_collection_linux.cc:48: gfx::Point
upper_left = balloons_[balloons_.size() - 1]->GetPosition();
Nit: I see this construct in various places in the code:
(ballons_[balloons_.size()-1] - would using ballons_.back() be more
readable/better? I don't feel strongly about this, so as usually it's up to you.

http://codereview.chromium.org/2915003/diff/7001/8007
File chrome/browser/notifications/balloon_collection_mac.mm (right):

http://codereview.chromium.org/2915003/diff/7001/8007#newcode38
chrome/browser/notifications/balloon_collection_mac.mm:38: // always align; this
should not be called.
Since this isn't a virtual function, why not just avoid defining it entirely on
Mac, so you get a compile-time error instead of runtime?

[John Gregg, 2010-07-14 20:45:17]

http://codereview.chromium.org/2915003/diff/7001/8004
File chrome/browser/notifications/balloon_collection.cc (right):

http://codereview.chromium.org/2915003/diff/7001/8004#newcode29
chrome/browser/notifications/balloon_collection.cc:29: const bool kUseOffsets =
false;
On 2010/07/14 20:28:23, Andrew T Wilson wrote:
> I'd use #define/#ifdef USE_OFFSETS instead. It seems weird to have code
compiled
> on the Mac that won't ever be executed.

Done.

http://codereview.chromium.org/2915003/diff/7001/8004#newcode169
chrome/browser/notifications/balloon_collection.cc:169:
MessageLoopForUI::current()->RemoveObserver(this);
On 2010/07/14 20:28:23, Andrew T Wilson wrote:
> This would be a good place to use that common #define, so the platform logic
is
> in a single place above. And maybe have a NOTREACHED statement on other
> platforms?

Put it all behind the USE_OFFSETS flag.  This won't compile on MAC since there's
no such thing as a MessageLoopForUI::Observer on Mac, but we can deal with that
later if something changes and we need this logic on mac.

http://codereview.chromium.org/2915003/diff/7001/8004#newcode182
chrome/browser/notifications/balloon_collection.cc:182:
(*it)->set_offset(gfx::Point(0, 0));
On 2010/07/14 20:28:23, Andrew T Wilson wrote:
> Nit: should remove unnecessary braces around for-loop body?

Done.

http://codereview.chromium.org/2915003/diff/7001/8005
File chrome/browser/notifications/balloon_collection_impl.h (right):

http://codereview.chromium.org/2915003/diff/7001/8005#newcode141
chrome/browser/notifications/balloon_collection_impl.h:141: void
RemoveMessageLoopObserver();
On 2010/07/14 20:28:23, Andrew T Wilson wrote:
> I'm wondering if we should only declare these APIs for WIN/LINUX, rather than
> declaring them but leaving their bodies blank (or leaving them undefined) on
> MAC?

Changed it to your suggested way.

http://codereview.chromium.org/2915003/diff/7001/8006
File chrome/browser/notifications/balloon_collection_linux.cc (right):

http://codereview.chromium.org/2915003/diff/7001/8006#newcode45
chrome/browser/notifications/balloon_collection_linux.cc:45: if
(balloons_.size() == 0)
On 2010/07/14 20:28:23, Andrew T Wilson wrote:
> Nit: Also consider balloons_.empty() - possibly more readable, but it's your
> call.

Done.

http://codereview.chromium.org/2915003/diff/7001/8006#newcode48
chrome/browser/notifications/balloon_collection_linux.cc:48: gfx::Point
upper_left = balloons_[balloons_.size() - 1]->GetPosition();
On 2010/07/14 20:28:23, Andrew T Wilson wrote:
> Nit: I see this construct in various places in the code:
> (ballons_[balloons_.size()-1] - would using ballons_.back() be more
> readable/better? I don't feel strongly about this, so as usually it's up to
you.

i've never used back() before... so personally I would find less readable.

http://codereview.chromium.org/2915003/diff/7001/8007
File chrome/browser/notifications/balloon_collection_mac.mm (right):

http://codereview.chromium.org/2915003/diff/7001/8007#newcode38
chrome/browser/notifications/balloon_collection_mac.mm:38: // always align; this
should not be called.
On 2010/07/14 20:28:23, Andrew T Wilson wrote:
> Since this isn't a virtual function, why not just avoid defining it entirely
on
> Mac, so you get a compile-time error instead of runtime?

I can & will do this with the new rearrangements of #if's

[Andrew T Wilson (Slow), 2010-07-14 21:58:42]

Looking very close - I think we can/should move more stuff inside USE_OFFSETS,
but that's it.

http://codereview.chromium.org/2915003/diff/19001/20004
File chrome/browser/notifications/balloon_collection.cc (right):

http://codereview.chromium.org/2915003/diff/19001/20004#newcode112
chrome/browser/notifications/balloon_collection.cc:112: gfx::Point offset;
I'm still not thrilled that things like |offset| are still being initialized
even if USE_OFFSETS is not set. Given that the original loop was only 6 lines, I
wonder if you're better off just keeping the entire original loop for the
non-USE_OFFSETS case, and moving the entire current loop inside USE_OFFSETS.
WDYT?

http://codereview.chromium.org/2915003/diff/19001/20004#newcode128
chrome/browser/notifications/balloon_collection.cc:128: it++;
nit: is ++it slightly more efficient?

http://codereview.chromium.org/2915003/diff/19001/20005
File chrome/browser/notifications/balloon_collection_impl.h (right):

http://codereview.chromium.org/2915003/diff/19001/20005#newcode32
chrome/browser/notifications/balloon_collection_impl.h:32: #if defined(OS_WIN)
|| defined(OS_LINUX)
Maybe use USE_OFFSETS here to enforce that the only reason we need this Observer
is to manage offsets?

http://codereview.chromium.org/2915003/diff/19001/20005#newcode170
chrome/browser/notifications/balloon_collection_impl.h:170: // Factory for
generating delayed reposition tasks on mouse motion.
Should these be wrapped in USE_OFFSETS also? Your call, I don't have a strong
feeling about this.

http://codereview.chromium.org/2915003/diff/19001/20007
File chrome/browser/notifications/balloon_collection_win.cc (right):

http://codereview.chromium.org/2915003/diff/19001/20007#newcode43
chrome/browser/notifications/balloon_collection_win.cc:43: if (balloons_.size()
== 0)
ballons_.empty()

[John Gregg, 2010-07-14 23:21:49]

http://codereview.chromium.org/2915003/diff/19001/20004
File chrome/browser/notifications/balloon_collection.cc (right):

http://codereview.chromium.org/2915003/diff/19001/20004#newcode112
chrome/browser/notifications/balloon_collection.cc:112: gfx::Point offset;
On 2010/07/14 21:58:42, Andrew T Wilson wrote:
> I'm still not thrilled that things like |offset| are still being initialized
> even if USE_OFFSETS is not set. Given that the original loop was only 6 lines,
I
> wonder if you're better off just keeping the entire original loop for the
> non-USE_OFFSETS case, and moving the entire current loop inside USE_OFFSETS.
> WDYT?

On 2010/07/14 21:58:42, Andrew T Wilson wrote:
> Looking very close - I think we can/should move more stuff inside USE_OFFSETS,
> but that's it.
> 
> http://codereview.chromium.org/2915003/diff/19001/20004
> File chrome/browser/notifications/balloon_collection.cc (right):
> 
> http://codereview.chromium.org/2915003/diff/19001/20004#newcode112
> chrome/browser/notifications/balloon_collection.cc:112: gfx::Point offset;
> I'm still not thrilled that things like |offset| are still being initialized
> even if USE_OFFSETS is not set. Given that the original loop was only 6 lines,
I
> wonder if you're better off just keeping the entire original loop for the
> non-USE_OFFSETS case, and moving the entire current loop inside USE_OFFSETS.
> WDYT?

Works for me.  I tried #if'ing absolutely everything but it turns to spaghetti
code.  But I think the amount of duplicated code is small and simple enough we
can just have both loops.

http://codereview.chromium.org/2915003/diff/19001/20004#newcode128
chrome/browser/notifications/balloon_collection.cc:128: it++;
On 2010/07/14 21:58:42, Andrew T Wilson wrote:
> nit: is ++it slightly more efficient?
> 

it's certainly not less efficient!  Changed.

http://codereview.chromium.org/2915003/diff/19001/20005
File chrome/browser/notifications/balloon_collection_impl.h (right):

http://codereview.chromium.org/2915003/diff/19001/20005#newcode32
chrome/browser/notifications/balloon_collection_impl.h:32: #if defined(OS_WIN)
|| defined(OS_LINUX)
On 2010/07/14 21:58:42, Andrew T Wilson wrote:
> Maybe use USE_OFFSETS here to enforce that the only reason we need this
Observer
> is to manage offsets?

Okay.  I had it this way because that interface doesn't exist on MAC at all, but
I don't see this needing to change.

http://codereview.chromium.org/2915003/diff/19001/20005#newcode170
chrome/browser/notifications/balloon_collection_impl.h:170: // Factory for
generating delayed reposition tasks on mouse motion.
On 2010/07/14 21:58:42, Andrew T Wilson wrote:
> Should these be wrapped in USE_OFFSETS also? Your call, I don't have a strong
> feeling about this.

Done.

http://codereview.chromium.org/2915003/diff/19001/20007
File chrome/browser/notifications/balloon_collection_win.cc (right):

http://codereview.chromium.org/2915003/diff/19001/20007#newcode43
chrome/browser/notifications/balloon_collection_win.cc:43: if (balloons_.size()
== 0)
On 2010/07/14 21:58:42, Andrew T Wilson wrote:
> ballons_.empty()

Done.

[Andrew T Wilson (Slow), 2010-07-15 19:12:15]

LGTM