Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12895007:
Send onsubmit query down to the instant extended overlay page when a navigation is performed from t… (Closed)
|
Created:
7 years, 9 months ago by robertshield Modified:
7 years, 8 months ago CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, dominich, David Black, samarth+watch_chromium.org, Jered Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSend onsubmit query down to the instant extended overlay page when a navigation is performed from the omnibox.
This should give the page sufficient information to close the dropdown.
BUG=180091
TEST=NONE
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193809
Patch Set 1 #
Total comments: 4
Patch Set 2 : Send submit to the instant_tab_ rather than the overlsy_. #
Total comments: 1
Patch Set 3 : Also send submit events to overlay_ on navigation. #
Total comments: 2
Patch Set 4 : Update OnNavigate comment. #
Total comments: 2
Patch Set 5 : Don't send submit events for non-url navigations. #
Total comments: 5
Patch Set 6 : Don't send url. Only send for current tab. #
Total comments: 8
Patch Set 7 : Sreeram's feedback. #Patch Set 8 : #
Total comments: 2
Patch Set 9 : Browser test. #Patch Set 10 : OnSubmit tweak + comment. #
Total comments: 4
Patch Set 11 : IPC-based-browsertest. #Patch Set 12 : Revert to original browser test, add comment re potential brittleness for posterity. #
Total comments: 2
Patch Set 13 : Update browser test, don't call Reset on submit. #Patch Set 14 : Check return value of UpdateSearchState. #
Messages
Total messages: 38 (0 generated)
Hi David, is this roughly what you had in mind?
Seems like the right approach to me. https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:614: if (!extended_enabled_ || !overlay_) Either overlay_ or instant_tab_ might exist. Send the message to the one that exists.
https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:614: if (!extended_enabled_ || !overlay_) On 2013/03/21 21:36:10, David Black wrote: > Either overlay_ or instant_tab_ might exist. Send the message to the one that > exists. Actually if the overlay exists, there's nothing to do. We already Hide() in that case. It's the InstantTab case that needs fixing.
Switched to sending submit to the instant_tab_. This doesn't by itself seem to fix the jank, is there an on-page change that needs to happen before I can test this? https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:614: if (!extended_enabled_ || !overlay_) On 2013/03/21 21:36:10, David Black wrote: > Either overlay_ or instant_tab_ might exist. Send the message to the one that > exists. Done. https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:614: if (!extended_enabled_ || !overlay_) On 2013/03/21 21:39:59, samarth wrote: > On 2013/03/21 21:36:10, David Black wrote: > > Either overlay_ or instant_tab_ might exist. Send the message to the one that > > exists. > > Actually if the overlay exists, there's nothing to do. We already Hide() in > that case. It's the InstantTab case that needs fixing. Done.
On 2013/03/22 03:12:55, robertshield wrote: > Switched to sending submit to the instant_tab_. This doesn't by itself seem to > fix the jank, is there an on-page change that needs to happen before I can test > this? It's my understanding that Jeremy has been working on the GWS-side half of this. And might already have submitted it? But even if so, it won't be live until next week. > > https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... > File chrome/browser/ui/search/instant_controller.cc (right): > > https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... > chrome/browser/ui/search/instant_controller.cc:614: if (!extended_enabled_ || > !overlay_) > On 2013/03/21 21:36:10, David Black wrote: > > Either overlay_ or instant_tab_ might exist. Send the message to the one that > > exists. > > Done. > > https://codereview.chromium.org/12895007/diff/1/chrome/browser/ui/search/inst... > chrome/browser/ui/search/instant_controller.cc:614: if (!extended_enabled_ || > !overlay_) > On 2013/03/21 21:39:59, samarth wrote: > > On 2013/03/21 21:36:10, David Black wrote: > > > Either overlay_ or instant_tab_ might exist. Send the message to the one > that > > > exists. > > > > Actually if the overlay exists, there's nothing to do. We already Hide() in > > that case. It's the InstantTab case that needs fixing. > > Done.
Yes, the GWS-side half is submitted but not live yet. https://codereview.chromium.org/12895007/diff/4002/chrome/browser/ui/search/i... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/12895007/diff/4002/chrome/browser/ui/search/i... chrome/browser/ui/search/instant_controller.cc:617: instant_tab_->Submit(user_text); No, this isn't what I meant. Check for instant_tab_, if it exists then send Submit to it, and if it doesn't exist, then check for overlay_ and send the Submit event to it if it exists. You need to handle both cases.
On 2013/03/22 19:02:37, David Black wrote: > Yes, the GWS-side half is submitted but not live yet. > > https://codereview.chromium.org/12895007/diff/4002/chrome/browser/ui/search/i... > File chrome/browser/ui/search/instant_controller.cc (right): > > https://codereview.chromium.org/12895007/diff/4002/chrome/browser/ui/search/i... > chrome/browser/ui/search/instant_controller.cc:617: > instant_tab_->Submit(user_text); > No, this isn't what I meant. Check for instant_tab_, if it exists then send > Submit to it, and if it doesn't exist, then check for overlay_ and send the > Submit event to it if it exists. You need to handle both cases. Added back sending events to overlay_.
On 2013/03/22 19:38:31, robertshield wrote: > On 2013/03/22 19:02:37, David Black wrote: > > Yes, the GWS-side half is submitted but not live yet. > > > > > https://codereview.chromium.org/12895007/diff/4002/chrome/browser/ui/search/i... > > File chrome/browser/ui/search/instant_controller.cc (right): > > > > > https://codereview.chromium.org/12895007/diff/4002/chrome/browser/ui/search/i... > > chrome/browser/ui/search/instant_controller.cc:617: > > instant_tab_->Submit(user_text); > > No, this isn't what I meant. Check for instant_tab_, if it exists then send > > Submit to it, and if it doesn't exist, then check for overlay_ and send the > > Submit event to it if it exists. You need to handle both cases. > > Added back sending events to overlay_. PTAL
On 2013/03/22 19:38:50, robertshield wrote: > On 2013/03/22 19:38:31, robertshield wrote: > > On 2013/03/22 19:02:37, David Black wrote: > > > Yes, the GWS-side half is submitted but not live yet. > > > > > > > > > https://codereview.chromium.org/12895007/diff/4002/chrome/browser/ui/search/i... > > > File chrome/browser/ui/search/instant_controller.cc (right): > > > > > > > > > https://codereview.chromium.org/12895007/diff/4002/chrome/browser/ui/search/i... > > > chrome/browser/ui/search/instant_controller.cc:617: > > > instant_tab_->Submit(user_text); > > > No, this isn't what I meant. Check for instant_tab_, if it exists then send > > > Submit to it, and if it doesn't exist, then check for overlay_ and send the > > > Submit event to it if it exists. You need to handle both cases. > > > > Added back sending events to overlay_. > > PTAL @David: ping
+Peter for OWNERS on the change to the omnibox code.
LGTM https://codereview.chromium.org/12895007/diff/12001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.h (right): https://codereview.chromium.org/12895007/diff/12001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.h:124: // Called when the user has navigated to a URL. This will send an onsubmit This comment seems a bit misleading given that the omnibox calls this before navigation rather than after.
Thanks! https://codereview.chromium.org/12895007/diff/12001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.h (right): https://codereview.chromium.org/12895007/diff/12001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.h:124: // Called when the user has navigated to a URL. This will send an onsubmit On 2013/03/25 18:47:38, Peter Kasting wrote: > This comment seems a bit misleading given that the omnibox calls this before > navigation rather than after. Done.
LGTM
samarth -> sreeram Will take a look shortly.
Please add a browser test as well. https://codereview.chromium.org/12895007/diff/19002/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/12895007/diff/19002/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:716: instant->OnNavigate(autocomplete_controller_->input().text()); Instant may be showing a search results preview, in which case, this isn't actually going to be a navigation. OnAutocompleteAccept() below will lead to Instant intercepting it in CommitIfPossible(). So: 1. This comment and the OnNavigate() name are slightly misleading. Perhaps call it OnSubmit() or OnMaybeNavigate() or such. 2. Instant shouldn't blindly send Submit() to the page in the implementation of OnNavigate(), because that will end up sending two submit() events in the case of the search results preview.
Sreeram, what do you think of this approach? Working on browser test next. https://codereview.chromium.org/12895007/diff/19002/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/12895007/diff/19002/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:716: instant->OnNavigate(autocomplete_controller_->input().text()); On 2013/03/26 17:43:46, sreeram wrote: > Instant may be showing a search results preview, in which case, this isn't > actually going to be a navigation. OnAutocompleteAccept() below will lead to > Instant intercepting it in CommitIfPossible(). So: > 1. This comment and the OnNavigate() name are slightly misleading. Perhaps call > it OnSubmit() or OnMaybeNavigate() or such. > 2. Instant shouldn't blindly send Submit() to the page in the implementation of > OnNavigate(), because that will end up sending two submit() events in the case > of the search results preview. I noticed that I can also call this code only when the transition type here is PAGE_TRANSITION_TYPED which aiui is the case only when a URL is being navigated too. What do you think of this approach (see latest patch)?
https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:716: if (match.transition == content::PAGE_TRANSITION_TYPED) { Surprisingly, this works. :) I thought perhaps it wouldn't work for cases where the match was a NAVSUGGEST, or a bookmark match or such. But in looking through all the autocomplete providers, it seems that whenever the match is a URL, it's always of transition TYPED (even if the user didn't actually type the whole thing, as in the case of a nav suggest). I'd encourage you to verify my findings, though. https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:719: instant->OnNavigateToURL(autocomplete_controller_->input().text()); Why do we need to send any text down to the page at all? Per the bug, the page already knows that the omnibox has a URL, and searchbox.value is already correct. It just shouldn't change, that's all. Also, we agreed on the bug that this should only happen when the user explicitly presses Enter. Whereas, this code path (OpenMatch()) is called for many other scenarios. We shouldn't be sending any Submit() event in those cases.
On 2013/03/26 20:05:42, sreeram wrote: > https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... > File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): > > https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... > chrome/browser/ui/omnibox/omnibox_edit_model.cc:716: if (match.transition == > content::PAGE_TRANSITION_TYPED) { > Surprisingly, this works. :) > > I thought perhaps it wouldn't work for cases where the match was a NAVSUGGEST, > or a bookmark match or such. But in looking through all the autocomplete > providers, it seems that whenever the match is a URL, it's always of transition > TYPED (even if the user didn't actually type the whole thing, as in the case of > a nav suggest). I'd encourage you to verify my findings, though. > > https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... > chrome/browser/ui/omnibox/omnibox_edit_model.cc:719: > instant->OnNavigateToURL(autocomplete_controller_->input().text()); > Why do we need to send any text down to the page at all? Per the bug, the page > already knows that the omnibox has a URL, and searchbox.value is already > correct. It just shouldn't change, that's all. > > Also, we agreed on the bug that this should only happen when the user explicitly > presses Enter. Whereas, this code path (OpenMatch()) is called for many other > scenarios. We shouldn't be sending any Submit() event in those cases. Well, we do want to clear the suggestions on e.g. user mouse-clicking on one of them. (This is what happens pre-1993 and I think it's correct.)
On 2013/03/27 18:01:19, gideonwald wrote: > Well, we do want to clear the suggestions on e.g. user mouse-clicking on one of > them. (This is what happens pre-1993 and I think it's correct.) If the user clicks on a suggestion with the mouse, the page already knows that the user wants to navigate and so it can close the dropdown itself. (In fact, it's the page that tells Chrome that the user wants to naviate to the suggested URL, using the navigateContentWindow API).
On 2013/03/27 18:24:51, sreeram wrote: > On 2013/03/27 18:01:19, gideonwald wrote: > > Well, we do want to clear the suggestions on e.g. user mouse-clicking on one > of > > them. (This is what happens pre-1993 and I think it's correct.) > > If the user clicks on a suggestion with the mouse, the page already knows that > the user wants to navigate and so it can close the dropdown itself. (In fact, > it's the page that tells Chrome that the user wants to naviate to the suggested > URL, using the navigateContentWindow API). Yes, closing the dropdown on suggestion click is currently implemented.
ptal, browser test still coming. https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:716: if (match.transition == content::PAGE_TRANSITION_TYPED) { On 2013/03/26 20:05:42, sreeram wrote: > Surprisingly, this works. :) > > I thought perhaps it wouldn't work for cases where the match was a NAVSUGGEST, > or a bookmark match or such. But in looking through all the autocomplete > providers, it seems that whenever the match is a URL, it's always of transition > TYPED (even if the user didn't actually type the whole thing, as in the case of > a nav suggest). I'd encourage you to verify my findings, though. Afaict, this is true. I cannot find any code path which sends this transition type without the value being a url. https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:719: instant->OnNavigateToURL(autocomplete_controller_->input().text()); On 2013/03/26 20:05:42, sreeram wrote: > Why do we need to send any text down to the page at all? Per the bug, the page > already knows that the omnibox has a URL, and searchbox.value is already > correct. It just shouldn't change, that's all. > Ok, removed the text and now send down the empty string. When we (David, Gideon and I) discussed this initially, it was suggested to send down the string the user actually typed. I do not know if the page needs this information. > Also, we agreed on the bug that this should only happen when the user explicitly > presses Enter. Whereas, this code path (OpenMatch()) is called for many other > scenarios. We shouldn't be sending any Submit() event in those cases. Afaict, OpenMatch being called for PAGE_TRANSITION_TYPED with disposition being CURRENT_TAB corresponds to an Enter key stroke. Indeed, this is the heuristic used by BrowserInstantController::OpenInstant() to send "INSTANT_COMMIT_PRESSED_ENTER". I have updated the check to include the disposition, let me know if you feel that is not sufficient.
https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/12895007/diff/28001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:716: if (match.transition == content::PAGE_TRANSITION_TYPED) { On 2013/03/27 21:44:05, robertshield wrote: > On 2013/03/26 20:05:42, sreeram wrote: > > Surprisingly, this works. :) > > > > I thought perhaps it wouldn't work for cases where the match was a NAVSUGGEST, > > or a bookmark match or such. But in looking through all the autocomplete > > providers, it seems that whenever the match is a URL, it's always of > transition > > TYPED (even if the user didn't actually type the whole thing, as in the case > of > > a nav suggest). I'd encourage you to verify my findings, though. > > Afaict, this is true. I cannot find any code path which sends this transition > type without the value being a url. TYPED always indicates a URL, but not all URLs are TYPED. For example, AcceptInput() will convert the transition to RELOAD if the user hit enter on the existing permanent URL or to LINK if the user pasted in a URL and hit enter. I don't know if you care about cases like this. If so you may need to hook AcceptInput() instead of here, to get the unmodified transition.
https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:716: // BrowserInstantController::OpenInstant You can remove the cagey wording here. I believe checking the disposition suffices to ensure that this was an Enter. https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:618: instant_tab_->Submit(L""); Re-using the Submit() method causes a small problem that the method (in searchbox.cc) will now overwrite searchbox.value with the query. If you don't want to invent a new IPC (and I'm in favour of not adding new IPCs), you need to special-case the empty query there. https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:621: overlay_->Submit(L""); Remove this. We shouldn't need to send any such info to the overlay. https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.h (right): https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.h:126: void OnNavigateToURL(); This is too similar to the other NavigateToURL() method in this class, which does something completely different. Perhaps call this "OmniboxNavigatedToURL"?
https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:716: // BrowserInstantController::OpenInstant On 2013/04/01 16:22:01, sreeram wrote: > You can remove the cagey wording here. I believe checking the disposition > suffices to ensure that this was an Enter. Done. https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:618: instant_tab_->Submit(L""); On 2013/04/01 16:22:01, sreeram wrote: > Re-using the Submit() method causes a small problem that the method (in > searchbox.cc) will now overwrite searchbox.value with the query. If you don't > want to invent a new IPC (and I'm in favour of not adding new IPCs), you need to > special-case the empty query there. Ok, I prevented overwriting of the query_ field in Searchbox::OnSubmit when called with an empty query string. https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.cc:621: overlay_->Submit(L""); On 2013/04/01 16:22:01, sreeram wrote: > Remove this. We shouldn't need to send any such info to the overlay. Done. https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_controller.h (right): https://codereview.chromium.org/12895007/diff/34002/chrome/browser/ui/search/... chrome/browser/ui/search/instant_controller.h:126: void OnNavigateToURL(); On 2013/04/01 16:22:01, sreeram wrote: > This is too similar to the other NavigateToURL() method in this class, which > does something completely different. Perhaps call this "OmniboxNavigatedToURL"? Done.
Looks good! Please add the browser test you mentioned. https://codereview.chromium.org/12895007/diff/46001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/12895007/diff/46001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:202: selection_start_ = selection_end_ = query_.size(); Put all these other things also into the 'if' block. (I.e., don't modify any state if |query| is empty). Please also add a comment as to why: // Submit() is called when the user hits Enter to commit the omnibox text. // If |query| is non-blank, the user committed a search. If it's blank, the // omnibox text was a URL, and the user is navigating to it, in which case // we shouldn't update the |query_| or associated state.
Added browser test, PTAL, thanks! https://codereview.chromium.org/12895007/diff/46001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/12895007/diff/46001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:202: selection_start_ = selection_end_ = query_.size(); On 2013/04/01 20:08:46, sreeram wrote: > Put all these other things also into the 'if' block. (I.e., don't modify any > state if |query| is empty). > > Please also add a comment as to why: > // Submit() is called when the user hits Enter to commit the omnibox text. > // If |query| is non-blank, the user committed a search. If it's blank, the > // omnibox text was a URL, and the user is navigating to it, in which case > // we shouldn't update the |query_| or associated state. Done.
https://codereview.chromium.org/12895007/diff/52001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/12895007/diff/52001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:497: GetIntFromJS(instant()->instant_tab()->contents(), "submitCount", I'm mildly worried that this is brittle. If in the future, AcceptInput() does enough things to cause the current WebContents to navigate, or even just for instant_tab_ to be reset to NULL, then this will fail. I don't see an easy solution, though. Do you? https://codereview.chromium.org/12895007/diff/52001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:498: &submit_count); Use UpdateSearchState() and verify both the submitCount as well as searchbox.value. (The important thing is to make just 1 call to execute script. If you perform two, you'll very likely run into the issue mentioned above, where the WebContents has already navigated, and instant_tab_ is null. Try it and confirm my hypothesis.)
https://codereview.chromium.org/12895007/diff/52001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/12895007/diff/52001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:497: GetIntFromJS(instant()->instant_tab()->contents(), "submitCount", On 2013/04/08 16:20:38, sreeram wrote: > I'm mildly worried that this is brittle. If in the future, AcceptInput() does > enough things to cause the current WebContents to navigate, or even just for > instant_tab_ to be reset to NULL, then this will fail. I don't see an easy > solution, though. Do you? Maybe. I could examine the IPCs being sent, by sticking an OutgoingMessageFilter on the associated RPH's channel proxy. This should be robust against changes to AcceptInput causing instant_tab_ to go away. Let me try that and see.
https://codereview.chromium.org/12895007/diff/52001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/12895007/diff/52001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:497: GetIntFromJS(instant()->instant_tab()->contents(), "submitCount", On 2013/04/09 14:49:29, robertshield wrote: > On 2013/04/08 16:20:38, sreeram wrote: > > I'm mildly worried that this is brittle. If in the future, AcceptInput() does > > enough things to cause the current WebContents to navigate, or even just for > > instant_tab_ to be reset to NULL, then this will fail. I don't see an easy > > solution, though. Do you? > > Maybe. I could examine the IPCs being sent, by sticking an OutgoingMessageFilter > on the associated RPH's channel proxy. > > This should be robust against changes to AcceptInput causing instant_tab_ to go > away. Let me try that and see. Here are some alternatives: 1. Get a handle to the RVH before AcceptInput, and hope that it stays alive long enough for you to ExecuteScript on it. This is the simplest; while also potentially brittle, I think we'll find out quickly if it breaks. 2. Add a beforeunload/unload listener to the page so that the navigation is forced to wait for it. So, for example, change the document title in beforeunload before popping up the "Stay or Leave?" dialog. Here, WaitForAppModalDialog() and check that the title has been changed to what you expect. 3. Don't add a beforeunload, but add an unload listener. From the unload, set a cookie, which you can check for here. (cf: chrome/browser/fast_shutdown_browsertest.cc).
On 2013/04/09 16:39:41, sreeram wrote: > https://codereview.chromium.org/12895007/diff/52001/chrome/browser/ui/search/... > File chrome/browser/ui/search/instant_extended_browsertest.cc (right): > > https://codereview.chromium.org/12895007/diff/52001/chrome/browser/ui/search/... > chrome/browser/ui/search/instant_extended_browsertest.cc:497: > GetIntFromJS(instant()->instant_tab()->contents(), "submitCount", > On 2013/04/09 14:49:29, robertshield wrote: > > On 2013/04/08 16:20:38, sreeram wrote: > > > I'm mildly worried that this is brittle. If in the future, AcceptInput() > does > > > enough things to cause the current WebContents to navigate, or even just for > > > instant_tab_ to be reset to NULL, then this will fail. I don't see an easy > > > solution, though. Do you? > > > > Maybe. I could examine the IPCs being sent, by sticking an > OutgoingMessageFilter > > on the associated RPH's channel proxy. > > > > This should be robust against changes to AcceptInput causing instant_tab_ to > go > > away. Let me try that and see. > > Here are some alternatives: > > 1. Get a handle to the RVH before AcceptInput, and hope that it stays alive long > enough for you to ExecuteScript on it. This is the simplest; while also > potentially brittle, I think we'll find out quickly if it breaks. > > 2. Add a beforeunload/unload listener to the page so that the navigation is > forced to wait for it. So, for example, change the document title in > beforeunload before popping up the "Stay or Leave?" dialog. Here, > WaitForAppModalDialog() and check that the title has been changed to what you > expect. > > 3. Don't add a beforeunload, but add an unload listener. From the unload, set a > cookie, which you can check for here. (cf: > chrome/browser/fast_shutdown_browsertest.cc). I posted a patch with the IPC-observer test to show you what I meant in all its hideousness :-) It is the most un-brittle test I can think of, but has the big down-side that it only tests the browser-side of the patch. I'll try out a combination of 1 and 3 from above as well, though I'm generally suspicious of unload handlers in tests (many/most such tests are marked flaky). 1. by itself seems no less brittle than my original attempt.
On 2013/04/09 17:37:33, robertshield wrote: > I posted a patch with the IPC-observer test to show you what I meant in all its > hideousness :-) It is the most un-brittle test I can think of, but has the big > down-side that it only tests the browser-side of the patch. Thanks for trying. Let's just leave it as brittle as it originally was, with a comment pointing it out (so that if the test fails or flakes, it may help people diagnosing).
On 2013/04/09 18:02:31, sreeram wrote: > On 2013/04/09 17:37:33, robertshield wrote: > > I posted a patch with the IPC-observer test to show you what I meant in all > its > > hideousness :-) It is the most un-brittle test I can think of, but has the big > > down-side that it only tests the browser-side of the patch. > > Thanks for trying. Let's just leave it as brittle as it originally was, with a > comment pointing it out (so that if the test fails or flakes, it may help people > diagnosing). Ok, reverted back to the original test and added a comment. Please take a look.
lgtm https://codereview.chromium.org/12895007/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/12895007/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:502: EXPECT_EQ(2, submit_count); Nit: Check that searchbox.value hasn't changed (as mentioned in a previous review comment).
Done and thanks for the reminder on searchbox.value, turns out Searchbox::Submit was still calling Reset(). https://codereview.chromium.org/12895007/diff/67001/chrome/browser/ui/search/... File chrome/browser/ui/search/instant_extended_browsertest.cc (right): https://codereview.chromium.org/12895007/diff/67001/chrome/browser/ui/search/... chrome/browser/ui/search/instant_extended_browsertest.cc:502: EXPECT_EQ(2, submit_count); On 2013/04/09 18:49:40, sreeram wrote: > Nit: Check that searchbox.value hasn't changed (as mentioned in a previous > review comment). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robertshield@chromium.org/12895007/71001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... 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/robertshield@chromium.org/12895007/86002
Message was sent while issue was closed.
Change committed as 193809 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
