chromeos: Delete old, unused contacts code.

chromeos: Delete old, unused contacts code.

This never got turned on.

BUG=350213
TBR=darin@chromium.org

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

[Daniel Erat, 2014-03-07 10:42:42]

Please let me know if I'm deleting something that I shouldn't be from the
autocomplete code. I tried to keep the seemingly-metrics-related protobuf stuff
around...

[Mark P, 2014-03-07 17:20:13]

https://codereview.chromium.org/190063004/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.cc (left):

https://codereview.chromium.org/190063004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.cc:155: IDR_OMNIBOX_SEARCH,
You're going to revert this change to this file after reading my other comments.

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
File chrome/common/autocomplete_match_type.cc (left):

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
chrome/common/autocomplete_match_type.cc:27: "contact",
You're going to revert this change to this file after reading my other comments.

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
File chrome/common/autocomplete_match_type.h (left):

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
chrome/common/autocomplete_match_type.h:37: CONTACT,                     // One
of the user's contacts.
I know nothing in the comments makes this clear, but we recently discovered that
you cannot rename or renumber the entries in this enum because the values are
stored in the ShortcutsDatabase.  (Deleting this will cause BOOKMARK_TITLE
shortcuts to be mislabeled.)  Instead, simply rename this enum
DEPRECATED_CONTACT

[Peter Kasting, 2014-03-07 17:43:53]

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
File chrome/common/autocomplete_match_type.h (left):

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
chrome/common/autocomplete_match_type.h:37: CONTACT,                     // One
of the user's contacts.
On 2014/03/07 17:20:14, Mark P wrote:
> I know nothing in the comments makes this clear, but we recently discovered
that
> you cannot rename or renumber the entries in this enum because the values are
> stored in the ShortcutsDatabase.  (Deleting this will cause BOOKMARK_TITLE
> shortcuts to be mislabeled.)  Instead, simply rename this enum
> DEPRECATED_CONTACT

Well, you can delete it, you just need to renumber anything in the database with
a higher number.

In this case that's actually probably preferred, since if we're removing support
for this type entirely, we should probably write a migration pass that nukes any
entries of this type out of the database.

[Mark P, 2014-03-07 18:53:21]

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
File chrome/common/autocomplete_match_type.h (left):

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
chrome/common/autocomplete_match_type.h:37: CONTACT,                     // One
of the user's contacts.
On 2014/03/07 17:43:53, Peter Kasting wrote:
> On 2014/03/07 17:20:14, Mark P wrote:
> > I know nothing in the comments makes this clear, but we recently discovered
> that
> > you cannot rename or renumber the entries in this enum because the values
are
> > stored in the ShortcutsDatabase.  (Deleting this will cause BOOKMARK_TITLE
> > shortcuts to be mislabeled.)  Instead, simply rename this enum
> > DEPRECATED_CONTACT
> 
> Well, you can delete it, you just need to renumber anything in the database
with
> a higher number.
> 
> In this case that's actually probably preferred, since if we're removing
support
> for this type entirely, we should probably write a migration pass that nukes
any
> entries of this type out of the database.

Personally, I'd rather leave an obsolete enum value / enum hole than bloat
Chrome permanently with code for a migration pass.

[Daniel Erat, 2014-03-07 21:52:56]

Thanks for catching that.

https://codereview.chromium.org/190063004/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.cc (left):

https://codereview.chromium.org/190063004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.cc:155: IDR_OMNIBOX_SEARCH,
On 2014/03/07 17:20:14, Mark P wrote:
> You're going to revert this change to this file after reading my other
comments.

Done.

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
File chrome/common/autocomplete_match_type.cc (left):

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
chrome/common/autocomplete_match_type.cc:27: "contact",
On 2014/03/07 17:20:14, Mark P wrote:
> You're going to revert this change to this file after reading my other
comments.

Done.

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
File chrome/common/autocomplete_match_type.h (left):

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
chrome/common/autocomplete_match_type.h:37: CONTACT,                     // One
of the user's contacts.
On 2014/03/07 18:53:22, Mark P wrote:
> On 2014/03/07 17:43:53, Peter Kasting wrote:
> > On 2014/03/07 17:20:14, Mark P wrote:
> > > I know nothing in the comments makes this clear, but we recently
discovered
> > that
> > > you cannot rename or renumber the entries in this enum because the values
> are
> > > stored in the ShortcutsDatabase.  (Deleting this will cause BOOKMARK_TITLE
> > > shortcuts to be mislabeled.)  Instead, simply rename this enum
> > > DEPRECATED_CONTACT
> > 
> > Well, you can delete it, you just need to renumber anything in the database
> with
> > a higher number.
> > 
> > In this case that's actually probably preferred, since if we're removing
> support
> > for this type entirely, we should probably write a migration pass that nukes
> any
> > entries of this type out of the database.
> 
> Personally, I'd rather leave an obsolete enum value / enum hole than bloat
> Chrome permanently with code for a migration pass.

Done.

[Peter Kasting, 2014-03-07 22:09:54]

Argh, I didn't mail this draft.  I still think this value should be removed with
a migration pass, see below.

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
File chrome/common/autocomplete_match_type.h (left):

https://codereview.chromium.org/190063004/diff/20001/chrome/common/autocomple...
chrome/common/autocomplete_match_type.h:37: CONTACT,                     // One
of the user's contacts.
On 2014/03/07 18:53:22, Mark P wrote:
> On 2014/03/07 17:43:53, Peter Kasting wrote:
> > On 2014/03/07 17:20:14, Mark P wrote:
> > > I know nothing in the comments makes this clear, but we recently
discovered
> > that
> > > you cannot rename or renumber the entries in this enum because the values
> are
> > > stored in the ShortcutsDatabase.  (Deleting this will cause BOOKMARK_TITLE
> > > shortcuts to be mislabeled.)  Instead, simply rename this enum
> > > DEPRECATED_CONTACT
> > 
> > Well, you can delete it, you just need to renumber anything in the database
> with
> > a higher number.
> > 
> > In this case that's actually probably preferred, since if we're removing
> support
> > for this type entirely, we should probably write a migration pass that nukes
> any
> > entries of this type out of the database.
> 
> Personally, I'd rather leave an obsolete enum value / enum hole than bloat
> Chrome permanently with code for a migration pass.

Migration passes aren't permanent.  We're supposed to rip them out after a year
or so.  (After I get done removing command-line flags I might have to start on
the "remove migration passes" task.)

Anyway, I'm still worried about ripping out code that deals with this enum value
but still having its entries in the database.  Makes me nervous.

[satorux1, 2014-03-09 02:04:12]

chrome/browser/chromeos/contacts/ and google_apis/drive/ LGTM

[Mark P, 2014-03-09 03:44:17]

On Fri, Mar 7, 2014 at 2:09 PM, <pkasting@chromium.org> wrote:

>
> Anyway, I'm still worried about ripping out code that deals with this
> enum value but still having its entries in the database.  Makes me
> nervous.
>

Dan claims that this provider was never turned on; there should be no
entries in the database.

--mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Daniel Erat, 2014-03-09 04:19:00]

On 2014/03/09 03:44:17, Mark P wrote:
> On Fri, Mar 7, 2014 at 2:09 PM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=pkasting@chromium.org>
wrote:
> 
> >
> > Anyway, I'm still worried about ripping out code that deals with this
> > enum value but still having its entries in the database.  Makes me
> > nervous.
> >
> 
> Dan claims that this provider was never turned on; there should be no
> entries in the database.

It was in chrome://flags but not publicized or turned on by default.

[Peter Kasting, 2014-03-09 23:13:56]

On 2014/03/09 04:19:00, Daniel Erat wrote:
> It was in chrome://flags but not publicized or turned on by default.

And it only applied to ChromeOS, and didn't apply to the Omnibox.  Right?  What
_did_ it do, what data could it have left behind, and what consequences could
happen to someone who'd turned it on once this patch lands?

[Daniel Erat, 2014-03-09 23:28:27]

On 2014/03/09 23:13:56, Peter Kasting wrote:
> On 2014/03/09 04:19:00, Daniel Erat wrote:
> > It was in chrome://flags but not publicized or turned on by default.
> 
> And it only applied to ChromeOS, and didn't apply to the Omnibox.  Right? 
What
> _did_ it do, what data could it have left behind, and what consequences could
> happen to someone who'd turned it on once this patch lands?

Yes, that's correct.

When the flag was enabled, this code periodically downloaded the user's contacts
from Google via the GData API, cached them to a local LevelDB database named
"Google Contacts"[1], and returned those contacts as results for the Ash app
launcher (then called the "app list"). I don't understand the autocomplete code
well enough to know if that could result in additional data being written (to
ShortcutsDatabase? or is that just used for web pages that the user navigates
to?) and was hoping that you could comment on that part. :-)

1. I could add new code somewhere to delete this at startup, although I don't
know if it's worthwhile to do so, given that the file was typically small and
only written for users who messed around with chrome://flags.

[Peter Kasting, 2014-03-10 00:43:25]

On 2014/03/09 23:28:27, Daniel Erat wrote:
> On 2014/03/09 23:13:56, Peter Kasting wrote:
> > On 2014/03/09 04:19:00, Daniel Erat wrote:
> > > It was in chrome://flags but not publicized or turned on by default.
> > 
> > And it only applied to ChromeOS, and didn't apply to the Omnibox.  Right? 
> What
> > _did_ it do, what data could it have left behind, and what consequences
could
> > happen to someone who'd turned it on once this patch lands?
> 
> Yes, that's correct.
> 
> When the flag was enabled, this code periodically downloaded the user's
contacts
> from Google via the GData API, cached them to a local LevelDB database named
> "Google Contacts"[1], and returned those contacts as results for the Ash app
> launcher (then called the "app list"). I don't understand the autocomplete
code
> well enough to know if that could result in additional data being written (to
> ShortcutsDatabase? or is that just used for web pages that the user navigates
> to?) and was hoping that you could comment on that part. :-)

It probably depends on how the app launcher works.  If the app launcher uses a
ShortcutsProvider, maybe it could create these kinds of shortcuts.  Otherwise
no.

I still don't love having an XXX_DEPRECATED value in the enum, but I guess it's
OK to land as-is.  I'll just try and clean this up next time we muck with the
enum.

[Daniel Erat, 2014-03-17 05:30:45]

(TBR Darin for deletion of unused code.)

Mark, do you mind LGTM-ing if you're happy with the current state of this?

I ran ToT with --enable-contacts set, let it sync, and then verified that this
change appeared to work fine with the same profile.

[Mark P, 2014-03-17 16:12:43]

I don't think you answered Peter's last question:
>>>
It probably depends on how the app launcher works.  If the app launcher uses a
ShortcutsProvider, maybe it could create these kinds of shortcuts.  Otherwise
no.
>>>

[Daniel Erat, 2014-03-18 00:38:35]

On 2014/03/17 16:12:43, Mark P wrote:
> I don't think you answered Peter's last question:
> >>>
> It probably depends on how the app launcher works.  If the app launcher uses a
> ShortcutsProvider, maybe it could create these kinds of shortcuts.  Otherwise
> no.
> >>>

Sorry, I didn't see a question mark there. :-P

I neglected to mention it, but my testing also included checking the shortcuts
database after enabling the old contacts code to verify that contact entries
didn't appear there. I'm not sure how to actually activate results from the old
code at this point; I think that the app launcher code for displaying contact
results is long-deleted. It now displays results from the people search backend
instead.

I believe that the app launcher/list does query ShortcutsProvider (based on the
use of AutocompleteClassifier::kDefaultOmniboxProviders in
chrome/browser/ui/app_list/search/omnibox_provider.cc) but querying shortcuts
seems entirely distinct in the code from creating them.

OmniboxNavigationObserver::OnSuccessfulNavigation() calls
ShortcutsBackend::AddOrUpdateShortcut() in response to navigations. I believe
that it was never possible for contact results to trigger direct navigations, as
the app launcher/list just made calls via a private extension API to handle
clicks on these results:
https://chromiumcodereview.appspot.com/10916322/diff/6001/chrome/browser/ui/a...

[Peter Kasting, 2014-03-18 00:40:09]

I'm satisfied.  I don't think anyone's shortcut database should legitimately
have contact entries in it.

LGTM

[Daniel Erat, 2014-03-18 01:25:04]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-03-18 01:26:20]

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

[commit-bot: I haz the power, 2014-03-18 06:15:10]

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

[commit-bot: I haz the power, 2014-03-18 06:15:12]

Retried try job too often on win_rel for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Daniel Erat, 2014-03-18 06:17:33]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-03-18 06:17:53]

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

[commit-bot: I haz the power, 2014-03-18 18:41:48]

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

[commit-bot: I haz the power, 2014-03-18 18:41:51]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Mark P, 2014-03-18 19:34:17]

lgtm
(for the record)

[Daniel Erat, 2014-03-18 20:56:12]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-03-18 21:03:46]

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

[commit-bot: I haz the power, 2014-03-18 21:13:18]

Change committed as 257754