|
|
Created:
4 years ago by jiahuiguo Modified:
3 years, 11 months ago CC:
Jared Saul, chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace hard-coded colors from ui::NativeTheme for consistency
and accessibility, and refactor to let views, not controllers
in control of presentation details, while the controller returns
a color id instead of a color.
BUG=666523
TEST=Smoke Test
Review-Url: https://codereview.chromium.org/2581513002
Cr-Commit-Position: refs/heads/master@{#442384}
Committed: https://chromium.googlesource.com/chromium/src/+/d9f57d21417b53ef91e922a63204d950010d0251
Patch Set 1 #
Total comments: 8
Patch Set 2 : Refactor callsites for colors #Patch Set 3 : Delete unused popup_constants.h includes #
Total comments: 14
Patch Set 4 : Tweak docs, add TODOs and modify OSX function call #Patch Set 5 : Using NativeThemeMac::instance() in OSX platform #Patch Set 6 : Using NativeTheme::GetInstanceForNativeUi() to get NativeThemeMac instance #
Total comments: 6
Patch Set 7 : Format comments and TODOs, and fix typos #Patch Set 8 : Resolved conflicts during patching #Messages
Total messages: 77 (60 generated)
Description was changed from ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility. BUG=666523 TEST=Smoke Test, see the attached screenshot for chrome on linux and android (version 6.0) ========== to ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility. The color differs from the previously coded one though. BUG=666523 TEST=Smoke Test, see the screenshot (linux) Pre: https://screenshot.googleplex.com/0eGvqWhGXG5 After: https://screenshot.googleplex.com/kETdQ7WEoZD ==========
jiahuiguo@google.com changed reviewers: + estade@chromium.org, lshang@chromium.org, mathp@chromium.org
jiahuiguo@google.com changed required reviewers: + estade@chromium.org, mathp@chromium.org
The CQ bit was checked by jiahuiguo@google.com 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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by jiahuiguo@google.com 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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by jiahuiguo@google.com 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.
Hi Jason, I'm thinking we should align with the omnibox here which uses kColorId_ResultsTable*, what do you think? https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/popup_constants.h:25: ui::NativeTheme::kColorId_TextfieldReadOnlyColor); Should we use kColorId_ResultsTableNormalDimmedText? https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/popup_constants.h:26: constexpr SkColor kPopupBackground = SK_ColorWHITE; Should we use kColorId_ResultsTableNormalBackground? https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/popup_constants.h:27: constexpr SkColor kValueTextColor = SK_ColorBLACK; Should we use kColorId_ResultsTableNormalText?
mathp@chromium.org changed reviewers: - lshang@chromium.org
https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/popup_constants.h:21: ui::NativeTheme::GetInstanceForNativeUi()->GetSystemColor( you shouldn't really be calling this here as it won't always return the correct native theme, and besides that you're creating new static initializers. The way this code is structured makes this harder to do right (i.e. views, not controllers, should be in control of presentation details like the exact color used to represent hovering). You'll need to change callsites so that the controller returns a color id instead of a color, and the view can do GetNativeTheme()->GetSystemColor(returned_id);
The CQ bit was checked by jiahuiguo@google.com 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 checked by jiahuiguo@google.com 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.
Description was changed from ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility. The color differs from the previously coded one though. BUG=666523 TEST=Smoke Test, see the screenshot (linux) Pre: https://screenshot.googleplex.com/0eGvqWhGXG5 After: https://screenshot.googleplex.com/kETdQ7WEoZD ========== to ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility. The color differs from the previously coded one though. BUG=666523 TEST=Smoke Test ==========
Hi Mathieu and Evan, Please review a changelist from jiahuiguo. https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/popup_constants.h:21: ui::NativeTheme::GetInstanceForNativeUi()->GetSystemColor( On 2016/12/15 17:42:14, Evan Stade wrote: > you shouldn't really be calling this here as it won't always return the correct > native theme, and besides that you're creating new static initializers. > > The way this code is structured makes this harder to do right (i.e. views, not > controllers, should be in control of presentation details like the exact color > used to represent hovering). You'll need to change callsites so that the > controller returns a color id instead of a color, and the view can do > GetNativeTheme()->GetSystemColor(returned_id); Done. https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/popup_constants.h:25: ui::NativeTheme::kColorId_TextfieldReadOnlyColor); On 2016/12/15 17:37:06, Mathieu (OOO Dec 21-Jan 4) wrote: > Should we use kColorId_ResultsTableNormalDimmedText? Done. https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/popup_constants.h:26: constexpr SkColor kPopupBackground = SK_ColorWHITE; On 2016/12/15 17:37:06, Mathieu (OOO Dec 21-Jan 4) wrote: > Should we use kColorId_ResultsTableNormalBackground? Done. https://codereview.chromium.org/2581513002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/popup_constants.h:27: constexpr SkColor kValueTextColor = SK_ColorBLACK; On 2016/12/15 17:37:06, Mathieu (OOO Dec 21-Jan 4) wrote: > Should we use kColorId_ResultsTableNormalText? Done.
this is great. Finally a second client for the ResultsTable colors!! lgtm with minor nits https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.h:78: // Returns the value font color_id of the row item according to its |index|. nit: s/color_id/color ID https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... chrome/browser/ui/autofill/popup_constants.h:10: // The size of the border around the entire results popup, in pixels. looks like this is actually in dip, not pixels, i.e. for a retina display (--force-device-scale-factor=2) it would be 2px. That said, it probably /should/ be 1 pixel as most strokes these days are 1px not 1 dip. tl;dr: please update the docs to match reality (dip) and consider changing reality to 1px in the future. (see location_bar/background_with_1px_border for an example)
Thanks, lgtm with nits https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:67: ui::NativeTheme::kColorId_UnfocusedBorderColor))); Is there a possibility to have a code path where we are using kColorId_FocusedBorderColor when hovered? Feel free to add as TODO(crbug.com/xxxxxx) https://cs.chromium.org/chromium/src/ui/native_theme/native_theme.h?rcl=0&l=260 https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:181: GetNativeTheme()->GetSystemColor( Should we have a TODO to have a GetLabelFontColorForRow similar to GetValueFontColorForRow [1] so that the cocoa impl [2] could use it too? [1] https://cs.chromium.org/search/?q=GetValueFontColorForRow&sq=package:chromium... [2] https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofil...
The CQ bit was checked by jiahuiguo@google.com 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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jiahuiguo@google.com 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: 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 jiahuiguo@google.com 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.
Hi Mathieu and Evan, Thanks for the comments, PTAL. https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.h:78: // Returns the value font color_id of the row item according to its |index|. On 2016/12/20 18:01:19, Evan Stade wrote: > nit: s/color_id/color ID Done. https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... chrome/browser/ui/autofill/popup_constants.h:10: // The size of the border around the entire results popup, in pixels. On 2016/12/20 18:01:19, Evan Stade wrote: > looks like this is actually in dip, not pixels, i.e. for a retina display > (--force-device-scale-factor=2) it would be 2px. That said, it probably /should/ > be 1 pixel as most strokes these days are 1px not 1 dip. > > tl;dr: please update the docs to match reality (dip) and consider changing > reality to 1px in the future. (see location_bar/background_with_1px_border for > an example) Changed the doc and filed a bug for changing to 1px here: https://crbug.com/676221 https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:67: ui::NativeTheme::kColorId_UnfocusedBorderColor))); On 2016/12/20 20:36:40, Mathieu (OOO Dec 21-Jan 4) wrote: > Is there a possibility to have a code path where we are using > kColorId_FocusedBorderColor when hovered? Feel free to add as > TODO(crbug.com/xxxxxx) > > https://cs.chromium.org/chromium/src/ui/native_theme/native_theme.h?rcl=0&l=260 Done. Added in https://crbug.com/676164 https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:181: GetNativeTheme()->GetSystemColor( On 2016/12/20 20:36:40, Mathieu (OOO Dec 21-Jan 4) wrote: > Should we have a TODO to have a GetLabelFontColorForRow similar to > GetValueFontColorForRow [1] so that the cocoa impl [2] could use it too? > > [1] > https://cs.chromium.org/search/?q=GetValueFontColorForRow&sq=package:chromium... > > [2] > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofil... Done, added TODO in https://crbug.com/678033
lgtm with nits to reference the bugs in the code. Feel free to submit when that is done, thanks! https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... chrome/browser/ui/autofill/popup_constants.h:10: // The size of the border around the entire results popup, in pixels. On 2017/01/03 19:37:52, jiahuiguo wrote: > On 2016/12/20 18:01:19, Evan Stade wrote: > > looks like this is actually in dip, not pixels, i.e. for a retina display > > (--force-device-scale-factor=2) it would be 2px. That said, it probably > /should/ > > be 1 pixel as most strokes these days are 1px not 1 dip. > > > > tl;dr: please update the docs to match reality (dip) and consider changing > > reality to 1px in the future. (see location_bar/background_with_1px_border for > > an example) > > Changed the doc and filed a bug for changing to 1px here: > https://crbug.com/676221 Can you reference the bug here like so: TODO(crbug.com/676221): Change this to pixels. https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:67: ui::NativeTheme::kColorId_UnfocusedBorderColor))); On 2017/01/03 19:37:52, jiahuiguo wrote: > On 2016/12/20 20:36:40, Mathieu (OOO Dec 21-Jan 4) wrote: > > Is there a possibility to have a code path where we are using > > kColorId_FocusedBorderColor when hovered? Feel free to add as > > TODO(crbug.com/xxxxxx) > > > > > https://cs.chromium.org/chromium/src/ui/native_theme/native_theme.h?rcl=0&l=260 > > Done. Added in https://crbug.com/676164 Please reference here: // TODO(crbug.com/676164): ... https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:181: GetNativeTheme()->GetSystemColor( On 2017/01/03 19:37:52, jiahuiguo wrote: > On 2016/12/20 20:36:40, Mathieu (OOO Dec 21-Jan 4) wrote: > > Should we have a TODO to have a GetLabelFontColorForRow similar to > > GetValueFontColorForRow [1] so that the cocoa impl [2] could use it too? > > > > [1] > > > https://cs.chromium.org/search/?q=GetValueFontColorForRow&sq=package:chromium... > > > > [2] > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofil... > > Done, added TODO in https://crbug.com/678033 Please reference in code as // TODO(crbug.com/678033): ...
Just some nits https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller.h:51: // or default popup background should be used. s/should be used/otherwise https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:81: // TODO(jiahuiguo) Add a function to return label fonr color ID for cocoa impl s/fonr/font https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/popup_constants.h:10: // TODO(crbug.com/676221): Changing the border size from dip to px s/Changing/Change
The CQ bit was checked by jiahuiguo@google.com 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.
The CQ bit was checked by jiahuiguo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2581513002/#ps120001 (title: "Format comments and TODOs, and fix typos")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jiahuiguo@google.com changed reviewers: + groby@chromium.org
jiahuiguo@google.com changed required reviewers: + groby@chromium.org
Hi Rachel, Can you help review the change in file chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm? Thanks, https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/autof... chrome/browser/ui/autofill/popup_constants.h:10: // The size of the border around the entire results popup, in pixels. On 2017/01/04 13:51:48, Mathieu Perreault wrote: > On 2017/01/03 19:37:52, jiahuiguo wrote: > > On 2016/12/20 18:01:19, Evan Stade wrote: > > > looks like this is actually in dip, not pixels, i.e. for a retina display > > > (--force-device-scale-factor=2) it would be 2px. That said, it probably > > /should/ > > > be 1 pixel as most strokes these days are 1px not 1 dip. > > > > > > tl;dr: please update the docs to match reality (dip) and consider changing > > > reality to 1px in the future. (see location_bar/background_with_1px_border > for > > > an example) > > > > Changed the doc and filed a bug for changing to 1px here: > > https://crbug.com/676221 > > Can you reference the bug here like so: > > TODO(crbug.com/676221): Change this to pixels. Done. https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:67: ui::NativeTheme::kColorId_UnfocusedBorderColor))); On 2016/12/20 20:36:40, Mathieu Perreault wrote: > Is there a possibility to have a code path where we are using > kColorId_FocusedBorderColor when hovered? Feel free to add as > TODO(crbug.com/xxxxxx) > > https://cs.chromium.org/chromium/src/ui/native_theme/native_theme.h?rcl=0&l=260 Done. https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2581513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:181: GetNativeTheme()->GetSystemColor( On 2017/01/04 13:51:48, Mathieu Perreault wrote: > On 2017/01/03 19:37:52, jiahuiguo wrote: > > On 2016/12/20 20:36:40, Mathieu (OOO Dec 21-Jan 4) wrote: > > > Should we have a TODO to have a GetLabelFontColorForRow similar to > > > GetValueFontColorForRow [1] so that the cocoa impl [2] could use it too? > > > > > > [1] > > > > > > https://cs.chromium.org/search/?q=GetValueFontColorForRow&sq=package:chromium... > > > > > > [2] > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofil... > > > > Done, added TODO in https://crbug.com/678033 > > Please reference in code as // TODO(crbug.com/678033): ... Done. https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller.h:51: // or default popup background should be used. On 2017/01/04 19:51:38, jsaul wrote: > s/should be used/otherwise Done. https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:81: // TODO(jiahuiguo) Add a function to return label fonr color ID for cocoa impl On 2017/01/04 19:51:38, jsaul wrote: > s/fonr/font Done. https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2581513002/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/popup_constants.h:10: // TODO(crbug.com/676221): Changing the border size from dip to px On 2017/01/04 19:51:38, jsaul wrote: > s/Changing/Change Done.
mathp@chromium.org changed reviewers: + rsesek@chromium.org - groby@chromium.org
mathp@chromium.org changed required reviewers: - groby@chromium.org
+Robert for Mac
lgtm
The CQ bit was checked by mathp@chromium.org
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jiahuiguo@google.com 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.
Description was changed from ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility. The color differs from the previously coded one though. BUG=666523 TEST=Smoke Test ========== to ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility, and refactor to let views, not controllers in control of presentation details, while the controller returns a color id instead of a color. BUG=666523 TEST=Smoke Test ==========
Description was changed from ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility, and refactor to let views, not controllers in control of presentation details, while the controller returns a color id instead of a color. BUG=666523 TEST=Smoke Test ========== to ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility, and refactor to let views, not controllers in control of presentation details, while the controller returns a color id instead of a color. BUG=666523 TEST=Smoke Test ==========
Description was changed from ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility, and refactor to let views, not controllers in control of presentation details, while the controller returns a color id instead of a color. BUG=666523 TEST=Smoke Test ========== to ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility, and refactor to let views, not controllers in control of presentation details, while the controller returns a color id instead of a color. BUG=666523 TEST=Smoke Test ==========
Description was changed from ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility, and refactor to let views, not controllers in control of presentation details, while the controller returns a color id instead of a color. BUG=666523 TEST=Smoke Test ========== to ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility, and refactor to let views, not controllers in control of presentation details, while the controller returns a color id instead of a color. BUG=666523 TEST=Smoke Test ==========
The CQ bit was checked by jiahuiguo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rsesek@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2581513002/#ps140001 (title: "Resolved conflicts during patching")
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": 140001, "attempt_start_ts": 1484001291790640, "parent_rev": "dc55ca6a8ab2daee62837eba062cdba8bae4d4da", "commit_rev": "d9f57d21417b53ef91e922a63204d950010d0251"}
Message was sent while issue was closed.
Description was changed from ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility, and refactor to let views, not controllers in control of presentation details, while the controller returns a color id instead of a color. BUG=666523 TEST=Smoke Test ========== to ========== Replace hard-coded colors from ui::NativeTheme for consistency and accessibility, and refactor to let views, not controllers in control of presentation details, while the controller returns a color id instead of a color. BUG=666523 TEST=Smoke Test Review-Url: https://codereview.chromium.org/2581513002 Cr-Commit-Position: refs/heads/master@{#442384} Committed: https://chromium.googlesource.com/chromium/src/+/d9f57d21417b53ef91e922a63204... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/d9f57d21417b53ef91e922a63204... |