Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11884037:
InstantExtended: Bail on TemplateURLs with no espv. (Closed)
|
Created:
7 years, 11 months ago by Jered Modified:
7 years, 11 months ago CC:
chromium-reviews, dhollowa+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInstantExtended: Bail on TemplateURLs with no espv.
During our field trial, some users saw InstantExtended enabled in
Chrome but not on the server. We think this is because they had
TemplateURLs pinned to old values which did not support the &espv
parameter. This bails out of Extended mode if that parameter is
not supported.
BUG=166167
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177333
Patch Set 1 #
Total comments: 2
Patch Set 2 : Braces. #
Total comments: 7
Patch Set 3 : Address comments. #
Total comments: 8
Patch Set 4 : Fallback to offline. #
Total comments: 4
Patch Set 5 : Split out edited check. #
Total comments: 2
Patch Set 6 : Fix comment. #Patch Set 7 : Fix browsertest. #
Messages
Total messages: 28 (0 generated)
Please review.
+beaudoin@ FYI in case this conflicts with search term extraction somehow.
Note that search_model.cc has a DCHECK for getting called when extended API is disabled. Does this not trip that check? What happens if we go from enabled -> disabled -> enabled? Will search mode be up to date in this state? (Actually things might already be broken since we allowed themes to change whether instant extended is enabled. IIRC, when I originally looked at this, instant extended status could only change at startup.) Thanks, Samarth https://codereview.chromium.org/11884037/diff/1/chrome/browser/search_engines... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/11884037/diff/1/chrome/browser/search_engines... chrome/browser/search_engines/template_url.cc:473: for (size_t i = 0; i < replacements_.size(); ++i) Braces here please.
On 2013/01/14 23:37:03, samarth wrote: > Note that search_model.cc has a DCHECK for getting called when extended API is > disabled. Does this not trip that check? Can you give an example of a situation which might trigger that? I think TemplateURL would have to be defined correctly when it's called, and then defined incorrectly where the DCHECK happens? > > What happens if we go from enabled -> disabled -> enabled? Will search mode be > up to date in this state? I don't think this change should affect that? > > (Actually things might already be broken since we allowed themes to change > whether instant extended is enabled. IIRC, when I originally looked at this, > instant extended status could only change at startup.) > > Thanks, > Samarth > > https://codereview.chromium.org/11884037/diff/1/chrome/browser/search_engines... > File chrome/browser/search_engines/template_url.cc (right): > > https://codereview.chromium.org/11884037/diff/1/chrome/browser/search_engines... > chrome/browser/search_engines/template_url.cc:473: for (size_t i = 0; i < > replacements_.size(); ++i) > Braces here please.
https://codereview.chromium.org/11884037/diff/1/chrome/browser/search_engines... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/11884037/diff/1/chrome/browser/search_engines... chrome/browser/search_engines/template_url.cc:473: for (size_t i = 0; i < replacements_.size(); ++i) On 2013/01/14 23:37:04, samarth wrote: > Braces here please. Done.
Some ideas on how to reuse the non-Google-specific code already in TemplateURL. https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... chrome/browser/search_engines/template_url.cc:476: } Not a huge fan of using the replacements to do that kind of logic since it suddenly means replacements are not only constants but have some semantic meaning. I would suggest you perform the replacements and check the resulting URL with HasSearchTermsReplacementKey. https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... chrome/browser/search_engines/template_url.cc:814: // TODO: Extend this to support other providers. You can assign these TODO to me, I've kind of made this my mission. :) https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... chrome/browser/search_engines/template_url.h:141: bool HasGoogleInstantExtendedParam() const; In the current case you should be able to use TemplateURLData::search_terms_replacement_key instead of a Google-specific method. You can probably do something similar to TemplateURL::HasSearchTermsReplacementKey instead of wrapping out your own.
Some questions for you beaudoin@ https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... chrome/browser/search_engines/template_url.cc:476: } On 2013/01/15 00:34:55, beaudoin wrote: > Not a huge fan of using the replacements to do that kind of logic since it > suddenly means replacements are not only constants but have some semantic > meaning. I would suggest you perform the replacements and check the resulting > URL with HasSearchTermsReplacementKey. I'm not sure I follow. &espv and GOOGLE_INSTANT_EXTENDED_ENABLED already have semantic meaning. Building a URL from this TemplateURL then parsing it to see if the result contains &espv seems roundabout. https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... chrome/browser/search_engines/template_url.cc:814: // TODO: Extend this to support other providers. On 2013/01/15 00:34:55, beaudoin wrote: > You can assign these TODO to me, I've kind of made this my mission. :) Cool, thanks. https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... chrome/browser/search_engines/template_url.h:141: bool HasGoogleInstantExtendedParam() const; On 2013/01/15 00:34:55, beaudoin wrote: > In the current case you should be able to use > TemplateURLData::search_terms_replacement_key instead of a Google-specific > method. You can probably do something similar to > TemplateURL::HasSearchTermsReplacementKey instead of wrapping out your own. I'm confused. Should supporting search terms replacement be equivalent with supporting Instant extended? I really want to know if this URL supports the param that tells the server that Instant Extended should be enabled.
https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... chrome/browser/search_engines/template_url.h:141: bool HasGoogleInstantExtendedParam() const; On 2013/01/15 19:06:28, Jered wrote: > On 2013/01/15 00:34:55, beaudoin wrote: > > In the current case you should be able to use > > TemplateURLData::search_terms_replacement_key instead of a Google-specific > > method. You can probably do something similar to > > TemplateURL::HasSearchTermsReplacementKey instead of wrapping out your own. > > I'm confused. Should supporting search terms replacement be equivalent with > supporting Instant extended? I really want to know if this URL supports the > param that tells the server that Instant Extended should be enabled. Yes, this conflation of the two notions of instant-extended-support versus search-term-replacement is becoming increasingly problematic. IOS does the latter but not the former, and my site-isolation work relies on the form but uses the 'espv' mechanism to signal the former, but is labelled in code as the latter.
On 2013/01/15 19:18:55, dhollowa wrote: > https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... > File chrome/browser/search_engines/template_url.h (right): > > https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... > chrome/browser/search_engines/template_url.h:141: bool > HasGoogleInstantExtendedParam() const; > On 2013/01/15 19:06:28, Jered wrote: > > On 2013/01/15 00:34:55, beaudoin wrote: > > > In the current case you should be able to use > > > TemplateURLData::search_terms_replacement_key instead of a Google-specific > > > method. You can probably do something similar to > > > TemplateURL::HasSearchTermsReplacementKey instead of wrapping out your own. > > > > I'm confused. Should supporting search terms replacement be equivalent with > > supporting Instant extended? I really want to know if this URL supports the > > param that tells the server that Instant Extended should be enabled. > > Yes, this conflation of the two notions of instant-extended-support versus > search-term-replacement is becoming increasingly problematic. IOS does the > latter but not the former, and my site-isolation work relies on the form but > uses the 'espv' mechanism to signal the former, but is labelled in code as the > latter. Just discussed this with David and it looks like we should really have a single URL param to indicate "instant extended is ON". As a result HasSearchTermsReplacementKey will be replaced by IsInstantExtendedURL or something like that. We will still rely on the TemplateURLData::search_terms_replacement_key variable but I will rename it (and all its shadows) everywhere. I've entered this bug to track this: crbug.com/170130 For now, I would say go ahead and use HasSearchTermsReplacementKey and I'll include your code in my refactoring. (Add a TODO somewhere to clarify.)
On 2013/01/15 19:56:42, beaudoin wrote: > On 2013/01/15 19:18:55, dhollowa wrote: > > > https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... > > File chrome/browser/search_engines/template_url.h (right): > > > > > https://codereview.chromium.org/11884037/diff/1007/chrome/browser/search_engi... > > chrome/browser/search_engines/template_url.h:141: bool > > HasGoogleInstantExtendedParam() const; > > On 2013/01/15 19:06:28, Jered wrote: > > > On 2013/01/15 00:34:55, beaudoin wrote: > > > > In the current case you should be able to use > > > > TemplateURLData::search_terms_replacement_key instead of a Google-specific > > > > method. You can probably do something similar to > > > > TemplateURL::HasSearchTermsReplacementKey instead of wrapping out your > own. > > > > > > I'm confused. Should supporting search terms replacement be equivalent with > > > supporting Instant extended? I really want to know if this URL supports the > > > param that tells the server that Instant Extended should be enabled. > > > > Yes, this conflation of the two notions of instant-extended-support versus > > search-term-replacement is becoming increasingly problematic. IOS does the > > latter but not the former, and my site-isolation work relies on the form but > > uses the 'espv' mechanism to signal the former, but is labelled in code as the > > latter. > > Just discussed this with David and it looks like we should really have a single > URL param to indicate "instant extended is ON". As a result > HasSearchTermsReplacementKey will be replaced by IsInstantExtendedURL or > something like that. We will still rely on the > TemplateURLData::search_terms_replacement_key variable but I will rename it (and > all its shadows) everywhere. I've entered this bug to track this: > crbug.com/170130 > > For now, I would say go ahead and use HasSearchTermsReplacementKey and I'll > include your code in my refactoring. (Add a TODO somewhere to clarify.) PTAL, ps#3 switches to using HasSearchTermsReplacementKey().
On 2013/01/14 23:48:26, Jered wrote: > On 2013/01/14 23:37:03, samarth wrote: > > Note that search_model.cc has a DCHECK for getting called when extended API is > > disabled. Does this not trip that check? > Can you give an example of a situation which might trigger that? I think > TemplateURL would have to be defined correctly when it's called, and then > defined incorrectly where the DCHECK happens? Here's what will cause a crash I believe: 1. Have google as your default engine, instant extended enabled 2. Open some tabs, do stuff 3. Switch providers (doesn't matter which: none of the others implement instant extended yet) 4. Open some tabs, do stuff You're only aggravating an already-present problem, but it'd be nice if we fixed it. Basically, anything that causes instant extended to change needs to result in search tab helpers resetting themselves. Samarth
On 2013/01/15 23:57:15, samarth wrote:
> On 2013/01/14 23:48:26, Jered wrote:
> > On 2013/01/14 23:37:03, samarth wrote:
> > > Note that search_model.cc has a DCHECK for getting called when extended
API
> is
> > > disabled. Does this not trip that check?
> > Can you give an example of a situation which might trigger that? I think
> > TemplateURL would have to be defined correctly when it's called, and then
> > defined incorrectly where the DCHECK happens?
>
> Here's what will cause a crash I believe:
> 1. Have google as your default engine, instant extended enabled
> 2. Open some tabs, do stuff
> 3. Switch providers (doesn't matter which: none of the others implement
instant
> extended yet)
> 4. Open some tabs, do stuff
>
> You're only aggravating an already-present problem, but it'd be nice if we
fixed
> it. Basically, anything that causes instant extended to change needs to
result
> in search tab helpers resetting themselves.
>
> Samarth
Hey dhollowa@, do bad things happen if I just remove this
DCHECK(IsInstantExtendedAPIEnabled(profile)) at search_model.cc:28? I do not see
what the harm would be, and actually I see some other code that looks strange if
we assume that this will not be called outside Instant Extended, for example in
browser.cc:
if (search_model_->mode().is_search_suggestions() ||
(chrome::search::IsInstantExtendedAPIEnabled(profile_) &&
!search_model_->mode().is_ntp() && state == BookmarkBar::DETACHED)) {
(how did we get into mode().is_search_suggestions() if
!IsInstantExtendedAPIEnabled?)
On 2013/01/16 02:30:52, Jered wrote:
> On 2013/01/15 23:57:15, samarth wrote:
> > On 2013/01/14 23:48:26, Jered wrote:
> > > On 2013/01/14 23:37:03, samarth wrote:
> > > > Note that search_model.cc has a DCHECK for getting called when extended
> API
> > is
> > > > disabled. Does this not trip that check?
> > > Can you give an example of a situation which might trigger that? I think
> > > TemplateURL would have to be defined correctly when it's called, and then
> > > defined incorrectly where the DCHECK happens?
> >
> > Here's what will cause a crash I believe:
> > 1. Have google as your default engine, instant extended enabled
> > 2. Open some tabs, do stuff
> > 3. Switch providers (doesn't matter which: none of the others implement
> instant
> > extended yet)
> > 4. Open some tabs, do stuff
> >
> > You're only aggravating an already-present problem, but it'd be nice if we
> fixed
> > it. Basically, anything that causes instant extended to change needs to
> result
> > in search tab helpers resetting themselves.
> >
> > Samarth
>
> Hey dhollowa@, do bad things happen if I just remove this
> DCHECK(IsInstantExtendedAPIEnabled(profile)) at search_model.cc:28? I do not
see
> what the harm would be, and actually I see some other code that looks strange
if
> we assume that this will not be called outside Instant Extended, for example
in
> browser.cc:
>
> if (search_model_->mode().is_search_suggestions() ||
> (chrome::search::IsInstantExtendedAPIEnabled(profile_) &&
> !search_model_->mode().is_ntp() && state == BookmarkBar::DETACHED)) {
>
> (how did we get into mode().is_search_suggestions() if
> !IsInstantExtendedAPIEnabled?)
Actually, I've filed crbug.com/170267, since this runs a bit deeper than just
SearchModel. I think we should take that as a separate issue.
LGTM, with one question/simplification. https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:37: return template_url->HasSearchTermsReplacementKey(GURL(search_url)); Couldn't we assume that any TemplateURL that has a non-empty search_terms_replacement_key support instant extended? Looking that the instant_url_ref actually contains the search_terms_replacement key looks like a DCHECK (since it's essentially hardcoded, coming from prepopulated_engines.json). Or is my assumption wrong and this "pinning" you talk about can occur on the instant_url_ref but not in the other TemplateURL fields?
As previously mentioned in the other thread, I don't think we should trust the instant URL at all if the user has modified the TemplateURL.
On 2013/01/16 02:30:52, Jered wrote:
> On 2013/01/15 23:57:15, samarth wrote:
> > On 2013/01/14 23:48:26, Jered wrote:
> > > On 2013/01/14 23:37:03, samarth wrote:
> > > > Note that search_model.cc has a DCHECK for getting called when extended
> API
> > is
> > > > disabled. Does this not trip that check?
> > > Can you give an example of a situation which might trigger that? I think
> > > TemplateURL would have to be defined correctly when it's called, and then
> > > defined incorrectly where the DCHECK happens?
> >
> > Here's what will cause a crash I believe:
> > 1. Have google as your default engine, instant extended enabled
> > 2. Open some tabs, do stuff
> > 3. Switch providers (doesn't matter which: none of the others implement
> instant
> > extended yet)
> > 4. Open some tabs, do stuff
> >
> > You're only aggravating an already-present problem, but it'd be nice if we
> fixed
> > it. Basically, anything that causes instant extended to change needs to
> result
> > in search tab helpers resetting themselves.
> >
> > Samarth
>
> Hey dhollowa@, do bad things happen if I just remove this
> DCHECK(IsInstantExtendedAPIEnabled(profile)) at search_model.cc:28? I do not
see
> what the harm would be, and actually I see some other code that looks strange
if
> we assume that this will not be called outside Instant Extended, for example
in
> browser.cc:
>
> if (search_model_->mode().is_search_suggestions() ||
> (chrome::search::IsInstantExtendedAPIEnabled(profile_) &&
> !search_model_->mode().is_ntp() && state == BookmarkBar::DETACHED)) {
>
> (how did we get into mode().is_search_suggestions() if
> !IsInstantExtendedAPIEnabled?)
Removing DCHECKs have no effect on behavior, since they're compiled out in
release builds anyway. They're just an aid to enforce invariants.
The browser.cc code is just weird.
https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... File chrome/browser/ui/search/search_unittest.cc (right): https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... chrome/browser/ui/search/search_unittest.cc:84: ASSERT_TRUE(TemplateURLServiceFactory::GetForProfile(test_util.profile())-> nit: ASSERT_NE(static_cast<...>(NULL), TemplateURLService...); https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... chrome/browser/ui/search/search_unittest.cc:101: "{google:instantExtendedEnabledParameter}"; This wouldn't cover the case when espv=0 I suppose. It probably doesn't matter, but is technically incorrect. https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... chrome/browser/ui/search/search_unittest.cc:106: ASSERT_TRUE(TemplateURLServiceFactory::GetForProfile(test_util.profile())-> nit: ASSERT_NE... as above.
+sreeram for instant After discussing with sreeram@, I've changed strategy here. Instead of completely bailing in this case, we'll fallback to the offline page, which is how we're planning to work when Instant is enabled but a provider doesn't support it. As a result, this change won't introduce any new runtime changes in IsInstantExtendedAPIEnabled(). https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... File chrome/browser/ui/search/search_unittest.cc (right): https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... chrome/browser/ui/search/search_unittest.cc:84: ASSERT_TRUE(TemplateURLServiceFactory::GetForProfile(test_util.profile())-> On 2013/01/16 17:30:05, dhollowa wrote: > nit: ASSERT_NE(static_cast<...>(NULL), TemplateURLService...); Done. https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... chrome/browser/ui/search/search_unittest.cc:101: "{google:instantExtendedEnabledParameter}"; On 2013/01/16 17:30:05, dhollowa wrote: > This wouldn't cover the case when espv=0 I suppose. It probably doesn't matter, > but is technically incorrect. The logic I'm adding only needs to know that the server supports &espv. Its value should be determined by EmbeddedSearchPageVersion() which is also what IsInstantExtendedAPIEnabled() checks, so a 0 value should disable. https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... chrome/browser/ui/search/search_unittest.cc:106: ASSERT_TRUE(TemplateURLServiceFactory::GetForProfile(test_util.profile())-> On 2013/01/16 17:30:05, dhollowa wrote: > nit: ASSERT_NE... as above. Done.
lgtm Ah, much better this way. https://codereview.chromium.org/11884037/diff/14005/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/11884037/diff/14005/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:1197: (!template_url->safe_for_autoreplace() || Not a big deal, but should we use the template url for regular instant either if it's been messed with?
lgtm
lgtm https://codereview.chromium.org/11884037/diff/14005/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/11884037/diff/14005/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:1197: (!template_url->safe_for_autoreplace() || On 2013/01/16 19:08:06, samarth wrote: > Not a big deal, but should we use the template url for regular instant either if > it's been messed with? Agreed. Let's do this even for regular mode.
https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/11884037/diff/11001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:37: return template_url->HasSearchTermsReplacementKey(GURL(search_url)); On 2013/01/16 14:52:27, beaudoin wrote: > Couldn't we assume that any TemplateURL that has a non-empty > search_terms_replacement_key support instant extended? Looking that the > instant_url_ref actually contains the search_terms_replacement key looks like a > DCHECK (since it's essentially hardcoded, coming from > prepopulated_engines.json). > Or is my assumption wrong and this "pinning" you talk about can occur on the > instant_url_ref but not in the other TemplateURL fields? Oops sorry missed this. Your assumption was correct (though the new approach avoids this). https://codereview.chromium.org/11884037/diff/14005/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/11884037/diff/14005/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:1197: (!template_url->safe_for_autoreplace() || On 2013/01/16 19:08:06, samarth wrote: > Not a big deal, but should we use the template url for regular instant either if > it's been messed with? Done. https://codereview.chromium.org/11884037/diff/14005/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:1197: (!template_url->safe_for_autoreplace() || On 2013/01/16 19:47:24, sreeram wrote: > On 2013/01/16 19:08:06, samarth wrote: > > Not a big deal, but should we use the template url for regular instant either > if > > it's been messed with? > > Agreed. Let's do this even for regular mode. Done.
LGTM - and I think you should work with ux to detect this situation and warn users, since I have a feeling it's more unintentional. https://codereview.chromium.org/11884037/diff/24001/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/11884037/diff/24001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:1194: // correct (since it is not editable). It's not editable, it's more of since we don't migrate the url.
Thanks for reviewing. https://codereview.chromium.org/11884037/diff/24001/chrome/browser/instant/in... File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/11884037/diff/24001/chrome/browser/instant/in... chrome/browser/instant/instant_controller.cc:1194: // correct (since it is not editable). On 2013/01/16 22:24:05, sky wrote: > It's not editable, it's more of since we don't migrate the url. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jered@chromium.org/11884037/27001
On 2013/01/16 22:53:13, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/jered%40chromium.org/11884037/27001 @sky @gideon I've created crbug.com/170490 . It would be a good idea generally, but probably even more now as we're launching instant extended. I'll let you triage it gideon.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jered@chromium.org/11884037/15005
Message was sent while issue was closed.
Change committed as 177333 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
