Initial version of web intents registry

Initial version of web intents registry

BUG=none
TEST=none


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

[James Hawkins, 2011-08-04 17:34:51]

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
File chrome/browser/intents/intents_registry.cc (right):

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.cc:12: for (QueryMap::iterator
it(queries.begin()); it != queries.end(); ++it) {
Also, `delete iter->second`?

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.cc:17: void
IntentsRegistry::Initialize(scoped_refptr<WebDataService> wds) {
Why do we need an Initialize method (as opposed to passing the wds into the
constructor)?

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
File chrome/browser/intents/intents_registry.h (right):

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:14: // Handles storing & retrieving of
web intents in the web database.
s/&/and/

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:15: // It provides filtering logic to
retrieve specific types of intents.
Use a more descriptive word than 'It'.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:16: class IntentsRegistry: public
WebDataServiceConsumer {
WebIntentsRegistry

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:22: class Consumer {
Specify this interface in a separate file (for iwyu, so clients dont have to
include all of web_intents_registry.h.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:36: // Initialize registry and bind to
a valid WebDataService.
Initializes, binds

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:45: // Request all intent providers
matching |action|.
s/Request/Returns/

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:50: // WebDataServiceConsumer
implementation:
s/:/./

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:55: struct IntentsQuery {
Perhaps we should forward declare the struct?

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:66: typedef
base::hash_map<WebDataService::Handle, IntentsQuery*> QueryMap;
typedefs go at the top of the private section.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:66: typedef
base::hash_map<WebDataService::Handle, IntentsQuery*> QueryMap;
Document this typedef.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:68: QueryMap queries;
Document all vars.

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

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/web_inte...
chrome/browser/intents/web_intent_data.h:14: GURL service_url; // URL for
service invocation.
Minimum two spaces before comment starts.

[groby-ooo-7-16, 2011-08-05 20:40:09]

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
File chrome/browser/intents/intents_registry.cc (right):

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.cc:12: for (QueryMap::iterator
it(queries.begin()); it != queries.end(); ++it) {
On 2011/08/04 17:34:51, James Hawkins wrote:
> Also, `delete iter->second`?

Done.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.cc:17: void
IntentsRegistry::Initialize(scoped_refptr<WebDataService> wds) {
On 2011/08/04 17:34:51, James Hawkins wrote:
> Why do we need an Initialize method (as opposed to passing the wds into the
> constructor)?

Skipped - need Initialize for dependency injection for tests. (Doing that in
ctor is a deep rabbit hole)

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
File chrome/browser/intents/intents_registry.h (right):

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:14: // Handles storing & retrieving of
web intents in the web database.
On 2011/08/04 17:34:51, James Hawkins wrote:
> s/&/and/

Done.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:15: // It provides filtering logic to
retrieve specific types of intents.
On 2011/08/04 17:34:51, James Hawkins wrote:
> Use a more descriptive word than 'It'.

Done.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:16: class IntentsRegistry: public
WebDataServiceConsumer {
On 2011/08/04 17:34:51, James Hawkins wrote:
> WebIntentsRegistry

Done.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:22: class Consumer {
On 2011/08/04 17:34:51, James Hawkins wrote:
> Specify this interface in a separate file (for iwyu, so clients dont have to
> include all of web_intents_registry.h.
Skipped, since it's closely tied to the registry. (QueryID type is shared)

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:36: // Initialize registry and bind to
a valid WebDataService.
On 2011/08/04 17:34:51, James Hawkins wrote:
> Initializes, binds

Done.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:45: // Request all intent providers
matching |action|.
On 2011/08/04 17:34:51, James Hawkins wrote:
> s/Request/Returns/

This is an async call (see use of consumer) - hence Request. There is no
immediate return value. Changed to "Requests".

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:50: // WebDataServiceConsumer
implementation:
On 2011/08/04 17:34:51, James Hawkins wrote:
> s/:/./

Done.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:55: struct IntentsQuery {
On 2011/08/04 17:34:51, James Hawkins wrote:
> Perhaps we should forward declare the struct?

Done.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:66: typedef
base::hash_map<WebDataService::Handle, IntentsQuery*> QueryMap;
On 2011/08/04 17:34:51, James Hawkins wrote:
> typedefs go at the top of the private section.

Done.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:66: typedef
base::hash_map<WebDataService::Handle, IntentsQuery*> QueryMap;
On 2011/08/04 17:34:51, James Hawkins wrote:
> typedefs go at the top of the private section.

Done.

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/intents_...
chrome/browser/intents/intents_registry.h:68: QueryMap queries;
On 2011/08/04 17:34:51, James Hawkins wrote:
> Document all vars.

Done.

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

http://codereview.chromium.org/7511005/diff/1/chrome/browser/intents/web_inte...
chrome/browser/intents/web_intent_data.h:14: GURL service_url; // URL for
service invocation.
On 2011/08/04 17:34:51, James Hawkins wrote:
> Minimum two spaces before comment starts.

Done.

[James Hawkins, 2011-08-05 20:47:11]

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

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:8: // Internal object
representing a single intents query.
Indentation

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:12: QueryID query_id_;  // unique
query identifier.
Indentation

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:35: const WDTypedResult* result)
{
line up params

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
File chrome/browser/intents/web_intents_registry.h (right):

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:16: class WebIntentsRegistry:
public WebDataServiceConsumer {
Space before colon.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:18: typedef int QueryID;  //
unique identifier for intent queries.
Capitalize comment, move to the line above the typedef.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:52: // Map web data requests to
intents queries.
Maps

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:54: typedef
base::hash_map<WebDataService::Handle, IntentsQuery*> QueryMap;
2 space indentation.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:60: QueryMap queries;  // Map for
all in-flight web data requests/intent queries.
In general, defining comments should be on the line above what it's defining
(similar to how we document functions). For more readability, I add blank lines
between member vars and the documentation of the next member var.

[groby-ooo-7-16, 2011-08-05 20:54:11]

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

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:8: // Internal object
representing a single intents query.
On 2011/08/05 20:47:12, James Hawkins wrote:
> Indentation

Done.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:12: QueryID query_id_;  // unique
query identifier.
On 2011/08/05 20:47:12, James Hawkins wrote:
> Indentation

Done.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:35: const WDTypedResult* result)
{
On 2011/08/05 20:47:12, James Hawkins wrote:
> line up params

Done.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
File chrome/browser/intents/web_intents_registry.h (right):

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:16: class WebIntentsRegistry:
public WebDataServiceConsumer {
On 2011/08/05 20:47:12, James Hawkins wrote:
> Space before colon.

Done.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:18: typedef int QueryID;  //
unique identifier for intent queries.
On 2011/08/05 20:47:12, James Hawkins wrote:
> Capitalize comment, move to the line above the typedef.

Done.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:52: // Map web data requests to
intents queries.
On 2011/08/05 20:47:12, James Hawkins wrote:
> Maps

Done.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:54: typedef
base::hash_map<WebDataService::Handle, IntentsQuery*> QueryMap;
On 2011/08/05 20:47:12, James Hawkins wrote:
> 2 space indentation.

Done.

http://codereview.chromium.org/7511005/diff/6001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:60: QueryMap queries;  // Map for
all in-flight web data requests/intent queries.
On 2011/08/05 20:47:12, James Hawkins wrote:
> In general, defining comments should be on the line above what it's defining
> (similar to how we document functions). For more readability, I add blank
lines
> between member vars and the documentation of the next member var.

Done.

[James Hawkins, 2011-08-05 20:55:28]

LGTM, thanks!

[commit-bot: I haz the power, 2011-08-06 01:27:09]

Change committed as 95710