Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1938243002:
[Mac][Material Design] Correct location bar editing text colors. (Closed)
Description[Mac][Material Design] Correct location bar editing text colors.
Currently the Omnibox formats the location bar's text using its normal
formatting rules, which means that if you type a URL the URL will be
white, but if what you're typing doesn't look like a URL it will be
black (which is probably the Omnibox defaulting to not knowing what
to do color-wise). This cl forces the location bar's text to be white
or black while editing in normal or Incognito mode, respectively, per
the Material Design spec.
This cl also cleans up some style mistakes I made in an earlier cl.
R=tapted@chromium.org
BUG=608437
Committed: https://crrev.com/c850fd0f9bd765aa5cd9fffd286cdb9f0cd9d2b4
Cr-Commit-Position: refs/heads/master@{#391263}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Check for empty string at the start. #Patch Set 3 : Rebase from ToT. #
Messages
Total messages: 18 (9 generated)
Description was changed from ========== [Mac][Material Design] Correct location bar editing text colors. Currently the Omnibox formats the location bar's text using its normal formatting rules, which means that if you type a URL the URL will be white, but if what you're typing doesn't look like a URL it will be black (which is probably the Omnibox defaulting to not knowing what to do color-wise). This cl forces the location bar's text to be white or black while editing in normal or Incognito mode, respectively, per the Material Design spec. R=tapted@chromium.org BUG=608437 ========== to ========== [Mac][Material Design] Correct location bar editing text colors. Currently the Omnibox formats the location bar's text using its normal formatting rules, which means that if you type a URL the URL will be white, but if what you're typing doesn't look like a URL it will be black (which is probably the Omnibox defaulting to not knowing what to do color-wise). This cl forces the location bar's text to be white or black while editing in normal or Incognito mode, respectively, per the Material Design spec. This cl also cleans up some style mistakes I made in an earlier cl. R=tapted@chromium.org BUG=608437 ==========
Description was changed from ========== [Mac][Material Design] Correct location bar editing text colors. Currently the Omnibox formats the location bar's text using its normal formatting rules, which means that if you type a URL the URL will be white, but if what you're typing doesn't look like a URL it will be black (which is probably the Omnibox defaulting to not knowing what to do color-wise). This cl forces the location bar's text to be white or black while editing in normal or Incognito mode, respectively, per the Material Design spec. This cl also cleans up some style mistakes I made in an earlier cl. R=tapted@chromium.org BUG=608437 ========== to ========== [Mac][Material Design] Correct location bar editing text colors. Currently the Omnibox formats the location bar's text using its normal formatting rules, which means that if you type a URL the URL will be white, but if what you're typing doesn't look like a URL it will be black (which is probably the Omnibox defaulting to not knowing what to do color-wise). This cl forces the location bar's text to be white or black while editing in normal or Incognito mode, respectively, per the Material Design spec. This cl also cleans up some style mistakes I made in an earlier cl. R=tapted@chromium.org BUG=608437 ==========
PTAL
lgtm % nit https://codereview.chromium.org/1938243002/diff/1/chrome/browser/ui/cocoa/omn... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/1938243002/diff/1/chrome/browser/ui/cocoa/omn... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:561: if (as_length) { nit: other places in this method don't check for an empty string - can this extra check be omitted? (Otherwise, I think it should be at the start of the method, like NSUInteger as_length = [attributedString length]; if (as_length == 0) return; )
PTAL https://codereview.chromium.org/1938243002/diff/1/chrome/browser/ui/cocoa/omn... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/1938243002/diff/1/chrome/browser/ui/cocoa/omn... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:561: if (as_length) { On 2016/05/02 23:49:03, tapted wrote: > nit: other places in this method don't check for an empty string - can this > extra check be omitted? > > (Otherwise, I think it should be at the start of the method, like > > NSUInteger as_length = [attributedString length]; > if (as_length == 0) > return; > > ) The other blocks of code do check for an empty string, just not explicitly. The problem is you don't want to addAttribute: on an empty attribute string, otherwise you'll crash. I moved the check to the beginning.
Actually I realize gave me a lgtm with nits so I will go ahead and land it.
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1938243002/#ps20001 (title: "Check for empty string at the start.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1938243002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1938243002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1938243002/#ps40001 (title: "Rebase from ToT.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1938243002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1938243002/40001
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Correct location bar editing text colors. Currently the Omnibox formats the location bar's text using its normal formatting rules, which means that if you type a URL the URL will be white, but if what you're typing doesn't look like a URL it will be black (which is probably the Omnibox defaulting to not knowing what to do color-wise). This cl forces the location bar's text to be white or black while editing in normal or Incognito mode, respectively, per the Material Design spec. This cl also cleans up some style mistakes I made in an earlier cl. R=tapted@chromium.org BUG=608437 ========== to ========== [Mac][Material Design] Correct location bar editing text colors. Currently the Omnibox formats the location bar's text using its normal formatting rules, which means that if you type a URL the URL will be white, but if what you're typing doesn't look like a URL it will be black (which is probably the Omnibox defaulting to not knowing what to do color-wise). This cl forces the location bar's text to be white or black while editing in normal or Incognito mode, respectively, per the Material Design spec. This cl also cleans up some style mistakes I made in an earlier cl. R=tapted@chromium.org BUG=608437 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Correct location bar editing text colors. Currently the Omnibox formats the location bar's text using its normal formatting rules, which means that if you type a URL the URL will be white, but if what you're typing doesn't look like a URL it will be black (which is probably the Omnibox defaulting to not knowing what to do color-wise). This cl forces the location bar's text to be white or black while editing in normal or Incognito mode, respectively, per the Material Design spec. This cl also cleans up some style mistakes I made in an earlier cl. R=tapted@chromium.org BUG=608437 ========== to ========== [Mac][Material Design] Correct location bar editing text colors. Currently the Omnibox formats the location bar's text using its normal formatting rules, which means that if you type a URL the URL will be white, but if what you're typing doesn't look like a URL it will be black (which is probably the Omnibox defaulting to not knowing what to do color-wise). This cl forces the location bar's text to be white or black while editing in normal or Incognito mode, respectively, per the Material Design spec. This cl also cleans up some style mistakes I made in an earlier cl. R=tapted@chromium.org BUG=608437 Committed: https://crrev.com/c850fd0f9bd765aa5cd9fffd286cdb9f0cd9d2b4 Cr-Commit-Position: refs/heads/master@{#391263} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c850fd0f9bd765aa5cd9fffd286cdb9f0cd9d2b4 Cr-Commit-Position: refs/heads/master@{#391263} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
