Move 'intents' parsing out of Extension

Move 'intents' parsing out of Extension

Patch from Joe Thomas <mhx348@motorola.com>;.

BUG=159265
TBR=ben@chromium.org for gyp

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

[Yoyo Zhou, 2013-01-04 01:04:24]

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc
(right):

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc:16:
namespace keys = extension_manifest_keys;
No need for this if it's only used once.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc:19:
namespace {
No need for this.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
File chrome/common/extensions/web_intents_handler.cc (right):

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.cc:167:
g_empty_intents_data.Get().intents_data;
This seems like one more layer of indirection than you actually need.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
File chrome/common/extensions/web_intents_handler.h (right):

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.h:19: typedef
std::vector<webkit_glue::WebIntentServiceData> WebIntentsServiceData;
This isn't a good name - you have hidden the pluralization in a subtle place.
WebIntentServiceDataVector or WebIntentServiceDataList would be much better.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.h:21: struct WebIntentsInfo :
public Extension::ManifestData {
Class needs a descriptive comment.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.h:26: static const
WebIntentsServiceData& intents_services(
unix_hacker_names are generally used only for trivial accessors.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.h:29: // List of intent services
that the extension provides, if any.
intent->intents for consistency

[Joe Thomas, 2013-01-04 03:33:35]

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc
(right):

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc:16:
namespace keys = extension_manifest_keys;
On 2013/01/04 01:04:25, Yoyo Zhou wrote:
> No need for this if it's only used once.

Done.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc:19:
namespace {
On 2013/01/04 01:04:25, Yoyo Zhou wrote:
> No need for this.

Done.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
File chrome/common/extensions/web_intents_handler.cc (right):

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.cc:167:
g_empty_intents_data.Get().intents_data;
On 2013/01/04 01:04:25, Yoyo Zhou wrote:
> This seems like one more layer of indirection than you actually need.

Done. Returning NULL instead and handling it in the caller functions.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
File chrome/common/extensions/web_intents_handler.h (right):

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.h:19: typedef
std::vector<webkit_glue::WebIntentServiceData> WebIntentsServiceData;
On 2013/01/04 01:04:25, Yoyo Zhou wrote:
> This isn't a good name - you have hidden the pluralization in a subtle place.
> WebIntentServiceDataVector or WebIntentServiceDataList would be much better.

Done. Changed to WebIntentServiceDataList

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.h:21: struct WebIntentsInfo :
public Extension::ManifestData {
On 2013/01/04 01:04:25, Yoyo Zhou wrote:
> Class needs a descriptive comment.

Done.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.h:26: static const
WebIntentsServiceData& intents_services(
On 2013/01/04 01:04:25, Yoyo Zhou wrote:
> unix_hacker_names are generally used only for trivial accessors.

Done. Changed to GetIntentsServices.

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.h:29: // List of intent services
that the extension provides, if any.
On 2013/01/04 01:04:25, Yoyo Zhou wrote:
> intent->intents for consistency

Done.

[Yoyo Zhou, 2013-01-05 00:27:42]

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
File chrome/common/extensions/web_intents_handler.cc (right):

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.cc:167:
g_empty_intents_data.Get().intents_data;
On 2013/01/04 03:33:35, Joe Thomas wrote:
> On 2013/01/04 01:04:25, Yoyo Zhou wrote:
> > This seems like one more layer of indirection than you actually need.
> 
> Done. Returning NULL instead and handling it in the caller functions.

Sorry, my comment was too cryptic. What I meant is you had wrapped this in a
struct. Why not just have a LazyInstance<WebIntentsServiceData>?

https://codereview.chromium.org/11741014/diff/2002/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc
(right):

https://codereview.chromium.org/11741014/diff/2002/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc:47:
ASSERT_TRUE(services != NULL);
This may or may not be relevant (depending on choice of  NULL/LazyInstance), but
the style for this is usually

ASSERT_TRUE(services);

[Joe Thomas, 2013-01-05 01:49:27]

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
File chrome/common/extensions/web_intents_handler.cc (right):

https://codereview.chromium.org/11741014/diff/2001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.cc:167:
g_empty_intents_data.Get().intents_data;
On 2013/01/05 00:27:42, Yoyo Zhou wrote:
> On 2013/01/04 03:33:35, Joe Thomas wrote:
> > On 2013/01/04 01:04:25, Yoyo Zhou wrote:
> > > This seems like one more layer of indirection than you actually need.
> > 
> > Done. Returning NULL instead and handling it in the caller functions.
> 
> Sorry, my comment was too cryptic. What I meant is you had wrapped this in a
> struct. Why not just have a LazyInstance<WebIntentsServiceData>?

No problems. Put back LazyInstance with WebIntentServiceDataList

https://codereview.chromium.org/11741014/diff/2002/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc
(right):

https://codereview.chromium.org/11741014/diff/2002/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc:47:
ASSERT_TRUE(services != NULL);
On 2013/01/05 00:27:43, Yoyo Zhou wrote:
> This may or may not be relevant (depending on choice of  NULL/LazyInstance),
but
> the style for this is usually
> 
> ASSERT_TRUE(services);

Not relevant. And I corrected ASSERT_TRUE(extension.get() != NULL) occurrences.

[Yoyo Zhou, 2013-01-05 01:51:19]

LGTM

https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
File
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc
(right):

https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc:44:
ASSERT_TRUE(extension.get());
You can also write this ASSERT_TRUE(extension);

https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/web_intents_handler.cc (right):

https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
chrome/common/extensions/web_intents_handler.cc:24: static
base::LazyInstance<WebIntentServiceDataList> g_empty_intents_data =
My preference is to put this next to where it's used.

[Joe Thomas, 2013-01-05 02:05:58]

+ ben for gyp changes
+ gbillock for chrome/browser/intents
+ mirandac for chrome/browser/profiles

Please review.  Thanks.

[Joe Thomas, 2013-01-05 03:02:03]

https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
File
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc
(right):

https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc:44:
ASSERT_TRUE(extension.get());
On 2013/01/05 01:51:19, Yoyo Zhou wrote:
> You can also write this ASSERT_TRUE(extension);

Done.

https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/web_intents_handler.cc (right):

https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
chrome/common/extensions/web_intents_handler.cc:24: static
base::LazyInstance<WebIntentServiceDataList> g_empty_intents_data =
On 2013/01/05 01:51:19, Yoyo Zhou wrote:
> My preference is to put this next to where it's used.

Done.

[Miranda Callahan, 2013-01-07 14:13:42]

profiles/* LGTM.

On 2013/01/05 03:02:03, Joe Thomas wrote:
>
https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
> File
>
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
>
chrome/common/extensions/manifest_tests/extension_manifests_web_intents_unittest.cc:44:
> ASSERT_TRUE(extension.get());
> On 2013/01/05 01:51:19, Yoyo Zhou wrote:
> > You can also write this ASSERT_TRUE(extension);
> 
> Done.
> 
>
https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
> File chrome/common/extensions/web_intents_handler.cc (right):
> 
>
https://codereview.chromium.org/11741014/diff/25001/chrome/common/extensions/...
> chrome/common/extensions/web_intents_handler.cc:24: static
> base::LazyInstance<WebIntentServiceDataList> g_empty_intents_data =
> On 2013/01/05 01:51:19, Yoyo Zhou wrote:
> > My preference is to put this next to where it's used.
> 
> Done.

[Joe Thomas, 2013-01-09 17:36:02]

@Greg Billock,

Ping. Could you please review chrome/browser/intents/ changes? Thanks

[Greg Billock, 2013-01-09 17:37:07]

On 2013/01/09 17:36:02, Joe Thomas wrote:
> @Greg Billock,
> 
> Ping. Could you please review chrome/browser/intents/ changes? Thanks

lgtm

[commit-bot: I haz the power, 2013-01-10 16:19:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11741014/35001

[commit-bot: I haz the power, 2013-01-10 16:19:48]

Presubmit check for 11741014-35001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.3s to calculate.

[commit-bot: I haz the power, 2013-01-10 16:21:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11741014/35001

[commit-bot: I haz the power, 2013-01-10 16:35:07]

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2013-01-10 16:48:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11741014/35001

[Joe Thomas, 2013-01-10 21:16:29]

Hello Yoyo,

Could you please review
chrome/browser/extensions/extension_special_storage_policy_unittest.cc in the
latest patch. I fixed a failing unittest. Sorry for the trouble.

Thanks

[Yoyo Zhou, 2013-01-10 21:56:36]

LGTM

[commit-bot: I haz the power, 2013-01-10 21:59:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11741014/42007

[commit-bot: I haz the power, 2013-01-11 04:58:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11741014/42007

[commit-bot: I haz the power, 2013-01-11 05:54:22]

Change committed as 176274