Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
229583002:
Omnibox Logging Clarifications (Closed)
|
Created:
6 years, 8 months ago by Mark P Modified:
6 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOmnibox Logging Clarifications
In OmniboxEditModel::OpenMatch, the difference between
popup_model->selected_index() and index is confusing.
This change removes references to the former from
within that function.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262824
Patch Set 1 #
Total comments: 4
Patch Set 2 : peter's comments #
Total comments: 3
Patch Set 3 : remove popup_model() check #Patch Set 4 : remove extra if (popup() test #Messages
Total messages: 13 (0 generated)
ptal
https://codereview.chromium.org/229583002/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left): https://codereview.chromium.org/229583002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_edit_model.cc:676: Nit: Don't remove this blank line https://codereview.chromium.org/229583002/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/229583002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_edit_model.cc:678: popup_model() && popup_model()->IsOpen() ? It doesn't seem like we need to check this. If the popup isn't open, the value we pass will be ignored.
https://codereview.chromium.org/229583002/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left): https://codereview.chromium.org/229583002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_edit_model.cc:676: On 2014/04/08 22:52:56, Peter Kasting wrote: > Nit: Don't remove this blank line Done. https://codereview.chromium.org/229583002/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/229583002/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_edit_model.cc:678: popup_model() && popup_model()->IsOpen() ? On 2014/04/08 22:52:56, Peter Kasting wrote: > It doesn't seem like we need to check this. If the popup isn't open, the value > we pass will be ignored. Done.
LGTM https://codereview.chromium.org/229583002/diff/20001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/229583002/diff/20001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_edit_model.cc:679: popup_model() ? Can we really have no popup model here? Maybe in some kind of unittest?
https://codereview.chromium.org/229583002/diff/20001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/229583002/diff/20001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_edit_model.cc:679: popup_model() ? On 2014/04/08 23:02:20, Peter Kasting wrote: > Can we really have no popup model here? Maybe in some kind of unittest? I didn't actually try skipping this test. Do you want me to investigate what happens?
On 2014/04/08 23:05:01, Mark P wrote: > https://codereview.chromium.org/229583002/diff/20001/chrome/browser/ui/omnibo... > File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): > > https://codereview.chromium.org/229583002/diff/20001/chrome/browser/ui/omnibo... > chrome/browser/ui/omnibox/omnibox_edit_model.cc:679: popup_model() ? > On 2014/04/08 23:02:20, Peter Kasting wrote: > > Can we really have no popup model here? Maybe in some kind of unittest? > > I didn't actually try skipping this test. Do you want me to investigate what > happens? It would be nice not to have the NULL-check if we don't need it.
https://codereview.chromium.org/229583002/diff/20001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/229583002/diff/20001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_edit_model.cc:679: popup_model() ? On 2014/04/08 23:05:01, Mark P wrote: > On 2014/04/08 23:02:20, Peter Kasting wrote: > > Can we really have no popup model here? Maybe in some kind of unittest? > > I didn't actually try skipping this test. Do you want me to investigate what > happens? Done. Running it through the try servers now. (It didn't cause crashes on my workstation when used interactively.)
I don't think this is a problem, but can you confirm that you too don't think this will cause problems with hide verbatim mode? --mark
On 2014/04/09 03:51:10, Mark P wrote: > I don't think this is a problem, but can you confirm that you too don't think > this will cause problems with hide verbatim mode? IIRC, in hide verbatim mode, we still have a selected entry in the popup, it's just not displayed. If that's true, then this is fine. Incidentally, it seems a little weird that we have code in this patch that does DCHECK(popup_model()), and then in the function directly below that, "if (popup_model() && ...". I dunno, maybe it has to be that way.
On 2014/04/09 05:39:07, Peter Kasting wrote: > On 2014/04/09 03:51:10, Mark P wrote: > > I don't think this is a problem, but can you confirm that you too don't think > > this will cause problems with hide verbatim mode? > > IIRC, in hide verbatim mode, we still have a selected entry in the popup, it's > just not displayed. If that's true, then this is fine. That's my understanding too. > Incidentally, it seems a little weird that we have code in this patch that does > DCHECK(popup_model()), and then in the function directly below that, "if > (popup_model() && ...". I dunno, maybe it has to be that way. I'll try to remove it. CQing now. --mark
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/229583002/60001
Message was sent while issue was closed.
Change committed as 262824 |
