[Web Intents] Inline installation of extensions in web intents picker.

[Web Intents] Inline installation of extensions in web intents picker.

BUG=113476
TEST=WebIntentsRegistryTest.GetIntentServicesWithExtensionId, WebIntentPickerControllerBrowserTest.ExtensionInstallSuccess

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=127716

[binji, 2012-03-15 00:48:54]

groby: web_intents_registry
jhawkins: rest

[James Hawkins, 2012-03-15 01:05:20]

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

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:33: void
AddMatchingServicesForExtension(const Extension& extension,
Document method and params.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:36: const IntentServiceList&
services(extension.intents_services());
Use '=' here?  It seems strange to use a copy constructor for const&

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:44: void
FilterServicesByMimetype(const string16& mimetype,
Document method and params.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:296: Consumer* consumer) {
DCHECK(consumer);

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:299: IntentsQuery* query =
Document ownership of |query|.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:319:
AddMatchingServicesForExtension(*extension, query->action_,
nit: Start of parameter lines must align on the same column.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:325: delete query;
Now I see the ownership.  Seems dangerous; why not Pass ownership to
DoGetIntent...()?

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

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:84: QueryID
GetIntentServicesWithExtensionId(const string16& action,
I'd do s/WithExtensionId/ForExtensionFilter/.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:134: // Performs the work of
|GetIntentServicesWithExtensionId|.
I'd do s/Performs the work/Implementation/.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
File chrome/browser/intents/web_intents_registry_unittest.cc (right):

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry_unittest.cc:115: iter !=
extensions_.end();
Save a row by putting ++iter up here.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:71: //
WebIntentsRegistryTrampoline -----------------------------------------------
Kill this line, preferably via fire.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:73: class
WebIntentsRegistryTrampoline : public WebIntentsRegistry::Consumer {
Document the class.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:76: typedef
base::Callback<void(const IntentServices&)> Callback;
Pick a less generic name than Callback.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:78: explicit
WebIntentsRegistryTrampoline(const Callback& callback);
|callback| must not be null?

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:90: Callback
callback_;
Document member variable.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:190: new
WebIntentsRegistryTrampoline(
Document ownership.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:270:
wrapper_->profile(),
Save rows by collapsing these parameters.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:506: const
std::vector<webkit_glue::WebIntentServiceData>& services) {
Document in the header that |services| must be a non-empty list.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.h:178: // Pointer to the
WebstoreInstaller, used to install extensions from the
You can take out the "Pointer to the WebStoreInstaller".

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_model.h (right):

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_model.h:157: string16 action_;
Document member variables.

[binji, 2012-03-15 17:53:14]

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

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:33: void
AddMatchingServicesForExtension(const Extension& extension,
On 2012/03/15 01:05:20, James Hawkins wrote:
> Document method and params.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:36: const IntentServiceList&
services(extension.intents_services());
On 2012/03/15 01:05:20, James Hawkins wrote:
> Use '=' here?  It seems strange to use a copy constructor for const&

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:44: void
FilterServicesByMimetype(const string16& mimetype,
On 2012/03/15 01:05:20, James Hawkins wrote:
> Document method and params.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:296: Consumer* consumer) {
On 2012/03/15 01:05:20, James Hawkins wrote:
> DCHECK(consumer);

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:299: IntentsQuery* query =
On 2012/03/15 01:05:20, James Hawkins wrote:
> Document ownership of |query|.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:319:
AddMatchingServicesForExtension(*extension, query->action_,
On 2012/03/15 01:05:20, James Hawkins wrote:
> nit: Start of parameter lines must align on the same column.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.cc:325: delete query;
On 2012/03/15 01:05:20, James Hawkins wrote:
> Now I see the ownership.  Seems dangerous; why not Pass ownership to
> DoGetIntent...()?

Done.

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

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:84: QueryID
GetIntentServicesWithExtensionId(const string16& action,
On 2012/03/15 01:05:20, James Hawkins wrote:
> I'd do s/WithExtensionId/ForExtensionFilter/.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:134: // Performs the work of
|GetIntentServicesWithExtensionId|.
On 2012/03/15 01:05:20, James Hawkins wrote:
> I'd do s/Performs the work/Implementation/.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
File chrome/browser/intents/web_intents_registry_unittest.cc (right):

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry_unittest.cc:115: iter !=
extensions_.end();
On 2012/03/15 01:05:20, James Hawkins wrote:
> Save a row by putting ++iter up here.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:71: //
WebIntentsRegistryTrampoline -----------------------------------------------
On 2012/03/15 01:05:20, James Hawkins wrote:
> Kill this line, preferably via fire.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:73: class
WebIntentsRegistryTrampoline : public WebIntentsRegistry::Consumer {
On 2012/03/15 01:05:20, James Hawkins wrote:
> Document the class.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:76: typedef
base::Callback<void(const IntentServices&)> Callback;
On 2012/03/15 01:05:20, James Hawkins wrote:
> Pick a less generic name than Callback.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:78: explicit
WebIntentsRegistryTrampoline(const Callback& callback);
On 2012/03/15 01:05:20, James Hawkins wrote:
> |callback| must not be null?

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:90: Callback
callback_;
On 2012/03/15 01:05:20, James Hawkins wrote:
> Document member variable.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:190: new
WebIntentsRegistryTrampoline(
On 2012/03/15 01:05:20, James Hawkins wrote:
> Document ownership.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:270:
wrapper_->profile(),
On 2012/03/15 01:05:20, James Hawkins wrote:
> Save rows by collapsing these parameters.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.cc:506: const
std::vector<webkit_glue::WebIntentServiceData>& services) {
On 2012/03/15 01:05:20, James Hawkins wrote:
> Document in the header that |services| must be a non-empty list.

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_controller.h:178: // Pointer to the
WebstoreInstaller, used to install extensions from the
On 2012/03/15 01:05:20, James Hawkins wrote:
> You can take out the "Pointer to the WebStoreInstaller".

Done.

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
File chrome/browser/ui/intents/web_intent_picker_model.h (right):

http://codereview.chromium.org/9595031/diff/9001/chrome/browser/ui/intents/we...
chrome/browser/ui/intents/web_intent_picker_model.h:157: string16 action_;
On 2012/03/15 01:05:20, James Hawkins wrote:
> Document member variables.

Done.

[groby-ooo-7-16, 2012-03-15 23:39:25]

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/intents/web_...
File chrome/browser/intents/web_intents_registry.h (right):

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry.h:86: const std::string&
extension_id,
Please simply point to a canonical location for mimetype specs. I.e.

  // |mimetype| must conform to definition as outlined for
  // WebIntentsRegistry::GetIntentServices.

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry.h:134: // Implementation of
|GetIntentServicesForExtensionFilter|.
Function names are not in ||

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model.cc (right):

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.cc:42: action_ = string16();
action_.clear() ?

[binji, 2012-03-16 22:17:21]

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/intents/web_...
File chrome/browser/intents/web_intents_registry.h (right):

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry.h:86: const std::string&
extension_id,
On 2012/03/15 23:39:25, groby wrote:
> Please simply point to a canonical location for mimetype specs. I.e.
> 
>   // |mimetype| must conform to definition as outlined for
>   // WebIntentsRegistry::GetIntentServices.

Done.

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry.h:134: // Implementation of
|GetIntentServicesForExtensionFilter|.
On 2012/03/15 23:39:25, groby wrote:
> Function names are not in ||

Done.

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model.cc (right):

http://codereview.chromium.org/9595031/diff/13003/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.cc:42: action_ = string16();
On 2012/03/15 23:39:25, groby wrote:
> action_.clear() ?

Done.

[James Hawkins, 2012-03-16 22:43:02]

LGTM, really clean.

http://codereview.chromium.org/9595031/diff/16001/chrome/browser/intents/web_...
File chrome/browser/intents/web_intents_registry.cc (right):

http://codereview.chromium.org/9595031/diff/16001/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry.cc:147: query->action_,
Optional nit: Parameters of called functions don't have to be one per-line, so
you could save a line here.

[groby-ooo-7-16, 2012-03-20 17:29:11]

lgtm