Web Intents: Hook up the register intent InfoBar with the WebIntentsRegistry.

Web Intents: Hook up the register intent InfoBar with the WebIntentsRegistry.

BUG=none
TEST=RegisterIntentHandlerInfoBarDelegateTest.Accept

R=groby@chromium.org

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

[James Hawkins, 2011-08-09 01:00:44]

Prob good for review now.

[groby-ooo-7-16, 2011-08-09 01:44:50]

On 2011/08/09 01:00:44, James Hawkins wrote:
> Prob good for review now.

LGTM (with a few nits)

[James Hawkins, 2011-08-09 01:49:16]

On 2011/08/09 01:44:50, groby wrote:
> On 2011/08/09 01:00:44, James Hawkins wrote:
> > Prob good for review now.
> 
> LGTM (with a few nits)

hmm, where are the nits?

[groby-ooo-7-16, 2011-08-09 01:58:15]

And here are the nits. Sorry.

http://codereview.chromium.org/7601013/diff/3016/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc
(right):

http://codereview.chromium.org/7601013/diff/3016/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc:75:
}
Don't we usually mark end-of-namespace with "// namespace" ?

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

http://codereview.chromium.org/7601013/diff/3016/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:40: virtual void
RegisterIntentProvider(const WebIntentData& intent);
Would you mind making the rest of the API virtual, too?

http://codereview.chromium.org/7601013/diff/3016/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/7601013/diff/3016/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.h:338: //
Is the empty comment intentional?

[James Hawkins, 2011-08-09 02:26:52]

http://codereview.chromium.org/7601013/diff/3016/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc
(right):

http://codereview.chromium.org/7601013/diff/3016/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc:75:
}
On 2011/08/09 01:58:15, groby wrote:
> Don't we usually mark end-of-namespace with "// namespace" ?

Done.

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

http://codereview.chromium.org/7601013/diff/3016/chrome/browser/intents/web_i...
chrome/browser/intents/web_intents_registry.h:40: virtual void
RegisterIntentProvider(const WebIntentData& intent);
On 2011/08/09 01:58:15, groby wrote:
> Would you mind making the rest of the API virtual, too? 

Hmm, I'd like to wait until tests are added for the respective methods. This one
is only overridden yet because we had to mock it.

http://codereview.chromium.org/7601013/diff/3016/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/7601013/diff/3016/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.h:338: //
On 2011/08/09 01:58:15, groby wrote:
> Is the empty comment intentional?

Done.

[James Hawkins, 2011-08-09 17:47:48]

ping

[groby-ooo-7-16, 2011-08-09 18:17:33]

LGTM (w/ a few nits)

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc
(right):

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc:48:
web_intents_registry_ = NULL;
Nit: .release() instead? (For consistency's sake)

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/autof...
File chrome/browser/webdata/autofill_table.h (right):

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/autof...
chrome/browser/webdata/autofill_table.h:22: class Time;
Are these changes related to the check-in at all? Or completely separate
cleanup? (same for autofill_table.cc,token_service_table.h)

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/web_d...
File chrome/browser/webdata/web_database_table.h (right):

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/web_d...
chrome/browser/webdata/web_database_table.h:9: #include "base/logging.h"
Nit - do we need this include?

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/web_i...
File chrome/browser/webdata/web_intents_table.cc (right):

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/web_i...
chrome/browser/webdata/web_intents_table.cc:89: // |intent.service_url|. It's
unlikely the user will be given the ability to
I'd think the user will remove unique intents. (I.e. don't share video on flickr
any more, but please still do share pictures there) - the inputs to delete are
the unique key over table

[James Hawkins, 2011-08-09 18:39:03]

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc
(right):

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc:48:
web_intents_registry_ = NULL;
On 2011/08/09 18:17:33, groby wrote:
> Nit: .release() instead? (For consistency's sake)

release() doesn't call Release(), whereas setting it to NULL does.

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/autof...
File chrome/browser/webdata/autofill_table.h (right):

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/autof...
chrome/browser/webdata/autofill_table.h:22: class Time;
On 2011/08/09 18:17:33, groby wrote:
> Are these changes related to the check-in at all? Or completely separate
> cleanup? (same for autofill_table.cc,token_service_table.h)

Indirectly. They were required by an iwyu change I made.

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/web_d...
File chrome/browser/webdata/web_database_table.h (right):

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/web_d...
chrome/browser/webdata/web_database_table.h:9: #include "base/logging.h"
On 2011/08/09 18:17:33, groby wrote:
> Nit - do we need this include?

Yes, for DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/web_i...
File chrome/browser/webdata/web_intents_table.cc (right):

http://codereview.chromium.org/7601013/diff/2005/chrome/browser/webdata/web_i...
chrome/browser/webdata/web_intents_table.cc:89: // |intent.service_url|. It's
unlikely the user will be given the ability to
On 2011/08/09 18:17:33, groby wrote:
> I'd think the user will remove unique intents. (I.e. don't share video on
flickr
> any more, but please still do share pictures there) - the inputs to delete are
> the unique key over table

Perhaps.  I'd like to leave the TODO since it is an unsettled question at this
point.

[James Hawkins, 2011-08-09 21:46:46]

+erg for profiles/

[Elliot Glaysher, 2011-08-09 22:09:18]

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/intents/web_...
File chrome/browser/intents/web_intent_data.h (right):

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/intents/web_...
chrome/browser/intents/web_intent_data.h:19: WebIntentData();
add dtor or clang will complain.

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

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry.h:18: public
base::RefCountedThreadSafe<WebIntentsRegistry> {
Why make this a RefCountedThreadSafe?

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:352: // Returns the WebIntentsRegistry
associated with this profile. Owned by the
We are trying to halt this pattern. The profile is to own no objects in the
future. Could you please rewrite this as a ProfileKeyedServiceFactory?

[James Hawkins, 2011-08-10 00:58:38]

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/intents/web_...
File chrome/browser/intents/web_intent_data.h (right):

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/intents/web_...
chrome/browser/intents/web_intent_data.h:19: WebIntentData();
On 2011/08/09 22:09:18, Elliot Glaysher wrote:
> add dtor or clang will complain.

Done.

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

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry.h:18: public
base::RefCountedThreadSafe<WebIntentsRegistry> {
On 2011/08/09 22:09:18, Elliot Glaysher wrote:
> Why make this a RefCountedThreadSafe?

Removed.

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7601013/diff/12001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:352: // Returns the WebIntentsRegistry
associated with this profile. Owned by the
On 2011/08/09 22:09:18, Elliot Glaysher wrote:
> We are trying to halt this pattern. The profile is to own no objects in the
> future. Could you please rewrite this as a ProfileKeyedServiceFactory?

Done.

[Elliot Glaysher, 2011-08-10 17:47:54]

LGTM with nits:

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/intents/web_...
File chrome/browser/intents/web_intents_registry_factory.cc (right):

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry_factory.cc:17: :
ProfileKeyedServiceFactory(ProfileDependencyManager::GetInstance()) {
nit: Please add the following comment to remind me to come in here when
WebDataService gets PKSFized:

  // TODO(erg): For Shutdown() order, we need to:
  //     DependsOn(WebDataServiceFactory::GetInstance());

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/intents/web_...
File chrome/browser/intents/web_intents_registry_factory.h (right):

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry_factory.h:24: //
nit: remove floating '//'

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service_factory.h (right):

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/themes/theme...
chrome/browser/themes/theme_service_factory.h:8: #include "base/logging.h"
Do you mean base/basictypes.h? I don't see log methods used.

[Elliot Glaysher, 2011-08-10 17:49:37]

Crap, one more thing that I forgot: Please add your new factory to the list at
the top of profile_dependency_manager.cc in AssertFactoriesBuilt() in
alphabetical order. It doesn't matter now, but it will in the future.

[James Hawkins, 2011-08-11 20:49:07]

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/intents/web_...
File chrome/browser/intents/web_intents_registry_factory.cc (right):

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry_factory.cc:17: :
ProfileKeyedServiceFactory(ProfileDependencyManager::GetInstance()) {
On 2011/08/10 17:47:54, Elliot Glaysher wrote:
> nit: Please add the following comment to remind me to come in here when
> WebDataService gets PKSFized:
> 
>   // TODO(erg): For Shutdown() order, we need to:
>   //     DependsOn(WebDataServiceFactory::GetInstance());

Done.

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/intents/web_...
File chrome/browser/intents/web_intents_registry_factory.h (right):

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/intents/web_...
chrome/browser/intents/web_intents_registry_factory.h:24: //
On 2011/08/10 17:47:54, Elliot Glaysher wrote:
> nit: remove floating '//'

Added the correct comment.

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service_factory.h (right):

http://codereview.chromium.org/7601013/diff/12029/chrome/browser/themes/theme...
chrome/browser/themes/theme_service_factory.h:8: #include "base/logging.h"
On 2011/08/10 17:47:54, Elliot Glaysher wrote:
> Do you mean base/basictypes.h? I don't see log methods used.

Oh damn, yes. Thanks.