Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2028933002:
Fix color of looking glass when searching on remote NTP. (Closed)
|
Created:
4 years, 6 months ago by Evan Stade Modified:
4 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix color of looking glass when searching on remote NTP.
BUG=616191
Committed: https://crrev.com/9aba6c9ce7bf118e44acd4158b5597cf9f91fd24
Cr-Commit-Position: refs/heads/master@{#397805}
Patch Set 1 #
Total comments: 2
Patch Set 2 : pkasting's fix #Patch Set 3 : fix typo #
Messages
Total messages: 24 (9 generated)
estade@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2028933002/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2028933002/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:868: GetToolbarModel()->GetSecurityLevel(false); The annoying thing is that toolbar model already takes editing state into account, but doesn't consider "editing or empty". I spent a while looking at how to push this editing or empty check further up (into OmniboxView for example) but I'm wary of what subtle changes might follow on from this as other callsites start behaving slightly differently. Also GetSecurityLevel is almost always called with false as the parameter, there's only one place that true is passed in AFAICT, which is OmniboxEditModel::ClassifyPage(). This seems unfortunate because most places that call this don't really care about editing state one way or another.
https://codereview.chromium.org/2028933002/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2028933002/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:868: GetToolbarModel()->GetSecurityLevel(false); On 2016/06/01 02:05:28, Evan Stade wrote: > The annoying thing is that toolbar model already takes editing state into > account, but doesn't consider "editing or empty". I looked up the history of "editing or empty" (it was added in http://crrev.com/44087 ) and after some more digging, I'm convinced that the toolbar model's GetSecurityState() function should check IsEditingOrEmpty() instead of checking input_in_progress(). This should work correctly for every caller I found and would also fix this issue. > Also GetSecurityLevel is almost always called with false as the parameter, > there's only one place that true is passed in AFAICT, which is > OmniboxEditModel::ClassifyPage(). This seems unfortunate because most places > that call this don't really care about editing state one way or another. The entire idea of caring about editing state is an artifact of search term replacement, which is dead. There's been a bug on file about ripping all that plumbing out for a couple of years now, assigned to groby@, and I now believe she'll never get to it. It would sure make a lot of omnibox and toolbar code much nicer...
On 2016/06/01 03:18:02, Peter Kasting wrote: > https://codereview.chromium.org/2028933002/diff/1/chrome/browser/ui/views/loc... > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > https://codereview.chromium.org/2028933002/diff/1/chrome/browser/ui/views/loc... > chrome/browser/ui/views/location_bar/location_bar_view.cc:868: > GetToolbarModel()->GetSecurityLevel(false); > On 2016/06/01 02:05:28, Evan Stade wrote: > > The annoying thing is that toolbar model already takes editing state into > > account, but doesn't consider "editing or empty". > > I looked up the history of "editing or empty" (it was added in > http://crrev.com/44087 ) and after some more digging, I'm convinced that the > toolbar model's GetSecurityState() function should check IsEditingOrEmpty() > instead of checking input_in_progress(). This should work correctly for every > caller I found and would also fix this issue. The problem being that IsEditingOrEmpty() is part of OmniboxView, not ToolbarModel. I had a patch that changed OmniboxView, which I'll post, but it still requires updating callsites (and still allows other code to call directly into ToolbarModel, therefore being subtly wrong for the empty case. > > > Also GetSecurityLevel is almost always called with false as the parameter, > > there's only one place that true is passed in AFAICT, which is > > OmniboxEditModel::ClassifyPage(). This seems unfortunate because most places > > that call this don't really care about editing state one way or another. > > The entire idea of caring about editing state is an artifact of search term > replacement, which is dead. There's been a bug on file about ripping all that > plumbing out for a couple of years now, assigned to groby@, and I now believe > she'll never get to it. It would sure make a lot of omnibox and toolbar code > much nicer... so can I just delete this line and then rip out all the ignore_editing params? https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox...
On 2016/06/01 21:14:47, Evan Stade wrote: > The problem being that IsEditingOrEmpty() is part of OmniboxView, not > ToolbarModel. I had a patch that changed OmniboxView, which I'll post [...] https://codereview.chromium.org/2031813002/
On 2016/06/01 21:14:47, Evan Stade wrote: > On 2016/06/01 03:18:02, Peter Kasting wrote: > > > https://codereview.chromium.org/2028933002/diff/1/chrome/browser/ui/views/loc... > > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > > > > https://codereview.chromium.org/2028933002/diff/1/chrome/browser/ui/views/loc... > > chrome/browser/ui/views/location_bar/location_bar_view.cc:868: > > GetToolbarModel()->GetSecurityLevel(false); > > On 2016/06/01 02:05:28, Evan Stade wrote: > > > The annoying thing is that toolbar model already takes editing state into > > > account, but doesn't consider "editing or empty". > > > > I looked up the history of "editing or empty" (it was added in > > http://crrev.com/44087 ) and after some more digging, I'm convinced that the > > toolbar model's GetSecurityState() function should check IsEditingOrEmpty() > > instead of checking input_in_progress(). This should work correctly for every > > caller I found and would also fix this issue. > > The problem being that IsEditingOrEmpty() is part of OmniboxView, not > ToolbarModel. I had a patch that changed OmniboxView, which I'll post, but it > still requires updating callsites (and still allows other code to call directly > into ToolbarModel, therefore being subtly wrong for the empty case. Yeah, that patch is scary. Callers can easily call the wrong place and get things subtly wrong. However, I think there's an easy way to implement what I'm thinking of. We don't need to call OmniboxView::IsEditingOrEmpty() from ToolbarModel; the only case when IsEditingOrEmpty() returns true when the user is not editing is when ToolbarModel::ShouldDisplayURL() returns false. So I think adding a call to that function to the existing input_in_progress() call in ToolbarModel::GetSecurityLevel() should do the trick on its own. (I wouldn't guard this under |ignore_editing| either, as it's part of the "underlying" security level and not the editing-specific NONE.) I audited all the other callers of ToolbarModel::input_in_progress() and I don't believe any of them should be changed similarly. I think this is the only spot. > > > Also GetSecurityLevel is almost always called with false as the parameter, > > > there's only one place that true is passed in AFAICT, which is > > > OmniboxEditModel::ClassifyPage(). This seems unfortunate because most places > > > that call this don't really care about editing state one way or another. > > > > The entire idea of caring about editing state is an artifact of search term > > replacement, which is dead. There's been a bug on file about ripping all that > > plumbing out for a couple of years now, assigned to groby@, and I now believe > > she'll never get to it. It would sure make a lot of omnibox and toolbar code > > much nicer... > > so can I just delete this line and then rip out all the ignore_editing params? I wouldn't do that, mostly because I don't want to leave the tree with this stuff mostly in place but not completely. If for some reason we were to try to reactivate any of it before ripping it all out we might think it would still work. I'd love to see all of the dead 1993 stuff torn out at once though. It would be a bit of a job to do it right for sure.
done.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028933002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM!
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028933002/40001
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028933002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028933002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix color of looking glass when searching on remote NTP. BUG=616191 ========== to ========== Fix color of looking glass when searching on remote NTP. BUG=616191 Committed: https://crrev.com/9aba6c9ce7bf118e44acd4158b5597cf9f91fd24 Cr-Commit-Position: refs/heads/master@{#397805} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9aba6c9ce7bf118e44acd4158b5597cf9f91fd24 Cr-Commit-Position: refs/heads/master@{#397805} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
