Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7981035:
Add icon to the panel titlebar on Mac. (Closed)
|
Created:
9 years, 3 months ago by jianli Modified:
9 years, 3 months ago CC:
chromium-reviews, jianli, dcheng, prasadt Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd icon to the panel titlebar on Mac.
Changed Panel.xib to add icon in NSView type.
BUG=none
TEST=manul test to verify it exists
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102326
Patch Set 1 #
Total comments: 12
Patch Set 2 : Fix #
Total comments: 2
Patch Set 3 : Fix #
Messages
Total messages: 15 (0 generated)
http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right): http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:213: remove extra line http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:253: [NSDictionary dictionaryWithObjectsAndKeys:[NSFont userFontOfSize:12], Don't make assumptions. You should be able to use [title_ font] to get the font used. http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:258: attributes:attributes]; Alternatively, does [title_ attributedStringValue] have the correct font tagging? If so, use it.
I tried using [title_ font] for the computation and it turned out that we miss the half part of last character. Same thing for using [title_ attributedStringValue]. Do you know why? On Wed, Sep 21, 2011 at 2:04 PM, <avi@chromium.org> wrote: > > http://codereview.chromium.**org/7981035/diff/1/chrome/** > browser/ui/panels/panel_**titlebar_view_cocoa.mm<http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm> > File chrome/browser/ui/panels/panel**_titlebar_view_cocoa.mm<http://panel_titlebar_view_cocoa.mm>(right): > > http://codereview.chromium.**org/7981035/diff/1/chrome/** > browser/ui/panels/panel_**titlebar_view_cocoa.mm#**newcode213<http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm#newcode213> > chrome/browser/ui/panels/panel**_titlebar_view_cocoa.mm:213<http://panel_titlebar_view_cocoa.mm:213> > : > remove extra line > > http://codereview.chromium.**org/7981035/diff/1/chrome/** > browser/ui/panels/panel_**titlebar_view_cocoa.mm#**newcode253<http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm#newcode253> > chrome/browser/ui/panels/panel**_titlebar_view_cocoa.mm:253<http://panel_titlebar_view_cocoa.mm:253>: > [NSDictionary > dictionaryWithObjectsAndKeys:[**NSFont userFontOfSize:12], > Don't make assumptions. You should be able to use [title_ font] to get > the font used. > > http://codereview.chromium.**org/7981035/diff/1/chrome/** > browser/ui/panels/panel_**titlebar_view_cocoa.mm#**newcode258<http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm#newcode258> > chrome/browser/ui/panels/panel**_titlebar_view_cocoa.mm:258<http://panel_titlebar_view_cocoa.mm:258> > : > attributes:attributes]; > Alternatively, does [title_ attributedStringValue] have the correct font > tagging? If so, use it. > > > http://codereview.chromium.**org/7981035/<http://codereview.chromium.org/7981... >
On 2011/09/21 21:12:55, jianli wrote: > I tried using [title_ font] for the computation and it turned out that we > miss the half part of last character. Is it the same as [NSFont userFontOfSize:12]? I don't like the idea that we're hardcoding in a font that may or may not match the one used by title_.
http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right): http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:245: NSRect closeButtonBounds = [closeButton_ bounds]; We should call those 'frame' ratehr then 'bounds' - because from their parent's NSView, they have frames (and we use setFrame later to set it. So it makes sense to name these variables iconFrame, etc and use [icon_ frame] to pull the initial values. The only one in the same cordinate system is 'bounds', it should indeed be 'bounds'. http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:258: attributes:attributes]; 'test' should be autoreleased in the same statement. Unless lifetime is manages explicitly, 'alloc', 'init' and 'autorelease' should always be in the same statement.
No. [title_ font] returns "Lucida Grande" while [NSFont userFontOfSize:] uses "Helvetica". On Wed, Sep 21, 2011 at 2:27 PM, <avi@chromium.org> wrote: > On 2011/09/21 21:12:55, jianli wrote: > >> I tried using [title_ font] for the computation and it turned out that we >> miss the half part of last character. >> > > Is it the same as [NSFont userFontOfSize:12]? I don't like the idea that > we're > hardcoding in a font that may or may not match the one used by title_. > > > http://codereview.chromium.**org/7981035/<http://codereview.chromium.org/7981... >
On 2011/09/21 22:07:23, jianli wrote: > No. [title_ font] returns "Lucida Grande" while [NSFont userFontOfSize:] > uses "Helvetica". Does the text field use Lucida Grande or Helvetica? If you're getting clipped when you measure using the right font, measuring with the wrong font isn't the answer to use.
Drive by... http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right): http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:245: NSRect closeButtonBounds = [closeButton_ bounds]; On 2011/09/21 21:32:11, Dmitry Titov wrote: > We should call those 'frame' ratehr then 'bounds' - because from their parent's > NSView, they have frames (and we use setFrame later to set it. So it makes sense > to name these variables iconFrame, etc and use [icon_ frame] to pull the initial > values. > > The only one in the same cordinate system is 'bounds', it should indeed be > 'bounds'. Is updateCloseButtonLayout also using bounds incorrectly?
On 2011/09/21 22:10:38, jennb wrote: > Is updateCloseButtonLayout also using bounds incorrectly? Yes, I think it does use them incorrectly at initialization. It should operate on button's frame.
I can change updateCloseButtonLayout to fix this problem in this patch. On Wed, Sep 21, 2011 at 3:14 PM, <dimich@chromium.org> wrote: > On 2011/09/21 22:10:38, jennb wrote: > >> Is updateCloseButtonLayout also using bounds incorrectly? >> > > Yes, I think it does use them incorrectly at initialization. It should > operate > on button's frame. > > > > http://codereview.chromium.**org/7981035/<http://codereview.chromium.org/7981... >
The text field is indeed using small system font "Lucida Grande". For some reason, [[title_ attributedStringValue] size] seems to return the smaller size. On Wed, Sep 21, 2011 at 3:10 PM, <avi@chromium.org> wrote: > On 2011/09/21 22:07:23, jianli wrote: > >> No. [title_ font] returns "Lucida Grande" while [NSFont userFontOfSize:] >> uses "Helvetica". >> > > Does the text field use Lucida Grande or Helvetica? If you're getting > clipped > when you measure using the right font, measuring with the wrong font isn't > the > answer to use. > > > http://codereview.chromium.**org/7981035/<http://codereview.chromium.org/7981... >
http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right): http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:213: On 2011/09/21 21:04:25, Avi wrote: > remove extra line Done. http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:245: NSRect closeButtonBounds = [closeButton_ bounds]; On 2011/09/21 21:32:11, Dmitry Titov wrote: > We should call those 'frame' ratehr then 'bounds' - because from their parent's > NSView, they have frames (and we use setFrame later to set it. So it makes sense > to name these variables iconFrame, etc and use [icon_ frame] to pull the initial > values. > > The only one in the same cordinate system is 'bounds', it should indeed be > 'bounds'. Changed to call sizeToFit before querying its bounds, per suggestion from Dave. http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:245: NSRect closeButtonBounds = [closeButton_ bounds]; On 2011/09/21 21:32:11, Dmitry Titov wrote: > We should call those 'frame' ratehr then 'bounds' - because from their parent's > NSView, they have frames (and we use setFrame later to set it. So it makes sense > to name these variables iconFrame, etc and use [icon_ frame] to pull the initial > values. > > The only one in the same cordinate system is 'bounds', it should indeed be > 'bounds'. Done. http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:245: NSRect closeButtonBounds = [closeButton_ bounds]; On 2011/09/21 22:10:38, jennb wrote: > On 2011/09/21 21:32:11, Dmitry Titov wrote: > > We should call those 'frame' ratehr then 'bounds' - because from their > parent's > > NSView, they have frames (and we use setFrame later to set it. So it makes > sense > > to name these variables iconFrame, etc and use [icon_ frame] to pull the > initial > > values. > > > > The only one in the same cordinate system is 'bounds', it should indeed be > > 'bounds'. > > Is updateCloseButtonLayout also using bounds incorrectly? Done. http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:253: [NSDictionary dictionaryWithObjectsAndKeys:[NSFont userFontOfSize:12], On 2011/09/21 21:04:25, Avi wrote: > Don't make assumptions. You should be able to use [title_ font] to get the font > used. Removed since it is not needed. http://codereview.chromium.org/7981035/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:258: attributes:attributes]; On 2011/09/21 21:32:11, Dmitry Titov wrote: > 'test' should be autoreleased in the same statement. Unless lifetime is manages > explicitly, 'alloc', 'init' and 'autorelease' should always be in the same > statement. Removed since it is not needed.
LGTM from me. http://codereview.chromium.org/7981035/diff/6001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right): http://codereview.chromium.org/7981035/diff/6001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:241: [title_ sizeToFit]; Magic :-)
Forgot to mention: http://codereview.chromium.org/7981035/diff/6001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right): http://codereview.chromium.org/7981035/diff/6001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:244: NSRect frame = [self frame]; Discussed offline: This one should still be 'bounds'.
lgtm Looks much better. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
