Add default intents methods for web data service.

Add default intents methods for web data service.

R=groby@chromium.org
BUG=114809
TEST=WebDataServiceTest.WebIntentsDefaultsTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122619

[groby-ooo-7-16, 2012-02-14 00:14:30]

http://codereview.chromium.org/9370043/diff/2001/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/9370043/diff/2001/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.cc:135: SetPort("*");
Why are we setting those here? Isn't that covered by the constructor already?

[Greg Billock, 2012-02-14 01:21:45]

http://codereview.chromium.org/9370043/diff/2001/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/9370043/diff/2001/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.cc:135: SetPort("*");
On 2012/02/14 00:14:30, groby wrote:
> Why are we setting those here? Isn't that covered by the constructor already? 

This is actually in another CL that has gone in already. It'll merge out of this
change. (Basically, the class wasn't resetting properly if it was ever set to
all-urls.)

[Greg Billock, 2012-02-14 19:56:18]

OK, this is now merged to head.

On 2012/02/14 01:21:45, Greg Billock wrote:
>
http://codereview.chromium.org/9370043/diff/2001/chrome/common/extensions/url...
> File chrome/common/extensions/url_pattern.cc (right):
> 
>
http://codereview.chromium.org/9370043/diff/2001/chrome/common/extensions/url...
> chrome/common/extensions/url_pattern.cc:135: SetPort("*");
> On 2012/02/14 00:14:30, groby wrote:
> > Why are we setting those here? Isn't that covered by the constructor
already? 
> 
> This is actually in another CL that has gone in already. It'll merge out of
this
> change. (Basically, the class wasn't resetting properly if it was ever set to
> all-urls.)

[Greg Billock, 2012-02-16 19:50:05]

On 2012/02/14 19:56:18, Greg Billock wrote:
> OK, this is now merged to head.
> 
> On 2012/02/14 01:21:45, Greg Billock wrote:
> >
>
http://codereview.chromium.org/9370043/diff/2001/chrome/common/extensions/url...
> > File chrome/common/extensions/url_pattern.cc (right):
> > 
> >
>
http://codereview.chromium.org/9370043/diff/2001/chrome/common/extensions/url...
> > chrome/common/extensions/url_pattern.cc:135: SetPort("*");
> > On 2012/02/14 00:14:30, groby wrote:
> > > Why are we setting those here? Isn't that covered by the constructor
> already? 
> > 
> > This is actually in another CL that has gone in already. It'll merge out of
> this
> > change. (Basically, the class wasn't resetting properly if it was ever set
to
> > all-urls.)

ping

[groby-ooo-7-16, 2012-02-16 23:14:38]

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
File chrome/browser/webdata/web_data_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
chrome/browser/webdata/web_data_service_unittest.cc:181: class
WebIntentsDefaultsConsumer : public WebDataServiceConsumer {
That's a lot of copy/paste - can we just templatize and typedef?

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
chrome/browser/webdata/web_data_service_unittest.cc:779: EXPECT_EQ(1U,
consumer.services_.size());
You probably want ASSERT_EQ on the size. If it's 0, you'll get nasty crashes in
the unit tests since they continue past a failed EXPECT. (Here and elsewhere)

[Greg Billock, 2012-02-17 01:12:36]

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
File chrome/browser/webdata/web_data_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
chrome/browser/webdata/web_data_service_unittest.cc:181: class
WebIntentsDefaultsConsumer : public WebDataServiceConsumer {
On 2012/02/16 23:14:38, groby wrote:
> That's a lot of copy/paste - can we just templatize and typedef?

This whole file has tons of this parallel structure. If they all used a common
template that'd perhaps be an advantage, but I don't know that picking out these
two will help. In any case, tests tend to be read more directly, and too much
fanciness just makes it harder to figure out what's going on.

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
chrome/browser/webdata/web_data_service_unittest.cc:779: EXPECT_EQ(1U,
consumer.services_.size());
On 2012/02/16 23:14:38, groby wrote:
> You probably want ASSERT_EQ on the size. If it's 0, you'll get nasty crashes
in
> the unit tests since they continue past a failed EXPECT. (Here and elsewhere)

Done.

[groby-ooo-7-16, 2012-02-17 02:15:01]

lgtm mod the static member.

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
File chrome/browser/webdata/web_data_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
chrome/browser/webdata/web_data_service_unittest.cc:181: class
WebIntentsDefaultsConsumer : public WebDataServiceConsumer {
At the very least, let's hoist WaitUntilCalled into an anonymous namespace. It
exists at least three times and does nothing class-specific.

On 2012/02/17 01:12:37, Greg Billock wrote:
> On 2012/02/16 23:14:38, groby wrote:
> > That's a lot of copy/paste - can we just templatize and typedef?
> 
> This whole file has tons of this parallel structure. If they all used a common
> template that'd perhaps be an advantage, but I don't know that picking out
these
> two will help. In any case, tests tend to be read more directly, and too much
> fanciness just makes it harder to figure out what's going on.

[Greg Billock, 2012-02-17 17:57:00]

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
File chrome/browser/webdata/web_data_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/9370043/diff/8001/chrome/browser/webda...
chrome/browser/webdata/web_data_service_unittest.cc:181: class
WebIntentsDefaultsConsumer : public WebDataServiceConsumer {
I like it. Done.

On 2012/02/17 02:15:01, groby wrote:
> At the very least, let's hoist WaitUntilCalled into an anonymous namespace. It
> exists at least three times and does nothing class-specific.
> 
> On 2012/02/17 01:12:37, Greg Billock wrote:
> > On 2012/02/16 23:14:38, groby wrote:
> > > That's a lot of copy/paste - can we just templatize and typedef?
> > 
> > This whole file has tons of this parallel structure. If they all used a
common
> > template that'd perhaps be an advantage, but I don't know that picking out
> these
> > two will help. In any case, tests tend to be read more directly, and too
much
> > fanciness just makes it harder to figure out what's going on.
>

[commit-bot: I haz the power, 2012-02-17 19:13:49]

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

[commit-bot: I haz the power, 2012-02-17 21:26:29]

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

[commit-bot: I haz the power, 2012-02-17 21:45:15]

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

[commit-bot: I haz the power, 2012-02-17 23:13:47]

Change committed as 122619