Fix for non-sticky defaults bug. Added a bunch of tests.

Fix for non-sticky defaults bug. Added a bunch of tests.

TBR=jhawkins@chromium.org
R=smckay@chromium.org,shess@chromium.org
BUG=148740
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157331

[Steve McKay, 2012-09-14 18:57:00]

LGTM with some nits.

https://codereview.chromium.org/10914278/diff/1/chrome/browser/intents/web_in...
File chrome/browser/intents/web_intents_registry.cc (right):

https://codereview.chromium.org/10914278/diff/1/chrome/browser/intents/web_in...
chrome/browser/intents/web_intents_registry.cc:291: // prefer it. If the new
value has a more specific pattern, prefer it.
Update the comment here to reflect the additional condition.

https://codereview.chromium.org/10914278/diff/1/chrome/browser/intents/web_in...
File chrome/browser/intents/web_intents_registry_unittest.cc (right):

https://codereview.chromium.org/10914278/diff/1/chrome/browser/intents/web_in...
chrome/browser/intents/web_intents_registry_unittest.cc:599: // Can get for
wildcard.
Does this address the bug you're fixing, or are you just adding a wildcard case?
From the code change it looks like the user date was at issue.

Add a crbug id to the comment. Help's future maintainers NOT re-factor your test
in a way that looses the intended guarantees.

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
File chrome/browser/webdata/web_intents_table.cc (right):

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
chrome/browser/webdata/web_intents_table.cc:98: if (!db_->Execute("DELETE FROM
web_intents_defaults WHERE scheme IS NULL"))
This'll ding startup time. We can do this one and only once in a database
version upgrade.

Feel free to do this in a different CL + add a tracking bug. Might be a good bug
for rouslan to get to know the database update system.

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
File chrome/browser/webdata/web_intents_table_unittest.cc (right):

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
chrome/browser/webdata/web_intents_table_unittest.cc:349:
ASSERT_EQ(URLPattern::PARSE_SUCCESS,
Nuke the assert, it's unrelated to the test. We can clean up the rest of the
class later.

[Greg Billock, 2012-09-14 19:12:28]

https://codereview.chromium.org/10914278/diff/1/chrome/browser/intents/web_in...
File chrome/browser/intents/web_intents_registry.cc (right):

https://codereview.chromium.org/10914278/diff/1/chrome/browser/intents/web_in...
chrome/browser/intents/web_intents_registry.cc:291: // prefer it. If the new
value has a more specific pattern, prefer it.
On 2012/09/14 18:57:00, Steve McKay wrote:
> Update the comment here to reflect the additional condition.

Done.

https://codereview.chromium.org/10914278/diff/1/chrome/browser/intents/web_in...
File chrome/browser/intents/web_intents_registry_unittest.cc (right):

https://codereview.chromium.org/10914278/diff/1/chrome/browser/intents/web_in...
chrome/browser/intents/web_intents_registry_unittest.cc:599: // Can get for
wildcard.
On 2012/09/14 18:57:00, Steve McKay wrote:
> Does this address the bug you're fixing, or are you just adding a wildcard
case?
> From the code change it looks like the user date was at issue.
> 
> Add a crbug id to the comment. Help's future maintainers NOT re-factor your
test
> in a way that looses the intended guarantees.

No, this is just a useful test we were missing I discovered while investigating.
The NULL scheme is the root cause of the problem, and is fixed in two ways.

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
File chrome/browser/webdata/web_intents_table.cc (right):

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
chrome/browser/webdata/web_intents_table.cc:98: if (!db_->Execute("DELETE FROM
web_intents_defaults WHERE scheme IS NULL"))
On 2012/09/14 18:57:00, Steve McKay wrote:
> This'll ding startup time. We can do this one and only once in a database
> version upgrade.
> 
> Feel free to do this in a different CL + add a tracking bug. Might be a good
bug
> for rouslan to get to know the database update system.

Scott, should we roll DB versions for this?

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
File chrome/browser/webdata/web_intents_table_unittest.cc (right):

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
chrome/browser/webdata/web_intents_table_unittest.cc:349:
ASSERT_EQ(URLPattern::PARSE_SUCCESS,
On 2012/09/14 18:57:00, Steve McKay wrote:
> Nuke the assert, it's unrelated to the test. We can clean up the rest of the
> class later.

Done.

[Scott Hess - ex-Googler, 2012-09-14 19:37:47]

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
File chrome/browser/webdata/web_intents_table.cc (right):

https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
chrome/browser/webdata/web_intents_table.cc:98: if (!db_->Execute("DELETE FROM
web_intents_defaults WHERE scheme IS NULL"))
On 2012/09/14 19:12:28, Greg Billock wrote:
> On 2012/09/14 18:57:00, Steve McKay wrote:
> > This'll ding startup time. We can do this one and only once in a database
> > version upgrade.
> > 
> > Feel free to do this in a different CL + add a tracking bug. Might be a good
> bug
> > for rouslan to get to know the database update system.
> 
> Scott, should we roll DB versions for this?

The UNIQUE constraint will be enforced by a tree keyed by the items in order. 
Since scheme is the second item in, this delete would effectively be a full
table scan, so it could be expensive to do every time.  [Depending on table
size, and what else you'll be getting up to with the table, etc.]

If you do version things, I suggest you add NOT NULL constraints to the columns
you want these to apply to.  That'll involve creating a new table and shuffling
things over, as the migration below already does.  Of course, that potentially
means you'll discover other cases where those columns are NULL, so you'll want
IS NOT NULL clauses in the WHERE of the INSERT-SELECT.

https://codereview.chromium.org/10914278/diff/5001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_intents_table.cc (right):

https://codereview.chromium.org/10914278/diff/5001/chrome/browser/webdata/web...
chrome/browser/webdata/web_intents_table.cc:97: // rendering the uniqueness
obsolete. Get rid of any such entries.
I think this mis-represents the problem, one NULL is not the same as another
NULL in case of UNIQUE for the same reason that x = NULL is not true if x is
NULL (which is why you say "x IS NULL" when making that test).

[Greg Billock, 2012-09-14 21:32:25]

On 2012/09/14 19:37:47, shess wrote:
>
https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
> File chrome/browser/webdata/web_intents_table.cc (right):
> 
>
https://codereview.chromium.org/10914278/diff/1/chrome/browser/webdata/web_in...
> chrome/browser/webdata/web_intents_table.cc:98: if (!db_->Execute("DELETE FROM
> web_intents_defaults WHERE scheme IS NULL"))
> On 2012/09/14 19:12:28, Greg Billock wrote:
> > On 2012/09/14 18:57:00, Steve McKay wrote:
> > > This'll ding startup time. We can do this one and only once in a database
> > > version upgrade.
> > > 
> > > Feel free to do this in a different CL + add a tracking bug. Might be a
good
> > bug
> > > for rouslan to get to know the database update system.
> > 
> > Scott, should we roll DB versions for this?
> 
> The UNIQUE constraint will be enforced by a tree keyed by the items in order. 
> Since scheme is the second item in, this delete would effectively be a full
> table scan, so it could be expensive to do every time.  [Depending on table
> size, and what else you'll be getting up to with the table, etc.]

We haven't released this problem to an official release or even to beta, so it's
only people with Canary and test builds that have profiles out of whack. I don't
expect this table to be very big for them or for users, and I anticipate just
removing this code the next release. That's why I didn't think advancing the
version made sense.

> 
> If you do version things, I suggest you add NOT NULL constraints to the
columns
> you want these to apply to.  That'll involve creating a new table and
shuffling
> things over, as the migration below already does.  Of course, that potentially
> means you'll discover other cases where those columns are NULL, so you'll want
> IS NOT NULL clauses in the WHERE of the INSERT-SELECT.
> 
>
https://codereview.chromium.org/10914278/diff/5001/chrome/browser/webdata/web...
> File chrome/browser/webdata/web_intents_table.cc (right):
> 
>
https://codereview.chromium.org/10914278/diff/5001/chrome/browser/webdata/web...
> chrome/browser/webdata/web_intents_table.cc:97: // rendering the uniqueness
> obsolete. Get rid of any such entries.
> I think this mis-represents the problem, one NULL is not the same as another
> NULL in case of UNIQUE for the same reason that x = NULL is not true if x is
> NULL (which is why you say "x IS NULL" when making that test).

[Greg Billock, 2012-09-14 21:32:33]

https://codereview.chromium.org/10914278/diff/5001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_intents_table.cc (right):

https://codereview.chromium.org/10914278/diff/5001/chrome/browser/webdata/web...
chrome/browser/webdata/web_intents_table.cc:97: // rendering the uniqueness
obsolete. Get rid of any such entries.
I see what you're saying, and that's what I'm trying to communicate here: if the
values are null, then you get duplicates where you thought you shouldn't. I'll
try to reword it.

On 2012/09/14 19:37:47, shess wrote:
> I think this mis-represents the problem, one NULL is not the same as another
> NULL in case of UNIQUE for the same reason that x = NULL is not true if x is
> NULL (which is why you say "x IS NULL" when making that test).

[Greg Billock, 2012-09-17 18:07:21]

https://codereview.chromium.org/10914278/diff/5001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_intents_table.cc (right):

https://codereview.chromium.org/10914278/diff/5001/chrome/browser/webdata/web...
chrome/browser/webdata/web_intents_table.cc:97: // rendering the uniqueness
obsolete. Get rid of any such entries.
OK, rephrased. Are you OK with this approach, Scott? I am accounting for the gap
elsewhere, so I could just remove this.

On 2012/09/14 21:32:33, Greg Billock wrote:
> I see what you're saying, and that's what I'm trying to communicate here: if
the
> values are null, then you get duplicates where you thought you shouldn't. I'll
> try to reword it.
> 
> On 2012/09/14 19:37:47, shess wrote:
> > I think this mis-represents the problem, one NULL is not the same as another
> > NULL in case of UNIQUE for the same reason that x = NULL is not true if x is
> > NULL (which is why you say "x IS NULL" when making that test).
>

[Scott Hess - ex-Googler, 2012-09-17 21:00:54]

LGTM.

The current wording of the comment sounds better.

I have no problem with deleting everything, or not, up to you.  Mostly, my point
was that the DELETE you coded will do a full table scan on that table every time
the DELETE is executed, even if there are not any NULL schema columns.  Tying it
to a version update would only do it once per user.

[willchan no longer on Chromium, 2012-09-17 21:52:45]

lgtm

[commit-bot: I haz the power, 2012-09-17 22:38:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10914278/9010

[commit-bot: I haz the power, 2012-09-17 22:38:30]

Presubmit check for 10914278-9010 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-09-17 22:41:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10914278/9010

[commit-bot: I haz the power, 2012-09-18 01:20:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-18 02:05:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10914278/20001

[commit-bot: I haz the power, 2012-09-18 03:24:12]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-18 04:34:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10914278/20001

[commit-bot: I haz the power, 2012-09-18 09:43:07]

Change committed as 157331