Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2636473003:
[Mac] Update the Text Color for the Avatar Button (Closed)
Description[Mac] Update the Text Color for the Avatar Button
The text color should be determined by the theme's color. If the theme
is dark, then the text color should be white. Otherwise, it should be
black.
BUG=644004
Review-Url: https://codereview.chromium.org/2636473003
Cr-Commit-Position: refs/heads/master@{#447156}
Committed: https://chromium.googlesource.com/chromium/src/+/a47be35337a78a8893d303600f76966e5d4dd225
Patch Set 1 #Patch Set 2 : Nits #Patch Set 3 : Fixed tests #
Total comments: 4
Patch Set 4 : Fix for shrike #
Total comments: 3
Patch Set 5 : Added a test #Patch Set 6 : Added comment #
Messages
Total messages: 43 (29 generated)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
spqchan@chromium.org changed reviewers: + shrike@chromium.org
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/13 22:54:05, spqchan wrote: > PTAL ping
lgtm https://codereview.chromium.org/2636473003/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.h (right): https://codereview.chromium.org/2636473003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.h:25: NSWindow* window_; You should notate that this is weak? https://codereview.chromium.org/2636473003/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/2636473003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:255: foregroundColor = [NSColor whiteColor]; It's too bad that we can't use a text color from the theme instead of forcing it white or black.
spqchan@chromium.org changed reviewers: + rsesek@chromium.org
thanks! +rsesek for ownership https://codereview.chromium.org/2636473003/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.h (right): https://codereview.chromium.org/2636473003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.h:25: NSWindow* window_; On 2017/01/26 22:10:13, shrike wrote: > You should notate that this is weak? Done. https://codereview.chromium.org/2636473003/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/2636473003/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:255: foregroundColor = [NSColor whiteColor]; On 2017/01/26 22:10:13, shrike wrote: > It's too bad that we can't use a text color from the theme instead of forcing it > white or black. Agree
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2636473003/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.h (right): https://codereview.chromium.org/2636473003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.h:28: - (id)initWithBrowser:(Browser*)browser window:(NSWindow*)window; Is |window| always the same as browser->window()->GetNativeWindow() ?
PTAL https://codereview.chromium.org/2636473003/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.h (right): https://codereview.chromium.org/2636473003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.h:28: - (id)initWithBrowser:(Browser*)browser window:(NSWindow*)window; On 2017/01/27 11:01:28, Robert Sesek wrote: > Is |window| always the same as browser->window()->GetNativeWindow() ? No, browser->window()->GetNativeWindow() is null
Would you please add a test? We want to make sure the text is dark in the default case and light when the frame color is dark.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/27 19:39:29, shrike wrote: > Would you please add a test? We want to make sure the text is dark in the > default case and light when the frame color is dark. done
On 2017/01/27 22:26:15, spqchan wrote: > On 2017/01/27 19:39:29, shrike wrote: > > Would you please add a test? We want to make sure the text is dark in the > > default case and light when the frame color is dark. > > done Great - thank you. lgtm.
https://codereview.chromium.org/2636473003/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.h (right): https://codereview.chromium.org/2636473003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.h:28: - (id)initWithBrowser:(Browser*)browser window:(NSWindow*)window; On 2017/01/27 18:24:49, spqchan wrote: > On 2017/01/27 11:01:28, Robert Sesek wrote: > > Is |window| always the same as browser->window()->GetNativeWindow() ? > > No, browser->window()->GetNativeWindow() is null Hmm. I don't understand why. From my reading of BrowserWindowController and BrowserWindowCocoa, GetNativeWindow() is window(), which returns [controller_ window], which should be the same as passing in the window in -installAvatar.
On 2017/01/30 16:54:10, Robert Sesek wrote: > https://codereview.chromium.org/2636473003/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/profiles/avatar_button_controller.h (right): > > https://codereview.chromium.org/2636473003/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/profiles/avatar_button_controller.h:28: - > (id)initWithBrowser:(Browser*)browser window:(NSWindow*)window; > On 2017/01/27 18:24:49, spqchan wrote: > > On 2017/01/27 11:01:28, Robert Sesek wrote: > > > Is |window| always the same as browser->window()->GetNativeWindow() ? > > > > No, browser->window()->GetNativeWindow() is null > > Hmm. I don't understand why. From my reading of BrowserWindowController and > BrowserWindowCocoa, GetNativeWindow() is window(), which returns [controller_ > window], which should be the same as passing in the window in -installAvatar. Sorry, I should've mentioned that browser->window() is null, not browser->window()->GetNativeWindow(). The reason why is here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.cc?rcl=0&l=441 browser's |window_| object is created via CreateBrowserWindow(). However, AvatarButtonController gets created in the CreateBrowserWindow() call, so if it calls back to browser, |window_| is still null. I'll add a comment for this :)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, thanks for the explanation
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shrike@chromium.org Link to the patchset: https://codereview.chromium.org/2636473003/#ps120001 (title: "Added comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485826448763590,
"parent_rev": "9427bfd430073a9caa0770ab6653b040520a8aab", "commit_rev":
"a47be35337a78a8893d303600f76966e5d4dd225"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] Update the Text Color for the Avatar Button The text color should be determined by the theme's color. If the theme is dark, then the text color should be white. Otherwise, it should be black. BUG=644004 ========== to ========== [Mac] Update the Text Color for the Avatar Button The text color should be determined by the theme's color. If the theme is dark, then the text color should be white. Otherwise, it should be black. BUG=644004 Review-Url: https://codereview.chromium.org/2636473003 Cr-Commit-Position: refs/heads/master@{#447156} Committed: https://chromium.googlesource.com/chromium/src/+/a47be35337a78a8893d303600f76... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/a47be35337a78a8893d303600f76... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
