Optimization: not sending answer server requests for voice queries.

Optimization: not sending answer server requests for voice queries.

This is because launcher closes after voice queries.

Also fixing a bug where a voice query was treated as a non-voice query for some short period of time. This happened because while "is voice query" flag was cleared, the query text remained non-empty. This caused, in particular, unnecessary answer server queries after the voice query is over. Stack example (with "is voice" already cleared, and query text is still non-empty):

0  app_list::SearchAnswerWebContentsDelegate::Update (this=0x48a03b686e0, is_voice_query=<optimized out>) at ../../chrome/browser/ui/app_list/search_answer_web_contents_delegate.cc:93
1  0x000064441d08df8a in AppListViewDelegate::OnTemplateURLServiceChanged (this=0x48a046fa000) at ../../chrome/browser/ui/app_list/app_list_view_delegate.cc:534
2  0x000064441c687230 in TemplateURLService::NotifyObservers (this=<optimized out>) at ../../components/search_engines/template_url_service.cc:2124
3  0x000064441c68ec06 in Update (this=0x48a0265a340, existing_turl=0x48a0350d600, new_values=...) at ../../components/search_engines/template_url_service.cc:1713
4  TemplateURLService::UpdateTemplateURLVisitTime (this=0x48a0265a340, url=0x48a0350d600) at ../../components/search_engines/template_url_service.cc:1787
5  0x000064441c689375 in TemplateURLService::UpdateKeywordSearchTermsForURL (this=0x48a0265a340, details=...) at ../../components/search_engines/template_url_service.cc:1781
6  0x000064441af6bb4f in non-virtual thunk to ChromeTemplateURLServiceClient::OnURLVisited(history::HistoryService*, ui::PageTransition, history::URLRow const&, std::vector<GURL, std::allocator<GURL> > const&, base::Time) ()
    at ../../chrome/browser/search_engines/chrome_template_url_service_client.cc:85
7  0x000064441c62bf32 in history::HistoryService::NotifyURLVisited (this=0x48a027df500, transition=805306373, row=..., redirects=std::vector of length 0, capacity 0, visit_time=...) at ../../components/history/core/browser/history_service.cc:1092
......

Some frames may be optimized away due to inlining.

Also other optimizations and cleanups; as a result, we now have exactly as many expensive requests (such as server queries) as necessary.

Bug=712331
Review-Url: https://codereview.chromium.org/2846733004
Cr-Commit-Position: refs/heads/master@{#468469}
Committed: https://chromium.googlesource.com/chromium/src/+/9815502de37719154e0e2f2bc8ad4caa7b5561fd

[vadimt, 2017-04-27 21:39:42]

vadimt@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xiyuan, 2017-04-27 21:59:31]

https://codereview.chromium.org/2846733004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/2846733004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:300:
model_->search_box()->SetText(base::string16());
Is this necessary? This would trigger another StartSearch call because query is
changed.

Can we optimize by comparing whether request url is actually changed in
SearchAnswerWebContentsDelegate::Update() ?

[vadimt, 2017-04-27 22:14:37]

https://codereview.chromium.org/2846733004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/2846733004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:300:
model_->search_box()->SetText(base::string16());
This is a fundamentally correct thing to do.
We should not find ourselves in a state when there is a non-empty query as-if
entered from keyboard.

This won't cause extra queries to the answer server because it doesn't send
queries for an empty text.

Also, I'd be surprised if we sent empty strings to autocomplete server (even
though I didn't thoroughly verify this).

[xiyuan, 2017-04-27 22:29:20]

https://codereview.chromium.org/2846733004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/2846733004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:300:
model_->search_box()->SetText(base::string16());
On 2017/04/27 22:14:37, vadimt wrote:
> This is a fundamentally correct thing to do.
> We should not find ourselves in a state when there is a non-empty query as-if
> entered from keyboard.
> 
> This won't cause extra queries to the answer server because it doesn't send
> queries for an empty text.
> 
> Also, I'd be surprised if we sent empty strings to autocomplete server (even
> though I didn't thoroughly verify this).

It will cause searches in other search providers. E.g. AppSearchProvider would
re-index and generate a recommendation list.

Not this CL's fault, but maybe |is_voice_query_| should be part of
SearchboxModel instead of a member in AppListViewDelegate. Then we can have a
ClearVoiceSearch flag to clear the flag with the query without notifying model
observers to avoid unnecessary searches. If we want to clear the query, maybe we
should do that.

[vadimt, 2017-04-29 00:48:28]

https://codereview.chromium.org/2846733004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/2846733004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:300:
model_->search_box()->SetText(base::string16());
Done.
Moved is_voice_query_ from AppListViewDelegate to SearchBoxModel.
Switched to a lazy model where the query doesn’t get cleared (which would cause
unnecessary searches) after a search even if the launcher closes. It will be
cleared and trigger a search when launcher reopens.
Fixed a bug when the launcher started search for the old query string upon
reopening.
Added more optimizations.
At the end, I traced expensive search operations during reopening, keyboard and
voice searches, and now they are invoked exactly when and as many times as
needed.

[vadimt, 2017-04-29 00:48:40]

The CQ bit was checked by vadimt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 00:49:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 01:15:39]

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

[commit-bot: I haz the power, 2017-04-29 01:15:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vadimt, 2017-04-29 01:35:56]

The CQ bit was checked by vadimt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 01:36:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 02:50:14]

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

[commit-bot: I haz the power, 2017-04-29 02:50:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vadimt, 2017-05-01 20:17:58]

The CQ bit was checked by vadimt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 20:18:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 20:46:50]

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

[commit-bot: I haz the power, 2017-05-01 20:46:51]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-01 21:16:31]

lgtm

Cool.

https://codereview.chromium.org/2846733004/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/2846733004/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:523: bool is_google =
nit: const , since you are here... :)

https://codereview.chromium.org/2846733004/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:528: if
(model_->search_engine_is_google() != is_google) {
nit: Move this test to AppListModel::SetSearchEngineIsGoogle and maybe make
SearchAnswerWebContentsDelegate an AppListModelObserver ?

https://codereview.chromium.org/2846733004/diff/60001/ui/app_list/search_box_...
File ui/app_list/search_box_model.cc (right):

https://codereview.chromium.org/2846733004/diff/60001/ui/app_list/search_box_...
ui/app_list/search_box_model.cc:30: SearchBoxModel::SearchBoxModel() :
is_voice_query_(false) {}
nit: prefer to put intializer in header

[vadimt, 2017-05-01 22:06:37]

Description was changed from

==========
Optimization: not sending answer server requests for voice queries.

This is because launcher closes after voice queries.

Also fixing a bug where a voice query was treated as a non-voice query for some
short period of time. This happened because while "is voice query" flag was
cleared, the query text remained non-empty. This caused, in particular,
unnecessary answer server queries after the voice query is over. Stack example
(with "is voice" already cleared, and query text is still non-empty):

0  app_list::SearchAnswerWebContentsDelegate::Update (this=0x48a03b686e0,
is_voice_query=<optimized out>) at
../../chrome/browser/ui/app_list/search_answer_web_contents_delegate.cc:93
1  0x000064441d08df8a in AppListViewDelegate::OnTemplateURLServiceChanged
(this=0x48a046fa000) at
../../chrome/browser/ui/app_list/app_list_view_delegate.cc:534
2  0x000064441c687230 in TemplateURLService::NotifyObservers (this=<optimized
out>) at ../../components/search_engines/template_url_service.cc:2124
3  0x000064441c68ec06 in Update (this=0x48a0265a340,
existing_turl=0x48a0350d600, new_values=...) at
../../components/search_engines/template_url_service.cc:1713
4  TemplateURLService::UpdateTemplateURLVisitTime (this=0x48a0265a340,
url=0x48a0350d600) at
../../components/search_engines/template_url_service.cc:1787
5  0x000064441c689375 in TemplateURLService::UpdateKeywordSearchTermsForURL
(this=0x48a0265a340, details=...) at
../../components/search_engines/template_url_service.cc:1781
6  0x000064441af6bb4f in non-virtual thunk to
ChromeTemplateURLServiceClient::OnURLVisited(history::HistoryService*,
ui::PageTransition, history::URLRow const&, std::vector<GURL,
std::allocator<GURL> > const&, base::Time) ()
    at
../../chrome/browser/search_engines/chrome_template_url_service_client.cc:85
7  0x000064441c62bf32 in history::HistoryService::NotifyURLVisited
(this=0x48a027df500, transition=805306373, row=..., redirects=std::vector of
length 0, capacity 0, visit_time=...) at
../../components/history/core/browser/history_service.cc:1092
......

Some frames may be optimized away due to inlining.

Bug=712331
==========

to

==========
Optimization: not sending answer server requests for voice queries.

This is because launcher closes after voice queries.

Also fixing a bug where a voice query was treated as a non-voice query for some
short period of time. This happened because while "is voice query" flag was
cleared, the query text remained non-empty. This caused, in particular,
unnecessary answer server queries after the voice query is over. Stack example
(with "is voice" already cleared, and query text is still non-empty):

0  app_list::SearchAnswerWebContentsDelegate::Update (this=0x48a03b686e0,
is_voice_query=<optimized out>) at
../../chrome/browser/ui/app_list/search_answer_web_contents_delegate.cc:93
1  0x000064441d08df8a in AppListViewDelegate::OnTemplateURLServiceChanged
(this=0x48a046fa000) at
../../chrome/browser/ui/app_list/app_list_view_delegate.cc:534
2  0x000064441c687230 in TemplateURLService::NotifyObservers (this=<optimized
out>) at ../../components/search_engines/template_url_service.cc:2124
3  0x000064441c68ec06 in Update (this=0x48a0265a340,
existing_turl=0x48a0350d600, new_values=...) at
../../components/search_engines/template_url_service.cc:1713
4  TemplateURLService::UpdateTemplateURLVisitTime (this=0x48a0265a340,
url=0x48a0350d600) at
../../components/search_engines/template_url_service.cc:1787
5  0x000064441c689375 in TemplateURLService::UpdateKeywordSearchTermsForURL
(this=0x48a0265a340, details=...) at
../../components/search_engines/template_url_service.cc:1781
6  0x000064441af6bb4f in non-virtual thunk to
ChromeTemplateURLServiceClient::OnURLVisited(history::HistoryService*,
ui::PageTransition, history::URLRow const&, std::vector<GURL,
std::allocator<GURL> > const&, base::Time) ()
    at
../../chrome/browser/search_engines/chrome_template_url_service_client.cc:85
7  0x000064441c62bf32 in history::HistoryService::NotifyURLVisited
(this=0x48a027df500, transition=805306373, row=..., redirects=std::vector of
length 0, capacity 0, visit_time=...) at
../../components/history/core/browser/history_service.cc:1092
......

Some frames may be optimized away due to inlining.

Also other optimizations and cleanups; as a result, we now have exactly as many
expensive requests (such as server queries) as necessary.

Bug=712331
==========

[vadimt, 2017-05-01 22:06:48]

The CQ bit was checked by vadimt@chromium.org

[vadimt, 2017-05-01 22:06:48]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2846733004/#ps80001
(title: "Last nits")

[commit-bot: I haz the power, 2017-05-01 22:07:52]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 22:55:43]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493676408379530,
"parent_rev": "ba1c3ee93552cd8532529c2aa878471b2348b5af", "commit_rev":
"9815502de37719154e0e2f2bc8ad4caa7b5561fd"}

[commit-bot: I haz the power, 2017-05-01 22:56:05]

Description was changed from

==========
Optimization: not sending answer server requests for voice queries.

This is because launcher closes after voice queries.

Also fixing a bug where a voice query was treated as a non-voice query for some
short period of time. This happened because while "is voice query" flag was
cleared, the query text remained non-empty. This caused, in particular,
unnecessary answer server queries after the voice query is over. Stack example
(with "is voice" already cleared, and query text is still non-empty):

0  app_list::SearchAnswerWebContentsDelegate::Update (this=0x48a03b686e0,
is_voice_query=<optimized out>) at
../../chrome/browser/ui/app_list/search_answer_web_contents_delegate.cc:93
1  0x000064441d08df8a in AppListViewDelegate::OnTemplateURLServiceChanged
(this=0x48a046fa000) at
../../chrome/browser/ui/app_list/app_list_view_delegate.cc:534
2  0x000064441c687230 in TemplateURLService::NotifyObservers (this=<optimized
out>) at ../../components/search_engines/template_url_service.cc:2124
3  0x000064441c68ec06 in Update (this=0x48a0265a340,
existing_turl=0x48a0350d600, new_values=...) at
../../components/search_engines/template_url_service.cc:1713
4  TemplateURLService::UpdateTemplateURLVisitTime (this=0x48a0265a340,
url=0x48a0350d600) at
../../components/search_engines/template_url_service.cc:1787
5  0x000064441c689375 in TemplateURLService::UpdateKeywordSearchTermsForURL
(this=0x48a0265a340, details=...) at
../../components/search_engines/template_url_service.cc:1781
6  0x000064441af6bb4f in non-virtual thunk to
ChromeTemplateURLServiceClient::OnURLVisited(history::HistoryService*,
ui::PageTransition, history::URLRow const&, std::vector<GURL,
std::allocator<GURL> > const&, base::Time) ()
    at
../../chrome/browser/search_engines/chrome_template_url_service_client.cc:85
7  0x000064441c62bf32 in history::HistoryService::NotifyURLVisited
(this=0x48a027df500, transition=805306373, row=..., redirects=std::vector of
length 0, capacity 0, visit_time=...) at
../../components/history/core/browser/history_service.cc:1092
......

Some frames may be optimized away due to inlining.

Also other optimizations and cleanups; as a result, we now have exactly as many
expensive requests (such as server queries) as necessary.

Bug=712331
==========

to

==========
Optimization: not sending answer server requests for voice queries.

This is because launcher closes after voice queries.

Also fixing a bug where a voice query was treated as a non-voice query for some
short period of time. This happened because while "is voice query" flag was
cleared, the query text remained non-empty. This caused, in particular,
unnecessary answer server queries after the voice query is over. Stack example
(with "is voice" already cleared, and query text is still non-empty):

0  app_list::SearchAnswerWebContentsDelegate::Update (this=0x48a03b686e0,
is_voice_query=<optimized out>) at
../../chrome/browser/ui/app_list/search_answer_web_contents_delegate.cc:93
1  0x000064441d08df8a in AppListViewDelegate::OnTemplateURLServiceChanged
(this=0x48a046fa000) at
../../chrome/browser/ui/app_list/app_list_view_delegate.cc:534
2  0x000064441c687230 in TemplateURLService::NotifyObservers (this=<optimized
out>) at ../../components/search_engines/template_url_service.cc:2124
3  0x000064441c68ec06 in Update (this=0x48a0265a340,
existing_turl=0x48a0350d600, new_values=...) at
../../components/search_engines/template_url_service.cc:1713
4  TemplateURLService::UpdateTemplateURLVisitTime (this=0x48a0265a340,
url=0x48a0350d600) at
../../components/search_engines/template_url_service.cc:1787
5  0x000064441c689375 in TemplateURLService::UpdateKeywordSearchTermsForURL
(this=0x48a0265a340, details=...) at
../../components/search_engines/template_url_service.cc:1781
6  0x000064441af6bb4f in non-virtual thunk to
ChromeTemplateURLServiceClient::OnURLVisited(history::HistoryService*,
ui::PageTransition, history::URLRow const&, std::vector<GURL,
std::allocator<GURL> > const&, base::Time) ()
    at
../../chrome/browser/search_engines/chrome_template_url_service_client.cc:85
7  0x000064441c62bf32 in history::HistoryService::NotifyURLVisited
(this=0x48a027df500, transition=805306373, row=..., redirects=std::vector of
length 0, capacity 0, visit_time=...) at
../../components/history/core/browser/history_service.cc:1092
......

Some frames may be optimized away due to inlining.

Also other optimizations and cleanups; as a result, we now have exactly as many
expensive requests (such as server queries) as necessary.

Bug=712331

Review-Url: https://codereview.chromium.org/2846733004
Cr-Commit-Position: refs/heads/master@{#468469}
Committed:
https://chromium.googlesource.com/chromium/src/+/9815502de37719154e0e2f2bc8ad...
==========

[commit-bot: I haz the power, 2017-05-01 22:56:06]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9815502de37719154e0e2f2bc8ad...

[vadimt, 2017-05-10 22:55:49]

https://codereview.chromium.org/2846733004/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/2846733004/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:523: bool is_google =
On 2017/05/01 21:16:31, xiyuan wrote:
> nit: const , since you are here... :)

Done.

https://codereview.chromium.org/2846733004/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:528: if
(model_->search_engine_is_google() != is_google) {
On 2017/05/01 21:16:31, xiyuan wrote:
> nit: Move this test to AppListModel::SetSearchEngineIsGoogle and maybe make
> SearchAnswerWebContentsDelegate an AppListModelObserver ?

Done.

https://codereview.chromium.org/2846733004/diff/60001/ui/app_list/search_box_...
File ui/app_list/search_box_model.cc (right):

https://codereview.chromium.org/2846733004/diff/60001/ui/app_list/search_box_...
ui/app_list/search_box_model.cc:30: SearchBoxModel::SearchBoxModel() :
is_voice_query_(false) {}
Done; keep forgetting!