Remove use of GetDefaultStoragePartition from chromeos/extensions

Remove use of GetDefaultStoragePartition from chromeos/extensions

This should fix the most of issues for v2 app file manager in bug 158837, but not all of them (specifically, file browser handlers different from file manager won't work on drive).

This should not change the behaviour for non isolated apps.

BUG=158837
TEST=None

(mechanical change in chrome/browser/ui/views/select_file_dialog_extension.cc)
[email protected]


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

[awong, 2012-11-12 20:17:13]

This code makes me uneasy. The "Site" URL is meant to be the result of
SiteInstance::GetSite().  We're hand crafting a bunch here, and I don't
understand the use case well enough to analyze the code.

I read through the bug, but I'm still not sure I understand what this extension
does. Do you have some time to talk over IM or VC?  That might be the fastest
path to resolving this.

http://codereview.chromium.org/11367032/diff/5001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_system_service.cc (right):

http://codereview.chromium.org/11367032/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_system_service.cc:71: const char
kFileManagerURL[] =
Eek. That is scary. Is I think it works right, but is there no way to actually
get the from the SiteInstance::GetSite() that is associated with this particular
class?

http://codereview.chromium.org/11367032/diff/5001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_handler_api.cc (right):

http://codereview.chromium.org/11367032/diff/5001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:340:
BrowserContext::GetStoragePartitionForSite(profile_, source_url_)->
source_url_ isn't quite right here.  The GURLs going into
GetStoragePartitionForSite() aren't freeform.  Also, it's supposed to designate
the permissions of the action.  AFAICT, this FileHandlerSelectFileFunction isn't
really running in the context of the webpage, but rather in the context of the
extension.  Is that correct?

http://codereview.chromium.org/11367032/diff/5001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_handler_api_test.cc
(right):

http://codereview.chromium.org/11367032/diff/5001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_handler_api_test.cc:177: //
mount point).
This is a bit of an abuse of the API. I don't undersatnd this code well enough,
but the fact that the global objects has a side effect across all partitions
feels like a bug in partitioning. Is there a way to make this add it to the
correct partition for the test?

[tbarzic, 2012-12-11 23:21:55]

Hey guys, can you take another look at this?
I removed handcrafting of the site urls from the cl (I undid the changes for
drive file system, this will have to be fixed in another cl; and refactored the
tests a bit)

Thanks,
Toni

[awong, 2012-12-14 02:04:58]

Hey Toni,

I skimmed through this, but will need to give it a better review tomorrow.  The
one comment I had is since we last talked, I've added a method into
ExtensionService that lets you retrieve the "Site GURL" directly from the
extension_id. Not sure of this will simplify your code at all.

Two high-level questions

(1) for each point you call GetStoragePartition(), are you getting the site
instance for what would be considered the "top-level context?" 

(2) Are normal extensions allowed to call into this code?  Or is this somehow
restricted only to V1 packaged Apps and V2 Apps?

-Albert

https://codereview.chromium.org/11367032/diff/26001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/11367032/diff/26001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:191:
content::SiteInstance* GetHandlerSiteInstance(Profile* profile,
Another way to do this now is to just get the site for extension_id.

See the newly added ExtensionService::GetSiteForExtensionId().

[tbarzic, 2012-12-14 03:55:03]

To answer your questions:

(1) Can you clarify what you mean by "top-level context"? Top-level compared to
what?

(2) Yeah, normal extensions are too allowed to call this code

https://codereview.chromium.org/11367032/diff/26001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/11367032/diff/26001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:191:
content::SiteInstance* GetHandlerSiteInstance(Profile* profile,
On 2012/12/14 02:04:58, awong wrote:
> Another way to do this now is to just get the site for extension_id.
> 
> See the newly added ExtensionService::GetSiteForExtensionId().

Cool, this will be useful :)

[awong, 2012-12-14 04:28:56]

On Thu, Dec 13, 2012 at 7:55 PM, <[email protected]> wrote:

> To answer your questions:
>
> (1) Can you clarify what you mean by "top-level context"? Top-level
> compared to
> what?
>

Sorry...let me stop making up terms.  "Site" is supposed to be the (scheme,
registered domain name) tuple for the URL that is being displayed in the
omnibox (aka the top-level frame for your DOM tree).

Why this matters is as follows.  Assume I have "http://mysite.com".  Now
let's say I have an extension with id ABCD that has a content script
running on mysite.com and that content script makes a filesystem URL
request.  Will it call your API with the SiteInstance for http://mysite.com,
or chrome-extension://ABCD?

-Albert



>
> (2) Yeah, normal extensions are too allowed to call this code
>
>
>
> https://codereview.chromium.**org/11367032/diff/26001/**
>
chrome/browser/chromeos/**extensions/file_handler_util.**cc<https://codereview.chromium.org/11367032/diff/26001/chrome/browser/chromeos/extensions/file_handler_util.cc>
> File chrome/browser/chromeos/**extensions/file_handler_util.**cc (right):
>
> https://codereview.chromium.**org/11367032/diff/26001/**
>
chrome/browser/chromeos/**extensions/file_handler_util.**cc#newcode191<https://codereview.chromium.org/11367032/diff/26001/chrome/browser/chromeos/extensions/file_handler_util.cc#newcode191>
> chrome/browser/chromeos/**extensions/file_handler_util.**cc:191:
> content::SiteInstance* GetHandlerSiteInstance(**Profile* profile,
> On 2012/12/14 02:04:58, awong wrote:
>
>> Another way to do this now is to just get the site for extension_id.
>>
>
>  See the newly added ExtensionService::**GetSiteForExtensionId().
>>
>
> Cool, this will be useful :)
>
>
https://codereview.chromium.**org/11367032/<https://codereview.chromium.org/1...
>

[tonibarzic, 2012-12-14 05:20:34]

On 2012/12/14 04:28:56, awong wrote:
> On Thu, Dec 13, 2012 at 7:55 PM, <mailto:[email protected]> wrote:
> 
> > To answer your questions:
> >
> > (1) Can you clarify what you mean by "top-level context"? Top-level
> > compared to
> > what?
> >
> 
> Sorry...let me stop making up terms.  "Site" is supposed to be the (scheme,
> registered domain name) tuple for the URL that is being displayed in the
> omnibox (aka the top-level frame for your DOM tree).
> 
> Why this matters is as follows.  Assume I have "http://mysite.com".  Now
> let's say I have an extension with id ABCD that has a content script
> running on http://mysite.com and that content script makes a filesystem URL
> request.  Will it call your API with the SiteInstance for http://mysite.com,
> or chrome-extension://ABCD?
> 
> -Albert
> 

Oh, I see.

I forgot to mention in original response that the fileBrowserPrivate API is
restricted to component extensions, so we can be sure that it will be called
only by internal Files app.

for the fileBrowserHandler API (and it's method selectFile), I think it's safe
to assume that it'll be called with the extension's SiteInstance (and from
top-level context). I  don't think content scripts can use this API (or any
other chrome.* API other than chrome.extension:
http://developer.chrome.com/extensions/content_scripts.html )

> 
> 
> >
> > (2) Yeah, normal extensions are too allowed to call this code
> >
> >
> >
> > https://codereview.chromium.**org/11367032/diff/26001/**
> >
>
chrome/browser/chromeos/**extensions/file_handler_util.**cc<https://codereview.chromium.org/11367032/diff/26001/chrome/browser/chromeos/extensions/file_handler_util.cc>
> > File chrome/browser/chromeos/**extensions/file_handler_util.**cc (right):
> >
> > https://codereview.chromium.**org/11367032/diff/26001/**
> >
>
chrome/browser/chromeos/**extensions/file_handler_util.**cc#newcode191<https://codereview.chromium.org/11367032/diff/26001/chrome/browser/chromeos/extensions/file_handler_util.cc#newcode191>
> > chrome/browser/chromeos/**extensions/file_handler_util.**cc:191:
> > content::SiteInstance* GetHandlerSiteInstance(**Profile* profile,
> > On 2012/12/14 02:04:58, awong wrote:
> >
> >> Another way to do this now is to just get the site for extension_id.
> >>
> >
> >  See the newly added ExtensionService::**GetSiteForExtensionId().
> >>
> >
> > Cool, this will be useful :)
> >
> >
>
https://codereview.chromium.**org/11367032/%3Chttps://codereview.chromium.org...>
> >

[awong, 2012-12-14 22:56:48]

Couple of nits, but the GetStoragePartition() calls LGTM.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:419:
std::string(kFileBrowserDomain),
You shouldn't need the explicit construction of an std::string() I don't think.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:593:
std::string(kFileBrowserDomain),
or here.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.h:22: class SiteInstance;
Unneeded?

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager_util.cc (right):

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager_util.cc:316: FilePath*
virtual_path) {
Add a note saying why we know that this is the correct site.  You said something
about component extensions (I don't know what those are).  If you could explain
how that guarantees that extension_id is always the the right site for the
top-level frame of the page causing this action, then that'd be great.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager_util.cc:473:
GetSiteForExtensionId(kFileBrowserDomain);
Same here.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:1192: // Revoke external file
access to third party extensions.
Same comment about knowing why this is safe.

[tbarzic, 2012-12-15 04:07:30]

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:419:
std::string(kFileBrowserDomain),
On 2012/12/14 22:56:48, awong wrote:
> You shouldn't need the explicit construction of an std::string() I don't
think.

Done.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:593:
std::string(kFileBrowserDomain),
On 2012/12/14 22:56:48, awong wrote:
> or here.

Done.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.h:22: class SiteInstance;
On 2012/12/14 22:56:48, awong wrote:
> Unneeded?

Done.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager_util.cc (right):

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager_util.cc:316: FilePath*
virtual_path) {
On 2012/12/14 22:56:48, awong wrote:
> Add a note saying why we know that this is the correct site.  You said
something
> about component extensions (I don't know what those are).  If you could
explain
> how that guarantees that extension_id is always the the right site for the
> top-level frame of the page causing this action, then that'd be great.

I'll try to come up with a better formulation over the weekend.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager_util.cc:473:
GetSiteForExtensionId(kFileBrowserDomain);
On 2012/12/14 22:56:48, awong wrote:
> Same here.

Done.

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11367032/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:1192: // Revoke external file
access to third party extensions.
On 2012/12/14 22:56:48, awong wrote:
> Same comment about knowing why this is safe.

Done.

[tbarzic, 2012-12-15 04:08:40]

+ mpcomplete (I need an owner for browser/extensions/)

[Matt Perry, 2012-12-17 18:16:45]

lgtm

https://codereview.chromium.org/11367032/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11367032/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:1192: // Revoke external file
access for the extension from it's file system
nit: its

[tbarzic, 2013-01-07 20:49:01]

On 2012/12/17 18:16:45, Matt Perry wrote:
> lgtm
> 
>
https://codereview.chromium.org/11367032/diff/47001/chrome/browser/extensions...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/11367032/diff/47001/chrome/browser/extensions...
> chrome/browser/extensions/extension_service.cc:1192: // Revoke external file
> access for the extension from it's file system
> nit: its

Zel, I'm waiting for your review :)

[zel, 2013-01-10 17:04:32]

lgtm

[commit-bot: I haz the power, 2013-01-10 20:54:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11367032/58003

[commit-bot: I haz the power, 2013-01-10 20:54:15]

Presubmit check for 11367032-58003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/views

Presubmit checks took 2.4s to calculate.

[tbarzic, 2013-01-10 21:04:27]

+sky (added TBR)

[commit-bot: I haz the power, 2013-01-10 21:08:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11367032/58003

[commit-bot: I haz the power, 2013-01-10 23:54:56]

Retried try job too often on linux_chromeos for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-11 03:02:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11367032/63003

[commit-bot: I haz the power, 2013-01-11 03:02:53]

Failed to apply patch for
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/extensions/file_browser_handler_api.cc
  Hunk #1 succeeded at 341 (offset 4 lines).
  Hunk #2 FAILED at 366.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/extensions/file_browser_handler_api.cc.rej

Patch:       chrome/browser/chromeos/extensions/file_browser_handler_api.cc
Index: chrome/browser/chromeos/extensions/file_browser_handler_api.cc
diff --git a/chrome/browser/chromeos/extensions/file_browser_handler_api.cc
b/chrome/browser/chromeos/extensions/file_browser_handler_api.cc
index
940a04f26a6260ac5298ae2cff3c3b69be8e11a6..e95c2fbd7f8548392a7d3fe66477c90876177814
100644
--- a/chrome/browser/chromeos/extensions/file_browser_handler_api.cc
+++ b/chrome/browser/chromeos/extensions/file_browser_handler_api.cc
@@ -337,7 +337,8 @@ void FileHandlerSelectFileFunction::OnFilePathSelected(
 
   // We have to open file system in order to create a FileEntry object for the
   // selected file path.
-  BrowserContext::GetDefaultStoragePartition(profile_)->
+  content::SiteInstance* site_instance = render_view_host()->GetSiteInstance();
+  BrowserContext::GetStoragePartition(profile_, site_instance)->
       GetFileSystemContext()->OpenFileSystem(
           source_url_.GetOrigin(), fileapi::kFileSystemTypeExternal, false,
           base::Bind(
@@ -365,8 +366,9 @@ void FileHandlerSelectFileFunction::OnFileSystemOpened(
 }
 
 void FileHandlerSelectFileFunction::GrantPermissions() {
+  content::SiteInstance* site_instance = render_view_host()->GetSiteInstance();
   fileapi::ExternalFileSystemMountPointProvider* external_provider =
-      BrowserContext::GetDefaultStoragePartition(profile_)->
+      BrowserContext::GetStoragePartition(profile_, site_instance)->
       GetFileSystemContext()->external_provider();
   DCHECK(external_provider);

[commit-bot: I haz the power, 2013-01-11 03:48:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11367032/85002

[commit-bot: I haz the power, 2013-01-11 04:40:26]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-11 05:56:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11367032/85002

[commit-bot: I haz the power, 2013-01-11 09:21:25]

Change committed as 176290