Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11415292:
[Search] No longer reverting omnibox text on instant search (Closed)
|
Created:
8 years ago by Mathieu Modified:
8 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, samarth Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Description[Search] No longer reverting omnibox text on instant search
Avoids flash of old text when committing an instant search.
BUG=153477, 159326, 165710
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173069
Patch Set 1 #Patch Set 2 : Sync plumbing for search_terms #Patch Set 3 : More tests #
Total comments: 6
Patch Set 4 : addressed comments #
Total comments: 2
Patch Set 5 : InstantController logic #Patch Set 6 : session_types comment #
Total comments: 18
Patch Set 7 : Addressed comments, fixed query refinement #Patch Set 8 : Moved to TemplateURLService #Patch Set 9 : Readded line #Patch Set 10 : indent #
Total comments: 20
Patch Set 11 : bracket #Patch Set 12 : addressed comments #Patch Set 13 : fix test #Patch Set 14 : fix tests #
Total comments: 5
Patch Set 15 : removed android_webview changes #Patch Set 16 : rebase #Patch Set 17 : Gutted #
Messages
Total messages: 38 (0 generated)
https://codereview.chromium.org/11415292/diff/2002/chrome/browser/sessions/se... File chrome/browser/sessions/session_types.cc (right): https://codereview.chromium.org/11415292/diff/2002/chrome/browser/sessions/se... chrome/browser/sessions/session_types.cc:262: !iterator->ReadString16(&search_terms_) || this isn't backwards compatible. it needs to go last and be optional. https://codereview.chromium.org/11415292/diff/2002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/11415292/diff/2002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:67: if (entry) bug! entry isn't always initialized https://codereview.chromium.org/11415292/diff/2002/content/browser/web_conten... File content/browser/web_contents/navigation_entry_impl.cc (right): https://codereview.chromium.org/11415292/diff/2002/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.cc:66: search_terms_(string16()), unneeded
Thanks! https://chromiumcodereview.appspot.com/11415292/diff/2002/chrome/browser/sess... File chrome/browser/sessions/session_types.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/2002/chrome/browser/sess... chrome/browser/sessions/session_types.cc:262: !iterator->ReadString16(&search_terms_) || On 2012/12/05 19:40:27, akalin wrote: > this isn't backwards compatible. it needs to go last and be optional. Done. Let me know if this is the way to do it. https://chromiumcodereview.appspot.com/11415292/diff/2002/chrome/browser/ui/t... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/2002/chrome/browser/ui/t... chrome/browser/ui/toolbar/toolbar_model_impl.cc:67: if (entry) On 2012/12/05 19:40:27, akalin wrote: > bug! entry isn't always initialized Done. Thanks. https://chromiumcodereview.appspot.com/11415292/diff/2002/content/browser/web... File content/browser/web_contents/navigation_entry_impl.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/2002/content/browser/web... content/browser/web_contents/navigation_entry_impl.cc:66: search_terms_(string16()), On 2012/12/05 19:40:27, akalin wrote: > unneeded Done.
still have to look again, but one comment https://codereview.chromium.org/11415292/diff/11001/chrome/browser/sessions/s... File chrome/browser/sessions/session_types.cc (right): https://codereview.chromium.org/11415292/diff/11001/chrome/browser/sessions/s... chrome/browser/sessions/session_types.cc:308: // Search terms is an optional field, defaults to empty. Perhaps: // If the search term field can't be found, leave it empty.
This is ready for your consideration. sreeram: chrome/browser/instant/instant_controller.cc chrome/browser/ui/toolbar/toolbar_model_impl.cc content/browser/web_contents/navigation_entry_impl* content/public/browser/navigation_entry.h akalin: android_webview/native/state_serializer* chrome/browser/sessions/session_types* Thanks! https://codereview.chromium.org/11415292/diff/11001/chrome/browser/sessions/s... File chrome/browser/sessions/session_types.cc (right): https://codereview.chromium.org/11415292/diff/11001/chrome/browser/sessions/s... chrome/browser/sessions/session_types.cc:308: // Search terms is an optional field, defaults to empty. On 2012/12/05 23:35:44, akalin wrote: > Perhaps: > > // If the search term field can't be found, leave it empty. Done.
Replace "sreeram" with "jered" in the above review email :) Thanks!
On 2012/12/06 22:05:10, Mathieu Perreault wrote: > Replace "sreeram" with "jered" in the above review email :) > > Thanks! I'm not an instant owner. Did you want +sky?
https://codereview.chromium.org/11415292/diff/26001/android_webview/native/st... File android_webview/native/state_serializer.cc (right): https://codereview.chromium.org/11415292/diff/26001/android_webview/native/st... android_webview/native/state_serializer.cc:214: string16 search_terms; will this ever try to load 'old' pickled entries? If so, it probably should go on the end https://codereview.chromium.org/11415292/diff/26001/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/11415292/diff/26001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/toolbar_model_impl.cc:52: NavigationEntry* entry; this still isn't fixed
https://codereview.chromium.org/11415292/diff/26001/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/11415292/diff/26001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:455: search_terms += last_suggestion_.text; Indent.
lgtm for instant_controller.cc Sending over to sky for instant OWNERS.
https://codereview.chromium.org/11415292/diff/26001/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (left): https://codereview.chromium.org/11415292/diff/26001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/toolbar_model_impl.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. Don't remove this. https://codereview.chromium.org/11415292/diff/26001/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/11415292/diff/26001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/toolbar_model_impl.cc:63: string16 search_terms = TryToExtractSearchTermsFromURL(); If we can build this on the fly do we need to persist it to disk by session/tab restore? https://codereview.chromium.org/11415292/diff/26001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/toolbar_model_impl.cc:66: entry->SetSearchTerms(search_terms); This seems like the wrong place to do this. Shouldn't we do this on navigation, perhaps in a tabhelper?
FYI, I patched this in and was still able to get an incorrect search query in the omnibox when quickly refining a search on a committed page.
On 2012/12/07 18:42:38, David Black wrote: > FYI, I patched this in and was still able to get an incorrect search query in > the omnibox when quickly refining a search on a committed page. Yes, I noticed this too and I'm investigating. Thanks for paying attention :)
Thanks for the comments. https://chromiumcodereview.appspot.com/11415292/diff/26001/android_webview/na... File android_webview/native/state_serializer.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/26001/android_webview/na... android_webview/native/state_serializer.cc:214: string16 search_terms; On 2012/12/06 23:14:46, akalin wrote: > will this ever try to load 'old' pickled entries? If so, it probably should go > on the end Done. I've put it at the end, but it won't fail if it's not present as it's a new field (similar to comments I got in session_types.cc). Let me know if that sounds reasonable to you, and how we can be sure this is the right way to do it. https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ins... File chrome/browser/instant/instant_controller.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ins... chrome/browser/instant/instant_controller.cc:455: search_terms += last_suggestion_.text; On 2012/12/07 00:16:03, Jered wrote: > Indent. Done. https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (left): https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2012/12/07 16:29:25, sky wrote: > Don't remove this. Done. Not sure what happened. https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:52: NavigationEntry* entry; On 2012/12/06 23:14:46, akalin wrote: > this still isn't fixed Done. Thanks. https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:63: string16 search_terms = TryToExtractSearchTermsFromURL(); On 2012/12/07 16:29:25, sky wrote: > If we can build this on the fly do we need to persist it to disk by session/tab > restore? We would like to restore the correct search terms regardless of virtual URLs, and maintain any "fixed" NavigationEntry in the navigation stack so that the back/forward experience maintains the correct terms. For example, we use SetSearchTerms in InstantController to fix a special case of the virtual URL not being correct for the moment of a navigation. We want that persisted. https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:66: entry->SetSearchTerms(search_terms); On 2012/12/07 16:29:25, sky wrote: > This seems like the wrong place to do this. Shouldn't we do this on navigation, > perhaps in a tabhelper? I agree that it's not necessarily elegant to put side-effects here. I've thought about having an Observer to NAV_ENTRY_COMMITTED, but I was wondering where I'd be putting it. Here, or OmniboxEditModel perhaps? What do you think?
On 2012/12/11 00:30:17, Mathieu Perreault wrote: > Thanks for the comments. > > https://chromiumcodereview.appspot.com/11415292/diff/26001/android_webview/na... > File android_webview/native/state_serializer.cc (right): > > https://chromiumcodereview.appspot.com/11415292/diff/26001/android_webview/na... > android_webview/native/state_serializer.cc:214: string16 search_terms; > On 2012/12/06 23:14:46, akalin wrote: > > will this ever try to load 'old' pickled entries? If so, it probably should > go > > on the end > > Done. I've put it at the end, but it won't fail if it's not present as it's a > new field (similar to comments I got in session_types.cc). Let me know if that > sounds reasonable to you, and how we can be sure this is the right way to do it. > > https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ins... > File chrome/browser/instant/instant_controller.cc (right): > > https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ins... > chrome/browser/instant/instant_controller.cc:455: search_terms += > last_suggestion_.text; > On 2012/12/07 00:16:03, Jered wrote: > > Indent. > > Done. > > https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... > File chrome/browser/ui/toolbar/toolbar_model_impl.cc (left): > > https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... > chrome/browser/ui/toolbar/toolbar_model_impl.cc:1: // Copyright 2012 The > Chromium Authors. All rights reserved. > On 2012/12/07 16:29:25, sky wrote: > > Don't remove this. > > Done. Not sure what happened. > > https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... > File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): > > https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... > chrome/browser/ui/toolbar/toolbar_model_impl.cc:52: NavigationEntry* entry; > On 2012/12/06 23:14:46, akalin wrote: > > this still isn't fixed > > Done. Thanks. > > https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... > chrome/browser/ui/toolbar/toolbar_model_impl.cc:63: string16 search_terms = > TryToExtractSearchTermsFromURL(); > On 2012/12/07 16:29:25, sky wrote: > > If we can build this on the fly do we need to persist it to disk by > session/tab > > restore? > > We would like to restore the correct search terms regardless of virtual URLs, > and maintain any "fixed" NavigationEntry in the navigation stack so that the > back/forward experience maintains the correct terms. For example, we use > SetSearchTerms in InstantController to fix a special case of the virtual URL not > being correct for the moment of a navigation. We want that persisted. > > https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... > chrome/browser/ui/toolbar/toolbar_model_impl.cc:66: > entry->SetSearchTerms(search_terms); > On 2012/12/07 16:29:25, sky wrote: > > This seems like the wrong place to do this. Shouldn't we do this on > navigation, > > perhaps in a tabhelper? > > I agree that it's not necessarily elegant to put side-effects here. I've thought > about having an Observer to NAV_ENTRY_COMMITTED, but I was wondering where I'd > be putting it. Here, or OmniboxEditModel perhaps? What do you think? Friendly ping sky@
https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:63: string16 search_terms = TryToExtractSearchTermsFromURL(); On 2012/12/11 00:30:17, Mathieu Perreault wrote: > On 2012/12/07 16:29:25, sky wrote: > > If we can build this on the fly do we need to persist it to disk by > session/tab > > restore? > > We would like to restore the correct search terms regardless of virtual URLs, > and maintain any "fixed" NavigationEntry in the navigation stack so that the > back/forward experience maintains the correct terms. For example, we use > SetSearchTerms in InstantController to fix a special case of the virtual URL not > being correct for the moment of a navigation. We want that persisted. > Ok, but that doesn't seem to indicate we need to persist them. Couldn't we reset when restoring? https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:66: entry->SetSearchTerms(search_terms); On 2012/12/11 00:30:17, Mathieu Perreault wrote: > On 2012/12/07 16:29:25, sky wrote: > > This seems like the wrong place to do this. Shouldn't we do this on > navigation, > > perhaps in a tabhelper? > > I agree that it's not necessarily elegant to put side-effects here. I've thought > about having an Observer to NAV_ENTRY_COMMITTED, but I was wondering where I'd > be putting it. Here, or OmniboxEditModel perhaps? What do you think? How about SearchTabHelper.
Thanks sky, please have another look. https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:63: string16 search_terms = TryToExtractSearchTermsFromURL(); On 2012/12/11 22:59:16, sky wrote: > On 2012/12/11 00:30:17, Mathieu Perreault wrote: > > On 2012/12/07 16:29:25, sky wrote: > > > If we can build this on the fly do we need to persist it to disk by > > session/tab > > > restore? > > > > We would like to restore the correct search terms regardless of virtual URLs, > > and maintain any "fixed" NavigationEntry in the navigation stack so that the > > back/forward experience maintains the correct terms. For example, we use > > SetSearchTerms in InstantController to fix a special case of the virtual URL > not > > being correct for the moment of a navigation. We want that persisted. > > > > Ok, but that doesn't seem to indicate we need to persist them. Couldn't we reset > when restoring? Well, as I mentioned there are special cases where we fix NavigationEntries as they come through and we use the search terms field to show omnibox text. When restoring with the method contained in this CL, we know for sure that the whole stack of navigation entries is correctly fixed (if the user presses the back button, there is no-reparsing and the text is as we want it to be). Without this field, reparsing in the correct way (i.e. handling special cases) upon restore would require the proper context in instant_controller, which is gone at this point since this is a restore. https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:66: entry->SetSearchTerms(search_terms); On 2012/12/11 22:59:16, sky wrote: > On 2012/12/11 00:30:17, Mathieu Perreault wrote: > > On 2012/12/07 16:29:25, sky wrote: > > > This seems like the wrong place to do this. Shouldn't we do this on > > navigation, > > > perhaps in a tabhelper? > > > > I agree that it's not necessarily elegant to put side-effects here. I've > thought > > about having an Observer to NAV_ENTRY_COMMITTED, but I was wondering where I'd > > be putting it. Here, or OmniboxEditModel perhaps? What do you think? > > How about SearchTabHelper. Done. Refactored TryToExtractSearchTermsFromURL to TemplateURLService, which it was relying on anyway. Now SearchTabHelper will update the search terms on NAV_ENTRY_COMMITTED.
https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:63: string16 search_terms = TryToExtractSearchTermsFromURL(); On 2012/12/12 19:37:53, Mathieu Perreault wrote: > On 2012/12/11 22:59:16, sky wrote: > > On 2012/12/11 00:30:17, Mathieu Perreault wrote: > > > On 2012/12/07 16:29:25, sky wrote: > > > > If we can build this on the fly do we need to persist it to disk by > > > session/tab > > > > restore? > > > > > > We would like to restore the correct search terms regardless of virtual > URLs, > > > and maintain any "fixed" NavigationEntry in the navigation stack so that the > > > back/forward experience maintains the correct terms. For example, we use > > > SetSearchTerms in InstantController to fix a special case of the virtual URL > > not > > > being correct for the moment of a navigation. We want that persisted. > > > > > > > Ok, but that doesn't seem to indicate we need to persist them. Couldn't we > reset > > when restoring? > > Well, as I mentioned there are special cases where we fix NavigationEntries as > they come through and we use the search terms field to show omnibox text. When > restoring with the method contained in this CL, we know for sure that the whole > stack of navigation entries is correctly fixed (if the user presses the back > button, there is no-reparsing and the text is as we want it to be). Without this > field, reparsing in the correct way (i.e. handling special cases) upon restore > would require the proper context in instant_controller, which is gone at this > point since this is a restore. Forgive my pushing back, but I want to make sure we really need to persist things to disk. What context do you need that isn't available upon restore? I would rather see the places that need the search terms extract them as necessary. Extracting is fairly cheap, results in far less API and doesn't change the on disk session state.
Thanks! https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/26001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:63: string16 search_terms = TryToExtractSearchTermsFromURL(); On 2012/12/12 21:18:05, sky wrote: > On 2012/12/12 19:37:53, Mathieu Perreault wrote: > > On 2012/12/11 22:59:16, sky wrote: > > > On 2012/12/11 00:30:17, Mathieu Perreault wrote: > > > > On 2012/12/07 16:29:25, sky wrote: > > > > > If we can build this on the fly do we need to persist it to disk by > > > > session/tab > > > > > restore? > > > > > > > > We would like to restore the correct search terms regardless of virtual > > URLs, > > > > and maintain any "fixed" NavigationEntry in the navigation stack so that > the > > > > back/forward experience maintains the correct terms. For example, we use > > > > SetSearchTerms in InstantController to fix a special case of the virtual > URL > > > not > > > > being correct for the moment of a navigation. We want that persisted. > > > > > > > > > > Ok, but that doesn't seem to indicate we need to persist them. Couldn't we > > reset > > > when restoring? > > > > Well, as I mentioned there are special cases where we fix NavigationEntries as > > they come through and we use the search terms field to show omnibox text. When > > restoring with the method contained in this CL, we know for sure that the > whole > > stack of navigation entries is correctly fixed (if the user presses the back > > button, there is no-reparsing and the text is as we want it to be). Without > this > > field, reparsing in the correct way (i.e. handling special cases) upon restore > > would require the proper context in instant_controller, which is gone at this > > point since this is a restore. > > Forgive my pushing back, but I want to make sure we really need to persist > things to disk. What context do you need that isn't available upon restore? > > I would rather see the places that need the search terms extract them as > necessary. Extracting is fairly cheap, results in far less API and doesn't > change the on disk session state. No problem, glad to discuss. I'll refer now to the proposed changes in instant_controller.cc. In this case, the user has typed out a query e.g. [q] and the preview makes a search for [quotes]. Upon committing a certain way, the omnibox will go through a state where the virtual URL contains "&q=q&". This needs to be fixed with the context that the actual query being done is [quotes] (stored in |last_suggestion.text|). The virtual URL will stay q=q which would be wrongly extracted. As far as API calls are concerned, this should be no more frequent than the changes to the virtual URL, title of the page and so forth. The sync team raised no concerns to that effect, although obviously we shouldn't be wasteful on purpose. We think it's essential here. Let me know.
Ok, I get now why you want to persist this state to disk. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... chrome/browser/search_engines/template_url_service.cc:443: string16 TemplateURLService::TryToExtractSearchTermsFromURL(GURL url) { How about making this a function in chrome/browser/ui/search/search_utils ? https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... chrome/browser/search_engines/template_url_service.cc:449: if (!template_url->profile_ || Why are you using template_url->profile_ instead of profile_ ? https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... File chrome/browser/search_engines/template_url_service.h (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... chrome/browser/search_engines/template_url_service.h:133: string16 TryToExtractSearchTermsFromURL(GURL url); const GURL& https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ses... File chrome/browser/sessions/session_types.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ses... chrome/browser/sessions/session_types.cc:309: if (!iterator->ReadString16(&search_terms_)) { no {} https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:636: controller()->GetInstant()->IsInstantExtendedSearch(); null check GetInstant() https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... File chrome/browser/ui/search/search_tab_helper.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/search/search_tab_helper.cc:106: if (web_contents()) { Do you really need this if? https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/search/search_tab_helper.cc:109: if (navigation_controller) { There is always a NavigationController. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/search/search_tab_helper.cc:110: content::NavigationEntry* entry = navigation_controller->GetActiveEntry(); Use committed_details->entry https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:53: NavigationEntry* entry = NULL; Move this inside if.
Thanks! https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... chrome/browser/search_engines/template_url_service.cc:443: string16 TemplateURLService::TryToExtractSearchTermsFromURL(GURL url) { On 2012/12/12 22:41:57, sky wrote: > How about making this a function in chrome/browser/ui/search/search_utils ? I thought it fit nicely here since it already used the template_url_service and template_url and it is similar to other functions in this file. If you don't mind it's getting late Eastern time and I would like to check this in today (dev cut tomorrow) if the rest seems reasonable to you. I propose to have a follow-up CL tomorrow that will do just that. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... chrome/browser/search_engines/template_url_service.cc:449: if (!template_url->profile_ || On 2012/12/12 22:41:57, sky wrote: > Why are you using template_url->profile_ instead of profile_ ? Done. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... File chrome/browser/search_engines/template_url_service.h (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... chrome/browser/search_engines/template_url_service.h:133: string16 TryToExtractSearchTermsFromURL(GURL url); On 2012/12/12 22:41:57, sky wrote: > const GURL& Done. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ses... File chrome/browser/sessions/session_types.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ses... chrome/browser/sessions/session_types.cc:309: if (!iterator->ReadString16(&search_terms_)) { On 2012/12/12 22:41:57, sky wrote: > no {} Done. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:636: controller()->GetInstant()->IsInstantExtendedSearch(); On 2012/12/12 22:41:57, sky wrote: > null check GetInstant() Done. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... File chrome/browser/ui/search/search_tab_helper.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/search/search_tab_helper.cc:106: if (web_contents()) { On 2012/12/12 22:41:57, sky wrote: > Do you really need this if? It comes from SearchModel and it is marked as "weak" over there, so I wanted to make sure. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/search/search_tab_helper.cc:109: if (navigation_controller) { On 2012/12/12 22:41:57, sky wrote: > There is always a NavigationController. Done. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/search/search_tab_helper.cc:110: content::NavigationEntry* entry = navigation_controller->GetActiveEntry(); On 2012/12/12 22:41:57, sky wrote: > Use committed_details->entry Done. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:53: NavigationEntry* entry = NULL; On 2012/12/12 22:41:57, sky wrote: > Move this inside if. Done.
Fix the if and LGTM as long you are going to move the function. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... chrome/browser/search_engines/template_url_service.cc:443: string16 TemplateURLService::TryToExtractSearchTermsFromURL(GURL url) { On 2012/12/12 23:37:24, Mathieu Perreault wrote: > On 2012/12/12 22:41:57, sky wrote: > > How about making this a function in chrome/browser/ui/search/search_utils ? > > I thought it fit nicely here since it already used the template_url_service and > template_url and it is similar to other functions in this file. > > If you don't mind it's getting late Eastern time and I would like to check this > in today (dev cut tomorrow) if the rest seems reasonable to you. I propose to > have a follow-up CL tomorrow that will do just that. This uses public state of TemplateURLService. Its best to keep such functions outside the class. https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... File chrome/browser/ui/search/search_tab_helper.cc (right): https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... chrome/browser/ui/search/search_tab_helper.cc:106: if (web_contents()) { On 2012/12/12 23:37:24, Mathieu Perreault wrote: > On 2012/12/12 22:41:57, sky wrote: > > Do you really need this if? > > It comes from SearchModel and it is marked as "weak" over there, so I wanted to > make sure. From WebContentsUserData: // A base class for classes attached to, and scoped to, the lifetime of a // WebContents. For example: The if isn't needed.
On 2012/12/13 00:48:18, sky wrote: > Fix the if and LGTM as long you are going to move the function. > > https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... > File chrome/browser/search_engines/template_url_service.cc (right): > > https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... > chrome/browser/search_engines/template_url_service.cc:443: string16 > TemplateURLService::TryToExtractSearchTermsFromURL(GURL url) { > On 2012/12/12 23:37:24, Mathieu Perreault wrote: > > On 2012/12/12 22:41:57, sky wrote: > > > How about making this a function in chrome/browser/ui/search/search_utils ? > > > > I thought it fit nicely here since it already used the template_url_service > and > > template_url and it is similar to other functions in this file. > > > > If you don't mind it's getting late Eastern time and I would like to check > this > > in today (dev cut tomorrow) if the rest seems reasonable to you. I propose to > > have a follow-up CL tomorrow that will do just that. > > This uses public state of TemplateURLService. Its best to keep such functions > outside the class. > > https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... > File chrome/browser/ui/search/search_tab_helper.cc (right): > > https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... > chrome/browser/ui/search/search_tab_helper.cc:106: if (web_contents()) { > On 2012/12/12 23:37:24, Mathieu Perreault wrote: > > On 2012/12/12 22:41:57, sky wrote: > > > Do you really need this if? > > > > It comes from SearchModel and it is marked as "weak" over there, so I wanted > to > > make sure. > > From WebContentsUserData: > > // A base class for classes attached to, and scoped to, the lifetime of a > // WebContents. For example: > > The if isn't needed. Pinging joth@ for android_webview LGTM
On 2012/12/13 01:47:56, Mathieu Perreault wrote: > On 2012/12/13 00:48:18, sky wrote: > > Fix the if and LGTM as long you are going to move the function. > > > > > https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... > > File chrome/browser/search_engines/template_url_service.cc (right): > > > > > https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/sea... > > chrome/browser/search_engines/template_url_service.cc:443: string16 > > TemplateURLService::TryToExtractSearchTermsFromURL(GURL url) { > > On 2012/12/12 23:37:24, Mathieu Perreault wrote: > > > On 2012/12/12 22:41:57, sky wrote: > > > > How about making this a function in chrome/browser/ui/search/search_utils > ? > > > > > > I thought it fit nicely here since it already used the template_url_service > > and > > > template_url and it is similar to other functions in this file. > > > > > > If you don't mind it's getting late Eastern time and I would like to check > > this > > > in today (dev cut tomorrow) if the rest seems reasonable to you. I propose > to > > > have a follow-up CL tomorrow that will do just that. > > > > This uses public state of TemplateURLService. Its best to keep such functions > > outside the class. > > > > > https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... > > File chrome/browser/ui/search/search_tab_helper.cc (right): > > > > > https://chromiumcodereview.appspot.com/11415292/diff/38002/chrome/browser/ui/... > > chrome/browser/ui/search/search_tab_helper.cc:106: if (web_contents()) { > > On 2012/12/12 23:37:24, Mathieu Perreault wrote: > > > On 2012/12/12 22:41:57, sky wrote: > > > > Do you really need this if? > > > > > > It comes from SearchModel and it is marked as "weak" over there, so I wanted > > to > > > make sure. > > > > From WebContentsUserData: > > > > // A base class for classes attached to, and scoped to, the lifetime of a > > // WebContents. For example: > > > > The if isn't needed. > > Pinging joth@ for android_webview LGTM Alternatively pinging benm@ for android_webview if MTV is OOO. Thanks!
+boliu, original author for this. https://codereview.chromium.org/11415292/diff/45002/android_webview/native/st... File android_webview/native/state_serializer.cc (right): https://codereview.chromium.org/11415292/diff/45002/android_webview/native/st... android_webview/native/state_serializer.cc:168: return false; fwiw the java-side of the webview API has no way to read/write search terms, so there's not much point us persisting these. (AIUI the content/ layer would never spontaneously push values into this field? It's purely up to the embedder to populate this field, and android_webview/ never would). but equally, doesn't hurt to have this here. https://codereview.chromium.org/11415292/diff/45002/android_webview/native/st... android_webview/native/state_serializer.cc:170: // Please update AW_STATE_VERSION if serialization format is changed. guess you didn't see this comment? Maybe put some bold flashing ascii art on it? https://codereview.chromium.org/11415292/diff/45002/android_webview/native/st... android_webview/native/state_serializer.cc:258: search_terms = string16(); this doesn't make sense? you're modifying a local, but it never makes it into entry->SetSearchTerms(search_terms). the tests should catch this. Following pattern above, it should be return false if the read failed. https://codereview.chromium.org/11415292/diff/45002/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11415292/diff/45002/content/public/browser/na... content/public/browser/navigation_entry.h:78: // Extracted Search Terms. Can be empty. nit: this comment is pretty terse... I had to hunt around in the CL description to find what search terms these were. (my guess was this is talking about historic find-in-page search terms) Also, as this seems to be far more a chrome browser feature rather than anything todo with the web-platform, would it make more sense to just have some sort of extensible field here that allows the embedding layer to tack additional data items onto a given history field? SetExtraData(string key, string value) or something (with some guidelines on how the keys are allocated)
If you do decide to add it for webview. You will need android_fyi_dbg trybot to run the android webview unit tests. https://codereview.chromium.org/11415292/diff/45002/android_webview/native/st... File android_webview/native/state_serializer.cc (right): https://codereview.chromium.org/11415292/diff/45002/android_webview/native/st... android_webview/native/state_serializer.cc:168: return false; On 2012/12/13 02:11:48, joth wrote: > fwiw the java-side of the webview API has no way to read/write search terms, so > there's not much point us persisting these. (AIUI the content/ layer would never > spontaneously push values into this field? It's purely up to the embedder to > populate this field, and android_webview/ never would). > > but equally, doesn't hurt to have this here. Right, agree with joth here that android webview won't be using this field, so there is no point in persisting it. Also save/restore is probably a hot path for android, but not that a single empty string will make a difference.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/11415292/53001
Presubmit check for 11415292-53001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/public/browser
Presubmit checks took 2.2s to calculate.
On 2012/12/13 04:38:22, I haz the power (commit-bot) wrote: > Presubmit check for 11415292-53001 failed and returned exit status 1. > > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for files in these directories: > content/public/browser > > Presubmit checks took 2.2s to calculate. Adding joi@ for the content/public/browser approval Adding jam@ because it's specified he should be on for any interface changes. Thanks!
On 2012/12/13 04:41:44, Mathieu Perreault wrote: > On 2012/12/13 04:38:22, I haz the power (commit-bot) wrote: > > Presubmit check for 11415292-53001 failed and returned exit status 1. > > > > > > Running presubmit commit checks ... > > > > ** Presubmit ERRORS ** > > Missing LGTM from an OWNER for files in these directories: > > content/public/browser > > > > Presubmit checks took 2.2s to calculate. > > Adding joi@ for the content/public/browser approval > Adding jam@ because it's specified he should be on for any interface changes. > > Thanks! it seems like a layering violation that content has to keep track of this data for chrome. can't you store this elsewhere (i.e. without modifying content)? see the content api docs: http://www.chromium.org/developers/content-module/content-api for more info
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/11415292/51002
On 2012/12/13 18:50:03, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/mathp%40chromium.org/11415292/51002 Gutted this CL to fix some of the related bugs (I am time constrained), will follow up with content/ people to determine how to best do this in a future CL. Thanks.
Retried try job too often on win_rel for step(s) nacl_integration
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/11415292/51002
Message was sent while issue was closed.
Change committed as 173069
Message was sent while issue was closed.
On 2012/12/14 03:37:28, I haz the power (commit-bot) wrote: > Change committed as 173069 This change broke refinements from a committed results page. I am preparing a rollback.
Message was sent while issue was closed.
On 2012/12/14 23:50:24, Jered wrote: > On 2012/12/14 03:37:28, I haz the power (commit-bot) wrote: > > Change committed as 173069 > > This change broke refinements from a committed results page. I am preparing a > rollback. Several cases are handled in https://chromiumcodereview.appspot.com/11570020/ now. Not sure why this simple change would break refinement. Have you tested with the change above?
Message was sent while issue was closed.
On 2012/12/17 16:25:20, Mathieu Perreault wrote: > On 2012/12/14 23:50:24, Jered wrote: > > On 2012/12/14 03:37:28, I haz the power (commit-bot) wrote: > > > Change committed as 173069 > > > > This change broke refinements from a committed results page. I am preparing a > > rollback. > > Several cases are handled in https://chromiumcodereview.appspot.com/11570020/ > now. Not sure why this simple change would break refinement. Have you tested > with the change above? I am not sure, either; we noticed the problem at trunk and looked at recently committed changes to determine that this was the cause. I will patch and test this again tomorrow.
Message was sent while issue was closed.
On 2012/12/18 00:56:29, Jered wrote: > On 2012/12/17 16:25:20, Mathieu Perreault wrote: > > On 2012/12/14 23:50:24, Jered wrote: > > > On 2012/12/14 03:37:28, I haz the power (commit-bot) wrote: > > > > Change committed as 173069 > > > > > > This change broke refinements from a committed results page. I am preparing > a > > > rollback. > > > > Several cases are handled in https://chromiumcodereview.appspot.com/11570020/ > > now. Not sure why this simple change would break refinement. Have you tested > > with the change above? > > I am not sure, either; we noticed the problem at trunk and looked at recently > committed changes to determine that this was the cause. I will patch and test > this again tomorrow. I can reproduce the problem with this patch. Type a query, press enter, then type a new query and press enter. instead of results, you see a blank page. The cause seems to be that the page is reading a blank query after OnSubmit fires and is showing an empty page instead of the results (which it starts rendering and then aborts). I can only imagine that the RevertAll call is doing "something" which prevents this, but the control flow is so convoluted, I have no idea what. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
