Remove BaseSearchProvider::set_in_app_list

Remove BaseSearchProvider::set_in_app_list

Instead, app_list::OmniboxProvider is responsible to set true to SearchTermsArgs::from_app_list.

BUG=388515
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285565

[Peter Kasting, 2014-07-17 18:11:26]

LGTM if this is actually correct, but I worry.

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/omnibox_provider.cc:72:
match_.search_terms_args->from_app_list = true;
Does this actually work?  I worry that we may not recompute the destination URL
after doing this.

[hashimoto, 2014-07-18 09:54:14]

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/omnibox_provider.cc:72:
match_.search_terms_args->from_app_list = true;
On 2014/07/17 18:11:26, Peter Kasting wrote:
> Does this actually work?  I worry that we may not recompute the destination
URL
> after doing this.

That's a valid concern.
I should have been aware of the lack of test coverage around RLZ related code.
Manually verified that the new code generate different "rlz=" parameters
depending on whether the search is initiated from the omnibox or the app list.

In the new patch set, destination_url_ is overwritten with a new value.
Is it OK to overwrite the destination URL value outside of
AutocompleteController?
It's doing almost the same thing as calling
AutocompleteController::UpdateMatchDestinationURL from OmniboxEditModel.

[Peter Kasting, 2014-07-18 17:37:06]

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/omnibox_provider.cc:72:
match_.search_terms_args->from_app_list = true;
On 2014/07/18 09:54:14, hashimoto wrote:
> On 2014/07/17 18:11:26, Peter Kasting wrote:
> > Does this actually work?  I worry that we may not recompute the destination
> URL
> > after doing this.
> 
> That's a valid concern.
> I should have been aware of the lack of test coverage around RLZ related code.
> Manually verified that the new code generate different "rlz=" parameters
> depending on whether the search is initiated from the omnibox or the app list.
> 
> In the new patch set, destination_url_ is overwritten with a new value.
> Is it OK to overwrite the destination URL value outside of
> AutocompleteController?
> It's doing almost the same thing as calling
> AutocompleteController::UpdateMatchDestinationURL from OmniboxEditModel.

I think it would be safer to actually call UpdateMatchDestinationURL() directly
instead of copying its functionality.  Is there any reason not to pass in the
AutocompleteController* to this constructor so we can do that?

[hashimoto, 2014-07-22 10:33:07]

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/omnibox_provider.cc:72:
match_.search_terms_args->from_app_list = true;
On 2014/07/18 17:37:06, Peter Kasting wrote:
> On 2014/07/18 09:54:14, hashimoto wrote:
> > On 2014/07/17 18:11:26, Peter Kasting wrote:
> > > Does this actually work?  I worry that we may not recompute the
destination
> > URL
> > > after doing this.
> > 
> > That's a valid concern.
> > I should have been aware of the lack of test coverage around RLZ related
code.
> > Manually verified that the new code generate different "rlz=" parameters
> > depending on whether the search is initiated from the omnibox or the app
list.
> > 
> > In the new patch set, destination_url_ is overwritten with a new value.
> > Is it OK to overwrite the destination URL value outside of
> > AutocompleteController?
> > It's doing almost the same thing as calling
> > AutocompleteController::UpdateMatchDestinationURL from OmniboxEditModel.
> 
> I think it would be safer to actually call UpdateMatchDestinationURL()
directly
> instead of copying its functionality.  Is there any reason not to pass in the
> AutocompleteController* to this constructor so we can do that?

I think UpdateMatchDestinationURL() cannot be used here because it takes
query_formulation_time and appends some parameter to the URL.
Should we add a new method to AutocompleteController which just recalculates the
URL?

[Peter Kasting, 2014-07-22 18:44:52]

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/omnibox_provider.cc:72:
match_.search_terms_args->from_app_list = true;
On 2014/07/22 10:33:07, hashimoto wrote:
> On 2014/07/18 17:37:06, Peter Kasting wrote:
> > On 2014/07/18 09:54:14, hashimoto wrote:
> > > On 2014/07/17 18:11:26, Peter Kasting wrote:
> > > > Does this actually work?  I worry that we may not recompute the
> destination
> > > URL
> > > > after doing this.
> > > 
> > > That's a valid concern.
> > > I should have been aware of the lack of test coverage around RLZ related
> code.
> > > Manually verified that the new code generate different "rlz=" parameters
> > > depending on whether the search is initiated from the omnibox or the app
> list.
> > > 
> > > In the new patch set, destination_url_ is overwritten with a new value.
> > > Is it OK to overwrite the destination URL value outside of
> > > AutocompleteController?
> > > It's doing almost the same thing as calling
> > > AutocompleteController::UpdateMatchDestinationURL from OmniboxEditModel.
> > 
> > I think it would be safer to actually call UpdateMatchDestinationURL()
> directly
> > instead of copying its functionality.  Is there any reason not to pass in
the
> > AutocompleteController* to this constructor so we can do that?
> 
> I think UpdateMatchDestinationURL() cannot be used here because it takes
> query_formulation_time and appends some parameter to the URL.
> Should we add a new method to AutocompleteController which just recalculates
the
> URL?

If there's a way to refactor the existing code so that one method calls the
other, that seems fine.

[hashimoto, 2014-07-23 10:30:42]

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/399063002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/omnibox_provider.cc:72:
match_.search_terms_args->from_app_list = true;
On 2014/07/22 18:44:52, Peter Kasting wrote:
> On 2014/07/22 10:33:07, hashimoto wrote:
> > On 2014/07/18 17:37:06, Peter Kasting wrote:
> > > On 2014/07/18 09:54:14, hashimoto wrote:
> > > > On 2014/07/17 18:11:26, Peter Kasting wrote:
> > > > > Does this actually work?  I worry that we may not recompute the
> > destination
> > > > URL
> > > > > after doing this.
> > > > 
> > > > That's a valid concern.
> > > > I should have been aware of the lack of test coverage around RLZ related
> > code.
> > > > Manually verified that the new code generate different "rlz=" parameters
> > > > depending on whether the search is initiated from the omnibox or the app
> > list.
> > > > 
> > > > In the new patch set, destination_url_ is overwritten with a new value.
> > > > Is it OK to overwrite the destination URL value outside of
> > > > AutocompleteController?
> > > > It's doing almost the same thing as calling
> > > > AutocompleteController::UpdateMatchDestinationURL from OmniboxEditModel.
> > > 
> > > I think it would be safer to actually call UpdateMatchDestinationURL()
> > directly
> > > instead of copying its functionality.  Is there any reason not to pass in
> the
> > > AutocompleteController* to this constructor so we can do that?
> > 
> > I think UpdateMatchDestinationURL() cannot be used here because it takes
> > query_formulation_time and appends some parameter to the URL.
> > Should we add a new method to AutocompleteController which just recalculates
> the
> > URL?
> 
> If there's a way to refactor the existing code so that one method calls the
> other, that seems fine.

Added a new method to AutocompleteController.

[Peter Kasting, 2014-07-23 18:07:05]

LGTM

[hashimoto, 2014-07-24 00:31:49]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-07-24 00:33:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/399063002/100001

[commit-bot: I haz the power, 2014-07-24 04:21:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[commit-bot: I haz the power, 2014-07-24 04:31:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-24 04:31:21]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[hashimoto, 2014-07-24 07:02:18]

Ted, could you take a look at this change as an owner of chrome/android and
chrome/browser/android?

This change renames UpdateMatchDestinationURL to
UpdateMatchDestinationURLWithQueryFormulationTime.
Is there any system we can use to warn Clank folks about the expected build
failure and tell them how to fix it?

[Ted C, 2014-07-24 16:25:58]

lgtm

https://codereview.chromium.org/399063002/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java
(left):

https://codereview.chromium.org/399063002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:238:
public String updateMatchDestinationUrl(int selectedIndex, long
elapsedTimeSinceInputChange) {
Can you leave this method here and mark it deprecated and have it just call into
the other method?  Then it won't break the downstream code and we can remove it
once we switch over.

[hashimoto, 2014-07-25 05:16:53]

https://codereview.chromium.org/399063002/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java
(left):

https://codereview.chromium.org/399063002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:238:
public String updateMatchDestinationUrl(int selectedIndex, long
elapsedTimeSinceInputChange) {
On 2014/07/24 16:25:58, Ted C wrote:
> Can you leave this method here and mark it deprecated and have it just call
into
> the other method?  Then it won't break the downstream code and we can remove
it
> once we switch over.

I thought leaving updateMatchDestinationUrl might confuse developers as
AutocompleteController::UpdateMatchDestinationUrl exists as a different method,
but it should be OK assuming the downstream code quickly switches to the new
method.

Done.

[hashimoto, 2014-07-25 07:54:09]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-07-25 07:55:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/399063002/160001

[commit-bot: I haz the power, 2014-07-25 12:19:18]

Change committed as 285565