Move/replace/rename URL-based extension getters from ExtensionService to/in ExtensionSet.

Move/replace/rename URL-based extension getters from ExtensionService to/in ExtensionSet.

ExtensionService::GetExtensionByURL -> GetByID with the host
ExtensionService::GetExtensionByWebExtent -> GetHostedAppByURL
GetByURL -> GetExtensionOrAppByURL

BUG=104091
TEST=existing tests


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

[Yoyo Zhou, 2011-12-07 23:30:52]

cc'ing abarth since he changed GetByURL recently

These uses of GetByURL are from the browser rather than the renderer, so a web
origin doesn't seem to make sense. Should the warning in ExtensionURLInfo be
less scary?

I left GetByWebExtent as taking a GURL argument (not ExtensionURLInfo) since
none of its current users (all in browser/) check origins, although I suppose it
could be misused in the future.

Along these lines, it'd be nice to replace the one (1) use of 
ExtensionService::ExtensionBindingsAllowed with ExtensionSet's version, but
there are again issues getting at an origin from chrome_web_ui_factory.cc.

[abarth-chromium, 2011-12-07 23:51:44]

Sorry this is trickier than it should be.  Generally speaking, you want to use
the SecurityOrigin whenever you can.  The fact that we use URLs at all for
retrieving extensions is a bug, but a hard one to fix.  The bugs introduced by
skipping the SecurityOrigin are unfortunately subtle.

I certainly understand that the callers of these functions won't all be perfect.
 We'll need to go through them again at some point in the future and take
another whack at moving to using the origin everywhere.

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.cc:613: extension =
service->extensions()->GetByWebExtent(params.source_url);
Ideally we'd have the SecurityOrigin here as well because we're talking about a
currently instantiated extension in a frame.  It might be sandboxed.

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/geolocation/c...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:557:
ext_service->extensions()->GetByURL(ExtensionURLInfo(requesting_frame));
This one probably needs a SecurityOrigin too.  Sandboxed iframes shouldn't get
geolocation privileges.

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:412:
ext_service->extensions()->GetByURL(ExtensionURLInfo(origin));
Same here.

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:155: return
service->extensions()->GetByURL(ExtensionURLInfo(site));
Same here.

[Yoyo Zhou, 2011-12-08 01:04:51]

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/geolocation/c...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:557:
ext_service->extensions()->GetByURL(ExtensionURLInfo(requesting_frame));
On 2011/12/07 23:51:44, abarth wrote:
> This one probably needs a SecurityOrigin too.  Sandboxed iframes shouldn't get
> geolocation privileges.

Looks like this one is changing a security origin to a string for IPC, which
becomes the requesting_frame here:

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/renderer/geol...

Is this okay? If so, is WebSecurityOrigin::createFromString safe to use?

There's a TODO in that code to make the IPCs use the actual origin. I could try
to do that, but the IPC serialization for WebSecurityOrigin hasn't been written
yet. So if this example works, it'll be simpler to convert to string (as here).

[abarth-chromium, 2011-12-08 01:09:13]

Using WebSecurityOrigin::toString and WebSecurityOrigin::createFromString is
fine.

[Yoyo Zhou, 2011-12-08 20:52:17]

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:412:
ext_service->extensions()->GetByURL(ExtensionURLInfo(origin));
On 2011/12/07 23:51:44, abarth wrote:
> Same here.

Turns out this one is actually also a WebSecurityOrigin converted into a string
converted into a GURL.

Any thoughts on whether GURL or string16 is better for this? I'd like to be
consistent.

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:155: return
service->extensions()->GetByURL(ExtensionURLInfo(site));
On 2011/12/07 23:51:44, abarth wrote:
> Same here.

I can't find any evidence that a SiteInstance might come from a sub-frame.

[abarth-chromium, 2011-12-08 21:01:04]

> Any thoughts on whether GURL or string16 is better for this? I'd like to be
> consistent.

string16.  It's a string representation of the SecurityOrigin, so its type
should be a string.

> I can't find any evidence that a SiteInstance might come from a sub-frame.

Okiedokes.

[Yoyo Zhou, 2011-12-09 00:18:55]

I spent a while trying to change the GURLs to string16s/WebSecurityOrigins for
desktop notification and geolocation origins; then I gave up and filed
crbug.com/106923.

Aaron: please review also.

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/8827013/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.cc:613: extension =
service->extensions()->GetByWebExtent(params.source_url);
On 2011/12/07 23:51:44, abarth wrote:
> Ideally we'd have the SecurityOrigin here as well because we're talking about
a
> currently instantiated extension in a frame.  It might be sandboxed.

Done.

[Aaron Boodman, 2011-12-09 16:05:19]

Cool, thanks for working through this.

I find myself having to think very hard at each place you changed about what
type of URL we have and whether it is OK to get back either an extension or app.

Maybe it would be good to make this more explicit. Like GetExtensionByURL could
take a flag indicating the type of extensions to search for (EXTENSION, APP,
HOSTED_APP, ANYTHING).

I'm not sure this is a good idea though. Let's talk about it at work today.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:393: return
extension->GetResourceURL(url.path());
This is kinda funky. We could just return extension->url().

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.cc:617:
WebSecurityOrigin::createFromString(params.origin),
Does this do the right thing with unique origins? Even if so, it might be better
to just teach IPC how to send WebSecurityOrigin directly so that nobody ever
needs to worry about whether serializing through strings works properly.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:310: const Extension* app =
extensions_.GetByURL(ExtensionURLInfo(url));
This reads a little funny. Can you change the variable name to 'extension' as
long as you're in here?

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:556:
ExtensionService* ext_service = profile_->GetExtensionService();
Styleguide says to avoid abbreviations.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:560:
WebSecurityOrigin::createFromString(
Nuts. This should be an easy one to cleanup though, in a different CL.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
chrome/browser/memory_details.cc:235: const Extension* extension =
extension_service->extensions()->
This doesn't seem like the right wrapping style to me. Shouldn't
'extension_service' go on a new line? I saw this in another place earlier and
forgot to comment.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
chrome/browser/memory_details.cc:236: GetByURL(ExtensionURLInfo(url));
This changes the behavior. Previously it would only find extensions and packaged
apps here.

http://codereview.chromium.org/8827013/diff/8002/chrome/renderer/extensions/s...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/renderer/extensions/s...
chrome/renderer/extensions/schema_generated_bindings.cc:528: params.origin =
origin.toString();
Can you call this source_origin for clarity?

[Yoyo Zhou, 2011-12-09 19:59:09]

Yes, when I changed each of the GetExtensionByURL sites, I had to specifically
check to see if they had already checked for chrome::kExtensionScheme already.
It seems this is one way, at least, to ensure that we only get an extension or
packaged app.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:393: return
extension->GetResourceURL(url.path());
On 2011/12/09 16:05:20, Aaron Boodman wrote:
> This is kinda funky. We could just return extension->url().

Doesn't that lose the path?

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.cc:617:
WebSecurityOrigin::createFromString(params.origin),
On 2011/12/09 16:05:20, Aaron Boodman wrote:
> Does this do the right thing with unique origins?

I assume so because Adam wrote earlier that "Using WebSecurityOrigin::toString
and WebSecurityOrigin::createFromString is fine."

> Even if so, it might be better
> to just teach IPC how to send WebSecurityOrigin directly so that nobody ever
> needs to worry about whether serializing through strings works properly.

Yeah, that would be best.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:310: const Extension* app =
extensions_.GetByURL(ExtensionURLInfo(url));
On 2011/12/09 16:05:20, Aaron Boodman wrote:
> This reads a little funny. Can you change the variable name to 'extension' as
> long as you're in here?

Uh, sure, I can see why this seems off.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:556:
ExtensionService* ext_service = profile_->GetExtensionService();
On 2011/12/09 16:05:20, Aaron Boodman wrote:
> Styleguide says to avoid abbreviations.

Ok. I'll fix that - here and some other places I found.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:560:
WebSecurityOrigin::createFromString(
On 2011/12/09 16:05:20, Aaron Boodman wrote:
> Nuts. This should be an easy one to cleanup though, in a different CL.

What do you mean - using a string16/WebSecurityOrigin instead of a GURL? Yes, it
seems wrong that the origin gets converted into a GURL, then converted back
here, but it's used as a GURL in many other places.

I started down that path and found the assumptions about GURL find their way
into all kinds of objects.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
chrome/browser/memory_details.cc:235: const Extension* extension =
extension_service->extensions()->
On 2011/12/09 16:05:20, Aaron Boodman wrote:
> This doesn't seem like the right wrapping style to me. Shouldn't
> 'extension_service' go on a new line? I saw this in another place earlier and
> forgot to comment.

Done.

http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
chrome/browser/memory_details.cc:236: GetByURL(ExtensionURLInfo(url));
On 2011/12/09 16:05:20, Aaron Boodman wrote:
> This changes the behavior. Previously it would only find extensions and
packaged
> apps here.

Arg. I did look at all of these carefully and my conclusion for this one was
inconclusive. I can't follow the logic in this function completely, but it looks
like this does cause a regression in about:memory.

I added a check to return to the old behavior and it seems fine now.

http://codereview.chromium.org/8827013/diff/8002/chrome/renderer/extensions/s...
File chrome/renderer/extensions/schema_generated_bindings.cc (right):

http://codereview.chromium.org/8827013/diff/8002/chrome/renderer/extensions/s...
chrome/renderer/extensions/schema_generated_bindings.cc:528: params.origin =
origin.toString();
On 2011/12/09 16:05:20, Aaron Boodman wrote:
> Can you call this source_origin for clarity?

Done.

[Aaron Boodman, 2011-12-09 20:13:12]

On Fri, Dec 9, 2011 at 11:59 AM,  <yoz@chromium.org> wrote:
> Yes, when I changed each of the GetExtensionByURL sites, I had to
> specifically
> check to see if they had already checked for chrome::kExtensionScheme
> already.
> It seems this is one way, at least, to ensure that we only get an extension
> or
> packaged app.

From meatspace conversations, my recommendation is:

ExtensionSet
  // Can return any type of item
  GetByID

  // Accepts chrome-extension, or http(s) URLs, can return any type of item
  GetExtensionOrAppByURL

  // Accepts http(s) URLs only, can only return hosted apps
  GetHostedAppByURL

People who have GURL and want an extension or packaged app only should
call GetByID(url.host()).

>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
> File chrome/browser/chrome_content_browser_client.cc (right):
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
> chrome/browser/chrome_content_browser_client.cc:393: return
> extension->GetResourceURL(url.path());
> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>
>> This is kinda funky. We could just return extension->url().
>
>
> Doesn't that lose the path?

Oh, I didn't realize what this was trying to do. Nevermind.

>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_function_dispatcher.cc (right):
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_function_dispatcher.cc:617:
> WebSecurityOrigin::createFromString(params.origin),
> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>
>> Does this do the right thing with unique origins?
>
>
> I assume so because Adam wrote earlier that "Using
>
> WebSecurityOrigin::toString and WebSecurityOrigin::createFromString is
> fine."
>
>> Even if so, it might be better
>> to just teach IPC how to send WebSecurityOrigin directly so that
>
> nobody ever
>>
>> needs to worry about whether serializing through strings works
>
> properly.
>
> Yeah, that would be best.
>
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_service.cc (right):
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_service.cc:310: const Extension* app
> = extensions_.GetByURL(ExtensionURLInfo(url));
> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>
>> This reads a little funny. Can you change the variable name to
>
> 'extension' as
>>
>> long as you're in here?
>
>
> Uh, sure, I can see why this seems off.
>
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
> File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
> (right):
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
> chrome/browser/geolocation/chrome_geolocation_permission_context.cc:556:
> ExtensionService* ext_service = profile_->GetExtensionService();
> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>
>> Styleguide says to avoid abbreviations.
>
>
> Ok. I'll fix that - here and some other places I found.
>
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
> chrome/browser/geolocation/chrome_geolocation_permission_context.cc:560:
> WebSecurityOrigin::createFromString(
> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>
>> Nuts. This should be an easy one to cleanup though, in a different CL.
>
>
> What do you mean - using a string16/WebSecurityOrigin instead of a GURL?
> Yes, it seems wrong that the origin gets converted into a GURL, then
> converted back here, but it's used as a GURL in many other places.
>
> I started down that path and found the assumptions about GURL find their
> way into all kinds of objects.

I meant by having the IPC send the origin in addition to the URL. I
didn't mean for you to do it in this change though.

>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
> File chrome/browser/memory_details.cc (right):
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
> chrome/browser/memory_details.cc:235: const Extension* extension =
> extension_service->extensions()->
> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>
>> This doesn't seem like the right wrapping style to me. Shouldn't
>> 'extension_service' go on a new line? I saw this in another place
>
> earlier and
>>
>> forgot to comment.
>
>
> Done.
>
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
> chrome/browser/memory_details.cc:236: GetByURL(ExtensionURLInfo(url));
> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>
>> This changes the behavior. Previously it would only find extensions
>
> and packaged
>>
>> apps here.
>
>
> Arg. I did look at all of these carefully and my conclusion for this one
> was inconclusive. I can't follow the logic in this function completely,
> but it looks like this does cause a regression in about:memory.
>
> I added a check to return to the old behavior and it seems fine now.
>
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/renderer/extensions/s...
> File chrome/renderer/extensions/schema_generated_bindings.cc (right):
>
>
http://codereview.chromium.org/8827013/diff/8002/chrome/renderer/extensions/s...
> chrome/renderer/extensions/schema_generated_bindings.cc:528:
> params.origin = origin.toString();
> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>
>> Can you call this source_origin for clarity?
>
>
> Done.
>
> http://codereview.chromium.org/8827013/

[Yoyo Zhou, 2011-12-09 20:21:16]

On Fri, Dec 9, 2011 at 12:12 PM, Aaron Boodman <aa@chromium.org> wrote:
> On Fri, Dec 9, 2011 at 11:59 AM,  <yoz@chromium.org> wrote:
>> Yes, when I changed each of the GetExtensionByURL sites, I had to
>> specifically
>> check to see if they had already checked for chrome::kExtensionScheme
>> already.
>> It seems this is one way, at least, to ensure that we only get an extension
>> or
>> packaged app.
>
> From meatspace conversations, my recommendation is:
>
> ExtensionSet
>  // Can return any type of item
>  GetByID
>
>  // Accepts chrome-extension, or http(s) URLs, can return any type of item
>  GetExtensionOrAppByURL

Also GetIDByURL -> GetExtensionOrAppIDByURL?

>  // Accepts http(s) URLs only, can only return hosted apps
>  GetHostedAppByURL
>
> People who have GURL and want an extension or packaged app only should
> call GetByID(url.host()).

SGTM

>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
>> File chrome/browser/chrome_content_browser_client.cc (right):
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
>> chrome/browser/chrome_content_browser_client.cc:393: return
>> extension->GetResourceURL(url.path());
>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>
>>> This is kinda funky. We could just return extension->url().
>>
>>
>> Doesn't that lose the path?
>
> Oh, I didn't realize what this was trying to do. Nevermind.
>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
>> File chrome/browser/extensions/extension_function_dispatcher.cc (right):
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
>> chrome/browser/extensions/extension_function_dispatcher.cc:617:
>> WebSecurityOrigin::createFromString(params.origin),
>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>
>>> Does this do the right thing with unique origins?
>>
>>
>> I assume so because Adam wrote earlier that "Using
>>
>> WebSecurityOrigin::toString and WebSecurityOrigin::createFromString is
>> fine."
>>
>>> Even if so, it might be better
>>> to just teach IPC how to send WebSecurityOrigin directly so that
>>
>> nobody ever
>>>
>>> needs to worry about whether serializing through strings works
>>
>> properly.
>>
>> Yeah, that would be best.
>>
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
>> File chrome/browser/extensions/extension_service.cc (right):
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
>> chrome/browser/extensions/extension_service.cc:310: const Extension* app
>> = extensions_.GetByURL(ExtensionURLInfo(url));
>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>
>>> This reads a little funny. Can you change the variable name to
>>
>> 'extension' as
>>>
>>> long as you're in here?
>>
>>
>> Uh, sure, I can see why this seems off.
>>
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
>> File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
>> (right):
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
>> chrome/browser/geolocation/chrome_geolocation_permission_context.cc:556:
>> ExtensionService* ext_service = profile_->GetExtensionService();
>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>
>>> Styleguide says to avoid abbreviations.
>>
>>
>> Ok. I'll fix that - here and some other places I found.
>>
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
>> chrome/browser/geolocation/chrome_geolocation_permission_context.cc:560:
>> WebSecurityOrigin::createFromString(
>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>
>>> Nuts. This should be an easy one to cleanup though, in a different CL.
>>
>>
>> What do you mean - using a string16/WebSecurityOrigin instead of a GURL?
>> Yes, it seems wrong that the origin gets converted into a GURL, then
>> converted back here, but it's used as a GURL in many other places.
>>
>> I started down that path and found the assumptions about GURL find their
>> way into all kinds of objects.
>
> I meant by having the IPC send the origin in addition to the URL. I
> didn't mean for you to do it in this change though.

 What the IPC sends is in fact the origin (but converted into GURL).

>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
>> File chrome/browser/memory_details.cc (right):
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
>> chrome/browser/memory_details.cc:235: const Extension* extension =
>> extension_service->extensions()->
>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>
>>> This doesn't seem like the right wrapping style to me. Shouldn't
>>> 'extension_service' go on a new line? I saw this in another place
>>
>> earlier and
>>>
>>> forgot to comment.
>>
>>
>> Done.
>>
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/memory_detail...
>> chrome/browser/memory_details.cc:236: GetByURL(ExtensionURLInfo(url));
>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>
>>> This changes the behavior. Previously it would only find extensions
>>
>> and packaged
>>>
>>> apps here.
>>
>>
>> Arg. I did look at all of these carefully and my conclusion for this one
>> was inconclusive. I can't follow the logic in this function completely,
>> but it looks like this does cause a regression in about:memory.
>>
>> I added a check to return to the old behavior and it seems fine now.
>>
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/renderer/extensions/s...
>> File chrome/renderer/extensions/schema_generated_bindings.cc (right):
>>
>>
http://codereview.chromium.org/8827013/diff/8002/chrome/renderer/extensions/s...
>> chrome/renderer/extensions/schema_generated_bindings.cc:528:
>> params.origin = origin.toString();
>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>
>>> Can you call this source_origin for clarity?
>>
>>
>> Done.
>>
>> http://codereview.chromium.org/8827013/

[Aaron Boodman, 2011-12-09 20:29:04]

On Fri, Dec 9, 2011 at 12:21 PM, Yoyo Zhou <yoz@chromium.org> wrote:
> On Fri, Dec 9, 2011 at 12:12 PM, Aaron Boodman <aa@chromium.org> wrote:
>> On Fri, Dec 9, 2011 at 11:59 AM,  <yoz@chromium.org> wrote:
>>> Yes, when I changed each of the GetExtensionByURL sites, I had to
>>> specifically
>>> check to see if they had already checked for chrome::kExtensionScheme
>>> already.
>>> It seems this is one way, at least, to ensure that we only get an extension
>>> or
>>> packaged app.
>>
>> From meatspace conversations, my recommendation is:
>>
>> ExtensionSet
>>  // Can return any type of item
>>  GetByID
>>
>>  // Accepts chrome-extension, or http(s) URLs, can return any type of item
>>  GetExtensionOrAppByURL
>
> Also GetIDByURL -> GetExtensionOrAppIDByURL?
>
>>  // Accepts http(s) URLs only, can only return hosted apps
>>  GetHostedAppByURL
>>
>> People who have GURL and want an extension or packaged app only should
>> call GetByID(url.host()).
>
> SGTM
>
>>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
>>> File chrome/browser/chrome_content_browser_client.cc (right):
>>>
>>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/chrome_conten...
>>> chrome/browser/chrome_content_browser_client.cc:393: return
>>> extension->GetResourceURL(url.path());
>>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>>
>>>> This is kinda funky. We could just return extension->url().
>>>
>>>
>>> Doesn't that lose the path?
>>
>> Oh, I didn't realize what this was trying to do. Nevermind.
>>
>>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
>>> File chrome/browser/extensions/extension_function_dispatcher.cc (right):
>>>
>>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
>>> chrome/browser/extensions/extension_function_dispatcher.cc:617:
>>> WebSecurityOrigin::createFromString(params.origin),
>>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>>
>>>> Does this do the right thing with unique origins?
>>>
>>>
>>> I assume so because Adam wrote earlier that "Using
>>>
>>> WebSecurityOrigin::toString and WebSecurityOrigin::createFromString is
>>> fine."
>>>
>>>> Even if so, it might be better
>>>> to just teach IPC how to send WebSecurityOrigin directly so that
>>>
>>> nobody ever
>>>>
>>>> needs to worry about whether serializing through strings works
>>>
>>> properly.
>>>
>>> Yeah, that would be best.
>>>
>>>
>>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
>>> File chrome/browser/extensions/extension_service.cc (right):
>>>
>>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/extensions/ex...
>>> chrome/browser/extensions/extension_service.cc:310: const Extension* app
>>> = extensions_.GetByURL(ExtensionURLInfo(url));
>>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>>
>>>> This reads a little funny. Can you change the variable name to
>>>
>>> 'extension' as
>>>>
>>>> long as you're in here?
>>>
>>>
>>> Uh, sure, I can see why this seems off.
>>>
>>>
>>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
>>> File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
>>> (right):
>>>
>>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
>>> chrome/browser/geolocation/chrome_geolocation_permission_context.cc:556:
>>> ExtensionService* ext_service = profile_->GetExtensionService();
>>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>>
>>>> Styleguide says to avoid abbreviations.
>>>
>>>
>>> Ok. I'll fix that - here and some other places I found.
>>>
>>>
>>>
http://codereview.chromium.org/8827013/diff/8002/chrome/browser/geolocation/c...
>>> chrome/browser/geolocation/chrome_geolocation_permission_context.cc:560:
>>> WebSecurityOrigin::createFromString(
>>> On 2011/12/09 16:05:20, Aaron Boodman wrote:
>>>>
>>>> Nuts. This should be an easy one to cleanup though, in a different CL.
>>>
>>>
>>> What do you mean - using a string16/WebSecurityOrigin instead of a GURL?
>>> Yes, it seems wrong that the origin gets converted into a GURL, then
>>> converted back here, but it's used as a GURL in many other places.
>>>
>>> I started down that path and found the assumptions about GURL find their
>>> way into all kinds of objects.
>>
>> I meant by having the IPC send the origin in addition to the URL. I
>> didn't mean for you to do it in this change though.
>
>  What the IPC sends is in fact the origin (but converted into GURL).

Oh, ok.

- a

[Yoyo Zhou, 2011-12-10 00:18:50]

I renamed the functions as described above and changed a bunch of the original
uses of GetExtensionByURL to use GetByID.

After discussing it with Adam, I also removed GetIDByURL.

http://codereview.chromium.org/8827013/diff/20025/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_navigation_observer.cc (left):

http://codereview.chromium.org/8827013/diff/20025/chrome/browser/extensions/e...
chrome/browser/extensions/extension_navigation_observer.cc:55:
extension_service->GetDisabledExtensionByWebExtent(nav_entry->url());
jstritar: note that I've changed this so it can now return packaged apps as well
as hosted apps. If that seems incorrect, let me know, but the comments and such
don't indicate that this should be hosted apps only.

[Yoyo Zhou, 2011-12-12 18:03:22]

It turns out the use case of GetIDByURL for chrome-extension: URLs where the
extension doesn't exist is actually important (at least for tests), so I had to
leave it alone after all.

[Aaron Boodman, 2011-12-12 18:08:55]

On Mon, Dec 12, 2011 at 10:03 AM,  <yoz@chromium.org> wrote:
> It turns out the use case of GetIDByURL for chrome-extension: URLs where the
> extension doesn't exist is actually important (at least for tests), so I had
> to
> leave it alone after all.

Seems kinda silly. Can we just modify/remove those tests? (It seems
like we can't do this for hosted apps anyway).

- a

[Yoyo Zhou, 2011-12-12 21:49:00]

On 2011/12/12 18:08:55, Aaron Boodman wrote:
> On Mon, Dec 12, 2011 at 10:03 AM,  <mailto:yoz@chromium.org> wrote:
> > It turns out the use case of GetIDByURL for chrome-extension: URLs where the
> > extension doesn't exist is actually important (at least for tests), so I had
> > to
> > leave it alone after all.
> 
> Seems kinda silly. Can we just modify/remove those tests? (It seems
> like we can't do this for hosted apps anyway).

It seems to be more effort than it's worth, given the sheer number of tests that
seem to be broken by this.

http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[Aaron Boodman, 2011-12-12 21:54:07]

K. LGTM

[commit-bot: I haz the power, 2011-12-12 22:02:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/8827013/28001

[commit-bot: I haz the power, 2011-12-12 23:21:14]

Try job failure for 8827013-28001 (retry) on win_rel for step "update".
It's a second try, previously, step "net_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-12-13 00:38:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/8827013/28001

[commit-bot: I haz the power, 2011-12-13 01:47:46]

Change committed as 114148