Make status bubble expand on Mac to fit URLs longer than the standard width....

Make status bubble expand on Mac to fit URLs longer than the standard width.

BUG=43193, 55770
TEST=status bubble resizes on Mac to fit URLs longer than standard bubble size.


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

[Nico, 2010-09-15 22:08:44]

General comment: Please be explicit which of your numbers are in view coordinate
space and which are in window coordinate space.

(Also try this with setting UI scale to something > 1 in Quartz Debug … it might
not look right there)

http://codereview.chromium.org/3437002/diff/11002/12002
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/3437002/diff/11002/12002#newcode595
chrome/browser/cocoa/status_bubble_mac.mm:595: int max_bubble_width =
window_frame.size.width;
NSWidth(window_frame)

also in other places

http://codereview.chromium.org/3437002/diff/11002/12002#newcode663
chrome/browser/cocoa/status_bubble_mac.mm:663: const int kScrollbarWidth = 16; 
// TODO(viettrungluu): ugh.
why todo(trungl)? poor guy.

[Miranda Callahan, 2010-09-15 22:35:52]

On 2010/09/15 22:08:44, Nico wrote:
> General comment: Please be explicit which of your numbers are in view
coordinate
> space and which are in window coordinate space.

Hmm.  All of my code deals with changing the width of the status bar, not its
position -- it is, therefore, coordinate-system-agnostic.

> (Also try this with setting UI scale to something > 1 in Quartz Debug … it
might
> not look right there)

Whoa, you're right -- it doesn't look right. But it's broken right now in stable
build in the same way (it must have always been this way on Mac) -- the text
isn't being elided correctly in long URL's, ever.

I have filed a bug against myself to fix eliding
(http://code.google.com/p/chromium/issues/detail?id=55770), and would like to do
that after committing this patch.

[Miranda Callahan, 2010-09-15 22:36:00]

http://codereview.chromium.org/3437002/diff/11002/12002
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/3437002/diff/11002/12002#newcode595
chrome/browser/cocoa/status_bubble_mac.mm:595: int max_bubble_width =
window_frame.size.width;
On 2010/09/15 22:08:44, Nico wrote:
> NSWidth(window_frame)
> 
> also in other places

I made this change, but can you explain why this method is preferable?

http://codereview.chromium.org/3437002/diff/11002/12002#newcode663
chrome/browser/cocoa/status_bubble_mac.mm:663: const int kScrollbarWidth = 16; 
// TODO(viettrungluu): ugh.
On 2010/09/15 22:08:44, Nico wrote:
> why todo(trungl)? poor guy.
oops -- let me change that.  I copied directly from trungl's code in
browser_window_controller.

[Nico, 2010-09-15 22:49:38]

On 2010/09/15 22:35:52, Miranda Callahan wrote:
> On 2010/09/15 22:08:44, Nico wrote:
> > General comment: Please be explicit which of your numbers are in view
> coordinate
> > space and which are in window coordinate space.
> 
> Hmm.  All of my code deals with changing the width of the status bar, not its
> position -- it is, therefore, coordinate-system-agnostic.

No. If you set the scale factor to something > 1, then the view width won't
change (instead, an implicit scale is set on the view's graphics context), but
the window width will be scaled. You need to call [view convertSize:size
toView:nil/fromView:nil] to convert sizes between view and window coordinates.

> 
> > (Also try this with setting UI scale to something > 1 in Quartz Debug … it
> might
> > not look right there)
> 
> Whoa, you're right -- it doesn't look right. But it's broken right now in
stable
> build in the same way (it must have always been this way on Mac) -- the text
> isn't being elided correctly in long URL's, ever.
> 
> I have filed a bug against myself to fix eliding
> (http://code.google.com/p/chromium/issues/detail?id=55770), and would like to
do
> that after committing this patch.

[Nico, 2010-09-15 22:49:45]

http://codereview.chromium.org/3437002/diff/11002/12002
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/3437002/diff/11002/12002#newcode595
chrome/browser/cocoa/status_bubble_mac.mm:595: int max_bubble_width =
window_frame.size.width;
On 2010/09/15 22:36:00, Miranda Callahan wrote:
> On 2010/09/15 22:08:44, Nico wrote:
> > NSWidth(window_frame)
> > 
> > also in other places
> 
> I made this change, but can you explain why this method is preferable?

Because it's 3 byte shorter :-) I also think it's slightly more readable

[Miranda Callahan, 2010-09-17 00:53:24]

On 2010/09/15 22:49:38, Nico wrote:
> On 2010/09/15 22:35:52, Miranda Callahan wrote:
> > On 2010/09/15 22:08:44, Nico wrote:
> > > General comment: Please be explicit which of your numbers are in view
> > coordinate
> > > space and which are in window coordinate space.
> > 
> > Hmm.  All of my code deals with changing the width of the status bar, not
its
> > position -- it is, therefore, coordinate-system-agnostic.
> 
> No. If you set the scale factor to something > 1, then the view width won't
> change (instead, an implicit scale is set on the view's graphics context), but
> the window width will be scaled. You need to call [view convertSize:size
> toView:nil/fromView:nil] to convert sizes between view and window coordinates.

Aha, I see!  Thanks to your wise counsel, I've changed the patch and also wiped
out the original bug in the status bubble,
http://code.google.com/p/chromium/issues/detail?id=55770.  

Please take another look.

[Nico, 2010-09-17 16:06:31]

LG with comments

http://codereview.chromium.org/3437002/diff/7002/15002
File chrome/browser/cocoa/status_bubble_mac.h (right):

http://codereview.chromium.org/3437002/diff/7002/15002#newcode155
chrome/browser/cocoa/status_bubble_mac.h:155: // Used to elide the original URL
if it's wider than the standard width.
s/Used.*it's/Needs to be passed to ElideUrl if it's/

http://codereview.chromium.org/3437002/diff/7002/15003
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/3437002/diff/7002/15003#newcode138
chrome/browser/cocoa/status_bubble_mac.mm:138: frame.size.width =
NSWidth(CalculateWindowFrame(false));
CalculateWindowFrame(/*expand=*/false);

also elsewhere

http://codereview.chromium.org/3437002/diff/7002/15003#newcode149
chrome/browser/cocoa/status_bubble_mac.mm:149: text_width = scaled_width.width;
static_cast<int>

http://codereview.chromium.org/3437002/diff/7002/15003#newcode161
chrome/browser/cocoa/status_bubble_mac.mm:161: // hover delay breaks things.
ExpandBubble is called explicitly in tests.
can you expand on "breaks things"?

http://codereview.chromium.org/3437002/diff/7002/15003#newcode593
chrome/browser/cocoa/status_bubble_mac.mm:593: if
(!expand_timer_factory_.empty())
is this check required? i'd think that RevokeAll() does that internally already

http://codereview.chromium.org/3437002/diff/7002/15003#newcode600
chrome/browser/cocoa/status_bubble_mac.mm:600: int max_bubble_width =
NSWidth(window_frame);
CGFloat, not int

http://codereview.chromium.org/3437002/diff/7002/15003#newcode601
chrome/browser/cocoa/status_bubble_mac.mm:601: int standard_bubble_width =
NSWidth(CalculateWindowFrame(false));
CGFloat

http://codereview.chromium.org/3437002/diff/7002/15003#newcode610
chrome/browser/cocoa/status_bubble_mac.mm:610: // Scale width from gfx::Font in
window coordinates to view coordinates.
this comment is the wrong way round. should be:

// Scale width from gfx::Font in view coordinates to window coordinates.

(you're calling toView:nil below, not fromView:nil. and that's correct, only the
comment is wrong)

[Miranda Callahan, 2010-09-17 16:33:12]

http://codereview.chromium.org/3437002/diff/7002/15002
File chrome/browser/cocoa/status_bubble_mac.h (right):

http://codereview.chromium.org/3437002/diff/7002/15002#newcode155
chrome/browser/cocoa/status_bubble_mac.h:155: // Used to elide the original URL
if it's wider than the standard width.
On 2010/09/17 16:06:36, Nico wrote:
> s/Used.*it's/Needs to be passed to ElideUrl if it's/

Fixed, and added more detail so as not to release an "it's" into the world
without a proper antecedent.  :-)

http://codereview.chromium.org/3437002/diff/7002/15003
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/3437002/diff/7002/15003#newcode138
chrome/browser/cocoa/status_bubble_mac.mm:138: frame.size.width =
NSWidth(CalculateWindowFrame(false));
On 2010/09/17 16:06:36, Nico wrote:
> CalculateWindowFrame(/*expand=*/false);
> 
> also elsewhere

Never seen this style before, but I like it.  Done.

http://codereview.chromium.org/3437002/diff/7002/15003#newcode149
chrome/browser/cocoa/status_bubble_mac.mm:149: text_width = scaled_width.width;
On 2010/09/17 16:06:36, Nico wrote:
> static_cast<int>

Done.

http://codereview.chromium.org/3437002/diff/7002/15003#newcode161
chrome/browser/cocoa/status_bubble_mac.mm:161: // hover delay breaks things.
ExpandBubble is called explicitly in tests.
On 2010/09/17 16:06:36, Nico wrote:
> can you expand on "breaks things"?

Changed to explain that tests shouldn't use animation.

http://codereview.chromium.org/3437002/diff/7002/15003#newcode600
chrome/browser/cocoa/status_bubble_mac.mm:600: int max_bubble_width =
NSWidth(window_frame);
On 2010/09/17 16:06:36, Nico wrote:
> CGFloat, not int

Done.

http://codereview.chromium.org/3437002/diff/7002/15003#newcode601
chrome/browser/cocoa/status_bubble_mac.mm:601: int standard_bubble_width =
NSWidth(CalculateWindowFrame(false));
On 2010/09/17 16:06:36, Nico wrote:
> CGFloat

Done.

http://codereview.chromium.org/3437002/diff/7002/15003#newcode610
chrome/browser/cocoa/status_bubble_mac.mm:610: // Scale width from gfx::Font in
window coordinates to view coordinates.
On 2010/09/17 16:06:36, Nico wrote:
> this comment is the wrong way round. should be:
> 
> // Scale width from gfx::Font in view coordinates to window coordinates.
> 
> (you're calling toView:nil below, not fromView:nil. and that's correct, only
the
> comment is wrong)

Done.