Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13963014:
Local omnibox treats navsuggest suggestions as queries. (Closed)
|
Created:
7 years, 8 months ago by dougw Modified:
7 years, 7 months ago CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, dougw+watch_chromium.org, gideonwald, dominich, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLocal omnibox treats navsuggest suggestions as queries.
Correctly identify matches that should display the search query.
BUG=231423
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197551
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 6
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 2
Patch Set 12 : #Patch Set 13 : #
Total comments: 2
Patch Set 14 : #Patch Set 15 : #
Total comments: 2
Patch Set 16 : Loop removed. #Patch Set 17 : Revert renaming change (now in a separate cl). #
Total comments: 16
Patch Set 18 : #
Total comments: 8
Patch Set 19 : #
Total comments: 1
Patch Set 20 : #
Messages
Total messages: 29 (0 generated)
https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:581: match->type == AutocompleteMatch::SEARCH_OTHER_ENGINE)) Why not just AutocompleteMatch::IsSearchType()? I also don't see why we need to keep the "from_search_provider" check. Please also add a test of some sort.
On 2013/04/16 17:28:26, sreeram wrote: > https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... > File chrome/browser/ui/search/instant_controller.cc (right): > > https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... > chrome/browser/ui/search/instant_controller.cc:581: match->type == > AutocompleteMatch::SEARCH_OTHER_ENGINE)) > Why not just AutocompleteMatch::IsSearchType()? I also don't see why we need to > keep the "from_search_provider" check. > > Please also add a test of some sort. I thought I'd test chrome.embeddedSearch.searchBox.nativeSuggestions, since there doesn't seem to be a readily accessible structure between it and InstantController::HandleAutocompleteResults. However, ExecuteScript fails when I return ("Uncaught SyntaxError: Illegal return statement"). It looks like it should be possible to return a value (http://selenium.googlecode.com/svn/trunk/docs/api/dotnet/html/M_OpenQA_Seleni...), but I can't seem to find an example. Can you point me to one or suggest an alternative approach?
On 2013/04/16 23:23:27, dougw wrote:
> However, ExecuteScript fails when I return ("Uncaught SyntaxError: Illegal
> return statement"). It looks like it should be possible to return a value
>
(http://selenium.googlecode.com/svn/trunk/docs/api/dotnet/html/M_OpenQA_Seleni...),
> but I can't seem to find an example. Can you point me to one or suggest an
> alternative approach?
Use GetIntFromJS(), GetStringFromJS(), etc., defined earlier in the same test
file.
Just tried it out and it looks good! https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:581: match->type == AutocompleteMatch::SEARCH_OTHER_ENGINE)) On 2013/04/16 17:28:27, sreeram wrote: > Why not just AutocompleteMatch::IsSearchType()? I also don't see why we need to > keep the "from_search_provider" check. > > Please also add a test of some sort. Sreeram: do you have any ideas for how to write a test for this that won't be flaky? Unless we have a way to mock out stuff and write a proper test, I think the test will be almost certainly be flaky. https://codereview.chromium.org/13963014/diff/5001/chrome/browser/ui/search/i... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13963014/diff/5001/chrome/browser/ui/search/i... chrome/browser/ui/search/instant_controller.cc:576: // If the search query should be displayed. nit: this comment isn't very useful. How about something like "Setting the search_query field tells the Instant page to treat the suggestion as a query." Or you could just leave this out altogether. Ideally we'd test this function directly by passing in a list of matches and checking the "output", but that's not really feasible today.
https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:581: match->type == AutocompleteMatch::SEARCH_OTHER_ENGINE)) On 2013/04/17 00:39:13, samarth wrote: > On 2013/04/16 17:28:27, sreeram wrote: > > Why not just AutocompleteMatch::IsSearchType()? I also don't see why we need > to > > keep the "from_search_provider" check. > > > > Please also add a test of some sort. > > Sreeram: do you have any ideas for how to write a test for this that won't be > flaky? Unless we have a way to mock out stuff and write a proper test, I think > the test will be almost certainly be flaky. How about something like this: SetOmniboxText("foo"); instant()->SetSuggestions(a nav suggestion); GetBoolFromJS(nativeSuggestions[0].is_search, &value);
I have that approach mostly implemented (now uploaded). It currently hangs on line 838 (the GetBoolFromJS for is_search, which is not defined, and unlike the first ExecuteScript, the second ExecuteScript isn't detecting that it isn't defined). Re: potential flakiness: I don't think it will return false positives, but it does depend on a navsuggest match being included. A navsuggest match isn't currently included, but I assume I can fix that. However, following this approach, we can't guarantee that one always will (unless we inject into InstantController::Update). On 2013/04/17 01:29:14, sreeram wrote: > https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... > File chrome/browser/ui/search/instant_controller.cc (right): > > https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... > chrome/browser/ui/search/instant_controller.cc:581: match->type == > AutocompleteMatch::SEARCH_OTHER_ENGINE)) > On 2013/04/17 00:39:13, samarth wrote: > > On 2013/04/16 17:28:27, sreeram wrote: > > > Why not just AutocompleteMatch::IsSearchType()? I also don't see why we need > > to > > > keep the "from_search_provider" check. > > > > > > Please also add a test of some sort. > > > > Sreeram: do you have any ideas for how to write a test for this that won't be > > flaky? Unless we have a way to mock out stuff and write a proper test, I > think > > the test will be almost certainly be flaky. > > How about something like this: > SetOmniboxText("foo"); > instant()->SetSuggestions(a nav suggestion); > GetBoolFromJS(nativeSuggestions[0].is_search, &value);
https://codereview.chromium.org/13963014/diff/12002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/13963014/diff/12002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:850: } This test uses the server overlay. We don't send any SearchProvider results to a server overlay, so I don't know how you'd expect any of this to work. You should cause the local overlay to be used (for example, by calling SetInstantEnabled() with the right values), and then test that.
https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:581: match->type == AutocompleteMatch::SEARCH_OTHER_ENGINE)) On 2013/04/16 17:28:27, sreeram wrote: > Why not just AutocompleteMatch::IsSearchType()? I also don't see why we need to > keep the "from_search_provider" check. > > Please also add a test of some sort. Done. https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:581: match->type == AutocompleteMatch::SEARCH_OTHER_ENGINE)) On 2013/04/17 01:29:14, sreeram wrote: > On 2013/04/17 00:39:13, samarth wrote: > > On 2013/04/16 17:28:27, sreeram wrote: > > > Why not just AutocompleteMatch::IsSearchType()? I also don't see why we need > > to > > > keep the "from_search_provider" check. > > > > > > Please also add a test of some sort. > > > > Sreeram: do you have any ideas for how to write a test for this that won't be > > flaky? Unless we have a way to mock out stuff and write a proper test, I > think > > the test will be almost certainly be flaky. > > How about something like this: > SetOmniboxText("foo"); > instant()->SetSuggestions(a nav suggestion); > GetBoolFromJS(nativeSuggestions[0].is_search, &value); Done. https://codereview.chromium.org/13963014/diff/5001/chrome/browser/ui/search/i... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13963014/diff/5001/chrome/browser/ui/search/i... chrome/browser/ui/search/instant_controller.cc:576: // If the search query should be displayed. On 2013/04/17 00:39:13, samarth wrote: > nit: this comment isn't very useful. How about something like "Setting the > search_query field tells the Instant page to treat the suggestion as a query." > Or you could just leave this out altogether. > > Ideally we'd test this function directly by passing in a list of matches and > checking the "output", but that's not really feasible today. Done. https://codereview.chromium.org/13963014/diff/12002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/13963014/diff/12002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:850: } On 2013/04/18 17:51:40, sreeram wrote: > This test uses the server overlay. We don't send any SearchProvider results to a > server overlay, so I don't know how you'd expect any of this to work. > > You should cause the local overlay to be used (for example, by calling > SetInstantEnabled() with the right values), and then test that. Done.
ping? https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13963014/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:581: match->type == AutocompleteMatch::SEARCH_OTHER_ENGINE)) On 2013/04/16 17:28:27, sreeram wrote: > Why not just AutocompleteMatch::IsSearchType()? I also don't see why we need to > keep the "from_search_provider" check. > > Please also add a test of some sort. Done.
Can you also rebase? Thanks, Samarth https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:834: INSTANT_COMPLETE_NOW, If you're setting INSTANT_COMPLETE_NOW (aka doing a blue-highlight), shouldn't this always be the top suggestion? Does this not work without the loop below?
https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:833: InstantSuggestion(ASCIIToUTF16("face"), Please set a valid URL (e.g.: "http://www.facemash.com/"). Otherwise, there's no reason to expect that it will be accepted as a navsuggestion. https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:871: } I don't understand why you need this loop at all. A navsuggestion will be ranked at the top, since there are no history matches to outscore it. So, all you have to do is check that the top nativesuggestion is the expected navsuggestion, and that it doesn't have is_query set. I mentioned this way back in comment #6 on the review thread: GetBoolFromJS(nativeSuggestions[0].is_search, &value); At line 838, after setting the navsuggestion, you might need to wait for the autocomplete results to be sent. See this pattern elsewhere in this file: // Wait for all autocomplete results to be sent to the page while (!omnibox()->model()->autocomplete_controller()->done()) { ... } If that doesn't work, please explain why.
https://codereview.chromium.org/13963014/diff/46001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/13963014/diff/46001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:849: InstantSuggestion(ASCIIToUTF16("http://cars.com"), This may be the problem. The query is not a prefix of the navsuggestion, so I suspect it's getting dropped somewhere. Why don't you use the earlier face/facemash.com example we discussed in an earlier comment?
https://codereview.chromium.org/13963014/diff/46001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/13963014/diff/46001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:849: InstantSuggestion(ASCIIToUTF16("http://cars.com"), I also tried that case (now uploaded). nativeSuggestions is empty. Also tried several variations on it - e.g. with "http://facemash.com" also for the query parameter to InstantSuggestion (which hangs). On 2013/04/25 01:44:01, sreeram wrote: > This may be the problem. The query is not a prefix of the navsuggestion, so I > suspect it's getting dropped somewhere. Why don't you use the earlier > http://face/facemash.com example we discussed in an earlier comment?
https://codereview.chromium.org/13963014/diff/54001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/13963014/diff/54001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:841: ASSERT_TRUE(SetOmniboxTextAndWaitForOverlayToShow("http://face")); Still wrong. The query should be "face". The suggestion should be "http://facemash.com/". Try that. Also, why WaitForOverlayToShow()? Just do SetOmniboxText().
https://codereview.chromium.org/13963014/diff/54001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/13963014/diff/54001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:841: ASSERT_TRUE(SetOmniboxTextAndWaitForOverlayToShow("http://face")); On 2013/04/25 02:02:25, sreeram wrote: > Still wrong. The query should be "face". The suggestion should be > "http://facemash.com/". Try that. > > Also, why WaitForOverlayToShow()? Just do SetOmniboxText(). Done.
https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:871: } A search-what-you-typed suggestion appears above the navsuggest suggestion. I could modify to specifically check the second suggestion, but the loop also helps to future-proof the test. One other thing: The test doesn't fail if a navsuggest suggestion is not encountered. I assume this is the preferred behavior. On 2013/04/22 16:25:49, sreeram wrote: > I don't understand why you need this loop at all. A navsuggestion will be ranked > at the top, since there are no history matches to outscore it. So, all you have > to do is check that the top nativesuggestion is the expected navsuggestion, and > that it doesn't have is_query set. I mentioned this way back in comment #6 on > the review thread: > GetBoolFromJS(nativeSuggestions[0].is_search, &value); > > At line 838, after setting the navsuggestion, you might need to wait for the > autocomplete results to be sent. See this pattern elsewhere in this file: > // Wait for all autocomplete results to be sent to the page > while (!omnibox()->model()->autocomplete_controller()->done()) { > ... > } > > If that doesn't work, please explain why.
https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:871: } On 2013/04/26 18:38:54, dougw wrote: > A search-what-you-typed suggestion appears above the navsuggest suggestion. I > could modify to specifically check the second suggestion, but the loop also > helps to future-proof the test. I think that's because the suggestions aren't in sorted order by default. If you look at the debug output, it's something like: AutocompleteResults: 1300 search-what-you-typed Search https://127.0.0.1:57692/files/instant_extended.html?strk=1&q=face '' 'face' 5 1301 navsuggest Search http://facemash.com/ '' '' 1 So the navsuggest is indeed ranking above the SWYT suggestion. You should be able to 1) sort the nativeSuggestions array by score in JS 2) ASSERT that the top suggestion is a navsuggest 3) EXPECT that it is not a search suggestion > > One other thing: The test doesn't fail if a navsuggest suggestion is not > encountered. I assume this is the preferred behavior. That's not so great because then it's possible for this behavior to break without this test failing. If this test becomes no longer sufficient in the future, it's best to know earlier rather than later. > > > On 2013/04/22 16:25:49, sreeram wrote: > > I don't understand why you need this loop at all. A navsuggestion will be > ranked > > at the top, since there are no history matches to outscore it. So, all you > have > > to do is check that the top nativesuggestion is the expected navsuggestion, > and > > that it doesn't have is_query set. I mentioned this way back in comment #6 on > > the review thread: > > GetBoolFromJS(nativeSuggestions[0].is_search, &value); > > > > At line 838, after setting the navsuggestion, you might need to wait for the > > autocomplete results to be sent. See this pattern elsewhere in this file: > > // Wait for all autocomplete results to be sent to the page > > while (!omnibox()->model()->autocomplete_controller()->done()) { > > ... > > } > > > > If that doesn't work, please explain why. > https://codereview.chromium.org/13963014/diff/58001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_test_utils.h (right): https://codereview.chromium.org/13963014/diff/58001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_test_utils.h:91: void FocusOmniboxAndWaitForInstantOverlaySupport(); Can you please document this change in the change description. Or probably better yet, move it to a separate no-op change?
https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/13963014/diff/21004/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:871: } On 2013/04/29 17:49:20, samarth wrote: > On 2013/04/26 18:38:54, dougw wrote: > > A search-what-you-typed suggestion appears above the navsuggest suggestion. I > > could modify to specifically check the second suggestion, but the loop also > > helps to future-proof the test. > > I think that's because the suggestions aren't in sorted order by default. If > you look at the debug output, it's something like: > > AutocompleteResults: > 1300 search-what-you-typed Search > https://127.0.0.1:57692/files/instant_extended.html?strk=1&q=face '' 'face' 5 > 1301 navsuggest Search http://facemash.com/ '' '' 1 > > So the navsuggest is indeed ranking above the SWYT suggestion. You should be > able to > 1) sort the nativeSuggestions array by score in JS > 2) ASSERT that the top suggestion is a navsuggest > 3) EXPECT that it is not a search suggestion > > > > > One other thing: The test doesn't fail if a navsuggest suggestion is not > > encountered. I assume this is the preferred behavior. > > That's not so great because then it's possible for this behavior to break > without this test failing. If this test becomes no longer sufficient in the > future, it's best to know earlier rather than later. > > > > > > > On 2013/04/22 16:25:49, sreeram wrote: > > > I don't understand why you need this loop at all. A navsuggestion will be > > ranked > > > at the top, since there are no history matches to outscore it. So, all you > > have > > > to do is check that the top nativesuggestion is the expected navsuggestion, > > and > > > that it doesn't have is_query set. I mentioned this way back in comment #6 > on > > > the review thread: > > > GetBoolFromJS(nativeSuggestions[0].is_search, &value); > > > > > > At line 838, after setting the navsuggestion, you might need to wait for the > > > autocomplete results to be sent. See this pattern elsewhere in this file: > > > // Wait for all autocomplete results to be sent to the page > > > while (!omnibox()->model()->autocomplete_controller()->done()) { > > > ... > > > } > > > > > > If that doesn't work, please explain why. > > > Done. https://codereview.chromium.org/13963014/diff/58001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_test_utils.h (right): https://codereview.chromium.org/13963014/diff/58001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_test_utils.h:91: void FocusOmniboxAndWaitForInstantOverlaySupport(); On 2013/04/29 17:49:20, samarth wrote: > Can you please document this change in the change description. Or probably > better yet, move it to a separate no-op change? Done.
https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:827: InstantExtendedTest, SearchQueryNotDisplayedForNavsuggest) { Nit: Move "InstantExtendedTest" to the previous line. https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:839: FocusOmniboxAndWaitForInstantSupport(); FocusOmniboxAndWaitForInstant***Extended***Support()? https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:865: "rankingData.relevance});")); Yikes. This is really hard to read. Please don't split variables across multiple lines. How about something like this: EXPECT_TRUE(ExecuteScript( "var sorted = chrome.embeddedSearch.searchBox.nativeSuggestions.sort(" "function (a, b) {" "return b.rankingData.relevance - a.rankingData.relevance;" "});")); https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:867: // Create an event listener that opens the top suggestion in a new tab. This comment has nothing to do with the next line. Just delete it. https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:870: int suggestions_count; Initialize this to an invalid value, such as -1. https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:873: EXPECT_TRUE(suggestions_count > 0); EXPECT_GT(suggestions_count, 0); https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:878: ASSERT_TRUE(type == "navsuggest"); EXPECT_EQ("navsuggest", type); https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:880: bool is_search_property_exists; Nit: Fix bad grammar: "is_search_property_defined" or some such. What's the advantage of testing the definedness of this property? Why not just ask whether it's true or false? Does anyone care about the difference between it being undefined versus it being defined and false?
https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:827: InstantExtendedTest, SearchQueryNotDisplayedForNavsuggest) { On 2013/04/29 22:26:31, sreeram wrote: > Nit: Move "InstantExtendedTest" to the previous line. Done. https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:839: FocusOmniboxAndWaitForInstantSupport(); This is intentional. The test hangs if we wait for: chrome::NOTIFICATION_INSTANT_NTP_SUPPORT_DETERMINED. The function names are misleading. I wrote a cl to fix this: crrev.com/14050016. On 2013/04/29 22:26:31, sreeram wrote: > FocusOmniboxAndWaitForInstant***Extended***Support()? https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:865: "rankingData.relevance});")); On 2013/04/29 22:26:31, sreeram wrote: > Yikes. This is really hard to read. Please don't split variables across multiple > lines. How about something like this: > EXPECT_TRUE(ExecuteScript( > "var sorted = chrome.embeddedSearch.searchBox.nativeSuggestions.sort(" > "function (a, b) {" > "return b.rankingData.relevance - a.rankingData.relevance;" > "});")); Done. https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:867: // Create an event listener that opens the top suggestion in a new tab. On 2013/04/29 22:26:31, sreeram wrote: > This comment has nothing to do with the next line. Just delete it. Done. https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:870: int suggestions_count; On 2013/04/29 22:26:31, sreeram wrote: > Initialize this to an invalid value, such as -1. Done. https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:873: EXPECT_TRUE(suggestions_count > 0); On 2013/04/29 22:26:31, sreeram wrote: > EXPECT_GT(suggestions_count, 0); Done. https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:878: ASSERT_TRUE(type == "navsuggest"); On 2013/04/29 22:26:31, sreeram wrote: > EXPECT_EQ("navsuggest", type); Done. https://codereview.chromium.org/13963014/diff/55005/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:880: bool is_search_property_exists; Done. GetBoolFromJS will hang if it is called when is_search isn't defined. On 2013/04/29 22:26:31, sreeram wrote: > Nit: Fix bad grammar: "is_search_property_defined" or some such. > > What's the advantage of testing the definedness of this property? Why not just > ask whether it's true or false? Does anyone care about the difference between it > being undefined versus it being defined and false?
lgtm with nits. Please wait for sreeram to finish reviewing as well. Thanks, Samarth https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:867: content::WebContents* overlay = instant()->GetOverlayContents(); Earlier in this function, L846, you use overlay()->contents(), which is the same thing. Define this up there and just use it throughout. https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:872: EXPECT_GT(suggestions_count, 0); ASSERT_GT .. doesn't make to continue if this fails. https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:877: EXPECT_EQ("navsuggest", type); Likewise, ASSERT https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:882: overlay, "sorted[0].hasOwnProperty('is_search')", What it you just look up "!!sorted[0].is_search" and check that it is false? That should cover both the false and undefined cases.
https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:867: content::WebContents* overlay = instant()->GetOverlayContents(); On 2013/04/30 01:03:47, samarth wrote: > Earlier in this function, L846, you use overlay()->contents(), which is the same > thing. Define this up there and just use it throughout. Done. https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:872: EXPECT_GT(suggestions_count, 0); On 2013/04/30 01:03:47, samarth wrote: > ASSERT_GT .. doesn't make to continue if this fails. Done. https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:877: EXPECT_EQ("navsuggest", type); On 2013/04/30 01:03:47, samarth wrote: > Likewise, ASSERT Done. https://codereview.chromium.org/13963014/diff/69001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:882: overlay, "sorted[0].hasOwnProperty('is_search')", On 2013/04/30 01:03:47, samarth wrote: > What it you just look up "!!sorted[0].is_search" and check that it is false? > That should cover both the false and undefined cases. Done.
lgtm https://codereview.chromium.org/13963014/diff/72001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): https://codereview.chromium.org/13963014/diff/72001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_interactive_uitest.cc:842: ASSERT_TRUE(chrome::IsLocalOnlyInstantExtendedAPIEnabled()); Nit: Do we really need this, given line 847 below?
Thanks! Yes, it hangs otherwise (can't quite remember why, but I can investigate if you'd like). On 2013/04/30 18:16:33, sreeram wrote: > lgtm > > https://codereview.chromium.org/13963014/diff/72001/chrome/browser/ui/search/... > File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right): > > https://codereview.chromium.org/13963014/diff/72001/chrome/browser/ui/search/... > chrome/browser/ui/search/instant_extended_interactive_uitest.cc:842: > ASSERT_TRUE(chrome::IsLocalOnlyInstantExtendedAPIEnabled()); > Nit: Do we really need this, given line 847 below?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dougw@chromium.org/13963014/72001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dougw@chromium.org/13963014/97001
Message was sent while issue was closed.
Change committed as 197551 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
