Issue 8144013: Add a check to the registry before the intent infobar is shown.

Greg Billock

9 years, 2 months ago (2011-10-05 18:30:18 UTC) #1

James Hawkins

http://codereview.chromium.org/8144013/diff/1/chrome/browser/intents/register_intent_handler_infobar_delegate.cc File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right): http://codereview.chromium.org/8144013/diff/1/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode67 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:67: class IntentCheckConsumer : public WebIntentsRegistry::Consumer { Add a method ...

9 years, 2 months ago (2011-10-05 18:37:12 UTC) #2

Greg Billock

There's some "intents" "provider" "service" terminology confusion in the DB layer here. Shall I repair ...

9 years, 2 months ago (2011-10-05 22:19:02 UTC) #3

James Hawkins

http://codereview.chromium.org/8144013/diff/8001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right): http://codereview.chromium.org/8144013/diff/8001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode73 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:73: if (!provider_exists) Braces required due to multi-line. http://codereview.chromium.org/8144013/diff/8001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode82 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:82: ...

9 years, 2 months ago (2011-10-05 22:27:13 UTC) #4

Greg Billock

http://codereview.chromium.org/8144013/diff/8001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right): http://codereview.chromium.org/8144013/diff/8001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode73 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:73: if (!provider_exists) On 2011/10/05 22:27:17, James Hawkins wrote: > ...

9 years, 2 months ago (2011-10-05 22:45:53 UTC) #5

James Hawkins

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right): http://codereview.chromium.org/8144013/diff/12001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode68 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:68: void CheckProvider( Are you sure the params don't fit ...

9 years, 2 months ago (2011-10-06 17:46:04 UTC) #6

Greg Billock

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right): http://codereview.chromium.org/8144013/diff/12001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode68 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:68: void CheckProvider( On 2011/10/06 17:46:04, James Hawkins wrote: > ...

9 years, 2 months ago (2011-10-06 20:21:00 UTC) #7

James Hawkins

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc#newcode98 chrome/browser/webdata/web_intents_table.cc:98: return false; On 2011/10/06 20:21:00, Greg Billock wrote: > ...

9 years, 2 months ago (2011-10-06 20:55:03 UTC) #8

Greg Billock

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc#newcode98 chrome/browser/webdata/web_intents_table.cc:98: return false; On 2011/10/06 20:55:03, James Hawkins wrote: > ...

9 years, 2 months ago (2011-10-06 21:37:57 UTC) #9

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_intents_table.cc (right):

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_intents_table.cc:98: return false;
On 2011/10/06 20:55:03, James Hawkins wrote:
> On 2011/10/06 20:21:00, Greg Billock wrote:
> > On 2011/10/06 17:46:04, James Hawkins wrote:
> > > Don't handle NOTREACHEDs by returning false. Leave the NOTREACHED and
remove
> > the
> > > return (and the braces).
> > 
> > In opt, NOTREACHED is just LOG(ERROR)
> 
> Correct. Chrome style is to not handle DCHECKs and NOTREACHEDs by returning
> early.  Better to crash then paper over an incorrect assertion.

That's the thing; it isn't a strong assertion and won't crash in opt. Perhaps a
better plan would be to have a stronger NOTREACHED, but the DB code is pretty
full of this idiom, which I think is the correct behavior given the existing
meaning of NOTREACHED.

http://codereview.chromium.org/8144013/diff/13014/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service_unittest.cc (right):

http://codereview.chromium.org/8144013/diff/13014/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service_unittest.cc:647: service.action =
ASCIIToUTF16("share1");
On 2011/10/06 20:55:03, James Hawkins wrote:
> Wha?  Why did you make this share1?  The point of my suggestion to register
two
> actions was so we'd return two actions instead of just one.  Also, now that I
> think about it, the name of this method is misleading, since we're getting web
> intents ForURLAndAction.  I really think it should just be URL (in the
> implementation as well).

Hmm. I see what you're saying about naming. The existing method should perhaps
be "GetWebIntentsForAction".

If we're reading by service_url, we may as well go back to the way I was doing
it before; it'll require a full table scan unless we add a service_url index. I
don't know if that's reasonable or not -- the action index will probably be
quite weak in practice. How expensive are multiple sqlite indices?

James Hawkins

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc#newcode98 chrome/browser/webdata/web_intents_table.cc:98: return false; On 2011/10/06 21:37:57, Greg Billock wrote: > ...

9 years, 2 months ago (2011-10-06 21:42:32 UTC) #10

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_intents_table.cc (right):

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_intents_table.cc:98: return false;
On 2011/10/06 21:37:57, Greg Billock wrote:
> On 2011/10/06 20:55:03, James Hawkins wrote:
> > On 2011/10/06 20:21:00, Greg Billock wrote:
> > > On 2011/10/06 17:46:04, James Hawkins wrote:
> > > > Don't handle NOTREACHEDs by returning false. Leave the NOTREACHED and
> remove
> > > the
> > > > return (and the braces).
> > > 
> > > In opt, NOTREACHED is just LOG(ERROR)
> > 
> > Correct. Chrome style is to not handle DCHECKs and NOTREACHEDs by returning
> > early.  Better to crash then paper over an incorrect assertion.
> 
> That's the thing; it isn't a strong assertion and won't crash in opt. Perhaps
a
> better plan would be to have a stronger NOTREACHED, but the DB code is pretty
> full of this idiom, which I think is the correct behavior given the existing
> meaning of NOTREACHED.

You're missing the point. I realize NOTREACHED itself will not crash in opt;
however, the next few lines of code will crash, which is what we want in the
wild.

http://codereview.chromium.org/8144013/diff/13014/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service_unittest.cc (right):

http://codereview.chromium.org/8144013/diff/13014/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service_unittest.cc:647: service.action =
ASCIIToUTF16("share1");
On 2011/10/06 21:37:57, Greg Billock wrote:
> On 2011/10/06 20:55:03, James Hawkins wrote:
> > Wha?  Why did you make this share1?  The point of my suggestion to register
> two
> > actions was so we'd return two actions instead of just one.  Also, now that
I
> > think about it, the name of this method is misleading, since we're getting
web
> > intents ForURLAndAction.  I really think it should just be URL (in the
> > implementation as well).
> 
> Hmm. I see what you're saying about naming. The existing method should perhaps
> be "GetWebIntentsForAction".
> 
> If we're reading by service_url, we may as well go back to the way I was doing
> it before; it'll require a full table scan unless we add a service_url index.
I
> don't know if that's reasonable or not -- the action index will probably be
> quite weak in practice. How expensive are multiple sqlite indices?

*shrugs*

Greg Billock

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc#newcode98 chrome/browser/webdata/web_intents_table.cc:98: return false; On 2011/10/06 21:42:32, James Hawkins wrote: > ...

9 years, 2 months ago (2011-10-06 22:13:11 UTC) #11

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_intents_table.cc (right):

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_intents_table.cc:98: return false;
On 2011/10/06 21:42:32, James Hawkins wrote:
> On 2011/10/06 21:37:57, Greg Billock wrote:
> > On 2011/10/06 20:55:03, James Hawkins wrote:
> > > On 2011/10/06 20:21:00, Greg Billock wrote:
> > > > On 2011/10/06 17:46:04, James Hawkins wrote:
> > > > > Don't handle NOTREACHEDs by returning false. Leave the NOTREACHED and
> > remove
> > > > the
> > > > > return (and the braces).
> > > > 
> > > > In opt, NOTREACHED is just LOG(ERROR)
> > > 
> > > Correct. Chrome style is to not handle DCHECKs and NOTREACHEDs by
returning
> > > early.  Better to crash then paper over an incorrect assertion.
> > 
> > That's the thing; it isn't a strong assertion and won't crash in opt.
Perhaps
> a
> > better plan would be to have a stronger NOTREACHED, but the DB code is
pretty
> > full of this idiom, which I think is the correct behavior given the existing
> > meaning of NOTREACHED.
> 
> You're missing the point. I realize NOTREACHED itself will not crash in opt;
> however, the next few lines of code will crash, which is what we want in the
> wild.

I hear you, but see a lot of evidence that the opposite view is widely held. It
looks to me like the authors of /webdata use this pattern basically everywhere,
presumably for good reason. Perhaps these failures are more common than you'd
hope or something. Or used to be, and we should make a CL to take them out
uniformly. Creativity in dealing with DB errors is seldom advisable.

http://codereview.chromium.org/8144013/diff/13014/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service_unittest.cc (right):

http://codereview.chromium.org/8144013/diff/13014/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service_unittest.cc:647: service.action =
ASCIIToUTF16("share1");
On 2011/10/06 21:42:32, James Hawkins wrote:
> On 2011/10/06 21:37:57, Greg Billock wrote:
> > On 2011/10/06 20:55:03, James Hawkins wrote:
> > > Wha?  Why did you make this share1?  The point of my suggestion to
register
> > two
> > > actions was so we'd return two actions instead of just one.  Also, now
that
> I
> > > think about it, the name of this method is misleading, since we're getting
> web
> > > intents ForURLAndAction.  I really think it should just be URL (in the
> > > implementation as well).
> > 
> > Hmm. I see what you're saying about naming. The existing method should
perhaps
> > be "GetWebIntentsForAction".
> > 
> > If we're reading by service_url, we may as well go back to the way I was
doing
> > it before; it'll require a full table scan unless we add a service_url
index.
> I
> > don't know if that's reasonable or not -- the action index will probably be
> > quite weak in practice. How expensive are multiple sqlite indices?
> 
> *shrugs*

As in "indices aren't that bad" or as in "oh well, full table scans aren't so
bad" or as in "yeah, let's just look up by action and url, then"?

SQLite docs I found suggest indices can be harmful for small tables; it's faster
to just scan. I'm not sure what 'small' means, though.

James Hawkins

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc#newcode98 chrome/browser/webdata/web_intents_table.cc:98: return false; On 2011/10/06 22:13:12, Greg Billock wrote: > ...

9 years, 2 months ago (2011-10-07 20:29:47 UTC) #12

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_intents_table.cc (right):

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_intents_table.cc:98: return false;
On 2011/10/06 22:13:12, Greg Billock wrote:
> On 2011/10/06 21:42:32, James Hawkins wrote:
> > On 2011/10/06 21:37:57, Greg Billock wrote:
> > > On 2011/10/06 20:55:03, James Hawkins wrote:
> > > > On 2011/10/06 20:21:00, Greg Billock wrote:
> > > > > On 2011/10/06 17:46:04, James Hawkins wrote:
> > > > > > Don't handle NOTREACHEDs by returning false. Leave the NOTREACHED
and
> > > remove
> > > > > the
> > > > > > return (and the braces).
> > > > > 
> > > > > In opt, NOTREACHED is just LOG(ERROR)
> > > > 
> > > > Correct. Chrome style is to not handle DCHECKs and NOTREACHEDs by
> returning
> > > > early.  Better to crash then paper over an incorrect assertion.
> > > 
> > > That's the thing; it isn't a strong assertion and won't crash in opt.
> Perhaps
> > a
> > > better plan would be to have a stronger NOTREACHED, but the DB code is
> pretty
> > > full of this idiom, which I think is the correct behavior given the
existing
> > > meaning of NOTREACHED.
> > 
> > You're missing the point. I realize NOTREACHED itself will not crash in opt;
> > however, the next few lines of code will crash, which is what we want in the
> > wild.
> 
> I hear you, but see a lot of evidence that the opposite view is widely held.
It
> looks to me like the authors of /webdata use this pattern basically
everywhere,
> presumably for good reason. Perhaps these failures are more common than you'd
> hope or something. Or used to be, and we should make a CL to take them out
> uniformly. Creativity in dealing with DB errors is seldom advisable.

They aren't common at all, and the returns for all of these need to be removed
(including other code that does the same thing).

See the very bottom of
https://sites.google.com/a/chromium.org/dev/developers/coding-style

http://codereview.chromium.org/8144013/diff/13014/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service_unittest.cc (right):

http://codereview.chromium.org/8144013/diff/13014/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service_unittest.cc:647: service.action =
ASCIIToUTF16("share1");
On 2011/10/06 22:13:12, Greg Billock wrote:
> On 2011/10/06 21:42:32, James Hawkins wrote:
> > On 2011/10/06 21:37:57, Greg Billock wrote:
> > > On 2011/10/06 20:55:03, James Hawkins wrote:
> > > > Wha?  Why did you make this share1?  The point of my suggestion to
> register
> > > two
> > > > actions was so we'd return two actions instead of just one.  Also, now
> that
> > I
> > > > think about it, the name of this method is misleading, since we're
getting
> > web
> > > > intents ForURLAndAction.  I really think it should just be URL (in the
> > > > implementation as well).
> > > 
> > > Hmm. I see what you're saying about naming. The existing method should
> perhaps
> > > be "GetWebIntentsForAction".
> > > 
> > > If we're reading by service_url, we may as well go back to the way I was
> doing
> > > it before; it'll require a full table scan unless we add a service_url
> index.
> > I
> > > don't know if that's reasonable or not -- the action index will probably
be
> > > quite weak in practice. How expensive are multiple sqlite indices?
> > 
> > *shrugs*
> 
> As in "indices aren't that bad" or as in "oh well, full table scans aren't so
> bad" or as in "yeah, let's just look up by action and url, then"?
> 
> SQLite docs I found suggest indices can be harmful for small tables; it's
faster
> to just scan. I'm not sure what 'small' means, though.

Per-offline, I wouldn't try to optimize this yet (and I don't know the answer to
your question).

Greg Billock

Modified the DB code to just scan looking for the service_url. Added a TODO with ...

9 years, 2 months ago (2011-10-07 23:00:48 UTC) #13

Modified the DB code to just scan looking for the service_url. Added a TODO with
a pointer towards what I think will be the final plan.

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_intents_table.cc (right):

http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_intents_table.cc:98: return false;
On 2011/10/07 20:29:48, James Hawkins wrote:
> On 2011/10/06 22:13:12, Greg Billock wrote:
> > On 2011/10/06 21:42:32, James Hawkins wrote:
> > > On 2011/10/06 21:37:57, Greg Billock wrote:
> > > > On 2011/10/06 20:55:03, James Hawkins wrote:
> > > > > On 2011/10/06 20:21:00, Greg Billock wrote:
> > > > > > On 2011/10/06 17:46:04, James Hawkins wrote:
> > > > > > > Don't handle NOTREACHEDs by returning false. Leave the NOTREACHED
> and
> > > > remove
> > > > > > the
> > > > > > > return (and the braces).
> > > > > > 
> > > > > > In opt, NOTREACHED is just LOG(ERROR)
> > > > > 
> > > > > Correct. Chrome style is to not handle DCHECKs and NOTREACHEDs by
> > returning
> > > > > early.  Better to crash then paper over an incorrect assertion.
> > > > 
> > > > That's the thing; it isn't a strong assertion and won't crash in opt.
> > Perhaps
> > > a
> > > > better plan would be to have a stronger NOTREACHED, but the DB code is
> > pretty
> > > > full of this idiom, which I think is the correct behavior given the
> existing
> > > > meaning of NOTREACHED.
> > > 
> > > You're missing the point. I realize NOTREACHED itself will not crash in
opt;
> > > however, the next few lines of code will crash, which is what we want in
the
> > > wild.
> > 
> > I hear you, but see a lot of evidence that the opposite view is widely held.
> It
> > looks to me like the authors of /webdata use this pattern basically
> everywhere,
> > presumably for good reason. Perhaps these failures are more common than
you'd
> > hope or something. Or used to be, and we should make a CL to take them out
> > uniformly. Creativity in dealing with DB errors is seldom advisable.
> 
> They aren't common at all, and the returns for all of these need to be removed
> (including other code that does the same thing).
> 
> See the very bottom of
> https://sites.google.com/a/chromium.org/dev/developers/coding-style

To the contrary, everywhere in /webdata NOTREACHED() is used, it is followed by
a similar return statement, with two exceptions: web_apps_table.cc:90, where the
code is already in a position where it should return true despite errors, and
web_data_service:667 where it is a void method and the last statement in the
method. As against about 150 instances of this pattern. They are common to the
point of ubiquity in this layer of the code.

That doesn't mean it isn't wrong; perhaps they're mostly copy-pasted from early
days and we should change it. I don't think striking out on our own here is a
good plan, though. A CL that strips this pattern sent to someone familiar with
the DB system would be better.

Greg Billock

On 2011/10/07 23:00:48, Greg Billock wrote: > Modified the DB code to just scan looking ...

9 years, 2 months ago (2011-10-11 17:52:54 UTC) #14

On 2011/10/07 23:00:48, Greg Billock wrote:
> Modified the DB code to just scan looking for the service_url. Added a TODO
with
> a pointer towards what I think will be the final plan.
> 
>
http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
> File chrome/browser/webdata/web_intents_table.cc (right):
> 
>
http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
> chrome/browser/webdata/web_intents_table.cc:98: return false;
> On 2011/10/07 20:29:48, James Hawkins wrote:
> > On 2011/10/06 22:13:12, Greg Billock wrote:
> > > On 2011/10/06 21:42:32, James Hawkins wrote:
> > > > On 2011/10/06 21:37:57, Greg Billock wrote:
> > > > > On 2011/10/06 20:55:03, James Hawkins wrote:
> > > > > > On 2011/10/06 20:21:00, Greg Billock wrote:
> > > > > > > On 2011/10/06 17:46:04, James Hawkins wrote:
> > > > > > > > Don't handle NOTREACHEDs by returning false. Leave the
NOTREACHED
> > and
> > > > > remove
> > > > > > > the
> > > > > > > > return (and the braces).
> > > > > > > 
> > > > > > > In opt, NOTREACHED is just LOG(ERROR)
> > > > > > 
> > > > > > Correct. Chrome style is to not handle DCHECKs and NOTREACHEDs by
> > > returning
> > > > > > early.  Better to crash then paper over an incorrect assertion.
> > > > > 
> > > > > That's the thing; it isn't a strong assertion and won't crash in opt.
> > > Perhaps
> > > > a
> > > > > better plan would be to have a stronger NOTREACHED, but the DB code is
> > > pretty
> > > > > full of this idiom, which I think is the correct behavior given the
> > existing
> > > > > meaning of NOTREACHED.
> > > > 
> > > > You're missing the point. I realize NOTREACHED itself will not crash in
> opt;
> > > > however, the next few lines of code will crash, which is what we want in
> the
> > > > wild.
> > > 
> > > I hear you, but see a lot of evidence that the opposite view is widely
held.
> > It
> > > looks to me like the authors of /webdata use this pattern basically
> > everywhere,
> > > presumably for good reason. Perhaps these failures are more common than
> you'd
> > > hope or something. Or used to be, and we should make a CL to take them out
> > > uniformly. Creativity in dealing with DB errors is seldom advisable.
> > 
> > They aren't common at all, and the returns for all of these need to be
removed
> > (including other code that does the same thing).
> > 
> > See the very bottom of
> > https://sites.google.com/a/chromium.org/dev/developers/coding-style
> 
> To the contrary, everywhere in /webdata NOTREACHED() is used, it is followed
by
> a similar return statement, with two exceptions: web_apps_table.cc:90, where
the
> code is already in a position where it should return true despite errors, and
> web_data_service:667 where it is a void method and the last statement in the
> method. As against about 150 instances of this pattern. They are common to the
> point of ubiquity in this layer of the code.
> 
> That doesn't mean it isn't wrong; perhaps they're mostly copy-pasted from
early
> days and we should change it. I don't think striking out on our own here is a
> good plan, though. A CL that strips this pattern sent to someone familiar with
> the DB system would be better.

Looking around /webdata a bit more, there are definitely cases where the code
just returns in these statement prepare error pathways without calling
NOTREACHED. IOW, these asserts are probably just debugging aids while
creating/changing these embedded string SQL statements. I can appreciate the
utility of that, but I'm happy to take out the assertion. I think it is unwise
to take out the return, though. Does that sound good to you?

James Hawkins

On 2011/10/11 17:52:54, Greg Billock wrote: > On 2011/10/07 23:00:48, Greg Billock wrote: > > ...

9 years, 2 months ago (2011-10-11 17:55:19 UTC) #15

On 2011/10/11 17:52:54, Greg Billock wrote:
> On 2011/10/07 23:00:48, Greg Billock wrote:
> > Modified the DB code to just scan looking for the service_url. Added a TODO
> with
> > a pointer towards what I think will be the final plan.
> > 
> >
>
http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
> > File chrome/browser/webdata/web_intents_table.cc (right):
> > 
> >
>
http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
> > chrome/browser/webdata/web_intents_table.cc:98: return false;
> > On 2011/10/07 20:29:48, James Hawkins wrote:
> > > On 2011/10/06 22:13:12, Greg Billock wrote:
> > > > On 2011/10/06 21:42:32, James Hawkins wrote:
> > > > > On 2011/10/06 21:37:57, Greg Billock wrote:
> > > > > > On 2011/10/06 20:55:03, James Hawkins wrote:
> > > > > > > On 2011/10/06 20:21:00, Greg Billock wrote:
> > > > > > > > On 2011/10/06 17:46:04, James Hawkins wrote:
> > > > > > > > > Don't handle NOTREACHEDs by returning false. Leave the
> NOTREACHED
> > > and
> > > > > > remove
> > > > > > > > the
> > > > > > > > > return (and the braces).
> > > > > > > > 
> > > > > > > > In opt, NOTREACHED is just LOG(ERROR)
> > > > > > > 
> > > > > > > Correct. Chrome style is to not handle DCHECKs and NOTREACHEDs by
> > > > returning
> > > > > > > early.  Better to crash then paper over an incorrect assertion.
> > > > > > 
> > > > > > That's the thing; it isn't a strong assertion and won't crash in
opt.
> > > > Perhaps
> > > > > a
> > > > > > better plan would be to have a stronger NOTREACHED, but the DB code
is
> > > > pretty
> > > > > > full of this idiom, which I think is the correct behavior given the
> > > existing
> > > > > > meaning of NOTREACHED.
> > > > > 
> > > > > You're missing the point. I realize NOTREACHED itself will not crash
in
> > opt;
> > > > > however, the next few lines of code will crash, which is what we want
in
> > the
> > > > > wild.
> > > > 
> > > > I hear you, but see a lot of evidence that the opposite view is widely
> held.
> > > It
> > > > looks to me like the authors of /webdata use this pattern basically
> > > everywhere,
> > > > presumably for good reason. Perhaps these failures are more common than
> > you'd
> > > > hope or something. Or used to be, and we should make a CL to take them
out
> > > > uniformly. Creativity in dealing with DB errors is seldom advisable.
> > > 
> > > They aren't common at all, and the returns for all of these need to be
> removed
> > > (including other code that does the same thing).
> > > 
> > > See the very bottom of
> > > https://sites.google.com/a/chromium.org/dev/developers/coding-style
> > 
> > To the contrary, everywhere in /webdata NOTREACHED() is used, it is followed
> by
> > a similar return statement, with two exceptions: web_apps_table.cc:90, where
> the
> > code is already in a position where it should return true despite errors,
and
> > web_data_service:667 where it is a void method and the last statement in the
> > method. As against about 150 instances of this pattern. They are common to
the
> > point of ubiquity in this layer of the code.
> > 
> > That doesn't mean it isn't wrong; perhaps they're mostly copy-pasted from
> early
> > days and we should change it. I don't think striking out on our own here is
a
> > good plan, though. A CL that strips this pattern sent to someone familiar
with
> > the DB system would be better.
> 
> Looking around /webdata a bit more, there are definitely cases where the code
> just returns in these statement prepare error pathways without calling
> NOTREACHED. IOW, these asserts are probably just debugging aids while
> creating/changing these embedded string SQL statements. I can appreciate the
> utility of that, but I'm happy to take out the assertion. I think it is unwise
> to take out the return, though. Does that sound good to you?

No, the SQL statements should not fail.  We need to document that with
NOTREACHEDs.  If they do fail in the wild, we don't want to paper over that with
a return; we want to crash later in the function.

I realize a lot of existing code is doing the wrong thing (either solely
returning, or NOTREACHED then returning).  That does not justify not doing the
right thing in new code.

Greg Billock

See sql/statement.h is_valid(): // Returns true if the statement can be executed. All functions can ...

9 years, 2 months ago (2011-10-11 18:22:14 UTC) #16

See sql/statement.h is_valid():

  // Returns true if the statement can be executed. All functions can
still  // be used if the statement is invalid, but they will return
failure or some  // default value. This is because the statement can
become invalid in the  // middle of executing a command if there is a
serioud error and the database  // has to be reset.  bool is_valid
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_usages&...
const { return ref_
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_jump_to...
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/connection.h&ct=xref_jump_t...;
}  // These operators allow conveniently checking if the statement is
valid  // or not. See the pattern above for an example.  operator bool
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_usages&...
const { return is_valid
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_jump_to...;
}  bool operator!
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_usages&...
const { return !is_valid
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_jump_to...;
}

I read this as discouraging mucking about with invalid statements. If
there's an error making them for whatever reason, the code won't necessarily
crash, it'll do something unplanned with the database. This is not a good
road to go down. :-)

(In other news, operator overloading is not always the wisest plan
either...) I'm trying to figure out who actually knows this code; this idiom
dates from the earliest import into svn, and may have just been copied from
some sqlite docs for all I know.

I think it is wiser to keep code novelty dealing with the database to a
minimum, despite the style guide, until we know what's going on. At that
point, they can all be left alone or fixed as the case may be.


On Tue, Oct 11, 2011 at 10:55 AM, <jhawkins@chromium.org> wrote:

> On 2011/10/11 17:52:54, Greg Billock wrote:
>
>> On 2011/10/07 23:00:48, Greg Billock wrote:
>> > Modified the DB code to just scan looking for the service_url. Added a
>> TODO
>> with
>> > a pointer towards what I think will be the final plan.
>> >
>> >
>>
>
> http://codereview.chromium.**org/8144013/diff/12001/chrome/**
>
browser/webdata/web_intents_**table.cc<http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc>
>
>> > File chrome/browser/webdata/web_**intents_table.cc (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/8144013/diff/12001/chrome/**
>
browser/webdata/web_intents_**table.cc#newcode98<http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc#newcode98>
>
>> > chrome/browser/webdata/web_**intents_table.cc:98: return false;
>> > On 2011/10/07 20:29:48, James Hawkins wrote:
>> > > On 2011/10/06 22:13:12, Greg Billock wrote:
>> > > > On 2011/10/06 21:42:32, James Hawkins wrote:
>> > > > > On 2011/10/06 21:37:57, Greg Billock wrote:
>> > > > > > On 2011/10/06 20:55:03, James Hawkins wrote:
>> > > > > > > On 2011/10/06 20:21:00, Greg Billock wrote:
>> > > > > > > > On 2011/10/06 17:46:04, James Hawkins wrote:
>> > > > > > > > > Don't handle NOTREACHEDs by returning false. Leave the
>> NOTREACHED
>> > > and
>> > > > > > remove
>> > > > > > > > the
>> > > > > > > > > return (and the braces).
>> > > > > > > >
>> > > > > > > > In opt, NOTREACHED is just LOG(ERROR)
>> > > > > > >
>> > > > > > > Correct. Chrome style is to not handle DCHECKs and NOTREACHEDs
>> by
>> > > > returning
>> > > > > > > early.  Better to crash then paper over an incorrect
>> assertion.
>> > > > > >
>> > > > > > That's the thing; it isn't a strong assertion and won't crash in
>>
> opt.
>
>> > > > Perhaps
>> > > > > a
>> > > > > > better plan would be to have a stronger NOTREACHED, but the DB
>> code
>>
> is
>
>> > > > pretty
>> > > > > > full of this idiom, which I think is the correct behavior given
>> the
>> > > existing
>> > > > > > meaning of NOTREACHED.
>> > > > >
>> > > > > You're missing the point. I realize NOTREACHED itself will not
>> crash
>>
> in
>
>> > opt;
>> > > > > however, the next few lines of code will crash, which is what we
>> want
>>
> in
>
>> > the
>> > > > > wild.
>> > > >
>> > > > I hear you, but see a lot of evidence that the opposite view is
>> widely
>> held.
>> > > It
>> > > > looks to me like the authors of /webdata use this pattern basically
>> > > everywhere,
>> > > > presumably for good reason. Perhaps these failures are more common
>> than
>> > you'd
>> > > > hope or something. Or used to be, and we should make a CL to take
>> them
>>
> out
>
>> > > > uniformly. Creativity in dealing with DB errors is seldom advisable.
>> > >
>> > > They aren't common at all, and the returns for all of these need to be
>> removed
>> > > (including other code that does the same thing).
>> > >
>> > > See the very bottom of
>> > > https://sites.google.com/a/**chromium.org/dev/developers/**
>>
coding-style<https://sites.google.com/a/chromium.org/dev/developers/coding-style>
>> >
>> > To the contrary, everywhere in /webdata NOTREACHED() is used, it is
>> followed
>> by
>> > a similar return statement, with two exceptions: web_apps_table.cc:90,
>> where
>> the
>> > code is already in a position where it should return true despite
>> errors,
>>
> and
>
>> > web_data_service:667 where it is a void method and the last statement in
>> the
>> > method. As against about 150 instances of this pattern. They are common
>> to
>>
> the
>
>> > point of ubiquity in this layer of the code.
>> >
>> > That doesn't mean it isn't wrong; perhaps they're mostly copy-pasted
>> from
>> early
>> > days and we should change it. I don't think striking out on our own here
>> is
>>
> a
>
>> > good plan, though. A CL that strips this pattern sent to someone
>> familiar
>>
> with
>
>> > the DB system would be better.
>>
>
>  Looking around /webdata a bit more, there are definitely cases where the
>> code
>> just returns in these statement prepare error pathways without calling
>> NOTREACHED. IOW, these asserts are probably just debugging aids while
>> creating/changing these embedded string SQL statements. I can appreciate
>> the
>> utility of that, but I'm happy to take out the assertion. I think it is
>> unwise
>> to take out the return, though. Does that sound good to you?
>>
>
> No, the SQL statements should not fail.  We need to document that with
> NOTREACHEDs.  If they do fail in the wild, we don't want to paper over that
> with
> a return; we want to crash later in the function.
>
> I realize a lot of existing code is doing the wrong thing (either solely
> returning, or NOTREACHED then returning).  That does not justify not doing
> the
> right thing in new code.
>
>
http://codereview.chromium.**org/8144013/<http://codereview.chromium.org/8144...
>

Greg Billock

I've been talking to Scott Hess about this. He's not really happy with the existing ...

9 years, 2 months ago (2011-10-11 23:39:19 UTC) #17

I've been talking to Scott Hess about this. He's not really happy with the
existing state of this pattern, but mostly because upstream calling code
isn't necessarily handling errors correctly.

The NOTREACHED is so that any schema changes unreflected in the code will
break hard in development or testing. The returns are because if statements
become invalid in production, you shouldn't continue. In that case, the
statement is invalid, but methods on it will continue to operate and fail
silently. (They shouldn't corrupt the DB; there are internal Statement
checks that guard against that.) Overall, his suggestion is to just keep
using this pattern although it isn't optimal; improvements are probably best
to make either in combination with Statement API or in calling code.


On Tue, Oct 11, 2011 at 11:22 AM, Greg Billock <gbillock@chromium.org>wrote:

> See sql/statement.h is_valid():
>
>   // Returns true if the statement can be executed. All functions can still 
// be used if the statement is invalid, but they will return failure or some  //
default value. This is because the statement can become invalid in the  //
middle of executing a command if there is a serioud error and the database  //
has to be reset.  bool is_valid
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_usages&...
const { return ref_
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_jump_to...
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/connection.h&ct=xref_jump_t...;
}  // These operators allow conveniently checking if the statement is valid  //
or not. See the pattern above for an example.  operator bool
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_usages&...
const { return is_valid
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_jump_to...;
}  bool operator!
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_usages&...
const { return !is_valid
<http://codesearch.google.com/#OAMlx_jo-ck/src/sql/statement.h&ct=xref_jump_to...;
}
>
> I read this as discouraging mucking about with invalid statements. If
> there's an error making them for whatever reason, the code won't necessarily
> crash, it'll do something unplanned with the database. This is not a good
> road to go down. :-)
>
> (In other news, operator overloading is not always the wisest plan
> either...) I'm trying to figure out who actually knows this code; this idiom
> dates from the earliest import into svn, and may have just been copied from
> some sqlite docs for all I know.
>
> I think it is wiser to keep code novelty dealing with the database to a
> minimum, despite the style guide, until we know what's going on. At that
> point, they can all be left alone or fixed as the case may be.
>
>
>
> On Tue, Oct 11, 2011 at 10:55 AM, <jhawkins@chromium.org> wrote:
>
>> On 2011/10/11 17:52:54, Greg Billock wrote:
>>
>>> On 2011/10/07 23:00:48, Greg Billock wrote:
>>> > Modified the DB code to just scan looking for the service_url. Added a
>>> TODO
>>> with
>>> > a pointer towards what I think will be the final plan.
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.**org/8144013/diff/12001/chrome/**
>>
browser/webdata/web_intents_**table.cc<http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc>
>>
>>> > File chrome/browser/webdata/web_**intents_table.cc (right):
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.**org/8144013/diff/12001/chrome/**
>>
browser/webdata/web_intents_**table.cc#newcode98<http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_intents_table.cc#newcode98>
>>
>>> > chrome/browser/webdata/web_**intents_table.cc:98: return false;
>>> > On 2011/10/07 20:29:48, James Hawkins wrote:
>>> > > On 2011/10/06 22:13:12, Greg Billock wrote:
>>> > > > On 2011/10/06 21:42:32, James Hawkins wrote:
>>> > > > > On 2011/10/06 21:37:57, Greg Billock wrote:
>>> > > > > > On 2011/10/06 20:55:03, James Hawkins wrote:
>>> > > > > > > On 2011/10/06 20:21:00, Greg Billock wrote:
>>> > > > > > > > On 2011/10/06 17:46:04, James Hawkins wrote:
>>> > > > > > > > > Don't handle NOTREACHEDs by returning false. Leave the
>>> NOTREACHED
>>> > > and
>>> > > > > > remove
>>> > > > > > > > the
>>> > > > > > > > > return (and the braces).
>>> > > > > > > >
>>> > > > > > > > In opt, NOTREACHED is just LOG(ERROR)
>>> > > > > > >
>>> > > > > > > Correct. Chrome style is to not handle DCHECKs and
>>> NOTREACHEDs by
>>> > > > returning
>>> > > > > > > early.  Better to crash then paper over an incorrect
>>> assertion.
>>> > > > > >
>>> > > > > > That's the thing; it isn't a strong assertion and won't crash
>>> in
>>>
>> opt.
>>
>>> > > > Perhaps
>>> > > > > a
>>> > > > > > better plan would be to have a stronger NOTREACHED, but the DB
>>> code
>>>
>> is
>>
>>> > > > pretty
>>> > > > > > full of this idiom, which I think is the correct behavior given
>>> the
>>> > > existing
>>> > > > > > meaning of NOTREACHED.
>>> > > > >
>>> > > > > You're missing the point. I realize NOTREACHED itself will not
>>> crash
>>>
>> in
>>
>>> > opt;
>>> > > > > however, the next few lines of code will crash, which is what we
>>> want
>>>
>> in
>>
>>> > the
>>> > > > > wild.
>>> > > >
>>> > > > I hear you, but see a lot of evidence that the opposite view is
>>> widely
>>> held.
>>> > > It
>>> > > > looks to me like the authors of /webdata use this pattern basically
>>> > > everywhere,
>>> > > > presumably for good reason. Perhaps these failures are more common
>>> than
>>> > you'd
>>> > > > hope or something. Or used to be, and we should make a CL to take
>>> them
>>>
>> out
>>
>>> > > > uniformly. Creativity in dealing with DB errors is seldom
>>> advisable.
>>> > >
>>> > > They aren't common at all, and the returns for all of these need to
>>> be
>>> removed
>>> > > (including other code that does the same thing).
>>> > >
>>> > > See the very bottom of
>>> > > https://sites.google.com/a/**chromium.org/dev/developers/**
>>>
coding-style<https://sites.google.com/a/chromium.org/dev/developers/coding-style>
>>> >
>>> > To the contrary, everywhere in /webdata NOTREACHED() is used, it is
>>> followed
>>> by
>>> > a similar return statement, with two exceptions: web_apps_table.cc:90,
>>> where
>>> the
>>> > code is already in a position where it should return true despite
>>> errors,
>>>
>> and
>>
>>> > web_data_service:667 where it is a void method and the last statement
>>> in the
>>> > method. As against about 150 instances of this pattern. They are common
>>> to
>>>
>> the
>>
>>> > point of ubiquity in this layer of the code.
>>> >
>>> > That doesn't mean it isn't wrong; perhaps they're mostly copy-pasted
>>> from
>>> early
>>> > days and we should change it. I don't think striking out on our own
>>> here is
>>>
>> a
>>
>>> > good plan, though. A CL that strips this pattern sent to someone
>>> familiar
>>>
>> with
>>
>>> > the DB system would be better.
>>>
>>
>>  Looking around /webdata a bit more, there are definitely cases where the
>>> code
>>> just returns in these statement prepare error pathways without calling
>>> NOTREACHED. IOW, these asserts are probably just debugging aids while
>>> creating/changing these embedded string SQL statements. I can appreciate
>>> the
>>> utility of that, but I'm happy to take out the assertion. I think it is
>>> unwise
>>> to take out the return, though. Does that sound good to you?
>>>
>>
>> No, the SQL statements should not fail.  We need to document that with
>> NOTREACHEDs.  If they do fail in the wild, we don't want to paper over
>> that with
>> a return; we want to crash later in the function.
>>
>> I realize a lot of existing code is doing the wrong thing (either solely
>> returning, or NOTREACHED then returning).  That does not justify not doing
>> the
>> right thing in new code.
>>
>>
http://codereview.chromium.**org/8144013/<http://codereview.chromium.org/8144...
>>
>
>

James Hawkins

I just finished a discussion with David Holloway where we codified how these statements should ...

9 years, 2 months ago (2011-10-11 23:45:13 UTC) #18

I just finished a discussion with David Holloway where we codified how
these statements should be handled.  The gist of it is:

so to summarize: "The Right Thing" is: (1) eliminate returns that pair
with NOTREACHED() calls.  (2) If a failure can happen because of user
data then |return false| should not be paired with a NOTREACHED() at
this level (but should be at the caller's level), (3) any method that
has only NOTREACHED() and no valid |return false| should become a void
function.

let's use that WARN_IF_NOT_RETURN_VALUE_CHECKED thingamagiger too.
then the compiler can help us out.

That was from a chat; I'm going to codify this into better
documentation.  The comments for sql::Statement need to be fixed as
well.

On Tue, Oct 11, 2011 at 4:39 PM, Greg Billock <gbillock@chromium.org> wrote:
> I've been talking to Scott Hess about this. He's not really happy with the
> existing state of this pattern, but mostly because upstream calling code
> isn't necessarily handling errors correctly.
> The NOTREACHED is so that any schema changes unreflected in the code will
> break hard in development or testing. The returns are because if statements
> become invalid in production, you shouldn't continue. In that case, the
> statement is invalid, but methods on it will continue to operate and fail
> silently. (They shouldn't corrupt the DB; there are internal Statement
> checks that guard against that.) Overall, his suggestion is to just keep
> using this pattern although it isn't optimal; improvements are probably best
> to make either in combination with Statement API or in calling code.
>
> On Tue, Oct 11, 2011 at 11:22 AM, Greg Billock <gbillock@chromium.org>
> wrote:
>>
>> See sql/statement.h is_valid():
>>
>>   // Returns true if the statement can be executed. All functions can
>> still
>>   // be used if the statement is invalid, but they will return failure or
>> some
>>   // default value. This is because the statement can become invalid in
>> the
>>   // middle of executing a command if there is a serioud error and the
>> database
>>   // has to be reset.
>>   bool is_valid() const { return ref_->is_valid(); }
>>
>>   // These operators allow conveniently checking if the statement is valid
>>   // or not. See the pattern above for an example.
>>   operator bool() const { return is_valid(); }
>>   bool operator!() const { return !is_valid(); }
>>
>> I read this as discouraging mucking about with invalid statements. If
>> there's an error making them for whatever reason, the code won't necessarily
>> crash, it'll do something unplanned with the database. This is not a good
>> road to go down. :-)
>> (In other news, operator overloading is not always the wisest plan
>> either...) I'm trying to figure out who actually knows this code; this idiom
>> dates from the earliest import into svn, and may have just been copied from
>> some sqlite docs for all I know.
>> I think it is wiser to keep code novelty dealing with the database to a
>> minimum, despite the style guide, until we know what's going on. At that
>> point, they can all be left alone or fixed as the case may be.
>>
>>
>> On Tue, Oct 11, 2011 at 10:55 AM, <jhawkins@chromium.org> wrote:
>>>
>>> On 2011/10/11 17:52:54, Greg Billock wrote:
>>>>
>>>> On 2011/10/07 23:00:48, Greg Billock wrote:
>>>> > Modified the DB code to just scan looking for the service_url. Added a
>>>> > TODO
>>>> with
>>>> > a pointer towards what I think will be the final plan.
>>>> >
>>>> >
>>>
>>>
>>>
http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
>>>>
>>>> > File chrome/browser/webdata/web_intents_table.cc (right):
>>>> >
>>>> >
>>>
>>>
>>>
http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
>>>>
>>>> > chrome/browser/webdata/web_intents_table.cc:98: return false;
>>>> > On 2011/10/07 20:29:48, James Hawkins wrote:
>>>> > > On 2011/10/06 22:13:12, Greg Billock wrote:
>>>> > > > On 2011/10/06 21:42:32, James Hawkins wrote:
>>>> > > > > On 2011/10/06 21:37:57, Greg Billock wrote:
>>>> > > > > > On 2011/10/06 20:55:03, James Hawkins wrote:
>>>> > > > > > > On 2011/10/06 20:21:00, Greg Billock wrote:
>>>> > > > > > > > On 2011/10/06 17:46:04, James Hawkins wrote:
>>>> > > > > > > > > Don't handle NOTREACHEDs by returning false. Leave the
>>>> NOTREACHED
>>>> > > and
>>>> > > > > > remove
>>>> > > > > > > > the
>>>> > > > > > > > > return (and the braces).
>>>> > > > > > > >
>>>> > > > > > > > In opt, NOTREACHED is just LOG(ERROR)
>>>> > > > > > >
>>>> > > > > > > Correct. Chrome style is to not handle DCHECKs and
>>>> > > > > > > NOTREACHEDs by
>>>> > > > returning
>>>> > > > > > > early.  Better to crash then paper over an incorrect
>>>> > > > > > > assertion.
>>>> > > > > >
>>>> > > > > > That's the thing; it isn't a strong assertion and won't crash
>>>> > > > > > in
>>>
>>> opt.
>>>>
>>>> > > > Perhaps
>>>> > > > > a
>>>> > > > > > better plan would be to have a stronger NOTREACHED, but the DB
>>>> > > > > > code
>>>
>>> is
>>>>
>>>> > > > pretty
>>>> > > > > > full of this idiom, which I think is the correct behavior
>>>> > > > > > given the
>>>> > > existing
>>>> > > > > > meaning of NOTREACHED.
>>>> > > > >
>>>> > > > > You're missing the point. I realize NOTREACHED itself will not
>>>> > > > > crash
>>>
>>> in
>>>>
>>>> > opt;
>>>> > > > > however, the next few lines of code will crash, which is what we
>>>> > > > > want
>>>
>>> in
>>>>
>>>> > the
>>>> > > > > wild.
>>>> > > >
>>>> > > > I hear you, but see a lot of evidence that the opposite view is
>>>> > > > widely
>>>> held.
>>>> > > It
>>>> > > > looks to me like the authors of /webdata use this pattern
>>>> > > > basically
>>>> > > everywhere,
>>>> > > > presumably for good reason. Perhaps these failures are more common
>>>> > > > than
>>>> > you'd
>>>> > > > hope or something. Or used to be, and we should make a CL to take
>>>> > > > them
>>>
>>> out
>>>>
>>>> > > > uniformly. Creativity in dealing with DB errors is seldom
>>>> > > > advisable.
>>>> > >
>>>> > > They aren't common at all, and the returns for all of these need to
>>>> > > be
>>>> removed
>>>> > > (including other code that does the same thing).
>>>> > >
>>>> > > See the very bottom of
>>>> > > https://sites.google.com/a/chromium.org/dev/developers/coding-style
>>>> >
>>>> > To the contrary, everywhere in /webdata NOTREACHED() is used, it is
>>>> > followed
>>>> by
>>>> > a similar return statement, with two exceptions: web_apps_table.cc:90,
>>>> > where
>>>> the
>>>> > code is already in a position where it should return true despite
>>>> > errors,
>>>
>>> and
>>>>
>>>> > web_data_service:667 where it is a void method and the last statement
>>>> > in the
>>>> > method. As against about 150 instances of this pattern. They are
>>>> > common to
>>>
>>> the
>>>>
>>>> > point of ubiquity in this layer of the code.
>>>> >
>>>> > That doesn't mean it isn't wrong; perhaps they're mostly copy-pasted
>>>> > from
>>>> early
>>>> > days and we should change it. I don't think striking out on our own
>>>> > here is
>>>
>>> a
>>>>
>>>> > good plan, though. A CL that strips this pattern sent to someone
>>>> > familiar
>>>
>>> with
>>>>
>>>> > the DB system would be better.
>>>
>>>> Looking around /webdata a bit more, there are definitely cases where the
>>>> code
>>>> just returns in these statement prepare error pathways without calling
>>>> NOTREACHED. IOW, these asserts are probably just debugging aids while
>>>> creating/changing these embedded string SQL statements. I can appreciate
>>>> the
>>>> utility of that, but I'm happy to take out the assertion. I think it is
>>>> unwise
>>>> to take out the return, though. Does that sound good to you?
>>>
>>> No, the SQL statements should not fail.  We need to document that with
>>> NOTREACHEDs.  If they do fail in the wild, we don't want to paper over
>>> that with
>>> a return; we want to crash later in the function.
>>>
>>> I realize a lot of existing code is doing the wrong thing (either solely
>>> returning, or NOTREACHED then returning).  That does not justify not
>>> doing the
>>> right thing in new code.
>>>
>>> http://codereview.chromium.org/8144013/
>>
>
>

Greg Billock

On Tue, Oct 11, 2011 at 4:44 PM, James Hawkins <jhawkins@chromium.org>wrote: > I just finished ...

9 years, 2 months ago (2011-10-12 17:01:36 UTC) #19

On Tue, Oct 11, 2011 at 4:44 PM, James Hawkins <jhawkins@chromium.org>wrote:

> I just finished a discussion with David Holloway where we codified how
> these statements should be handled.  The gist of it is:
>
> so to summarize: "The Right Thing" is: (1) eliminate returns that pair
> with NOTREACHED() calls.  (2) If a failure can happen because of user
> data then |return false| should not be paired with a NOTREACHED() at
> this level (but should be at the caller's level), (3) any method that
> has only NOTREACHED() and no valid |return false| should become a void
> function.
>
> let's use that WARN_IF_NOT_RETURN_VALUE_CHECKED thingamagiger too.
> then the compiler can help us out.
>
> That was from a chat; I'm going to codify this into better
> documentation.  The comments for sql::Statement need to be fixed as
> well.
>

Statement should also change contract to at least DCHECK if used in an
invalid state. (That's where this NOTREACHED is much better placed.)

This looks like a good plan to me. For this CL, I think we should follow
Scott's advice and just commit it to be changed with a wider overhaul.



>
> On Tue, Oct 11, 2011 at 4:39 PM, Greg Billock <gbillock@chromium.org>
> wrote:
> > I've been talking to Scott Hess about this. He's not really happy with
> the
> > existing state of this pattern, but mostly because upstream calling code
> > isn't necessarily handling errors correctly.
> > The NOTREACHED is so that any schema changes unreflected in the code will
> > break hard in development or testing. The returns are because if
> statements
> > become invalid in production, you shouldn't continue. In that case, the
> > statement is invalid, but methods on it will continue to operate and fail
> > silently. (They shouldn't corrupt the DB; there are internal Statement
> > checks that guard against that.) Overall, his suggestion is to just keep
> > using this pattern although it isn't optimal; improvements are probably
> best
> > to make either in combination with Statement API or in calling code.
> >
> > On Tue, Oct 11, 2011 at 11:22 AM, Greg Billock <gbillock@chromium.org>
> > wrote:
> >>
> >> See sql/statement.h is_valid():
> >>
> >>   // Returns true if the statement can be executed. All functions can
> >> still
> >>   // be used if the statement is invalid, but they will return failure
> or
> >> some
> >>   // default value. This is because the statement can become invalid in
> >> the
> >>   // middle of executing a command if there is a serioud error and the
> >> database
> >>   // has to be reset.
> >>   bool is_valid() const { return ref_->is_valid(); }
> >>
> >>   // These operators allow conveniently checking if the statement is
> valid
> >>   // or not. See the pattern above for an example.
> >>   operator bool() const { return is_valid(); }
> >>   bool operator!() const { return !is_valid(); }
> >>
> >> I read this as discouraging mucking about with invalid statements. If
> >> there's an error making them for whatever reason, the code won't
> necessarily
> >> crash, it'll do something unplanned with the database. This is not a
> good
> >> road to go down. :-)
> >> (In other news, operator overloading is not always the wisest plan
> >> either...) I'm trying to figure out who actually knows this code; this
> idiom
> >> dates from the earliest import into svn, and may have just been copied
> from
> >> some sqlite docs for all I know.
> >> I think it is wiser to keep code novelty dealing with the database to a
> >> minimum, despite the style guide, until we know what's going on. At that
> >> point, they can all be left alone or fixed as the case may be.
> >>
> >>
> >> On Tue, Oct 11, 2011 at 10:55 AM, <jhawkins@chromium.org> wrote:
> >>>
> >>> On 2011/10/11 17:52:54, Greg Billock wrote:
> >>>>
> >>>> On 2011/10/07 23:00:48, Greg Billock wrote:
> >>>> > Modified the DB code to just scan looking for the service_url. Added
> a
> >>>> > TODO
> >>>> with
> >>>> > a pointer towards what I think will be the final plan.
> >>>> >
> >>>> >
> >>>
> >>>
> >>>
>
http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
> >>>>
> >>>> > File chrome/browser/webdata/web_intents_table.cc (right):
> >>>> >
> >>>> >
> >>>
> >>>
> >>>
>
http://codereview.chromium.org/8144013/diff/12001/chrome/browser/webdata/web_...
> >>>>
> >>>> > chrome/browser/webdata/web_intents_table.cc:98: return false;
> >>>> > On 2011/10/07 20:29:48, James Hawkins wrote:
> >>>> > > On 2011/10/06 22:13:12, Greg Billock wrote:
> >>>> > > > On 2011/10/06 21:42:32, James Hawkins wrote:
> >>>> > > > > On 2011/10/06 21:37:57, Greg Billock wrote:
> >>>> > > > > > On 2011/10/06 20:55:03, James Hawkins wrote:
> >>>> > > > > > > On 2011/10/06 20:21:00, Greg Billock wrote:
> >>>> > > > > > > > On 2011/10/06 17:46:04, James Hawkins wrote:
> >>>> > > > > > > > > Don't handle NOTREACHEDs by returning false. Leave the
> >>>> NOTREACHED
> >>>> > > and
> >>>> > > > > > remove
> >>>> > > > > > > > the
> >>>> > > > > > > > > return (and the braces).
> >>>> > > > > > > >
> >>>> > > > > > > > In opt, NOTREACHED is just LOG(ERROR)
> >>>> > > > > > >
> >>>> > > > > > > Correct. Chrome style is to not handle DCHECKs and
> >>>> > > > > > > NOTREACHEDs by
> >>>> > > > returning
> >>>> > > > > > > early.  Better to crash then paper over an incorrect
> >>>> > > > > > > assertion.
> >>>> > > > > >
> >>>> > > > > > That's the thing; it isn't a strong assertion and won't
> crash
> >>>> > > > > > in
> >>>
> >>> opt.
> >>>>
> >>>> > > > Perhaps
> >>>> > > > > a
> >>>> > > > > > better plan would be to have a stronger NOTREACHED, but the
> DB
> >>>> > > > > > code
> >>>
> >>> is
> >>>>
> >>>> > > > pretty
> >>>> > > > > > full of this idiom, which I think is the correct behavior
> >>>> > > > > > given the
> >>>> > > existing
> >>>> > > > > > meaning of NOTREACHED.
> >>>> > > > >
> >>>> > > > > You're missing the point. I realize NOTREACHED itself will not
> >>>> > > > > crash
> >>>
> >>> in
> >>>>
> >>>> > opt;
> >>>> > > > > however, the next few lines of code will crash, which is what
> we
> >>>> > > > > want
> >>>
> >>> in
> >>>>
> >>>> > the
> >>>> > > > > wild.
> >>>> > > >
> >>>> > > > I hear you, but see a lot of evidence that the opposite view is
> >>>> > > > widely
> >>>> held.
> >>>> > > It
> >>>> > > > looks to me like the authors of /webdata use this pattern
> >>>> > > > basically
> >>>> > > everywhere,
> >>>> > > > presumably for good reason. Perhaps these failures are more
> common
> >>>> > > > than
> >>>> > you'd
> >>>> > > > hope or something. Or used to be, and we should make a CL to
> take
> >>>> > > > them
> >>>
> >>> out
> >>>>
> >>>> > > > uniformly. Creativity in dealing with DB errors is seldom
> >>>> > > > advisable.
> >>>> > >
> >>>> > > They aren't common at all, and the returns for all of these need
> to
> >>>> > > be
> >>>> removed
> >>>> > > (including other code that does the same thing).
> >>>> > >
> >>>> > > See the very bottom of
> >>>> > >
> https://sites.google.com/a/chromium.org/dev/developers/coding-style
> >>>> >
> >>>> > To the contrary, everywhere in /webdata NOTREACHED() is used, it is
> >>>> > followed
> >>>> by
> >>>> > a similar return statement, with two exceptions:
> web_apps_table.cc:90,
> >>>> > where
> >>>> the
> >>>> > code is already in a position where it should return true despite
> >>>> > errors,
> >>>
> >>> and
> >>>>
> >>>> > web_data_service:667 where it is a void method and the last
> statement
> >>>> > in the
> >>>> > method. As against about 150 instances of this pattern. They are
> >>>> > common to
> >>>
> >>> the
> >>>>
> >>>> > point of ubiquity in this layer of the code.
> >>>> >
> >>>> > That doesn't mean it isn't wrong; perhaps they're mostly copy-pasted
> >>>> > from
> >>>> early
> >>>> > days and we should change it. I don't think striking out on our own
> >>>> > here is
> >>>
> >>> a
> >>>>
> >>>> > good plan, though. A CL that strips this pattern sent to someone
> >>>> > familiar
> >>>
> >>> with
> >>>>
> >>>> > the DB system would be better.
> >>>
> >>>> Looking around /webdata a bit more, there are definitely cases where
> the
> >>>> code
> >>>> just returns in these statement prepare error pathways without calling
> >>>> NOTREACHED. IOW, these asserts are probably just debugging aids while
> >>>> creating/changing these embedded string SQL statements. I can
> appreciate
> >>>> the
> >>>> utility of that, but I'm happy to take out the assertion. I think it
> is
> >>>> unwise
> >>>> to take out the return, though. Does that sound good to you?
> >>>
> >>> No, the SQL statements should not fail.  We need to document that with
> >>> NOTREACHEDs.  If they do fail in the wild, we don't want to paper over
> >>> that with
> >>> a return; we want to crash later in the function.
> >>>
> >>> I realize a lot of existing code is doing the wrong thing (either
> solely
> >>> returning, or NOTREACHED then returning).  That does not justify not
> >>> doing the
> >>> right thing in new code.
> >>>
> >>> http://codereview.chromium.org/8144013/
> >>
> >
> >
>

Greg Billock

OK, I think we've got the boat launched on a Statement cleanup. If you lgtm ...

9 years, 2 months ago (2011-10-14 22:32:51 UTC) #20

Greg Billock

updated. On 2011/10/14 22:32:51, Greg Billock wrote: > OK, I think we've got the boat ...

9 years, 1 month ago (2011-10-26 23:43:25 UTC) #21

James Hawkins

http://codereview.chromium.org/8144013/diff/37001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right): http://codereview.chromium.org/8144013/diff/37001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode68 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:68: void CheckProvider( Move to an unnamed namespace. http://codereview.chromium.org/8144013/diff/37001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode70 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:70: ...

9 years, 1 month ago (2011-10-26 23:50:04 UTC) #22

Greg Billock

http://codereview.chromium.org/8144013/diff/37001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right): http://codereview.chromium.org/8144013/diff/37001/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode68 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:68: void CheckProvider( On 2011/10/26 23:50:06, James Hawkins wrote: > ...

9 years, 1 month ago (2011-10-29 00:03:19 UTC) #23

James Hawkins

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/intents/register_intent_handler_infobar_delegate.h File chrome/browser/intents/register_intent_handler_infobar_delegate.h (right): http://codereview.chromium.org/8144013/diff/44001/chrome/browser/intents/register_intent_handler_infobar_delegate.h#newcode35 chrome/browser/intents/register_intent_handler_infobar_delegate.h:35: // already been registered. If all registration details are ...

9 years, 1 month ago (2011-10-29 21:41:32 UTC) #24

Greg Billock

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/intents/register_intent_handler_infobar_delegate.h File chrome/browser/intents/register_intent_handler_infobar_delegate.h (right): http://codereview.chromium.org/8144013/diff/44001/chrome/browser/intents/register_intent_handler_infobar_delegate.h#newcode35 chrome/browser/intents/register_intent_handler_infobar_delegate.h:35: // already been registered. If all registration details are ...

9 years, 1 month ago (2011-10-31 17:07:08 UTC) #25

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/intents/regi...
File chrome/browser/intents/register_intent_handler_infobar_delegate.h (right):

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/intents/regi...
chrome/browser/intents/register_intent_handler_infobar_delegate.h:35: // already
been registered. If all registration details are the same, it does
On 2011/10/29 21:41:32, James Hawkins wrote:
> This second sentence should probably be reworded to reflect what the method
does
> instead of what it does not do.  The whole thing could be simplified to:
> 
> "Shows the intent registration InfoBar iff |service| has not already been
> registered."

I like that.

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.cc:264: const string16& action,
On 2011/10/29 21:41:32, James Hawkins wrote:
> Optional: You can save a line by moving |consumer| up to this line.
> 
> Here and elsewhere.

I think when split having these long args one per line is more digestible.

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.h (right):

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.h:372: // Get all web intent providers
registered using the specified |service_url|.
On 2011/10/29 21:41:32, James Hawkins wrote:
> nit: Document |consumer|.

Done.

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_intents_table.cc (right):

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_intents_table.cc:16: DCHECK(s);
On 2011/10/29 21:41:32, James Hawkins wrote:
> FYI this DCHECK is about the pointer |s| being non-NULL, not
> Statement::operator!() being true, so we really are saying "This point will
> never be NULL", so returning here is wrong.

There's an operator overload on !Statement which is an alias for is_valid. I'm
not in love with that idiom, and I've been pondering just stripping it in that
other cross-cutting CL, especially since it drastically reduces the number of
uses. 

The early return is necessary to satisfy the method's contract. Otherwise
passing an invalid statement will cause callers to believe the call has
succeeded when in fact it has failed.

James Hawkins

http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_intents_table.cc File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_intents_table.cc#newcode16 chrome/browser/webdata/web_intents_table.cc:16: DCHECK(s); On 2011/10/31 17:07:09, Greg Billock wrote: > On ...

9 years, 1 month ago (2011-11-02 07:23:46 UTC) #26

Greg Billock

On 2011/11/02 07:23:46, James Hawkins wrote: > http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_intents_table.cc > File chrome/browser/webdata/web_intents_table.cc (right): > > http://codereview.chromium.org/8144013/diff/44001/chrome/browser/webdata/web_intents_table.cc#newcode16 ...

9 years, 1 month ago (2011-11-02 15:51:32 UTC) #27

James Hawkins

LGTM with nits. http://codereview.chromium.org/8144013/diff/57002/chrome/browser/intents/register_intent_handler_infobar_delegate.cc File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right): http://codereview.chromium.org/8144013/diff/57002/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode92 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:92: base::Unretained(registry), Remove base::Unretained() from |registry|. Only ...

9 years, 1 month ago (2011-11-09 17:32:42 UTC) #28

Greg Billock

http://codereview.chromium.org/8144013/diff/57002/chrome/browser/intents/register_intent_handler_infobar_delegate.cc File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right): http://codereview.chromium.org/8144013/diff/57002/chrome/browser/intents/register_intent_handler_infobar_delegate.cc#newcode92 chrome/browser/intents/register_intent_handler_infobar_delegate.cc:92: base::Unretained(registry), On 2011/11/09 17:32:42, James Hawkins wrote: > Remove ...

9 years, 1 month ago (2011-11-10 00:21:31 UTC) #29

James Hawkins

http://codereview.chromium.org/8144013/diff/62003/chrome/browser/webdata/web_data_service.h File chrome/browser/webdata/web_data_service.h (right): http://codereview.chromium.org/8144013/diff/62003/chrome/browser/webdata/web_data_service.h#newcode378 chrome/browser/webdata/web_data_service.h:378: // Get all web intent providers registered using the ...

9 years, 1 month ago (2011-11-10 00:48:49 UTC) #30

Greg Billock

http://codereview.chromium.org/8144013/diff/62003/chrome/browser/webdata/web_data_service.h File chrome/browser/webdata/web_data_service.h (right): http://codereview.chromium.org/8144013/diff/62003/chrome/browser/webdata/web_data_service.h#newcode378 chrome/browser/webdata/web_data_service.h:378: // Get all web intent providers registered using the ...

9 years, 1 month ago (2011-11-10 17:59:41 UTC) #31

commit-bot: I haz the power

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

9 years, 1 month ago (2011-11-11 15:57:57 UTC) #32

Change committed as 109644

Issue 8144013: Add a check to the registry before the intent infobar is shown. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Move logic to DB level #

Patch Set 3 : Add doc #

Patch Set 4 : Merge to head #

Patch Set 5 : New test #

Patch Set 6 : Read DB only based on service_url. #

Patch Set 7 : Rebase #

Patch Set 8 : Fix up returns. #

Patch Set 9 : More updates #

Patch Set 10 : Add some comments #

Patch Set 11 : Fix is_valid call and merge namespace change #

Patch Set 12 : Merge fix-up #

Patch Set 13 : Bypass type check for existence query. #

Patch Set 14 : Fix up merge #

Patch Set 15 : Rebase to trunk, review comments. #

Patch Set 16 : Fix test webdata calls. #

Patch Set 17 : Fix provider to service in comments. #

Messages