Add default query method to WebIntentsRegistry.

Add default query method to WebIntentsRegistry.

R=groby@chromium.org
BUG=115214
TEST=WebIntentsRegistryTest.TestGetDefaults


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

[groby-ooo-7-16, 2012-02-22 01:40:18]

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
File chrome/browser/intents/web_intents_registry.cc (right):

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.cc:50: GURL url_;
IWYU: #include "googleurl/src/gurl.h"

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.cc:148: if
(default_service.user_date <= 0 && iter->user_date >= 0)
We should probably check if the default service is actually in a loaded
extension, if it's in an extension.

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.cc:264: query->query_id_ =
next_query_id_++;
Since we now have three places that build a query, all very similar - should we
have a CreateQuery call?

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.cc:271:
queries_[query->pending_query_] = query;
And similarly, a RegisterQuery() for the previous two lines?

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
File chrome/browser/intents/web_intents_registry.h (right):

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.h:45: const DefaultWebIntentService&
default_service) = 0;
Should this be a callback instead?

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.h:119: virtual void
OnWebDataServiceDefaultsRequestDone(
This looks like its part of WebDataServiceConsumer implementation, which it
isn't. Move up above that?

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
File chrome/browser/intents/web_intents_registry_unittest.cc (right):

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry_unittest.cc:350:
default_service.suppression = 4;
What does user_date = 1 and suppression = 4 mean? (Can you make those constants,
or comment?)

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry_unittest.cc:368: consumer.default_ =
DefaultWebIntentService();
Can you comment the test cases? I.e. "Retrieve existing default", "No existing
default for action", "no existing default for type"? (That's my assumption what
those test, at least)

[Greg Billock, 2012-02-23 23:39:00]

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
File chrome/browser/intents/web_intents_registry.cc (right):

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.cc:50: GURL url_;
On 2012/02/22 01:40:18, groby wrote:
> IWYU: #include "googleurl/src/gurl.h"

Done.

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.cc:148: if
(default_service.user_date <= 0 && iter->user_date >= 0)
On 2012/02/22 01:40:18, groby wrote:
> We should probably check if the default service is actually in a loaded
> extension, if it's in an extension.

Done.

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.cc:264: query->query_id_ =
next_query_id_++;
On 2012/02/22 01:40:18, groby wrote:
> Since we now have three places that build a query, all very similar - should
we
> have a CreateQuery call?

Created some constructors. That condenses things a little, but perhaps at the
cost of transparency... Tell me what you think.

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.cc:271:
queries_[query->pending_query_] = query;
On 2012/02/22 01:40:18, groby wrote:
> And similarly, a RegisterQuery() for the previous two lines?

I don't think that'd buy that much. This looks clearer and about as compact.

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
File chrome/browser/intents/web_intents_registry.h (right):

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.h:45: const DefaultWebIntentService&
default_service) = 0;
On 2012/02/22 01:40:18, groby wrote:
> Should this be a callback instead? 

Yeah, I wanted to talk to you about this. Shall I kill out this Consumer class
and just make callbacks? I am very tempted, since they are just special-purpose,
but it means changing a bunch of code. Perhaps I should do that first in a
separate refactor...

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry.h:119: virtual void
OnWebDataServiceDefaultsRequestDone(
On 2012/02/22 01:40:18, groby wrote:
> This looks like its part of WebDataServiceConsumer implementation, which it
> isn't. Move up above that?

Oops. It shouldn't OVERRIDE. Marking it as a delegate I hoped to show the
relationship to the above method. I'll make it more explicit.

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
File chrome/browser/intents/web_intents_registry_unittest.cc (right):

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry_unittest.cc:350:
default_service.suppression = 4;
On 2012/02/22 01:40:18, groby wrote:
> What does user_date = 1 and suppression = 4 mean? (Can you make those
constants,
> or comment?)

Nothing. Just filler to make sure they're preserved.

https://chromiumcodereview.appspot.com/9430027/diff/1/chrome/browser/intents/...
chrome/browser/intents/web_intents_registry_unittest.cc:368: consumer.default_ =
DefaultWebIntentService();
On 2012/02/22 01:40:18, groby wrote:
> Can you comment the test cases? I.e. "Retrieve existing default", "No existing
> default for action", "no existing default for type"? (That's my assumption
what
> those test, at least) 

Done.

[groby-ooo-7-16, 2012-02-27 18:23:29]

lgtm w/ nit

http://codereview.chromium.org/9430027/diff/1/chrome/browser/intents/web_inte...
File chrome/browser/intents/web_intents_registry.h (right):

http://codereview.chromium.org/9430027/diff/1/chrome/browser/intents/web_inte...
chrome/browser/intents/web_intents_registry.h:45: const DefaultWebIntentService&
default_service) = 0;
Probably a separate refactor, so we can land this thing...

On 2012/02/23 23:39:00, Greg Billock wrote:
> On 2012/02/22 01:40:18, groby wrote:
> > Should this be a callback instead? 
> 
> Yeah, I wanted to talk to you about this. Shall I kill out this Consumer class
> and just make callbacks? I am very tempted, since they are just
special-purpose,
> but it means changing a bunch of code. Perhaps I should do that first in a
> separate refactor...

http://codereview.chromium.org/9430027/diff/5001/chrome/browser/intents/web_i...
File chrome/browser/intents/web_intents_registry.cc (right):

http://codereview.chromium.org/9430027/diff/5001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:137: bool
WebIntentsRegistry::IsContainingExtensionDisabled(const std::string& url) {
nit: I'd prefer Is..Enabled - negations in 'Is' functions make for wonderful
reading in if() statements if you need to negate the result :)

[Greg Billock, 2012-02-28 18:31:56]

http://codereview.chromium.org/9430027/diff/5001/chrome/browser/intents/web_i...
File chrome/browser/intents/web_intents_registry.cc (right):

http://codereview.chromium.org/9430027/diff/5001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:137: bool
WebIntentsRegistry::IsContainingExtensionDisabled(const std::string& url) {
On 2012/02/27 18:23:29, groby wrote:
> nit: I'd prefer Is..Enabled - negations in 'Is' functions make for wonderful
> reading in if() statements if you need to negate the result :)

Done.

[groby-ooo-7-16, 2012-02-28 18:35:07]

lgtm

[commit-bot: I haz the power, 2012-02-29 00:58:46]

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

[commit-bot: I haz the power, 2012-02-29 04:06:01]

Try job failure for 9430027-8002 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-02-29 04:06:07]

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

[commit-bot: I haz the power, 2012-02-29 07:54:36]

Change committed as 124150