Remove AutocompleteMatch::is_starred

Remove AutocompleteMatch::is_starred

Instead of letting every AutocompleteProvider implementation depend on BookmarkModel,
let OmniboxPopupModel and others take the responsibility to check the bookmark status.

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

[Ilya Sherman, 2014-07-24 17:47:30]

https://codereview.chromium.org/415053002/diff/40001/components/metrics/proto...
File components/metrics/proto/omnibox_event.proto (left):

https://codereview.chromium.org/415053002/diff/40001/components/metrics/proto...
components/metrics/proto/omnibox_event.proto:232: 
This should remain, but perhaps be marked deprecated (and if you decide to
change the protobuf file, you'll want to update the server-side copy as well).

[Peter Kasting, 2014-07-24 18:51:34]

I don't know if I love this change, since it seems like we'll end up making far
more calls to the bookmark model, but I guess that should be fast enough that
this is OK.

My only worry here is about whether anything will break because we now compute
the starred state after we may have updated the match destination URLs (by
calling AutocompleteController::UpdateMatchDestinationURL()).  Can we have a
case where we had a bookmarked URL that a provider returned, but then this
function changed it to a similar non-bookmarked URL, thus resulting in no star
appearing?  I actually don't think we can (or at least, that we can have one we
care about), since I think this is only relevant for search result URLs, which
presumably people don't bookmark much -- but it's worth thinking through to be
sure.

https://codereview.chromium.org/415053002/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/415053002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:426:
UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.UpdateStarredTime",
Make sure this historgram gets properly marked as deprecated in any XML files
which refer to it.  Ping mpearson if you're not sure where to look.

https://codereview.chromium.org/415053002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/omnibox/omnibox.js (left):

https://codereview.chromium.org/415053002/diff/40001/chrome/browser/resources...
chrome/browser/resources/omnibox/omnibox.js:130: 'A green checkmark indicates
that the result has been bookmarked.'),
This is still useful information to know.  I think we should leave this here,
and instead of passing down match.is_starred, we should compute the starred
state of all the matches and pass that down.

[hashimoto, 2014-07-25 08:36:12]

https://codereview.chromium.org/415053002/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/415053002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:426:
UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.UpdateStarredTime",
On 2014/07/24 18:51:34, Peter Kasting wrote:
> Make sure this historgram gets properly marked as deprecated in any XML files
> which refer to it.  Ping mpearson if you're not sure where to look.

There is no line in histograms.xml which corresponds to this histogram.
According to the original change which added this line, this histogram is
intended to appear only on about:histograms locally.
(https://codereview.chromium.org/23469012#msg14)

https://codereview.chromium.org/415053002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/omnibox/omnibox.js (left):

https://codereview.chromium.org/415053002/diff/40001/chrome/browser/resources...
chrome/browser/resources/omnibox/omnibox.js:130: 'A green checkmark indicates
that the result has been bookmarked.'),
On 2014/07/24 18:51:34, Peter Kasting wrote:
> This is still useful information to know.  I think we should leave this here,
> and instead of passing down match.is_starred, we should compute the starred
> state of all the matches and pass that down.

Added code to fill |starred| in OmniboxUIHandler.

https://codereview.chromium.org/415053002/diff/40001/components/metrics/proto...
File components/metrics/proto/omnibox_event.proto (left):

https://codereview.chromium.org/415053002/diff/40001/components/metrics/proto...
components/metrics/proto/omnibox_event.proto:232: 
On 2014/07/24 17:47:29, Ilya Sherman wrote:
> This should remain, but perhaps be marked deprecated (and if you decide to
> change the protobuf file, you'll want to update the server-side copy as well).
Thanks,
Marked as deprecated.

[hashimoto, 2014-07-25 08:57:43]

On 2014/07/24 18:51:34, Peter Kasting wrote:
> I don't know if I love this change, since it seems like we'll end up making
far
> more calls to the bookmark model, but I guess that should be fast enough that
> this is OK.
I guess it shouldn't cause serious peroformance regression because
AutocompleteResult::SortAndCull() limits the number of results to kMaxMatches (=
6).
> 
> My only worry here is about whether anything will break because we now compute
> the starred state after we may have updated the match destination URLs (by
> calling AutocompleteController::UpdateMatchDestinationURL()).  Can we have a
> case where we had a bookmarked URL that a provider returned, but then this
> function changed it to a similar non-bookmarked URL, thus resulting in no star
> appearing?  I actually don't think we can (or at least, that we can have one
we
> care about), since I think this is only relevant for search result URLs, which
> presumably people don't bookmark much -- but it's worth thinking through to be
> sure.
UpdateMatchDestinationURL() updates the destination URL only when the match's
search_terms_args is not NULL.
Since BaseSearchProvider::CreateSearchSuggestion is the only place under
chrome/browser/autocomplete where AutocompleteMatch::search_terms_args can get
set,
we can ignore all providers other than SearchProvider and ZeroSuggestProvider.
(ShortcutsBackend is also using CreateSearchSuggestion(), but it's not for
creating the result AutocompleteMatch.)
As you noted, I think we don't have to care about cases where search URLs are
bookmarked.

Even if there are people who bookmark search URLs, they should bookmark the URLs
modified by UpdateMatchDestinationURL(), not the original one.
So I guess it's impossible to show stars for search URLs no matter whether we
check the bookmark state in SearchProvider or in OmniboxPopupModel.
> 
>
https://codereview.chromium.org/415053002/diff/40001/chrome/browser/autocompl...
> File chrome/browser/autocomplete/search_provider.cc (left):
> 
>
https://codereview.chromium.org/415053002/diff/40001/chrome/browser/autocompl...
> chrome/browser/autocomplete/search_provider.cc:426:
> UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.UpdateStarredTime",
> Make sure this historgram gets properly marked as deprecated in any XML files
> which refer to it.  Ping mpearson if you're not sure where to look.
> 
>
https://codereview.chromium.org/415053002/diff/40001/chrome/browser/resources...
> File chrome/browser/resources/omnibox/omnibox.js (left):
> 
>
https://codereview.chromium.org/415053002/diff/40001/chrome/browser/resources...
> chrome/browser/resources/omnibox/omnibox.js:130: 'A green checkmark indicates
> that the result has been bookmarked.'),
> This is still useful information to know.  I think we should leave this here,
> and instead of passing down match.is_starred, we should compute the starred
> state of all the matches and pass that down.

[Peter Kasting, 2014-07-25 18:18:44]

On 2014/07/25 08:57:43, hashimoto wrote:
> Even if there are people who bookmark search URLs, they should bookmark the
URLs
> modified by UpdateMatchDestinationURL(), not the original one.

That assumes people only search using the omnibox, and that the omnibox won't
generate identical AQS and the like between two otherwise-identical searches --
neither of which seems guaranteed.  That said, I still agree that we shouldn't
worry about this case.

I'll try to re-review this afternoon.

[Ilya Sherman, 2014-07-25 20:13:56]

https://codereview.chromium.org/415053002/diff/60001/components/metrics/proto...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/415053002/diff/60001/components/metrics/proto...
components/metrics/proto/omnibox_event.proto:231: optional bool is_starred = 4
[deprecated=true];
Thanks.  Please make this changer server-side as well.

[Peter Kasting, 2014-07-26 02:12:08]

LGTM

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:149: for (size_t i =  0; i
< result->combined_results.size(); ++i) {
Nit: Use an iterator instead of an index here and in the other loops.

[hashimoto, 2014-07-28 06:49:43]

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:149: for (size_t i =  0; i
< result->combined_results.size(); ++i) {
On 2014/07/26 02:12:08, Peter Kasting wrote:
> Nit: Use an iterator instead of an index here and in the other loops.

IIUC mojo::Array doesn't provide iterators.
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...)

https://codereview.chromium.org/415053002/diff/60001/components/metrics/proto...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/415053002/diff/60001/components/metrics/proto...
components/metrics/proto/omnibox_event.proto:231: optional bool is_starred = 4
[deprecated=true];
On 2014/07/25 20:13:56, Ilya Sherman wrote:
> Thanks.  Please make this changer server-side as well.

OK, I will.

[hashimoto, 2014-07-28 06:49:52]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-07-28 06:50:07]

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

[commit-bot: I haz the power, 2014-07-28 07:01:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-28 07:03:11]

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

[commit-bot: I haz the power, 2014-07-28 07:03:12]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[hashimoto, 2014-07-28 07:47:24]

Ilya, could you review this change as an owner of chrome/browser/metrics and
components/metrics?

Ted, could you review this change as an owner of chrome/browser/android?

[Peter Kasting, 2014-07-28 11:11:02]

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:149: for (size_t i =  0; i
< result->combined_results.size(); ++i) {
On 2014/07/28 06:49:43, hashimoto wrote:
> On 2014/07/26 02:12:08, Peter Kasting wrote:
> > Nit: Use an iterator instead of an index here and in the other loops.
> 
> IIUC mojo::Array doesn't provide iterators.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...)

Then unless it is infeasible to do so, can you add iterators (might have to be
just const_iterators?) to that class?  If the underlying storage is an actual
array, this should just be a matter of returning some pointers.

[Ted C, 2014-07-28 15:12:45]

On 2014/07/28 11:11:02, Peter Kasting wrote:
>
https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):
> 
>
https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:149: for (size_t i =  0;
i
> < result->combined_results.size(); ++i) {
> On 2014/07/28 06:49:43, hashimoto wrote:
> > On 2014/07/26 02:12:08, Peter Kasting wrote:
> > > Nit: Use an iterator instead of an index here and in the other loops.
> > 
> > IIUC mojo::Array doesn't provide iterators.
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...)
> 
> Then unless it is infeasible to do so, can you add iterators (might have to be
> just const_iterators?) to that class?  If the underlying storage is an actual
> array, this should just be a matter of returning some pointers.

chrome/browser/android - lgtm

[Ilya Sherman, 2014-07-28 21:01:28]

LGTM, thanks.

[hashimoto, 2014-07-29 02:03:49]

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:149: for (size_t i =  0; i
< result->combined_results.size(); ++i) {
On 2014/07/28 11:11:02, Peter Kasting wrote:
> On 2014/07/28 06:49:43, hashimoto wrote:
> > On 2014/07/26 02:12:08, Peter Kasting wrote:
> > > Nit: Use an iterator instead of an index here and in the other loops.
> > 
> > IIUC mojo::Array doesn't provide iterators.
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...)
> 
> Then unless it is infeasible to do so, can you add iterators (might have to be
> just const_iterators?) to that class?  If the underlying storage is an actual
If we are to use iterators here, it should be non-const as we are modifying the
contents of the array.
> array, this should just be a matter of returning some pointers.
mojo::Array is basically an array, but not when the stored type is a move-only
type.
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...)
Using a pointer as an iterator doesn't work for this case ((&a[0] + 1) != &a[1]
may be true with mojo::Array) so adding iterators for mojo::Array should require
a certain amount of hassle.

[Peter Kasting, 2014-07-29 02:19:51]

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:149: for (size_t i =  0; i
< result->combined_results.size(); ++i) {
On 2014/07/29 02:03:49, hashimoto wrote:
> On 2014/07/28 11:11:02, Peter Kasting wrote:
> > On 2014/07/28 06:49:43, hashimoto wrote:
> > > On 2014/07/26 02:12:08, Peter Kasting wrote:
> > > > Nit: Use an iterator instead of an index here and in the other loops.
> > > 
> > > IIUC mojo::Array doesn't provide iterators.
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...)
> > 
> > Then unless it is infeasible to do so, can you add iterators (might have to
be
> > just const_iterators?) to that class?  If the underlying storage is an
actual
> If we are to use iterators here, it should be non-const as we are modifying
the
> contents of the array.

That sounds scary given that the header file for Mojo::Array says that it
provides "read-only access" to the array data.  Maybe that comment is incorrect?

It seems from array_internal.h like ultimately the underlying type ArrayTraits
deals with is always a vector.  Can't we just typedef a Traits::iterator type to
the underlying vector iterator and add begin() and end() functions that return
the iterators of that vector?

[hashimoto, 2014-07-29 02:46:36]

On 2014/07/29 02:19:51, Peter Kasting wrote:
>
https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):
> 
>
https://codereview.chromium.org/415053002/diff/60001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:149: for (size_t i =  0;
i
> < result->combined_results.size(); ++i) {
> On 2014/07/29 02:03:49, hashimoto wrote:
> > On 2014/07/28 11:11:02, Peter Kasting wrote:
> > > On 2014/07/28 06:49:43, hashimoto wrote:
> > > > On 2014/07/26 02:12:08, Peter Kasting wrote:
> > > > > Nit: Use an iterator instead of an index here and in the other loops.
> > > > 
> > > > IIUC mojo::Array doesn't provide iterators.
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...)
> > > 
> > > Then unless it is infeasible to do so, can you add iterators (might have
to
> be
> > > just const_iterators?) to that class?  If the underlying storage is an
> actual
> > If we are to use iterators here, it should be non-const as we are modifying
> the
> > contents of the array.
> 
> That sounds scary given that the header file for Mojo::Array says that it
> provides "read-only access" to the array data.  Maybe that comment is
incorrect?
It's apparently outdated as it already exposes non-const operator[] as a public
method.
> 
> It seems from array_internal.h like ultimately the underlying type ArrayTraits
> deals with is always a vector.  Can't we just typedef a Traits::iterator type
to
> the underlying vector iterator and add begin() and end() functions that return
> the iterators of that vector?

std::vector<StorageType>::iterator cannot be used directly as the iterator of
mojo::Array because we cannot get T& by simply dereferencing it.
vector<StorageType>::iterator should be wrapped by a class which implements
operator* to return *reinterpret_cast<T*>(iter->buf).
This wrapper class should also implement other operators (++, --, +/- vs. int, -
vs another iterator, +=, -=, ->, >, >=, <, <=, ==, !=, ...) and iterator traits
(http://www.cplusplus.com/reference/iterator/iterator_traits/).

Can we go ahead with iterating with indices for now?
Even if we are to add iterators to mojo::Array, I'd like to do it in a separate
patch as it should be not trivial and may cause a lengthy design discussion.

[Peter Kasting, 2014-07-29 03:12:12]

On 2014/07/29 02:46:36, hashimoto wrote:
> Can we go ahead with iterating with indices for now?
> Even if we are to add iterators to mojo::Array, I'd like to do it in a
separate
> patch as it should be not trivial and may cause a lengthy design discussion.

OK.

[hashimoto, 2014-07-29 03:57:04]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-07-29 03:58:18]

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

[commit-bot: I haz the power, 2014-07-29 07:44:58]

Change committed as 286110

[Mark P, 2014-07-30 17:37:37]

Reviewers,

In the future, please tell me when someone wants to remove logging code that I
added.  In this case it happens to be okay (it's been a few months since I
looked at that field with logs analysis), but I don't like the precedent and
would've appreciated a warning.

thanks,
mark

[Ilya Sherman, 2014-07-30 20:31:31]

On 2014/07/30 17:37:37, Mark P wrote:
> Reviewers,
> 
> In the future, please tell me when someone wants to remove logging code that I
> added.  In this case it happens to be okay (it's been a few months since I
> looked at that field with logs analysis), but I don't like the precedent and
> would've appreciated a warning.
> 
> thanks,
> mark

Sorry about that.  I'll try to make sure to forward omnibox proto reviews to you
in the future :)