The text color for BookmarkButton sometimes doesn't get updated.

The text color for BookmarkButton sometimes doesn't get updated.

This occurs on startup or when dragging a new bookmark onto the bookmark bar.

This is because [BookmarkButtonCell setText:] was being called after [BookmarkButtonCell setTextColor:]

I modified BookmarkButtonCell so it would save the text color and reapply it when setText gets called.

BUG=88694
TEST=Manually. Choose a dark theme like glow and see if the BookmarkButtonCell text color turns white. Try relaunching and draging urls into the bookmark bar and see that it works properly.


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

[keishi1, 2011-07-11 14:41:25]

I couldn't figure out why [BookmarkButtonCell setTextColor:] was calling
[NSButton setAttributedTitle:] instead of [NSButtonCell setAttributedTitle:].

[John Grabowski, 2011-07-11 21:48:12]

http://codereview.chromium.org/7326043/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h (right):

http://codereview.chromium.org/7326043/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h:32:
scoped_nsobject<NSColor> textColor_;
Looks like the convention in this class is to have a comment for all data
members.  You should add one for your new member.

http://codereview.chromium.org/7326043/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h:64: -
(void)applyTextColor;
Reading the header it would be unclear when or why someone should call
applyTextColor.  Shouldn't that be automatic?

Seems like you'd want this to be a "private" protocol.

[keishi1, 2011-07-12 01:20:23]

On 2011/07/11 21:48:12, John Grabowski wrote:
>
http://codereview.chromium.org/7326043/diff/1/chrome/browser/ui/cocoa/bookmar...
> File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h (right):
> 
>
http://codereview.chromium.org/7326043/diff/1/chrome/browser/ui/cocoa/bookmar...
> chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h:32:
> scoped_nsobject<NSColor> textColor_;
> Looks like the convention in this class is to have a comment for all data
> members.  You should add one for your new member.
> 
>
http://codereview.chromium.org/7326043/diff/1/chrome/browser/ui/cocoa/bookmar...
> chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h:64: -
> (void)applyTextColor;
> Reading the header it would be unclear when or why someone should call
> applyTextColor.  Shouldn't that be automatic?
> 
> Seems like you'd want this to be a "private" protocol.

Thanks. Fixed.

[John Grabowski, 2011-07-12 02:15:38]

LGTM

Thanks for fixing this!

http://codereview.chromium.org/7326043/diff/5001/chrome/browser/ui/cocoa/book...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

http://codereview.chromium.org/7326043/diff/5001/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:48: [self
configureBookmarkButtonCell];
Minor nit: we now have 3 method calls with blank lines between them.
Might be better to remove the blank lines (49 and 51).

http://codereview.chromium.org/7326043/diff/5001/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:180: -
(void)applyTextColor {
Add comment in code explaining why this is here.  E.g. "we must apply the color
after any setTitle call".

[commit-bot: I haz the power, 2011-07-12 13:02:22]

Change committed as 92158