Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11876045:
[Search] Store and recall search terms using NavigationEntry to improve search term extraction (Closed)
|
Created:
7 years, 11 months ago by Mathieu Modified:
7 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, melevin, jam, gideonwald, dominich, marja+watch_chromium.org, David Black, samarth+watch_chromium.org, darin-cc_chromium.org, Jered, samarth Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Description[Search] Store and recall search terms using NavigationEntry to improve search term extraction
Adds a new field to NavigationEntry called ExtraData, meant to be used to store extra browser features on a per-need basis. Used here to store extracted search terms.
BUG=159326, 153477, 167067
TEST=NavigationEntryTest, TabNavigationTest
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180020
Patch Set 1 #Patch Set 2 : comment #Patch Set 3 : Condition for fixing #Patch Set 4 : Clean #
Total comments: 1
Patch Set 5 : Indentation #
Total comments: 17
Patch Set 6 : addressed comments #
Total comments: 4
Patch Set 7 : Addressed comments #
Total comments: 7
Patch Set 8 : Addressed comments, some refactor #Patch Set 9 : modified one test, rebased on other CL #Patch Set 10 : https server #Patch Set 11 : rebase #Patch Set 12 : Reduced scope to NavigationEntry #Patch Set 13 : const #
Total comments: 18
Patch Set 14 : Addressed comments #
Total comments: 2
Patch Set 15 : drop const #
Total comments: 6
Patch Set 16 : alignment #
Total comments: 8
Patch Set 17 : Comments #Patch Set 18 : clear #
Total comments: 2
Patch Set 19 : comment #Patch Set 20 : reupload #Messages
Total messages: 43 (0 generated)
Hi, This is ready for review. jam@/joi@: Changes to NavigationEntry's interface, as outlined in design doc. sreeram@: Instant. sky@: sessions/, content/browser and ui/ Thanks
Please provide a link to the design doc. Cheers, Jói On Wed, Jan 16, 2013 at 12:16 PM, <mathp@chromium.org> wrote: > Reviewers: John Abd-El-Malek, sreeram, sky, Jói, > > Message: > Hi, > > This is ready for review. > > jam@/joi@: Changes to NavigationEntry's interface, as outlined in design > doc. > sreeram@: Instant. > sky@: sessions/, content/browser and ui/ > > Thanks > > Description: > [Search] Store and recall search terms using NavigationEntry > > Adds a new field to NavigationEntry called ExtraData, meant to be used to > store > extra browser features on a per-need basis. Used here to store extracted > search > terms. > > BUG= > TEST=NavigationEntryTest, TabNavigationTest > > > Please review this at https://codereview.chromium.org/11876045/ > > SVN Base: http://git.chromium.org/chromium/src.git@master > > Affected files: > M chrome/browser/instant/instant_controller.h > M chrome/browser/instant/instant_controller.cc > M chrome/browser/sessions/session_types.h > M chrome/browser/sessions/session_types.cc > M chrome/browser/sessions/session_types_unittest.cc > M chrome/browser/ui/omnibox/omnibox_edit_model.cc > M chrome/browser/ui/toolbar/toolbar_model_impl.cc > M content/browser/web_contents/navigation_entry_impl.h > M content/browser/web_contents/navigation_entry_impl.cc > M content/browser/web_contents/navigation_entry_impl_unittest.cc > M content/public/browser/navigation_entry.h > >
On 2013/01/16 21:38:01, Jói wrote: > Please provide a link to the design doc. > > Cheers, > Jói > > > On Wed, Jan 16, 2013 at 12:16 PM, <mailto:mathp@chromium.org> wrote: > > Reviewers: John Abd-El-Malek, sreeram, sky, Jói, > > > > Message: > > Hi, > > > > This is ready for review. > > > > jam@/joi@: Changes to NavigationEntry's interface, as outlined in design > > doc. > > sreeram@: Instant. > > sky@: sessions/, content/browser and ui/ > > > > Thanks > > > > Description: > > [Search] Store and recall search terms using NavigationEntry > > > > Adds a new field to NavigationEntry called ExtraData, meant to be used to > > store > > extra browser features on a per-need basis. Used here to store extracted > > search > > terms. > > > > BUG= > > TEST=NavigationEntryTest, TabNavigationTest > > > > > > Please review this at https://codereview.chromium.org/11876045/ > > > > SVN Base: http://git.chromium.org/chromium/src.git%40master > > > > Affected files: > > M chrome/browser/instant/instant_controller.h > > M chrome/browser/instant/instant_controller.cc > > M chrome/browser/sessions/session_types.h > > M chrome/browser/sessions/session_types.cc > > M chrome/browser/sessions/session_types_unittest.cc > > M chrome/browser/ui/omnibox/omnibox_edit_model.cc > > M chrome/browser/ui/toolbar/toolbar_model_impl.cc > > M content/browser/web_contents/navigation_entry_impl.h > > M content/browser/web_contents/navigation_entry_impl.cc > > M content/browser/web_contents/navigation_entry_impl_unittest.cc > > M content/public/browser/navigation_entry.h > > > > Hi, This is the document that was discussed over email with jam@: https://docs.google.com/a/google.com/document/d/1TJzCnDpC6axW3se1p_WSSSuBBJRO... Thanks
Great, thanks. On Wed, Jan 16, 2013 at 1:54 PM, <mathp@chromium.org> wrote: > On 2013/01/16 21:38:01, Jói wrote: >> >> Please provide a link to the design doc. > > >> Cheers, >> Jói > > > >> On Wed, Jan 16, 2013 at 12:16 PM, <mailto:mathp@chromium.org> wrote: >> > Reviewers: John Abd-El-Malek, sreeram, sky, Jói, >> > >> > Message: >> > Hi, >> > >> > This is ready for review. >> > >> > jam@/joi@: Changes to NavigationEntry's interface, as outlined in design >> > doc. >> > sreeram@: Instant. >> > sky@: sessions/, content/browser and ui/ >> > >> > Thanks >> > >> > Description: >> > [Search] Store and recall search terms using NavigationEntry >> > >> > Adds a new field to NavigationEntry called ExtraData, meant to be used >> > to >> > store >> > extra browser features on a per-need basis. Used here to store extracted >> > search >> > terms. >> > >> > BUG= >> > TEST=NavigationEntryTest, TabNavigationTest >> > >> > >> > Please review this at https://codereview.chromium.org/11876045/ >> > >> > SVN Base: http://git.chromium.org/chromium/src.git%40master >> > >> > Affected files: >> > M chrome/browser/instant/instant_controller.h >> > M chrome/browser/instant/instant_controller.cc >> > M chrome/browser/sessions/session_types.h >> > M chrome/browser/sessions/session_types.cc >> > M chrome/browser/sessions/session_types_unittest.cc >> > M chrome/browser/ui/omnibox/omnibox_edit_model.cc >> > M chrome/browser/ui/toolbar/toolbar_model_impl.cc >> > M content/browser/web_contents/navigation_entry_impl.h >> > M content/browser/web_contents/navigation_entry_impl.cc >> > M content/browser/web_contents/navigation_entry_impl_unittest.cc >> > M content/public/browser/navigation_entry.h >> > >> > > > > Hi, > > This is the document that was discussed over email with jam@: > > https://docs.google.com/a/google.com/document/d/1TJzCnDpC6axW3se1p_WSSSuBBJRO... > > Thanks > > https://chromiumcodereview.appspot.com/11876045/
https://codereview.chromium.org/11876045/diff/8001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/11876045/diff/8001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:553: entry->SetExtraData(kSearchTermsKey, UTF8ToUTF16(query)); Is there a reason we do this here rather than in a WebContentsHelper? https://codereview.chromium.org/11876045/diff/8001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): https://codereview.chromium.org/11876045/diff/8001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:51: static const std::string kSearchTermsKey; static should not be objects, only primitive types. https://codereview.chromium.org/11876045/diff/8001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/11876045/diff/8001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:76: // If search terms were set explicitely on the NavigationEntry, use those. explicitly https://codereview.chromium.org/11876045/diff/8001/content/browser/web_conten... File content/browser/web_contents/navigation_entry_impl.cc (right): https://codereview.chromium.org/11876045/diff/8001/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.cc:277: }; no ; here and 287 https://codereview.chromium.org/11876045/diff/8001/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.cc:282: if (iter == extra_data_.end()) { no {} https://codereview.chromium.org/11876045/diff/8001/content/browser/web_conten... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/8001/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:77: virtual void SetExtraData(const std::string key, const std::string& https://codereview.chromium.org/11876045/diff/8001/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:79: virtual const bool GetExtraData(const std::string key, const std::string& https://codereview.chromium.org/11876045/diff/8001/content/public/browser/nav... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/8001/content/public/browser/nav... content/public/browser/navigation_entry.h:187: virtual void SetExtraData(const std::string key, const string16& data) = 0; con std::string& key
Since John is already reviewing this, my review is redundant; you need his approval anyway for the content/ changes outside of content/public/. Removing myself from the list of reviewers. Cheers, Jói
Thanks for the comments! https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/inst... File chrome/browser/instant/instant_controller.cc (right): https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/inst... chrome/browser/instant/instant_controller.cc:553: entry->SetExtraData(kSearchTermsKey, UTF8ToUTF16(query)); On 2013/01/16 22:32:37, sky wrote: > Is there a reason we do this here rather than in a WebContentsHelper? We set the displayed search terms here because InstantController contains the whole context needed to display the correct search terms. In the lines before this IF statement, we use |last_omnibox_text_| and |last_suggestion_.text| as well as the commit |type| to determine the correct terms to display. This is not context that we would have in a helper, say SearchTabHelper. https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/inst... File chrome/browser/instant/instant_controller.h (right): https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/inst... chrome/browser/instant/instant_controller.h:51: static const std::string kSearchTermsKey; On 2013/01/16 22:32:37, sky wrote: > static should not be objects, only primitive types. Done. https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/ui/t... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/ui/t... chrome/browser/ui/toolbar/toolbar_model_impl.cc:76: // If search terms were set explicitely on the NavigationEntry, use those. On 2013/01/16 22:32:37, sky wrote: > explicitly Done. https://chromiumcodereview.appspot.com/11876045/diff/8001/content/browser/web... File content/browser/web_contents/navigation_entry_impl.cc (right): https://chromiumcodereview.appspot.com/11876045/diff/8001/content/browser/web... content/browser/web_contents/navigation_entry_impl.cc:277: }; On 2013/01/16 22:32:37, sky wrote: > no ; here and 287 Done. https://chromiumcodereview.appspot.com/11876045/diff/8001/content/browser/web... content/browser/web_contents/navigation_entry_impl.cc:282: if (iter == extra_data_.end()) { On 2013/01/16 22:32:37, sky wrote: > no {} Done. https://chromiumcodereview.appspot.com/11876045/diff/8001/content/browser/web... File content/browser/web_contents/navigation_entry_impl.h (right): https://chromiumcodereview.appspot.com/11876045/diff/8001/content/browser/web... content/browser/web_contents/navigation_entry_impl.h:77: virtual void SetExtraData(const std::string key, On 2013/01/16 22:32:37, sky wrote: > const std::string& Done. https://chromiumcodereview.appspot.com/11876045/diff/8001/content/browser/web... content/browser/web_contents/navigation_entry_impl.h:79: virtual const bool GetExtraData(const std::string key, On 2013/01/16 22:32:37, sky wrote: > const std::string& Done. https://chromiumcodereview.appspot.com/11876045/diff/8001/content/public/brow... File content/public/browser/navigation_entry.h (right): https://chromiumcodereview.appspot.com/11876045/diff/8001/content/public/brow... content/public/browser/navigation_entry.h:187: virtual void SetExtraData(const std::string key, const string16& data) = 0; On 2013/01/16 22:32:37, sky wrote: > con std::string& key Done.
content/public lgtm, I defer to other reviewers for the rest (i.e. sky for content). sorry dont have the bandwidth to look at the rest now https://codereview.chromium.org/11876045/diff/18001/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/18001/content/public/browser/na... content/public/browser/navigation_entry.h:186: // Set extra browser data on this NavigationEntry. nit: no "browser"
https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/inst... File chrome/browser/instant/instant_controller.cc (right): https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/inst... chrome/browser/instant/instant_controller.cc:553: entry->SetExtraData(kSearchTermsKey, UTF8ToUTF16(query)); On 2013/01/17 15:59:29, Mathieu Perreault wrote: > On 2013/01/16 22:32:37, sky wrote: > > Is there a reason we do this here rather than in a WebContentsHelper? > > We set the displayed search terms here because InstantController contains the > whole context needed to display the correct search terms. In the lines before > this IF statement, we use |last_omnibox_text_| and |last_suggestion_.text| as > well as the commit |type| to determine the correct terms to display. This is not > context that we would have in a helper, say SearchTabHelper. Make sense. We also persist search terms to the database. See TemplateURLService::UpdateKeywordSearchTermsForURL. Should that set the search term too? If not, then kSearchTermsKey should be named more narrowly.
I mentioned 18 different test cases to you yesterday, but I lied: it's actually 24 :) We should test the cross-product of Start: 1. NTP 2. Committed results page 3. Non-search page with full-height overlay 4. Non-search page with partial-height overlay Search page update: 1. Search page has updated URL to include current search terms (3-second pause on google) 2. Search page has not yet updated URL to include current search terms Commit via: 1. Press enter 2. Click on suggestion 3. Click elsewhere on search page What would be really good is if we actually started codifying these in tests. I'm very close to landing https://codereview.chromium.org/11592004/ (awaiting final OK from sreeram) that'll give us a basic test harness. Can you patch that in and see if it's possible to write a browser test that confirms that there is no flashing in the omnibox? Thanks, Samarth https://codereview.chromium.org/11876045/diff/5012/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/11876045/diff/5012/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:748: // This means a committed page in state search called setValue(). We should Instead of setting temporary text, calling setValue() on a committed page should just update the search terms in the nav entry. https://codereview.chromium.org/11876045/diff/18001/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/11876045/diff/18001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/toolbar_model_impl.cc:76: // If search terms were set explicitly on the NavigationEntry, use those. Factor this out into a GetSearchTermsFromNavigationEntry that may return "".
On Thu, Jan 17, 2013 at 9:23 AM, <sky@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/inst... > File chrome/browser/instant/instant_controller.cc (right): > > https://chromiumcodereview.appspot.com/11876045/diff/8001/chrome/browser/inst... > chrome/browser/instant/instant_controller.cc:553: > entry->SetExtraData(kSearchTermsKey, UTF8ToUTF16(query)); > On 2013/01/17 15:59:29, Mathieu Perreault wrote: >> >> On 2013/01/16 22:32:37, sky wrote: >> > Is there a reason we do this here rather than in a > > WebContentsHelper? > >> We set the displayed search terms here because InstantController > > contains the >> >> whole context needed to display the correct search terms. In the lines > > before >> >> this IF statement, we use |last_omnibox_text_| and > > |last_suggestion_.text| as >> >> well as the commit |type| to determine the correct terms to display. > > This is not >> >> context that we would have in a helper, say SearchTabHelper. > > > Make sense. We also persist search terms to the database. See > TemplateURLService::UpdateKeywordSearchTermsForURL. Should that set the > search term too? If not, then kSearchTermsKey should be named more > narrowly. In thinking about this a bit more I want two things: . kSearchTermsKey should have better docs indicating only stored for default search provider and extracted from instant. . The TemplateURLService code I mentioned should make of this code when possible so that we can keep the db in sync. -Scott
Sreeram agreed that the history code/TemplateURLService will not need to used "fixed" search terms that we display in the omnibox. Eventually, the page updates its URL's #q fragment to contain the correct search terms, and the terms parsed for history will be correct. I've updated the name of the key used in instant_controller.h as well as added documentation to indicate the narrower scope of the extra data field. Addressed comments, going through test scenarios now. https://chromiumcodereview.appspot.com/11876045/diff/18001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/11876045/diff/18001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:76: // If search terms were set explicitly on the NavigationEntry, use those. On 2013/01/17 17:40:45, samarth wrote: > Factor this out into a GetSearchTermsFromNavigationEntry that may return "". Done. https://chromiumcodereview.appspot.com/11876045/diff/18001/content/public/bro... File content/public/browser/navigation_entry.h (right): https://chromiumcodereview.appspot.com/11876045/diff/18001/content/public/bro... content/public/browser/navigation_entry.h:186: // Set extra browser data on this NavigationEntry. On 2013/01/17 16:24:22, John Abd-El-Malek wrote: > nit: no "browser" Done.
https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... File chrome/browser/instant/instant_controller.cc (right): https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... chrome/browser/instant/instant_controller.cc:539: query += UTF16ToUTF8(last_suggestion_.text); This isn't quite right. See https://chromiumcodereview.appspot.com/11415276/#msg3 for my recommendation on how to do it. Specifically, last_suggestion_.text could in fact be a blue highlight, in which case we shouldn't add it to the query (it's already part of last_omnibox_text_). Of course, if there's blue highlight, it means it's a URL, so we won't actually commit an Instant preview, but we shouldn't depend on that to not run into this bug. https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... chrome/browser/instant/instant_controller.cc:549: net::EscapeQueryParamValue(query, true))); Huh. Why are we not using SetExtraData here? https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... chrome/browser/instant/instant_controller.cc:559: NavigationEntryUpdated(); We don't need this anymore, I think. This was originally added to let the SearchTabHelper() know about the fixed up URL (see http://crrev.com/164271). Instead, SearchTabHelper should always just look at not just the URL, but also the active nav entry's search_terms extra data to determine if it should be in MODE_SEARCH_RESULTS. (I.e., you should add that fix to this CL.) https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:671: } I see what you are trying to do here, but I'd like to understand something first: To be clear, the flash we are trying to fix is this: 1. You are on a committed search results page for "foo". The omnibox is therefore showing the extracted search terms, i.e., [foo]. 2. You type "bar" and press Enter quickly. 3. The omnibox flashes "foo" briefly before showing "bar" again. I think what's happening is that view_->RevertAll() goes back to the permanent text, which is what we had in step 1, thus showing "foo". Then, the controller_->OnAutocompleteAccept() call below ends up calling InstantController::CommitIfPossible(), which sends "bar" to the page (via InstantTab), which asynchronously (i.e., at some later time) updates the URL to contain "#q=bar", thus we end up seeing "bar" again in the omnibox. Now, my questions are: 1. Why do we not see this flash when committing the Instant overlay (as opposed to when the query is sent to InstantTab)? 2. More generally, in regular omnibox (i.e., without any Instant whatsoever, without extended mode), say you are on "foo.com" and now type "bar.com" and press Enter. Clearly, we come through this same code path. Why do we not see a flash of "foo.com"? I think the answers to those questions will help me understand whether this is the only way to fix the flash (or say whether we could also fix it by not doing search terms extraction when in_revert_ is true, as opposed to encoding more Instant knowledge into the omnibox edit model).
I've taken a stab at tests, testing for enter commit and "focus lost" commit. I can't seem to make search term extraction from the template URL work (hence why I'm testing for the hack only in the "focus lost" case), but I wanted to get this out there so that sreeram@ and/or samarth@ can comment on the rest. https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... File chrome/browser/instant/instant_controller.cc (right): https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... chrome/browser/instant/instant_controller.cc:539: query += UTF16ToUTF8(last_suggestion_.text); On 2013/01/22 04:34:47, sreeram wrote: > This isn't quite right. See > https://chromiumcodereview.appspot.com/11415276/#msg3 for my recommendation on > how to do it. Specifically, last_suggestion_.text could in fact be a blue > highlight, in which case we shouldn't add it to the query (it's already part of > last_omnibox_text_). > > Of course, if there's blue highlight, it means it's a URL, so we won't actually > commit an Instant preview, but we shouldn't depend on that to not run into this > bug. Done. https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... chrome/browser/instant/instant_controller.cc:549: net::EscapeQueryParamValue(query, true))); On 2013/01/22 04:34:47, sreeram wrote: > Huh. Why are we not using SetExtraData here? There is a case where grey text is showing and last_suggestion_.text is empty or wrong when a suggestion is clicked (a call from the page such as acceptSuggestion would be good). As an example, user types [johnny], " depp" is showing as grey text, but upon clicking on the suggestion "johnny depp movies", CommitIfPossible has no indication that the latter query is the good one (here, |last_omnibox_text_| = "johnny", |last_suggestion_.text| = " depp"). I use the virtual URL hack to add the partial query ("johnny depp"), after which search term extraction will extract the real query. There is no flash since the user very quickly sees the query being formed ("johnny depp" and then "johnny depp movies"). There is also the case where grey text is showing (" depp") and last_suggestion_.text is empty. Another case where the page and chrome are not in sync. Same principle applies. I cannot use SetExtraData because this would mean it is final: the omnibox will favor ExtraData and take it as ground truth. This could result in wrong text in the omnibox, permanently ("johnny depp"). This is making my head spin. I wish we could trust last_suggestion_.text all the time. WDYT? https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... chrome/browser/instant/instant_controller.cc:559: NavigationEntryUpdated(); On 2013/01/22 04:34:47, sreeram wrote: > We don't need this anymore, I think. This was originally added to let the > SearchTabHelper() know about the fixed up URL (see http://crrev.com/164271). > Instead, SearchTabHelper should always just look at not just the URL, but also > the active nav entry's search_terms extra data to determine if it should be in > MODE_SEARCH_RESULTS. (I.e., you should add that fix to this CL.) I've added a check for the search terms in NavigationEntryUpdated(). Since the virtual URL hack is still around (for now), I've kept it as well. Let me know if that sounds reasonable.
On 2013/01/22 23:28:15, Mathieu Perreault wrote: > I've taken a stab at tests, testing for enter commit and "focus lost" commit. I > can't seem to make search term extraction from the template URL work (hence why > I'm testing for the hack only in the "focus lost" case), but I wanted to get > this out there so that sreeram@ and/or samarth@ can comment on the rest. > > https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... > File chrome/browser/instant/instant_controller.cc (right): > > https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... > chrome/browser/instant/instant_controller.cc:539: query += > UTF16ToUTF8(last_suggestion_.text); > On 2013/01/22 04:34:47, sreeram wrote: > > This isn't quite right. See > > https://chromiumcodereview.appspot.com/11415276/#msg3 for my recommendation on > > how to do it. Specifically, last_suggestion_.text could in fact be a blue > > highlight, in which case we shouldn't add it to the query (it's already part > of > > last_omnibox_text_). > > > > Of course, if there's blue highlight, it means it's a URL, so we won't > actually > > commit an Instant preview, but we shouldn't depend on that to not run into > this > > bug. > > Done. > > https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... > chrome/browser/instant/instant_controller.cc:549: > net::EscapeQueryParamValue(query, true))); > On 2013/01/22 04:34:47, sreeram wrote: > > Huh. Why are we not using SetExtraData here? > > There is a case where grey text is showing and last_suggestion_.text is empty or > wrong when a suggestion is clicked (a call from the page such as > acceptSuggestion would be good). > > As an example, user types [johnny], " depp" is showing as grey text, but upon > clicking on the suggestion "johnny depp movies", CommitIfPossible has no > indication that the latter query is the good one (here, |last_omnibox_text_| = > "johnny", |last_suggestion_.text| = " depp"). I use the virtual URL hack to add > the partial query ("johnny depp"), after which search term extraction will > extract the real query. There is no flash since the user very quickly sees the > query being formed ("johnny depp" and then "johnny depp movies"). > > There is also the case where grey text is showing (" depp") and > last_suggestion_.text is empty. Another case where the page and chrome are not > in sync. Same principle applies. > > I cannot use SetExtraData because this would mean it is final: the omnibox will > favor ExtraData and take it as ground truth. This could result in wrong text in > the omnibox, permanently ("johnny depp"). > > This is making my head spin. I wish we could trust last_suggestion_.text all the > time. WDYT? > > https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... > chrome/browser/instant/instant_controller.cc:559: NavigationEntryUpdated(); > On 2013/01/22 04:34:47, sreeram wrote: > > We don't need this anymore, I think. This was originally added to let the > > SearchTabHelper() know about the fixed up URL (see http://crrev.com/164271). > > Instead, SearchTabHelper should always just look at not just the URL, but also > > the active nav entry's search_terms extra data to determine if it should be in > > MODE_SEARCH_RESULTS. (I.e., you should add that fix to this CL.) > > I've added a check for the search terms in NavigationEntryUpdated(). Since the > virtual URL hack is still around (for now), I've kept it as well. Let me know if > that sounds reasonable. Okay, tests work as expected now (based on https://codereview.chromium.org/12035062/ currently in review, allowing HTTPS server in the test). Please have a look at the content of the CL.
On 2013/01/23 22:20:15, Mathieu Perreault wrote: > On 2013/01/22 23:28:15, Mathieu Perreault wrote: > > I've taken a stab at tests, testing for enter commit and "focus lost" commit. > I > > can't seem to make search term extraction from the template URL work (hence > why > > I'm testing for the hack only in the "focus lost" case), but I wanted to get > > this out there so that sreeram@ and/or samarth@ can comment on the rest. > > > > > https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... > > File chrome/browser/instant/instant_controller.cc (right): > > > > > https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... > > chrome/browser/instant/instant_controller.cc:539: query += > > UTF16ToUTF8(last_suggestion_.text); > > On 2013/01/22 04:34:47, sreeram wrote: > > > This isn't quite right. See > > > https://chromiumcodereview.appspot.com/11415276/#msg3 for my recommendation > on > > > how to do it. Specifically, last_suggestion_.text could in fact be a blue > > > highlight, in which case we shouldn't add it to the query (it's already part > > of > > > last_omnibox_text_). > > > > > > Of course, if there's blue highlight, it means it's a URL, so we won't > > actually > > > commit an Instant preview, but we shouldn't depend on that to not run into > > this > > > bug. > > > > Done. > > > > > https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... > > chrome/browser/instant/instant_controller.cc:549: > > net::EscapeQueryParamValue(query, true))); > > On 2013/01/22 04:34:47, sreeram wrote: > > > Huh. Why are we not using SetExtraData here? > > > > There is a case where grey text is showing and last_suggestion_.text is empty > or > > wrong when a suggestion is clicked (a call from the page such as > > acceptSuggestion would be good). > > > > As an example, user types [johnny], " depp" is showing as grey text, but upon > > clicking on the suggestion "johnny depp movies", CommitIfPossible has no > > indication that the latter query is the good one (here, |last_omnibox_text_| = > > "johnny", |last_suggestion_.text| = " depp"). I use the virtual URL hack to > add > > the partial query ("johnny depp"), after which search term extraction will > > extract the real query. There is no flash since the user very quickly sees the > > query being formed ("johnny depp" and then "johnny depp movies"). > > > > There is also the case where grey text is showing (" depp") and > > last_suggestion_.text is empty. Another case where the page and chrome are not > > in sync. Same principle applies. > > > > I cannot use SetExtraData because this would mean it is final: the omnibox > will > > favor ExtraData and take it as ground truth. This could result in wrong text > in > > the omnibox, permanently ("johnny depp"). > > > > This is making my head spin. I wish we could trust last_suggestion_.text all > the > > time. WDYT? > > > > > https://chromiumcodereview.appspot.com/11876045/diff/24001/chrome/browser/ins... > > chrome/browser/instant/instant_controller.cc:559: NavigationEntryUpdated(); > > On 2013/01/22 04:34:47, sreeram wrote: > > > We don't need this anymore, I think. This was originally added to let the > > > SearchTabHelper() know about the fixed up URL (see http://crrev.com/164271). > > > Instead, SearchTabHelper should always just look at not just the URL, but > also > > > the active nav entry's search_terms extra data to determine if it should be > in > > > MODE_SEARCH_RESULTS. (I.e., you should add that fix to this CL.) > > > > I've added a check for the search terms in NavigationEntryUpdated(). Since the > > virtual URL hack is still around (for now), I've kept it as well. Let me know > if > > that sounds reasonable. > > Okay, tests work as expected now (based on > https://codereview.chromium.org/12035062/ currently in review, allowing HTTPS > server in the test). Please have a look at the content of the CL. No longer based on other CL. HTTPS server integrated in InstantTestBase. sreeram@, please have a look.
I don't follow your arguments for continuing to use virtual URL. Okay, so in some cases, last_omnibox_text_ is wrong or last_suggestion_ is wrong. Those are being fixed in your other pending CL (https://codereview.chromium.org/12035083/). Regardless, if you are going to put query terms Q into the virtual URL, you might as well just do SetExtraData(Q) instead. Aside, I had posed some questions earlier ("Now, my questions are:"). Consider those resolved. I understand what's going on now. So, I think we can achieve these in this CL: 1. No virtual URL hacks. 2. No changes to omnibox_edit_model.cc, and thus no need to expose new accessors in InstantController.
We do need one extra fix in this CL, to handle the following scenario (the same scenario I described in comment #13 in this thread): 1. Say you are on a search results page for "foo". --> The active nav entry's URL is "...#q=foo". Omnibox is showing "foo". 2. You type "bar" and press Enter quickly. --> InstantController intercepts the OnAutocompleteAccept() and sends it as a Submit() call through to the page (i.e., through instant_tab_). --> At this point, if we don't do anything else, the omnibox will show "foo", since it did a view_->RevertAll() just before calling OnAutocompleteAccept(). 3. Eventually, the page will receive the Submit() IPC and cause a new nav entry with "...#q=bar". So, the fix is to SetExtraData(bar) in step 2. However, we can't do that to the *current* nav entry. Otherwise, after step 3, if you hit "Back", you'd end up on the search results page from step 1, but with the omnibox still showing "bar". So, a neat solution here is to add a transient nav entry and set *its* extra data to "bar". For convenience, we can just clone the current nav entry and just set the new extra data (if the user hits Reload before step 3 happens, we'll reload the page from step 1, but that's okay. I don't want to play games with virtual URLs and such). When the page causes the new nav in step 3, the transient entry is automatically discarded. Jered is working in this same "transient entry" space in https://codereview.chromium.org/12001002/.
On 2013/01/25 20:39:01, sreeram wrote: > We do need one extra fix in this CL, to handle the following scenario (the same > scenario I described in comment #13 in this thread): > > 1. Say you are on a search results page for "foo". > --> The active nav entry's URL is "...#q=foo". Omnibox is showing "foo". > 2. You type "bar" and press Enter quickly. > --> InstantController intercepts the OnAutocompleteAccept() and sends it as a > Submit() call through to the page (i.e., through instant_tab_). > --> At this point, if we don't do anything else, the omnibox will show "foo", > since it did a view_->RevertAll() just before calling OnAutocompleteAccept(). > 3. Eventually, the page will receive the Submit() IPC and cause a new nav entry > with "...#q=bar". > > So, the fix is to SetExtraData(bar) in step 2. However, we can't do that to the > *current* nav entry. Otherwise, after step 3, if you hit "Back", you'd end up on > the search results page from step 1, but with the omnibox still showing "bar". > > So, a neat solution here is to add a transient nav entry and set *its* extra > data to "bar". For convenience, we can just clone the current nav entry and just > set the new extra data (if the user hits Reload before step 3 happens, we'll > reload the page from step 1, but that's okay. I don't want to play games with > virtual URLs and such). When the page causes the new nav in step 3, the > transient entry is automatically discarded. > > Jered is working in this same "transient entry" space in > https://codereview.chromium.org/12001002/. Couple of points: - Not preventing the "Revert" in omnibox_edit_model.cc is necessarily going to cause a flash of the old terms, whether we correct them or not in CommitIfPossible right after (even with a transient entry). Revert goes to the view, and the view takes the permanent text that it has in its model. That was why my fix was to set the text to |last_omnibox_text_| right there in omnibox_edit_model, and the need for the accessor. - I think we can probably make it work without virtual URL hacks. I have a solution now that works with both the cases of clicking on the page and clicking on a suggestion. It involves creating a transient entry on any normal commit (not from a committed page) with the terms in |last_omnibox_text_|. When the page is eventually updated with the correct terms, the omnibox updates. I don't like this solution, as it is a 2-step process (partial terms, then correct terms). We should chat face to face about this.
On 2013/01/30 16:19:50, Mathieu Perreault wrote: > - Not preventing the "Revert" in omnibox_edit_model.cc is necessarily going to > cause a flash of the old terms, whether we correct them or not in > CommitIfPossible right after (even with a transient entry). Revert goes to the > view, and the view takes the permanent text that it has in its model. That was > why my fix was to set the text to |last_omnibox_text_| right there in > omnibox_edit_model, and the need for the accessor. No. The flash occurs only when the code yields to the event loop. Consider this scenario (without Instant): 1. You are on a page whose permanent text is "foo.com". 2. You edit the omnibox and type "bar.com" and press Enter. --> Revert() is called, which reverts the view to "foo.com". --> But, in the same sequence of calls, OnAutocompleteAccept() is called, which makes the text "bar.com" again. --> No flash. So, please read comment #18 above (jn the code review thread) on why there's a flash in the Instant case, and what we need to do to fix it. > - I think we can probably make it work without virtual URL hacks. I have a > solution now that works with both the cases of clicking on the page and clicking > on a suggestion. It involves creating a transient entry on any normal commit > (not from a committed page) with the terms in |last_omnibox_text_|. When the > page is eventually updated with the correct terms, the omnibox updates. I don't > like this solution, as it is a 2-step process (partial terms, then correct > terms). We should chat face to face about this. There's no way to avoid "partial terms" if you are clicking on a suggestion on the page (since the browser has no idea what you clicked on until the page tells us). That's fine. I don't understand the "not from a committed page" part. We should do this even if we are talking to a committed page (InstantTab). In fact, comment #18 is talking exactly about that scenario. But yes, let's talk on VC and clear this up.
On 2013/01/30 17:22:35, sreeram wrote: > On 2013/01/30 16:19:50, Mathieu Perreault wrote: > > - Not preventing the "Revert" in omnibox_edit_model.cc is necessarily going to > > cause a flash of the old terms, whether we correct them or not in > > CommitIfPossible right after (even with a transient entry). Revert goes to the > > view, and the view takes the permanent text that it has in its model. That was > > why my fix was to set the text to |last_omnibox_text_| right there in > > omnibox_edit_model, and the need for the accessor. > > No. The flash occurs only when the code yields to the event loop. Consider this > scenario (without Instant): > 1. You are on a page whose permanent text is "foo.com". > 2. You edit the omnibox and type "bar.com" and press Enter. > --> Revert() is called, which reverts the view to "foo.com". > --> But, in the same sequence of calls, OnAutocompleteAccept() is called, which > makes the text "bar.com" again. > --> No flash. > > So, please read comment #18 above (jn the code review thread) on why there's a > flash in the Instant case, and what we need to do to fix it. > > > - I think we can probably make it work without virtual URL hacks. I have a > > solution now that works with both the cases of clicking on the page and > clicking > > on a suggestion. It involves creating a transient entry on any normal commit > > (not from a committed page) with the terms in |last_omnibox_text_|. When the > > page is eventually updated with the correct terms, the omnibox updates. I > don't > > like this solution, as it is a 2-step process (partial terms, then correct > > terms). We should chat face to face about this. > > There's no way to avoid "partial terms" if you are clicking on a suggestion on > the page (since the browser has no idea what you clicked on until the page tells > us). That's fine. I don't understand the "not from a committed page" part. We > should do this even if we are talking to a committed page (InstantTab). In fact, > comment #18 is talking exactly about that scenario. > > But yes, let's talk on VC and clear this up. sky@: Reduced the scope of this CL to make some progress. Kept only the interface change to NavigationEntry. Please have a look.
https://codereview.chromium.org/11876045/diff/44001/chrome/browser/sessions/s... File chrome/browser/sessions/session_types.cc (right): https://codereview.chromium.org/11876045/diff/44001/chrome/browser/sessions/s... chrome/browser/sessions/session_types.cc:315: search_terms_ = string16(); Nit: search_terms_.clear(); https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:9: #include "base/string16.h" Nit: No need for this (since it's already included in search.h). https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:154: return string16(); Nit: If you rewrite this as the following, it will help the compiler perform NRVO: string16 search_terms; if (!entry->GetExtraData(std::string(kInstantExtendedSearchTermsKey), &search_terms)) search_terms.clear(); return search_terms; In fact, I'd drop the 'if' check and std::string conversions and just do this: string16 search_terms; entry->GetExtraData(kInstantExtendedSearchTermsKey, &search_terms); return search_terms; https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.h (right): https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... chrome/browser/ui/search/search.h:17: namespace content { Nit: Add a blank line between "class Profile" and "namespace content {". https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:10: #include "base/values.h" Remove this? (it appears to be unused) https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:82: string16* out_value) const OVERRIDE; virtual bool GetExtraData(const std::string& key, string16* data) const OVERRIDE; https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:274: typedef std::map<std::string, string16> ExtraDataMap; This typedef seems unnecessary, given that it's only used in one place. https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl_unittest.cc (right): https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl_unittest.cc:229: EXPECT_EQ(output, test_data); Swap the EXPECT_EQ arguments (here and on line 227). The convention is EXPECT_EQ(expected, actual). https://codereview.chromium.org/11876045/diff/44001/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/44001/content/public/browser/na... content/public/browser/navigation_entry.h:194: virtual const bool GetExtraData(const std::string& key, string16* out_value) virtual bool GetExtraData(const std::string& key, string16* data) const = 0;
Thanks for the fast review! https://codereview.chromium.org/11876045/diff/44001/chrome/browser/sessions/s... File chrome/browser/sessions/session_types.cc (right): https://codereview.chromium.org/11876045/diff/44001/chrome/browser/sessions/s... chrome/browser/sessions/session_types.cc:315: search_terms_ = string16(); On 2013/01/30 21:41:39, sreeram wrote: > Nit: search_terms_.clear(); Done. https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:9: #include "base/string16.h" On 2013/01/30 21:41:39, sreeram wrote: > Nit: No need for this (since it's already included in search.h). Done. https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:154: return string16(); On 2013/01/30 21:41:39, sreeram wrote: > Nit: If you rewrite this as the following, it will help the compiler perform > NRVO: > string16 search_terms; > if (!entry->GetExtraData(std::string(kInstantExtendedSearchTermsKey), > &search_terms)) > search_terms.clear(); > return search_terms; > > In fact, I'd drop the 'if' check and std::string conversions and just do this: > string16 search_terms; > entry->GetExtraData(kInstantExtendedSearchTermsKey, &search_terms); > return search_terms; Done. https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.h (right): https://codereview.chromium.org/11876045/diff/44001/chrome/browser/ui/search/... chrome/browser/ui/search/search.h:17: namespace content { On 2013/01/30 21:41:39, sreeram wrote: > Nit: Add a blank line between "class Profile" and "namespace content {". Done. https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:10: #include "base/values.h" On 2013/01/30 21:41:39, sreeram wrote: > Remove this? (it appears to be unused) Done. https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:82: string16* out_value) const OVERRIDE; On 2013/01/30 21:41:39, sreeram wrote: > virtual bool GetExtraData(const std::string& key, string16* data) const > OVERRIDE; Done. Renamed throughout. https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:274: typedef std::map<std::string, string16> ExtraDataMap; On 2013/01/30 21:41:39, sreeram wrote: > This typedef seems unnecessary, given that it's only used in one place. Done. https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl_unittest.cc (right): https://codereview.chromium.org/11876045/diff/44001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl_unittest.cc:229: EXPECT_EQ(output, test_data); On 2013/01/30 21:41:39, sreeram wrote: > Swap the EXPECT_EQ arguments (here and on line 227). The convention is > EXPECT_EQ(expected, actual). Done. Sorry I missed this. https://codereview.chromium.org/11876045/diff/44001/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/44001/content/public/browser/na... content/public/browser/navigation_entry.h:194: virtual const bool GetExtraData(const std::string& key, string16* out_value) On 2013/01/30 21:41:39, sreeram wrote: > virtual bool GetExtraData(const std::string& key, string16* data) const = 0; Done.
https://codereview.chromium.org/11876045/diff/41003/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/41003/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:81: string16* data) const OVERRIDE; Drop the const in "virtual const bool" (here and everywhere else). It adds no value.
Thanks! https://codereview.chromium.org/11876045/diff/41003/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/41003/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:81: string16* data) const OVERRIDE; On 2013/01/30 22:04:05, sreeram wrote: > Drop the const in "virtual const bool" (here and everywhere else). It adds no > value. Done.
https://codereview.chromium.org/11876045/diff/54002/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.cc (right): https://codereview.chromium.org/11876045/diff/54002/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.cc:288: string16* data) const { Align to previous line. https://codereview.chromium.org/11876045/diff/54002/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/54002/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:81: string16* data) const OVERRIDE; Align this to the "const std::string&" on the previous line. https://codereview.chromium.org/11876045/diff/54002/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/54002/content/public/browser/na... content/public/browser/navigation_entry.h:195: const = 0; This fits on the previous line.
Thanks. Sorry about that. https://codereview.chromium.org/11876045/diff/54002/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.cc (right): https://codereview.chromium.org/11876045/diff/54002/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.cc:288: string16* data) const { On 2013/01/30 22:22:53, sreeram wrote: > Align to previous line. Done. https://codereview.chromium.org/11876045/diff/54002/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/54002/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:81: string16* data) const OVERRIDE; On 2013/01/30 22:22:53, sreeram wrote: > Align this to the "const std::string&" on the previous line. Done. https://codereview.chromium.org/11876045/diff/54002/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/54002/content/public/browser/na... content/public/browser/navigation_entry.h:195: const = 0; On 2013/01/30 22:22:53, sreeram wrote: > This fits on the previous line. Done.
lgtm You still need OWNERS approval, though.
On 2013/01/30 22:35:33, sreeram wrote: > lgtm > > You still need OWNERS approval, though. Thanks. sky@: Please take a look.
https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:273: std::map<std::string, string16> extra_data_; Do you see the big warning on 182? Do we not need this data persisted? https://codereview.chromium.org/11876045/diff/57001/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/57001/content/public/browser/na... content/public/browser/navigation_entry.h:194: virtual bool GetExtraData(const std::string& key, string16* data) const = 0; Document return value. Also, how does one clear a key?
https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:273: std::map<std::string, string16> extra_data_; On 2013/01/30 23:32:03, sky wrote: > Do you see the big warning on 182? Do we not need this data persisted? Yes, hence the changes to chrome/browser/sessions/* in this CL. However, the warning has something new about Android WebView state_serializer that wasn't there a month or two ago. @mathp: Though we don't do query terms extraction for Android, I think we should make those changes now, so that when Android eventually catches up, we won't forget to. Also, just to confirm: The c/b/sessions stuff handles TabRestore as well, right?
https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:273: std::map<std::string, string16> extra_data_; On 2013/01/30 23:39:05, sreeram wrote: > On 2013/01/30 23:32:03, sky wrote: > > Do you see the big warning on 182? Do we not need this data persisted? > > Yes, hence the changes to chrome/browser/sessions/* in this CL. > > However, the warning has something new about Android WebView state_serializer > that wasn't there a month or two ago. @mathp: Though we don't do query terms > extraction for Android, I think we should make those changes now, so that when > Android eventually catches up, we won't forget to. > > Also, just to confirm: The c/b/sessions stuff handles TabRestore as well, right? That only persists the session terms, not the map.
https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:273: std::map<std::string, string16> extra_data_; On 2013/01/31 01:34:15, sky wrote: > That only persists the session terms, not the map. Right. The map is meant to be a generic bucket for higher level modules (chrome/...). I don't think we want to assume that everything in the map needs to be persisted. Stuff that should be persisted should be handled specifically (since, for example, we'll need to add specific fields to the sync protobufs). Of course, now that it's easy to add data to this map without modifying the header, future users of needs-to-persist data may not see the warning, so they may not realize that they need to take extra steps to persist their data. Options: 1. Repeat the warning here, to the effect that just adding data to the map won't automatically persist it. 2. Add a specific field for query terms, instead of this generic map. I think @jam preferred the map, though. Your thoughts?
On 2013/01/30 23:39:04, sreeram wrote: > https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... > File content/browser/web_contents/navigation_entry_impl.h (right): > > https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... > content/browser/web_contents/navigation_entry_impl.h:273: std::map<std::string, > string16> extra_data_; > On 2013/01/30 23:32:03, sky wrote: > > Do you see the big warning on 182? Do we not need this data persisted? > > Yes, hence the changes to chrome/browser/sessions/* in this CL. > > However, the warning has something new about Android WebView state_serializer > that wasn't there a month or two ago. @mathp: Though we don't do query terms > extraction for Android, I think we should make those changes now, so that when > Android eventually catches up, we won't forget to. > > Also, just to confirm: The c/b/sessions stuff handles TabRestore as well, right? Indeed we persist just the search terms. I didn't want to persist the whole map as this would be a backdoor to have many things synced without approval or discussion (I had to convince the sync team that syncing search terms was worthwhile). As for Android, on a previous CL, the Android WebView people specifically expressed that they felt it should not be persisted: https://codereview.chromium.org/11415292/#msg24
Thanks. https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11876045/diff/57001/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:273: std::map<std::string, string16> extra_data_; On 2013/01/31 02:14:11, sreeram wrote: > On 2013/01/31 01:34:15, sky wrote: > > That only persists the session terms, not the map. > > Right. The map is meant to be a generic bucket for higher level modules > (chrome/...). I don't think we want to assume that everything in the map needs > to be persisted. Stuff that should be persisted should be handled specifically > (since, for example, we'll need to add specific fields to the sync protobufs). > Of course, now that it's easy to add data to this map without modifying the > header, future users of needs-to-persist data may not see the warning, so they > may not realize that they need to take extra steps to persist their data. > > Options: > 1. Repeat the warning here, to the effect that just adding data to the map won't > automatically persist it. > 2. Add a specific field for query terms, instead of this generic map. I think > @jam preferred the map, though. > > Your thoughts? Option 1 is preferred. I added a warning. https://codereview.chromium.org/11876045/diff/57001/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/57001/content/public/browser/na... content/public/browser/navigation_entry.h:194: virtual bool GetExtraData(const std::string& key, string16* data) const = 0; On 2013/01/30 23:32:03, sky wrote: > Document return value. Also, how does one clear a key? Done. I would say clearing is the responsibility of the caller. In this case, search_terms gets clear()'ed in the Session service and is persisted that way.
I'm OK with not arbitrarily persisting, but that isn't at all documented. https://codereview.chromium.org/11876045/diff/57001/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/57001/content/public/browser/na... content/public/browser/navigation_entry.h:194: virtual bool GetExtraData(const std::string& key, string16* data) const = 0; On 2013/01/31 14:45:32, Mathieu Perreault wrote: > On 2013/01/30 23:32:03, sky wrote: > > Document return value. Also, how does one clear a key? > > Done. I would say clearing is the responsibility of the caller. In this case, > search_terms gets clear()'ed in the Session service and is persisted that way. There is no clear, only SetExtraData(key, ""), which is not the same as clearing. That's my point. How does one clear a value?
Hi Scott, I documented that the extra_data_ field is not persisted, similar to other fields that are not persisted in navigation_entry.h. I pointed to TabNavigation where one can get an example of how specific data is persisted (the search_terms). As for clearing in the map, I assume you were suggesting to add a method to clear the data for a specific key. I added that and updated the test. Thanks!
LGTM https://codereview.chromium.org/11876045/diff/47004/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/47004/content/public/browser/na... content/public/browser/navigation_entry.h:192: // Set extra data on this NavigationEntry according to the specified |key|. Please add a comment here that this data is not persisted by default.
https://codereview.chromium.org/11876045/diff/47004/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11876045/diff/47004/content/public/browser/na... content/public/browser/navigation_entry.h:192: // Set extra data on this NavigationEntry according to the specified |key|. On 2013/01/31 20:52:15, sky wrote: > Please add a comment here that this data is not persisted by default. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/11876045/57003
Failed to trigger a try job on android_clang_dbg HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/11876045/61001
Message was sent while issue was closed.
Change committed as 180020 |
