Issue 7917006: Extension loading extracts intents from Manifest data.

Issue 7917006: Extension loading extracts intents from Manifest data. (Closed)

Created:
9 years, 3 months ago by groby-ooo-7-16

Modified:
9 years, 3 months ago

Reviewers:
Aaron Boodman, commit-bot: I haz the power, James Hawkins, Greg Billock

CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, mihaip+watch_chromium.org, Paweł Hajdan Jr.

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

Extension loading extracts intents from Manifest data. BUG=none TEST=ExtensionManifestTest.WebIntents Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102409

Patch Set 3 : Fixed review issues #

Patch Set 4 : Fixed broken unit test #

Patch Set 5 : Fixed includes for moved headers #

Created: 9 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+227 lines, -1 line)			Patch
M	chrome/browser/extensions/extension_icon_manager_unittest.cc	View	1 2 3	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/common/extensions/extension.h	View	1 2 3 4	4 chunks	+7 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/extension.cc	View	1 2 3 4	3 chunks	+73 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/extension_constants.h	View	1	3 chunks	+13 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/extension_constants.cc	View	1 2 3 4	3 chunks	+19 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/extension_manifests_unittest.cc	View	1 2 3 4	1 chunk	+45 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/manifest_tests/intent_invalid_1.json	View		1 chunk	+5 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/manifest_tests/intent_invalid_2.json	View		1 chunk	+7 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/manifest_tests/intent_invalid_3.json	View	1	1 chunk	+9 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/manifest_tests/intent_invalid_4.json	View	1	1 chunk	+9 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/manifest_tests/intent_invalid_5.json	View	1	1 chunk	+9 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/manifest_tests/intent_invalid_6.json	View	1	1 chunk	+9 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/manifest_tests/intent_valid.json	View	1	1 chunk	+12 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/manifest_tests/intent_valid_minimal.json	View	1	1 chunk	+8 lines, -0 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

groby-ooo-7-16

jhawkins: Please review aa: OWNERS check, please? gbillock: FYI

9 years, 3 months ago (2011-09-16 02:46:04 UTC) #1

Greg Billock

http://codereview.chromium.org/7917006/diff/1/chrome/common/extensions/extension.cc File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/7917006/diff/1/chrome/common/extensions/extension.cc#newcode181 chrome/common/extensions/extension.cc:181: DCHECK(intents); Add the flag check. http://codereview.chromium.org/7917006/diff/1/chrome/test/data/extensions/manifest_tests/intent_valid.json File chrome/test/data/extensions/manifest_tests/intent_valid.json (right): ...

9 years, 3 months ago (2011-09-16 15:43:17 UTC) #2

groby-ooo-7-16

9 years, 3 months ago (2011-09-16 17:40:36 UTC) #3

Greg Billock

9 years, 3 months ago (2011-09-16 18:45:51 UTC) #4

http://codereview.chromium.org/7917006/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7917006/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:181: DCHECK(intents);
On 2011/09/16 17:40:37, groby wrote:
> On 2011/09/16 15:43:17, Greg Billock wrote:
> > Add the flag check.
> Which flag check?

if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableWebIntents))

http://codereview.chromium.org/7917006/diff/1/chrome/test/data/extensions/man...
File chrome/test/data/extensions/manifest_tests/intent_valid.json (right):

http://codereview.chromium.org/7917006/diff/1/chrome/test/data/extensions/man...
chrome/test/data/extensions/manifest_tests/intent_valid.json:5:
"http://webintents.org/share" : {
On 2011/09/16 17:40:37, groby wrote:
> On 2011/09/16 15:43:17, Greg Billock wrote:
> > So the big difference from this to the schema in
> > https://github.com/mozilla/openwebapps/blob/develop/docs/ACTIVITIES.md is
that
> > one's keyed by action. Is the idea that keying by service URL is better
> because
> > it may be that an app wants to install several urls for the same action? It
> > seems more likely to me that the same service URL would handle multiple
action
> > types.
> 
> I'm confused - this is modelled after the one in ACTIVTIES.md. Straight from
> that page:
> 
>   services: {
>     "http://webactivities.org/share": {
>        type: [ "application/url", "text/*", "image/*" ]
>        path: "/services/share.html"
>     }
>   }
> 
> Yes, some key words are different - but that's just a suggestion, and easily
> changed.

Perhaps I'm puzzled by 'action' appearing in the attribute list. In the Mozilla
proposal, the url key is the action, and then in the attributes they have the
service url (they call it 'path'). That they could both be urls is a bit
confusing, but the semantics of the action url is the intent action, and the
service url/path is what url the browser should load.

groby-ooo-7-16

9 years, 3 months ago (2011-09-17 01:14:42 UTC) #5

http://codereview.chromium.org/7917006/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7917006/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:181: DCHECK(intents);
On 2011/09/16 18:45:51, Greg Billock wrote:
> On 2011/09/16 17:40:37, groby wrote:
> > On 2011/09/16 15:43:17, Greg Billock wrote:
> > > Add the flag check.
> > Which flag check?
> 
> if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableWebIntents))

Done.

http://codereview.chromium.org/7917006/diff/1/chrome/test/data/extensions/man...
File chrome/test/data/extensions/manifest_tests/intent_valid.json (right):

http://codereview.chromium.org/7917006/diff/1/chrome/test/data/extensions/man...
chrome/test/data/extensions/manifest_tests/intent_valid.json:5:
"http://webintents.org/share" : {
On 2011/09/16 18:45:51, Greg Billock wrote:
> On 2011/09/16 17:40:37, groby wrote:
> > On 2011/09/16 15:43:17, Greg Billock wrote:
> > > So the big difference from this to the schema in
> > > https://github.com/mozilla/openwebapps/blob/develop/docs/ACTIVITIES.md is
> that
> > > one's keyed by action. Is the idea that keying by service URL is better
> > because
> > > it may be that an app wants to install several urls for the same action?
It
> > > seems more likely to me that the same service URL would handle multiple
> action
> > > types.
> > 
> > I'm confused - this is modelled after the one in ACTIVTIES.md. Straight from
> > that page:
> > 
> >   services: {
> >     "http://webactivities.org/share": {
> >        type: [ "application/url", "text/*", "image/*" ]
> >        path: "/services/share.html"
> >     }
> >   }
> > 
> > Yes, some key words are different - but that's just a suggestion, and easily
> > changed.
> 
> Perhaps I'm puzzled by 'action' appearing in the attribute list. In the
Mozilla
> proposal, the url key is the action, and then in the attributes they have the
> service url (they call it 'path'). That they could both be urls is a bit
> confusing, but the semantics of the action url is the intent action, and the
> service url/path is what url the browser should load.

No, you were right - I puzzled myself with this. Fixed up, key is now action,
and the keyword is "path" instead of "action"

James Hawkins

http://codereview.chromium.org/7917006/diff/6001/chrome/common/extensions/extension.cc File chrome/common/extensions/extension.cc (left): http://codereview.chromium.org/7917006/diff/6001/chrome/common/extensions/extension.cc#oldcode176 chrome/common/extensions/extension.cc:176: Keep this blank line. http://codereview.chromium.org/7917006/diff/6001/chrome/common/extensions/extension.cc File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/7917006/diff/6001/chrome/common/extensions/extension.cc#newcode1182 ...

9 years, 3 months ago (2011-09-19 18:51:40 UTC) #6

groby-ooo-7-16

9 years, 3 months ago (2011-09-19 21:33:59 UTC) #7

groby-ooo-7-16

Adjusted for headers that have been moved. (That move fixes the deps check in the ...

9 years, 3 months ago (2011-09-22 21:09:19 UTC) #9

commit-bot: I haz the power

CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/7917006/17001

9 years, 3 months ago (2011-09-22 22:45:27 UTC) #10

Change committed as 102409

Expand Messages | Collapse Messages