Use the Mac omnibox field's font as the basis for the fonts used in the field and popup.

Use the Mac omnibox field's font as the basis for the fonts used in the field and popup.
NSAttributedString has fixed default font, unless overridden, this
basically makes the font used consistent with the field size.  Theming
will most likely change this again, but we can be prettier in the
meanwhile.

[Scott Hess - ex-Googler, 2009-05-14 00:30:14]

I got annoyed by the randomness of the fonts for autocomplete.  This makes them
consistent.  It does not answer the question of "What font should we use in the
Omnibox", because I suspect that needs to be done in consultation with UX
people, or, at least, in concert with size changes along the location bar based
on the target font's height.

[pink (ping after 24hrs), 2009-05-14 16:11:24]

LGTM with issues addressed.

http://codereview.chromium.org/115334/diff/1/3
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/115334/diff/1/3#newcode76
Line 76: static NSMutableAttributedString* MatchText(const AutocompleteMatch&
match,
this should just return a NSAttributedString. The callers don't care that it's
mutable.

http://codereview.chromium.org/115334/diff/1/3#newcode82
Line 82: static NSMutableAttributedString* DecorateMatchedString(
same here.

http://codereview.chromium.org/115334/diff/1/4
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/115334/diff/1/4#newcode264
Line 264: size_t rows = model_->result().size();
const?

http://codereview.chromium.org/115334/diff/1/4#newcode266
Line 266: for (size_t ii = 0; ii < rows; ++ii) {
why |ii|?

http://codereview.chromium.org/115334/diff/1/5
File chrome/browser/autocomplete/autocomplete_popup_view_mac_unittest.mm
(right):

http://codereview.chromium.org/115334/diff/1/5#newcode84
Line 84: NSColor* color_;
comment that these are weak references. What autorelease pool are these going
into?

http://codereview.chromium.org/115334/diff/1/5#newcode84
Line 84: NSColor* color_;
comment that these are weak references. What autorelease pool are these going
into?

[Scott Hess - ex-Googler, 2009-05-14 18:40:44]

Addressed all but fully converting DecorateMatchedString() to non-mutable.

http://codereview.chromium.org/115334/diff/1/3
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/115334/diff/1/3#newcode76
Line 76: static NSMutableAttributedString* MatchText(const AutocompleteMatch&
match,
On 2009/05/14 16:11:24, pink wrote:
> this should just return a NSAttributedString. The callers don't care that it's
> mutable.

MatchText() callers certainly don't care.  I made DecorateMatchedString() return
mutable because MatchText() needs to append things together, and set the
line-break style for the overall string.  The button cell's line-break mode
doesn't apply in this case, it has to be in the attributed string.

I can go non-mutable and spin off additional objects.  Or I could pass base
attributes into DecorateMatchedString(), which wouldn't be too bad.  I'll go
poke at the code to see what looks pretty and post again, let me know if you
have a particular preference.

http://codereview.chromium.org/115334/diff/1/4
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/115334/diff/1/4#newcode264
Line 264: size_t rows = model_->result().size();
On 2009/05/14 16:11:24, pink wrote:
> const?

Good call.

http://codereview.chromium.org/115334/diff/1/4#newcode266
Line 266: for (size_t ii = 0; ii < rows; ++ii) {
On 2009/05/14 16:11:24, pink wrote:
> why |ii|?

Once upon a time, I developed the habit to use ii, jj, kk, because they are a
lot easier to search for than i, j, k.  Since that project is long ago, these
days it mostly manifests when I'm typing things from scratch, as opposed to
looking at some other code (which is probably why "NSRect r" isn't rr).

http://codereview.chromium.org/115334/diff/1/5
File chrome/browser/autocomplete/autocomplete_popup_view_mac_unittest.mm
(right):

http://codereview.chromium.org/115334/diff/1/5#newcode84
Line 84: NSColor* color_;
On 2009/05/14 16:11:24, pink wrote:
> comment that these are weak references. What autorelease pool are these going
> into?

Ick.  I had made that change simply for convenience, so that everyone would get
consistent test inputs.  But, AFAICT these guys are probably now not in an
autorelease pool at all, which is probably bad.

Converting to PlatformTest and initializing in SetUp().  Let me know if you'd
prefer creation functions instead, there's not magic about having a literal
shared object reference for these tests.

[pink (ping after 24hrs), 2009-05-14 19:09:01]

http://codereview.chromium.org/115334/diff/1/3
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/115334/diff/1/3#newcode76
Line 76: static NSMutableAttributedString* MatchText(const AutocompleteMatch&
match,
On 2009/05/14 18:40:44, shess wrote:
> On 2009/05/14 16:11:24, pink wrote:
> > this should just return a NSAttributedString. The callers don't care that
it's
> > mutable.
> 
> MatchText() callers certainly don't care.  I made DecorateMatchedString()
return
> mutable because MatchText() needs to append things together, and set the
> line-break style for the overall string.  The button cell's line-break mode
> doesn't apply in this case, it has to be in the attributed string.
> 
> I can go non-mutable and spin off additional objects.  Or I could pass base
> attributes into DecorateMatchedString(), which wouldn't be too bad.  I'll go
> poke at the code to see what looks pretty and post again, let me know if you
> have a particular preference.

No preference. You can probably leave it as Mutable if that makes things easier.

[Scott Hess - ex-Googler, 2009-05-14 21:54:55]

Thanks.

http://codereview.chromium.org/115334/diff/1/3
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/115334/diff/1/3#newcode76
Line 76: static NSMutableAttributedString* MatchText(const AutocompleteMatch&
match,
On 2009/05/14 19:09:01, pink wrote:
> On 2009/05/14 18:40:44, shess wrote:
> > On 2009/05/14 16:11:24, pink wrote:
> > > this should just return a NSAttributedString. The callers don't care that
> it's
> > > mutable.
> > 
> > MatchText() callers certainly don't care.  I made DecorateMatchedString()
> return
> > mutable because MatchText() needs to append things together, and set the
> > line-break style for the overall string.  The button cell's line-break mode
> > doesn't apply in this case, it has to be in the attributed string.
> > 
> > I can go non-mutable and spin off additional objects.  Or I could pass base
> > attributes into DecorateMatchedString(), which wouldn't be too bad.  I'll go
> > poke at the code to see what looks pretty and post again, let me know if you
> > have a particular preference.
> 
> No preference. You can probably leave it as Mutable if that makes things
easier.

I couldn't see a good approach which didn't either result in the caller having
to make mutable copies for purposes of making a couple more manipulations, or
modifying the decorator to accept extraneous stuff so that the attributed
strings come out right from the get-go.  There's probably a deep discussion on
the order of string versus stringbuffer lurking in here, but I'm inclined to
just leave it mutable, since it only exists as a helper for MatchText().