Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20587003:
InstantExtended: record initial focus state for omnibox interactions. (Closed)
|
Created:
7 years, 5 months ago by samarth Modified:
7 years, 4 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInstantExtended: record initial focus state for omnibox interactions.
Keep track of whether omnibox was visibly or invisibly focused when user first
started typing and record that state in OmniboxLog.
BUG=264069
R=isherman@chromium.org, mpearson@chromium.org, pkasting@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216067
Patch Set 1 #
Total comments: 9
Patch Set 2 : Dont reset in Revert. #Patch Set 3 : Address comments. #Patch Set 4 : Rebase. #Patch Set 5 : Separate enum. #Patch Set 6 : Fix comments. #
Total comments: 18
Patch Set 7 : Rebase. #Patch Set 8 : Address comment. #Patch Set 9 : Add parens. #Patch Set 10 : Remove obsolete field. #Patch Set 11 : Remove field. #Patch Set 12 : #
Messages
Total messages: 38 (0 generated)
Need to test this out a bit more but I think this works. Thanks, Samarth
This looks reasonable to me. I notice there are other calls to SetFocus() besides RestoreState(). I wonder if some of those paths to SetFocus() don't end up calling OnAfterPossibleChange() along the way. --mark
+pkasting for OWNERS Played with this some more and it works as far as I can tell. On 2013/07/26 01:04:18, Mark P wrote: > This looks reasonable to me. I notice there are other calls to SetFocus() > besides RestoreState(). I wonder if some of those paths to SetFocus() don't end > up calling OnAfterPossibleChange() along the way. I think that's fine. Ultimately if the user typed something, it will go through OnAfterPossibleChange at least once. The line in RestoreState is necessary to make sure we handle tab switching correctly. Thanks, Samarth
I'll lgtm this, partially so it gets the metrics OWNERS stamp, but please wait for Peter's review. --mark
https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.cc:453: focus_state_for_input_ = OMNIBOX_FOCUS_NONE; Is this right? What if the user reverts everything via escape, and then just hits enter alone to reload the current page URL? Won't this still be OMNIBOX_FOCUS_NONE when we call PageClassification() below?
https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.cc:453: focus_state_for_input_ = OMNIBOX_FOCUS_NONE; On 2013/07/26 22:58:09, Peter Kasting wrote: > Is this right? What if the user reverts everything via escape, and then just > hits enter alone to reload the current page URL? Won't this still be > OMNIBOX_FOCUS_NONE when we call PageClassification() below? That works today because PageClassifcation is only called from AcceptMatch, which is not triggered in this flow. I assume we will soon also call PageClassification before the autocomplete system is kicked off, but that will also require that the user have changed something, right?
https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.cc:453: focus_state_for_input_ = OMNIBOX_FOCUS_NONE; On 2013/07/26 23:00:55, samarth wrote: > On 2013/07/26 22:58:09, Peter Kasting wrote: > > Is this right? What if the user reverts everything via escape, and then just > > hits enter alone to reload the current page URL? Won't this still be > > OMNIBOX_FOCUS_NONE when we call PageClassification() below? > > That works today because PageClassifcation is only called from AcceptMatch, > which is not triggered in this flow. > > I assume we will soon also call PageClassification before the autocomplete > system is kicked off, but that will also require that the user have changed > something, right? Ok, Peter convinced me that not keeping this here is the correct behavior. Specifically, if the user: 1. Focuses in the fakebox 2. Starts typing 3. Hits escape 4. Types some more and hits enter We will record it as a FAKEBOX interaction (not as OMNIBOX as I was doing before).
Comments for both of you below. https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.cc:453: focus_state_for_input_ = OMNIBOX_FOCUS_NONE; On 2013/07/26 23:00:55, samarth wrote: > On 2013/07/26 22:58:09, Peter Kasting wrote: > > Is this right? What if the user reverts everything via escape, and then just > > hits enter alone to reload the current page URL? Won't this still be > > OMNIBOX_FOCUS_NONE when we call PageClassification() below? > > That works today because PageClassifcation is only called from AcceptMatch, > which is not triggered in this flow. Turns out you mean OpenMatch(), which is called, but we only call ClassifyPage() in the "popup is open" branch. Samarth and I chatted and he's going to remove the line here, which will make the focus state persistent through reverts as long as the omnibox retains focus -- which I think is correct. Mark, I think you may want to rethink the way that the logging in OpenMatch() only happens when the popup is open. This means that for example if you reload the page by hitting enter, or switch tabs and then switch back and hit enter on your previous edits, we don't log anything. That may bias your stats. https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1279: } else if (focus_state_for_input_ == OMNIBOX_FOCUS_INVISIBLE) { Nit: No else after return; also, per Chromium style guide, don't handle DCHECK failure by having an "else if" here and a NOTREACHED()/return below. Just do this after the DCHECK: return (focus_state_for_input_ == OMNIBOX_FOCUS_INVISIBLE) ? OmniboxEventProto::INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS : OmniboxEventProto::INSTANT_NEW_TAB_PAGE_WITH_OMNIBOX_AS_STARTING_FOCUS; (This also requires somewhere above it: using metrics::OmniboxEventProto; ...which seems like an OK way to save some lines elsewhere anyway.) https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.h:419: OmniboxFocusState focus_state_for_input_; The name of this seems unclear, and the type might need changing too. Seems like the name should be something more like "initial_focus_source_", and we want something more like: enum FocusSource { INVALID, OMNIBOX, FAKEBOX, }; I suppose we could continue to use OmniboxFocusState for this, but it seems less clear -- it would at least need commentary. Or perhaps we should just change OmniboxFocusState to FocusedOmniboxField or some equivalent, with the values I list above, which would probably still be equivalently clear everywhere else (didn't look to see).
https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.cc:453: focus_state_for_input_ = OMNIBOX_FOCUS_NONE; On 2013/07/26 23:22:34, Peter Kasting wrote: > On 2013/07/26 23:00:55, samarth wrote: > > On 2013/07/26 22:58:09, Peter Kasting wrote: > > > Is this right? What if the user reverts everything via escape, and then > just > > > hits enter alone to reload the current page URL? Won't this still be > > > OMNIBOX_FOCUS_NONE when we call PageClassification() below? > > > > That works today because PageClassifcation is only called from AcceptMatch, > > which is not triggered in this flow. > > Turns out you mean OpenMatch(), which is called, but we only call ClassifyPage() > in the "popup is open" branch. > > Samarth and I chatted and he's going to remove the line here, which will make > the focus state persistent through reverts as long as the omnibox retains focus > -- which I think is correct. Okay. That sounds fine. > Mark, I think you may want to rethink the way that the logging in OpenMatch() > only happens when the popup is open. This means that for example if you reload > the page by hitting enter, or switch tabs and then switch back and hit enter on > your previous edits, we don't log anything. That may bias your stats. Good point. I hadn't thought about in 1.5 years; it's time to think again. Filed bug https://code.google.com/p/chromium/issues/detail?id=264992
Thanks, Samarth https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1279: } else if (focus_state_for_input_ == OMNIBOX_FOCUS_INVISIBLE) { On 2013/07/26 23:22:34, Peter Kasting wrote: > Nit: No else after return; also, per Chromium style guide, don't handle DCHECK > failure by having an "else if" here and a NOTREACHED()/return below. Just do > this after the DCHECK: > > return (focus_state_for_input_ == OMNIBOX_FOCUS_INVISIBLE) ? > OmniboxEventProto::INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS : > OmniboxEventProto::INSTANT_NEW_TAB_PAGE_WITH_OMNIBOX_AS_STARTING_FOCUS; > > (This also requires somewhere above it: > > using metrics::OmniboxEventProto; > > ...which seems like an OK way to save some lines elsewhere anyway.) Done. https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): https://codereview.chromium.org/20587003/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.h:419: OmniboxFocusState focus_state_for_input_; On 2013/07/26 23:22:34, Peter Kasting wrote: > The name of this seems unclear, and the type might need changing too. > > Seems like the name should be something more like "initial_focus_source_", and > we want something more like: > > enum FocusSource { > INVALID, > OMNIBOX, > FAKEBOX, > }; > > I suppose we could continue to use OmniboxFocusState for this, but it seems less > clear -- it would at least need commentary. Or perhaps we should just change > OmniboxFocusState to FocusedOmniboxField or some equivalent, with the values I > list above, which would probably still be equivalently clear everywhere else > (didn't look to see). So when I initially wrote the code around invisible focus, I consciously tried to keep the precise notion of a "fakebox" out of the omnibox (and the browser code in general), and everything here talks about the visibility of the cursor. So my options are: (1) Keep this is as-is and just add some more comments. (2) Change the metrics enum to also say visible/invisible focus. (3) Change the omnibox code to have FocusedOmniboxField or something. I have a slight preference for (2). What about you?
On 2013/07/29 18:55:35, samarth wrote: > So when I initially wrote the code around invisible focus, I consciously tried > to keep the precise notion of a "fakebox" out of the omnibox (and the browser > code in general), and everything here talks about the visibility of the cursor. I am fine with discarding that concept separation. In fact, I'd prefer if the omnibox code explicitly understood and talked about the idea of a fakebox. This helps motivate what's going on in the code much more clearly to a reader, and makes it more likely that if we remove the idea of the fakebox we'll actually see and update all the omnibox code that's affected. > So my options are: > (1) Keep this is as-is and just add some more comments. > (2) Change the metrics enum to also say visible/invisible focus. > (3) Change the omnibox code to have FocusedOmniboxField or something. > > I have a slight preference for (2). What about you? If (3) is your restatement of my previous suggestion, I prefer that. But I don't like the actual term "FocusedOmniboxField" as I don't think of the omnibox as a single entity with two "fields" :).
Two FYI comments: I'm fine with changing the name of the proposed new PageClassification enums if that's what you want. When you next sync, you'll have to resolve against this recently-submitted change: https://codereview.chromium.org/20747002/ This is straightforward. What this means is you'll have to add the new enum entries in two separate places (protocol buffer as you have an in a separate declaration in autocomplete_input.h) and also the elements to the function AsOmniboxEventPageClassification() in metrics_log.cc. --mark
+isherman for metrics On 2013/07/29 19:15:47, Peter Kasting wrote: > If (3) is your restatement of my previous suggestion, I prefer that. But I > don't like the actual term "FocusedOmniboxField" as I don't think of the omnibox > as a single entity with two "fields" :). Yes, I don't like that either. In fact, I tried to combine the two enums but it doesn't really work because it really does make more sense to me for the browser-code to be talking about whether the focus in the omnibox is visible or not (rather than what source it came from, etc.). But I did add a separate enum for focus_source_ to make that field clearer. Mark: after syncing, I noticed that the page classification function is called more frequently, and in particular before user input actually starts. So I changed that code to return OMNIBOX in those cases. Let me know if that's OK. Thanks, Samarth
On Fri, Aug 2, 2013 at 10:24 AM, <samarth@chromium.org> wrote: > > Mark: after syncing, I noticed that the page classification function is > called > more frequently, and in particular before user input actually starts. So I > changed that code to return OMNIBOX in those cases. Let me know if that's > OK. > That fine. It's getting called because the omnibox calls it whenever the omnibox runs a query. I hate that the omnibox runs some queries (I think they're always blank) on startup, but never investigated enough to get rid of those calls. --mark > > Thanks, > Samarth > > https://codereview.chromium.**org/20587003/<https://codereview.chromium.org/2... >
https://codereview.chromium.org/20587003/diff/24001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_input.h (right): https://codereview.chromium.org/20587003/diff/24001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_input.h:68: INSTANT_NEW_TAB_PAGE_WITH_OMNIBOX_AS_STARTING_FOCUS = 7, It's inappropriate to add this data here. This enum is explicitly about "the type of page currently displayed". This split that you've added does not distinguish between page types, instead it adds in another piece of orthogonal data. One could easily imagine adding such a split to all the other types, and then having a third piece of data that we want to include and doubling all the states again, etc. Capture the data you need by looking at an additional piece of info somewhere, do not include it in the PageClassification. https://codereview.chromium.org/20587003/diff/24001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/20587003/diff/24001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1029: focus_source_ = focus_state_ == OMNIBOX_FOCUS_VISIBLE ? OMNIBOX : FAKEBOX; Nit: Parens around binary subexpr https://codereview.chromium.org/20587003/diff/24001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1277: return focus_source_ == FAKEBOX ? Nit: Parens here too
https://codereview.chromium.org/20587003/diff/24001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_input.h (right): https://codereview.chromium.org/20587003/diff/24001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_input.h:68: INSTANT_NEW_TAB_PAGE_WITH_OMNIBOX_AS_STARTING_FOCUS = 7, On 2013/08/02 17:59:38, Peter Kasting wrote: > It's inappropriate to add this data here. This enum is explicitly about "the > type of page currently displayed". This split that you've added does not > distinguish between page types, instead it adds in another piece of orthogonal > data. One could easily imagine adding such a split to all the other types, and > then having a third piece of data that we want to include and doubling all the > states again, etc. > > Capture the data you need by looking at an additional piece of info somewhere, > do not include it in the PageClassification. I explicitly asked him to include it in PageClassification. I think you're right that now the name of and comment by the enum is misleading. Perhaps OmniboxContext or OmniboxEntryPoint might be better.
On 2013/08/02 18:10:39, Mark P wrote: > I explicitly asked him to include it in PageClassification. I think you're > right that now the name of and comment by the enum is misleading. Perhaps > OmniboxContext or OmniboxEntryPoint might be better. Changing the name would definitely help, but I still think combining this data is a mistake. There is no obvious reason why this focus data is communicated on one of the page types but not others, so it's both confusing today and inviting future expansion. I don't understand why we can't track the focus state separately. Why must it be in this enum?
On Fri, Aug 2, 2013 at 11:12 AM, <pkasting@chromium.org> wrote: > On 2013/08/02 18:10:39, Mark P wrote: > >> I explicitly asked him to include it in PageClassification. I think >> you're >> right that now the name of and comment by the enum is misleading. Perhaps >> OmniboxContext or OmniboxEntryPoint might be better. >> > > Changing the name would definitely help, but I still think combining this > data > is a mistake. There is no obvious reason why this focus data is > communicated on > one of the page types but not others, so it's both confusing today and > inviting > future expansion. I don't understand why we can't track the focus state > separately. Why must it be in this enum? > I seriously don't want to have to muck with AutocompleteInput again and pass this focus state everywhere. Nor do I want to change our logging parameters for aqs= and for omnibox events in UMA. This would be a real headache. Given that the invisible focus can only appear on the new tab page (correct me if I'm wrong), then why shouldn't it be listed as, in effect, a subtype of instant extended new tab page? --mark > https://codereview.chromium.**org/20587003/<https://codereview.chromium.org/2... >
On 2013/08/02 18:18:40, Mark P wrote: > I seriously don't want to have to muck with AutocompleteInput again and > pass this focus state everywhere. That's not a valid reason. If that were the clearest code, we should do that, regardless of current inconvenience. The struct I suggested converting all these callers to might make changes like this more palatable. > Nor do I want to change our logging > parameters for aqs= and for omnibox events in UMA. This would be a real > headache. I don't think I totally understand which changes to those would be necessary. But again, I am resistant to justifying a strange Chrome change by saying "it's too much hassle to do it some other way." > Given that the invisible focus can only appear on the new tab page (correct > me if I'm wrong), then why shouldn't it be listed as, in effect, a subtype > of instant extended new tab page? What about when we have instant again? Wouldn't we want the current page classification to differ between "new tab page" and "some instant page"? And then of course you could have gotten that instant page to show via an edit sequence that started with either focus source... (In any case, though, this last argument is the one that I find most valid.)
On Fri, Aug 2, 2013 at 11:25 AM, <pkasting@chromium.org> wrote: > > Given that the invisible focus can only appear on the new tab page >> (correct >> me if I'm wrong), then why shouldn't it be listed as, in effect, a subtype >> of instant extended new tab page? >> > > What about when we have instant again? Wouldn't we want the current page > classification to differ between "new tab page" and "some instant page"? > And > then of course you could have gotten that instant page to show via an edit > sequence that started with either focus source... > > (In any case, though, this last argument is the one that I find most > valid.) > If we decide we care about more omnibox entry points in the future, we can add new entries to this enum ("some instant page", "some instant page on which the user focussed on the omnibox with invisible focus"). Given the current state (and likely future state), it's hard for me to imagine why we need the full enum X focus state product. Almost all of those entries are impossible. --mark > > https://codereview.chromium.**org/20587003/<https://codereview.chromium.org/2... >
OK. You ultimately understand this better than me, and I'd rather get to something more important :). LGTM
On Fri, Aug 2, 2013 at 11:36 AM, <pkasting@chromium.org> wrote: > I'd rather get to something more important :). > So would I. :-) --mark > > LGTM > > https://codereview.chromium.**org/20587003/<https://codereview.chromium.org/2... >
metrics changes LGTM https://codereview.chromium.org/20587003/diff/24001/chrome/browser/metrics/me... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/20587003/diff/24001/chrome/browser/metrics/me... chrome/browser/metrics/metrics_log.cc:158: return OmniboxEventProto::OBSOLETE_INSTANT_NEW_TAB_PAGE; nit: NOTREACHED()? Maybe you could even remove the enum value on the AutocompleteInput side? https://codereview.chromium.org/20587003/diff/24001/chrome/common/metrics/pro... File chrome/common/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/20587003/diff/24001/chrome/common/metrics/pro... chrome/common/metrics/proto/omnibox_event.proto:82: INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS = 8; Please get the changes to the protocol buffer approved and landed in google3 prior to landing this CL.
https://codereview.chromium.org/20587003/diff/24001/chrome/browser/metrics/me... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/20587003/diff/24001/chrome/browser/metrics/me... chrome/browser/metrics/metrics_log.cc:158: return OmniboxEventProto::OBSOLETE_INSTANT_NEW_TAB_PAGE; You realize this suggestion is the exact opposite of the previous code you suggested and reviewed? :-) https://codereview.chromium.org/20747002/ https://codereview.chromium.org/20587003/diff/24001/chrome/common/metrics/pro... File chrome/common/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/20587003/diff/24001/chrome/common/metrics/pro... chrome/common/metrics/proto/omnibox_event.proto:82: INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS = 8; On 2013/08/02 21:47:32, Ilya Sherman wrote: > Please get the changes to the protocol buffer approved and landed in google3 > prior to landing this CL. samarth: I'll do this for you.
https://codereview.chromium.org/20587003/diff/24001/chrome/browser/metrics/me... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/20587003/diff/24001/chrome/browser/metrics/me... chrome/browser/metrics/metrics_log.cc:158: return OmniboxEventProto::OBSOLETE_INSTANT_NEW_TAB_PAGE; On 2013/08/02 21:53:24, Mark P wrote: > > You realize this suggestion is the exact opposite of the previous code you > suggested and reviewed? :-) > https://codereview.chromium.org/20747002/ How so? I'm not recommending adding a default case for this switch stmt. I'm recommending removing the enumerated constant AutocompleteInput::OBSOLETE_INSTANT_NEW_TAB_PAGE, which reduces the number of cases that we need to handle in this switch stmt... while still preserving the property that failing to handle new enumerated values would be a compile-time error.
https://codereview.chromium.org/20587003/diff/24001/chrome/browser/metrics/me... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/20587003/diff/24001/chrome/browser/metrics/me... chrome/browser/metrics/metrics_log.cc:158: return OmniboxEventProto::OBSOLETE_INSTANT_NEW_TAB_PAGE; On 2013/08/02 22:01:49, Ilya Sherman wrote: > On 2013/08/02 21:53:24, Mark P wrote: > > > > You realize this suggestion is the exact opposite of the previous code you > > suggested and reviewed? :-) > > https://codereview.chromium.org/20747002/ > > How so? I'm not recommending adding a default case for this switch stmt. I'm > recommending removing the enumerated constant > AutocompleteInput::OBSOLETE_INSTANT_NEW_TAB_PAGE, which reduces the number of > cases that we need to handle in this switch stmt... while still preserving the > property that failing to handle new enumerated values would be a compile-time > error. Oh, yes, I misread your suggestion. It sounds smart. Let's do it.
https://codereview.chromium.org/20587003/diff/24001/chrome/common/metrics/pro... File chrome/common/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/20587003/diff/24001/chrome/common/metrics/pro... chrome/common/metrics/proto/omnibox_event.proto:82: INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS = 8; On 2013/08/02 21:53:24, Mark P wrote: > On 2013/08/02 21:47:32, Ilya Sherman wrote: > > Please get the changes to the protocol buffer approved and landed in google3 > > prior to landing this CL. > > samarth: I'll do this for you. This is done. https://cr/50306267 As part of this review, the logs team suggested replacing the above line 72 OBSOLETE_INSTANT_NEW_TAB_PAGE = 5; // new tab page rendered by Instant with // The instant new tab page enum value was deprecated on August 2, 2013. OBSOLETE_INSTANT_NEW_TAB_PAGE = 5;
Thanks for making the change Mark! Samarth https://codereview.chromium.org/20587003/diff/24001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/20587003/diff/24001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1029: focus_source_ = focus_state_ == OMNIBOX_FOCUS_VISIBLE ? OMNIBOX : FAKEBOX; On 2013/08/02 17:59:38, Peter Kasting wrote: > Nit: Parens around binary subexpr Done. https://codereview.chromium.org/20587003/diff/24001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1277: return focus_source_ == FAKEBOX ? On 2013/08/02 17:59:38, Peter Kasting wrote: > Nit: Parens here too Done. https://codereview.chromium.org/20587003/diff/24001/chrome/common/metrics/pro... File chrome/common/metrics/proto/omnibox_event.proto (right): https://codereview.chromium.org/20587003/diff/24001/chrome/common/metrics/pro... chrome/common/metrics/proto/omnibox_event.proto:82: INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS = 8; On 2013/08/03 00:19:03, Mark P wrote: > On 2013/08/02 21:53:24, Mark P wrote: > > On 2013/08/02 21:47:32, Ilya Sherman wrote: > > > Please get the changes to the protocol buffer approved and landed in google3 > > > prior to landing this CL. > > > > samarth: I'll do this for you. > > This is done. > https://cr/50306267 > > As part of this review, the logs team suggested replacing the above line 72 > OBSOLETE_INSTANT_NEW_TAB_PAGE = 5; // new tab page rendered by Instant > with > // The instant new tab page enum value was deprecated on August 2, 2013. > OBSOLETE_INSTANT_NEW_TAB_PAGE = 5; Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/samarth@chromium.org/20587003/46001
https://chromiumcodereview.appspot.com/20587003/diff/24001/chrome/browser/met... File chrome/browser/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/20587003/diff/24001/chrome/browser/met... chrome/browser/metrics/metrics_log.cc:158: return OmniboxEventProto::OBSOLETE_INSTANT_NEW_TAB_PAGE; On 2013/08/02 22:08:42, Mark P wrote: > On 2013/08/02 22:01:49, Ilya Sherman wrote: > > On 2013/08/02 21:53:24, Mark P wrote: > > > > > > You realize this suggestion is the exact opposite of the previous code you > > > suggested and reviewed? :-) > > > https://codereview.chromium.org/20747002/ > > > > How so? I'm not recommending adding a default case for this switch stmt. I'm > > recommending removing the enumerated constant > > AutocompleteInput::OBSOLETE_INSTANT_NEW_TAB_PAGE, which reduces the number of > > cases that we need to handle in this switch stmt... while still preserving the > > property that failing to handle new enumerated values would be a compile-time > > error. > > Oh, yes, I misread your suggestion. It sounds smart. Let's do it. Please take this suggestion from Ilya before you submit.
https://chromiumcodereview.appspot.com/20587003/diff/24001/chrome/browser/met... File chrome/browser/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/20587003/diff/24001/chrome/browser/met... chrome/browser/metrics/metrics_log.cc:158: return OmniboxEventProto::OBSOLETE_INSTANT_NEW_TAB_PAGE; On 2013/08/06 17:22:14, Mark P wrote: > On 2013/08/02 22:08:42, Mark P wrote: > > On 2013/08/02 22:01:49, Ilya Sherman wrote: > > > On 2013/08/02 21:53:24, Mark P wrote: > > > > > > > > You realize this suggestion is the exact opposite of the previous code you > > > > suggested and reviewed? :-) > > > > https://codereview.chromium.org/20747002/ > > > > > > How so? I'm not recommending adding a default case for this switch stmt. > I'm > > > recommending removing the enumerated constant > > > AutocompleteInput::OBSOLETE_INSTANT_NEW_TAB_PAGE, which reduces the number > of > > > cases that we need to handle in this switch stmt... while still preserving > the > > > property that failing to handle new enumerated values would be a > compile-time > > > error. > > > > Oh, yes, I misread your suggestion. It sounds smart. Let's do it. > > Please take this suggestion from Ilya before you submit. Oops sorry. Thanks for catching that. Done. (I left the field commented-out in the file to ensure we don't accidentally reuse that value.)
https://chromiumcodereview.appspot.com/20587003/diff/24001/chrome/browser/met... File chrome/browser/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/20587003/diff/24001/chrome/browser/met... chrome/browser/metrics/metrics_log.cc:158: return OmniboxEventProto::OBSOLETE_INSTANT_NEW_TAB_PAGE; On 2013/08/06 17:30:45, samarth wrote: > On 2013/08/06 17:22:14, Mark P wrote: > > On 2013/08/02 22:08:42, Mark P wrote: > > > On 2013/08/02 22:01:49, Ilya Sherman wrote: > > > > On 2013/08/02 21:53:24, Mark P wrote: > > > > > > > > > > You realize this suggestion is the exact opposite of the previous code > you > > > > > suggested and reviewed? :-) > > > > > https://codereview.chromium.org/20747002/ > > > > > > > > How so? I'm not recommending adding a default case for this switch stmt. > > I'm > > > > recommending removing the enumerated constant > > > > AutocompleteInput::OBSOLETE_INSTANT_NEW_TAB_PAGE, which reduces the number > > of > > > > cases that we need to handle in this switch stmt... while still preserving > > the > > > > property that failing to handle new enumerated values would be a > > compile-time > > > > error. > > > > > > Oh, yes, I misread your suggestion. It sounds smart. Let's do it. > > > > Please take this suggestion from Ilya before you submit. > > Oops sorry. Thanks for catching that. Done. (I left the field commented-out > in the file to ensure we don't accidentally reuse that value.) You might as well remove the value from autocomplete_input. It's not stored anywhere, and nothing bad will happen if reuse that enum value. (Obviously, keep the enum value in the proto file; we need to not reuse that one.)
https://chromiumcodereview.appspot.com/20587003/diff/24001/chrome/browser/met... File chrome/browser/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/20587003/diff/24001/chrome/browser/met... chrome/browser/metrics/metrics_log.cc:158: return OmniboxEventProto::OBSOLETE_INSTANT_NEW_TAB_PAGE; On 2013/08/06 17:34:14, Mark P wrote: > On 2013/08/06 17:30:45, samarth wrote: > > On 2013/08/06 17:22:14, Mark P wrote: > > > On 2013/08/02 22:08:42, Mark P wrote: > > > > On 2013/08/02 22:01:49, Ilya Sherman wrote: > > > > > On 2013/08/02 21:53:24, Mark P wrote: > > > > > > > > > > > > You realize this suggestion is the exact opposite of the previous code > > you > > > > > > suggested and reviewed? :-) > > > > > > https://codereview.chromium.org/20747002/ > > > > > > > > > > How so? I'm not recommending adding a default case for this switch > stmt. > > > I'm > > > > > recommending removing the enumerated constant > > > > > AutocompleteInput::OBSOLETE_INSTANT_NEW_TAB_PAGE, which reduces the > number > > > of > > > > > cases that we need to handle in this switch stmt... while still > preserving > > > the > > > > > property that failing to handle new enumerated values would be a > > > compile-time > > > > > error. > > > > > > > > Oh, yes, I misread your suggestion. It sounds smart. Let's do it. > > > > > > Please take this suggestion from Ilya before you submit. > > > > Oops sorry. Thanks for catching that. Done. (I left the field commented-out > > in the file to ensure we don't accidentally reuse that value.) > > You might as well remove the value from autocomplete_input. It's not stored > anywhere, and nothing bad will happen if reuse that enum value. (Obviously, > keep the enum value in the proto file; we need to not reuse that one.) OK, done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/samarth@chromium.org/20587003/75001
Retried try job too often on win_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/samarth@chromium.org/20587003/75001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Message was sent while issue was closed.
Committed patchset #12 manually as r216067 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
