Issue 684493002: Don't persist and sync omnibox extension keywords.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+220 lines, -137 lines)			Patch
M	chrome/browser/resources/options/search_engine_manager_engine_list.js	View		3 chunks	+2 lines, -5 lines	0 comments	Download
M	chrome/browser/ui/search_engines/keyword_editor_controller.cc	View	1 2	2 chunks	+5 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/webui/options/search_engine_manager_handler.h	View	1 2	1 chunk	+1 line, -2 lines	0 comments	Download
M	chrome/browser/ui/webui/options/search_engine_manager_handler.cc	View		5 chunks	+11 lines, -8 lines	0 comments	Download
M	components/search_engines/keyword_table.h	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/search_engines/keyword_table.cc	View	1 2 3 4 5	2 chunks	+8 lines, -0 lines	0 comments	Download
M	components/search_engines/search_host_to_urls_map.cc	View	1 2	2 chunks	+2 lines, -0 lines	0 comments	Download
M	components/search_engines/template_url_service.h	View		2 chunks	+3 lines, -1 line	0 comments	Download
M	components/search_engines/template_url_service.cc	View	1 2 3 4	16 chunks	+41 lines, -33 lines	0 comments	Download
M	components/search_engines/template_url_service_sync_unittest.cc	View	1 2 3 4	8 chunks	+45 lines, -16 lines	0 comments	Download
M	components/search_engines/template_url_service_unittest.cc	View	1 2 3 4	6 chunks	+26 lines, -35 lines	0 comments	Download
A +	components/test/data/web_database/version_58.sql	View	1 2 3 4	1 chunk	+16 lines, -30 lines	0 comments	Download
M	components/webdata/common/web_database.cc	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	components/webdata/common/web_database_migration_unittest.cc	View	1 2 3 4	2 chunks	+57 lines, -1 line	0 comments	Download

Messages

Total messages: 19 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Dan Beam

i don't really know this code that well... https://codereview.chromium.org/684493002/diff/1/chrome/browser/resources/options/search_engine_manager_engine_list.js File chrome/browser/resources/options/search_engine_manager_engine_list.js (left): https://codereview.chromium.org/684493002/diff/1/chrome/browser/resources/options/search_engine_manager_engine_list.js#oldcode185 chrome/browser/resources/options/search_engine_manager_engine_list.js:185: this.nameField_.disabled ...

6 years, 1 month ago (2014-10-28 01:36:59 UTC) #4

vasilii

https://codereview.chromium.org/684493002/diff/1/chrome/browser/resources/options/search_engine_manager_engine_list.js File chrome/browser/resources/options/search_engine_manager_engine_list.js (left): https://codereview.chromium.org/684493002/diff/1/chrome/browser/resources/options/search_engine_manager_engine_list.js#oldcode185 chrome/browser/resources/options/search_engine_manager_engine_list.js:185: this.nameField_.disabled = true; On 2014/10/28 01:36:59, Dan Beam wrote: ...

6 years, 1 month ago (2014-10-28 14:01:39 UTC) #5

Peter Kasting

This is basically a style review, rather than a functional one; it's beyond me to ...

6 years, 1 month ago (2014-10-29 18:55:51 UTC) #8

vasilii

The extension keywords are to be removed regardless of '_' number. The normal keywords are ...

6 years, 1 month ago (2014-11-03 15:14:31 UTC) #9

Peter Kasting

https://codereview.chromium.org/684493002/diff/20001/components/search_engines/template_url_service.cc File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/684493002/diff/20001/components/search_engines/template_url_service.cc#newcode879 components/search_engines/template_url_service.cc:879: // crbug/411197. We don't store omnibox extensions anymore. On ...

6 years, 1 month ago (2014-11-03 18:42:03 UTC) #10

vasilii

6 years, 1 month ago (2014-11-04 18:22:30 UTC) #11

https://codereview.chromium.org/684493002/diff/20001/components/search_engine...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/684493002/diff/20001/components/search_engine...
components/search_engines/template_url_service.cc:879: // crbug/411197. We don't
store omnibox extensions anymore.
On 2014/11/03 18:42:03, Peter Kasting wrote:
> On 2014/11/03 15:14:30, vasilii wrote:
> > On 2014/10/29 18:55:51, Peter Kasting wrote:
> > > Nit: Don't refer to bug numbers in comments; just give the reader all the
> > > context he needs right here.
> > > 
> > > Is this code going to become unnecessary at some point?  Should we be
doing
> > this
> > > as a database version migration pass instead?
> > 
> > It can be removed in the next version.
> 
> Really?  What if someone doesn't run Chrome for a couple months or something?

Then probably in 2 versions. The code is no longer needed if the user restarted
Chrome at least once.

https://codereview.chromium.org/684493002/diff/20001/components/search_engine...
components/search_engines/template_url_service.cc:1449:
syncer::SyncChange::ACTION_DELETE,
On 2014/11/03 18:42:03, Peter Kasting wrote:
> On 2014/11/03 15:14:30, vasilii wrote:
> > On 2014/10/29 18:55:51, Peter Kasting wrote:
> > > Will this mean if a user syncs a newer Chrome version, his older Chrome
> > versions
> > > will suddenly have their omnibox keywords stop working?
> > 
> > Yeah, there is a race condition here. Don't we believe that Chrome updates
> > itself very fast?
> 
> People frequently have long-lived Chrome processes.  For example, my work
> machine tends to have Chrome instances that run for weeks at a time without me
> restarting them.
> 
> Assume someone has sync running between an old and a new version of Chrome. 
> What will the consequences be?  Are the two going to continually fight each
> other?  Will the older copy of Chrome just have these keywords break until
it's
> updated to a newer version, with no other effects?
> 
> I don't know if we need to be smarter here.  Maybe the sync guys would have
> ideas.

Let's leave them in the Sync model then but ignore them on the client side. We
can remove the keywords from Sync when we believe there are no old versions
running.

Peter Kasting

6 years, 1 month ago (2014-11-04 19:11:56 UTC) #12

https://codereview.chromium.org/684493002/diff/20001/components/search_engine...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/684493002/diff/20001/components/search_engine...
components/search_engines/template_url_service.cc:879: // crbug/411197. We don't
store omnibox extensions anymore.
On 2014/11/04 18:22:30, vasilii wrote:
> On 2014/11/03 18:42:03, Peter Kasting wrote:
> > On 2014/11/03 15:14:30, vasilii wrote:
> > > On 2014/10/29 18:55:51, Peter Kasting wrote:
> > > > Nit: Don't refer to bug numbers in comments; just give the reader all
the
> > > > context he needs right here.
> > > > 
> > > > Is this code going to become unnecessary at some point?  Should we be
> doing
> > > this
> > > > as a database version migration pass instead?
> > > 
> > > It can be removed in the next version.
> > 
> > Really?  What if someone doesn't run Chrome for a couple months or
something?
> 
> Then probably in 2 versions. The code is no longer needed if the user
restarted
> Chrome at least once.

Here's the situation I'm worried about:

* User has some persisted extension keywords.
* User doesn't use Chrome for a long time.
* User starts Chrome on a version where we've removed this code.

I worry that in this case, we try to load the old extension keywords, and other
code in this file breaks in weird ways because it assumes that these aren't
persisted.

We can deal with this worry in a couple ways:
(1) Leave this code in long enough that the scenario above is very unlikely. 
There won't be a specific time when we know it's impossible, however.
(2) Ensure that if such keywords do get loaded here, nothing bad actually
happens.  Then on shutdown the keywords hopefully won't be persisted back to
disk and we'll be OK.
(3) Instead of doing this here, do this as a database version change in the web
data code (this is different than a prepopulate data change).  This allows us to
introduce a migration pass that will be run once for anyone whose data files are
old, no matter what the source and target Chrome versions are.  It also allows
us to remove the pass someday and do so in a way that triggers an error message,
or recreating the file, if someone manages to run a new Chrome version with an
outdated file.

Above, you're basically suggesting (1).  I think (2) and (3) are safer, and (3)
would make me most comfortable because it's more future-proof, more performant
(we don't scan the URLs if we're not running the migration pass), and keeps code
related to behavioral changes about what data we store in a dedicated place in
the web data code.

Doing (3) would mostly involve moving this code to a new
MigrateToVersionXAndBlah() function in keyword_table.h alongside the migrations
already there.

WDYT?

https://codereview.chromium.org/684493002/diff/60001/components/search_engine...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/684493002/diff/60001/components/search_engine...
components/search_engines/template_url_service.cc:1444: // The omnibox keywords
aren't synced anymore.
How about this:

We used to sync keywords associated with omnibox extensions, but no longer want
to.  However, if we delete these keywords from sync, we'll break any synced old
versions of Chrome which were relying on them.  Instead, for now we simply
ignore these.
TODO(vasilii): After a few Chrome versions, change this to go ahead and delete
these from sync.

vasilii

6 years, 1 month ago (2014-11-05 18:04:50 UTC) #13

https://codereview.chromium.org/684493002/diff/20001/components/search_engine...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/684493002/diff/20001/components/search_engine...
components/search_engines/template_url_service.cc:879: // crbug/411197. We don't
store omnibox extensions anymore.
On 2014/11/04 19:11:56, Peter Kasting wrote:
> On 2014/11/04 18:22:30, vasilii wrote:
> > On 2014/11/03 18:42:03, Peter Kasting wrote:
> > > On 2014/11/03 15:14:30, vasilii wrote:
> > > > On 2014/10/29 18:55:51, Peter Kasting wrote:
> > > > > Nit: Don't refer to bug numbers in comments; just give the reader all
> the
> > > > > context he needs right here.
> > > > > 
> > > > > Is this code going to become unnecessary at some point?  Should we be
> > doing
> > > > this
> > > > > as a database version migration pass instead?
> > > > 
> > > > It can be removed in the next version.
> > > 
> > > Really?  What if someone doesn't run Chrome for a couple months or
> something?
> > 
> > Then probably in 2 versions. The code is no longer needed if the user
> restarted
> > Chrome at least once.
> 
> Here's the situation I'm worried about:
> 
> * User has some persisted extension keywords.
> * User doesn't use Chrome for a long time.
> * User starts Chrome on a version where we've removed this code.
> 
> I worry that in this case, we try to load the old extension keywords, and
other
> code in this file breaks in weird ways because it assumes that these aren't
> persisted.
> 
> We can deal with this worry in a couple ways:
> (1) Leave this code in long enough that the scenario above is very unlikely. 
> There won't be a specific time when we know it's impossible, however.
> (2) Ensure that if such keywords do get loaded here, nothing bad actually
> happens.  Then on shutdown the keywords hopefully won't be persisted back to
> disk and we'll be OK.
> (3) Instead of doing this here, do this as a database version change in the
web
> data code (this is different than a prepopulate data change).  This allows us
to
> introduce a migration pass that will be run once for anyone whose data files
are
> old, no matter what the source and target Chrome versions are.  It also allows
> us to remove the pass someday and do so in a way that triggers an error
message,
> or recreating the file, if someone manages to run a new Chrome version with an
> outdated file.
> 
> Above, you're basically suggesting (1).  I think (2) and (3) are safer, and
(3)
> would make me most comfortable because it's more future-proof, more performant
> (we don't scan the URLs if we're not running the migration pass), and keeps
code
> related to behavioral changes about what data we store in a dedicated place in
> the web data code.
> 
> Doing (3) would mostly involve moving this code to a new
> MigrateToVersionXAndBlah() function in keyword_table.h alongside the
migrations
> already there.
> 
> WDYT?

Implemented #3.

https://codereview.chromium.org/684493002/diff/60001/components/search_engine...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/684493002/diff/60001/components/search_engine...
components/search_engines/template_url_service.cc:1444: // The omnibox keywords
aren't synced anymore.
On 2014/11/04 19:11:56, Peter Kasting wrote:
> How about this:
> 
> We used to sync keywords associated with omnibox extensions, but no longer
want
> to.  However, if we delete these keywords from sync, we'll break any synced
old
> versions of Chrome which were relying on them.  Instead, for now we simply
> ignore these.
> TODO(vasilii): After a few Chrome versions, change this to go ahead and delete
> these from sync.

Done.

Peter Kasting

LGTM! https://codereview.chromium.org/684493002/diff/80001/components/search_engines/keyword_table.h File components/search_engines/keyword_table.h (right): https://codereview.chromium.org/684493002/diff/80001/components/search_engines/keyword_table.h#newcode143 components/search_engines/keyword_table.h:143: bool MigrateToVersion59RemoveExtensionsKeywords(); Tiny nit: "ExtensionKeywords" seems more grammatical ...

6 years, 1 month ago (2014-11-05 23:21:14 UTC) #14

vasilii

https://codereview.chromium.org/684493002/diff/80001/components/search_engines/keyword_table.h File components/search_engines/keyword_table.h (right): https://codereview.chromium.org/684493002/diff/80001/components/search_engines/keyword_table.h#newcode143 components/search_engines/keyword_table.h:143: bool MigrateToVersion59RemoveExtensionsKeywords(); On 2014/11/05 23:21:14, Peter Kasting wrote: > ...

6 years, 1 month ago (2014-11-06 10:42:34 UTC) #15

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/684493002/100001

6 years, 1 month ago (2014-11-06 10:43:43 UTC) #17

commit-bot: I haz the power

6 years, 1 month ago (2014-11-06 12:00:02 UTC) #19

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/1fb8425a95c4681b95d174a225cdd562daac525f
Cr-Commit-Position: refs/heads/master@{#303010}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages