New FileSystemURL cracking

New FileSystemURL cracking

follow up on https://codereview.chromium.org/11648027/

Instead of cracking FileSystemURL in its ctor (somewhat magically, using singleton IsolatedContext and SystemExternalMountPoints), require FileSystemURL to be cracked explicitly.

The FileSystemURL can be cracked using specific MountPoints implementation  or using specific FileSystemContext  which will select available MountPoints implementation that should be used to crack the url. 
(CrackURL/CreateCrackedFileSystemURL methods).

Modify FileSystemURL usages to use new cracking methods.

BUG=158837
TEST=content_unittests: IsolatedContextTest.* ExternalMountPointsTest.* FileSystemContextTest.*


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

[kinuko, 2013-01-16 04:05:18]

https://codereview.chromium.org/11787028/diff/22043/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.h (right):

https://codereview.chromium.org/11787028/diff/22043/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.h:71: explicit FileSystemURL(const GURL&
filesystem_url);
How hard it will be if we drop this constructor or private to a certain class
(like FileSystemContext)? I noticed at least one usage of non_cracked_url in
c/b/cros//file_handler_util.cc, but could there be a lot?  (Maybe I can just try
making this private and see how many compiler errors I'd get)

[tbarzic, 2013-01-16 04:43:23]

https://codereview.chromium.org/11787028/diff/22043/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.h (right):

https://codereview.chromium.org/11787028/diff/22043/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.h:71: explicit FileSystemURL(const GURL&
filesystem_url);
On 2013/01/16 04:05:18, kinuko wrote:
> How hard it will be if we drop this constructor or private to a certain class
> (like FileSystemContext)? I noticed at least one usage of non_cracked_url in
> c/b/cros//file_handler_util.cc, but could there be a lot?  (Maybe I can just
try
> making this private and see how many compiler errors I'd get)

I guess we could get away with making it private to FileSystemContext (and maybe
ExternalMountPoints for webkit/fileapi/syncable/syncable_file_system_util.cc),
I'll take a closer look at this tomorrow.
For uses in file_handler_util/file_browser_private, I plan to remove those
really soon (I started working on that in this patch, but later decided do do
that after this lands, to avoid bloating this cl even more). Outside that, afaik
FileSystemURL(GURL) is only used in tests, but I'll have to double-check.

[tbarzic, 2013-01-17 21:02:41]

On 2013/01/16 04:43:23, tbarzic wrote:
>
https://codereview.chromium.org/11787028/diff/22043/webkit/fileapi/file_syste...
> File webkit/fileapi/file_system_url.h (right):
> 
>
https://codereview.chromium.org/11787028/diff/22043/webkit/fileapi/file_syste...
> webkit/fileapi/file_system_url.h:71: explicit FileSystemURL(const GURL&
> filesystem_url);
> On 2013/01/16 04:05:18, kinuko wrote:
> > How hard it will be if we drop this constructor or private to a certain
class
> > (like FileSystemContext)? I noticed at least one usage of non_cracked_url in
> > c/b/cros//file_handler_util.cc, but could there be a lot?  (Maybe I can just
> try
> > making this private and see how many compiler errors I'd get)
> 
> I guess we could get away with making it private to FileSystemContext (and
maybe
> ExternalMountPoints for webkit/fileapi/syncable/syncable_file_system_util.cc),
> I'll take a closer look at this tomorrow.
> For uses in file_handler_util/file_browser_private, I plan to remove those
> really soon (I started working on that in this patch, but later decided do do
> that after this lands, to avoid bloating this cl even more). Outside that,
afaik
> FileSystemURL(GURL) is only used in tests, but I'll have to double-check.

Ok, I've made non empty FileSystemURL ctors private (and friended FileSystemURL,
ExternalMountPoints and IsolatedContext).
I added static methods for creating non-cracked fs urls for use in tests (where
there are cases non-cracked URL is actually needed, e.g. testing "CrackURL"
methods).

[tbarzic, 2013-01-17 22:57:18]

this also fixes http://crbug.com/160693 since it removed ChromeOS drive file
system to FileSystemContext specific mount points, so there is no conflict with
syncable file system "drive" in system mount points (which was breaking the
tests).

we could have a similar problem if one day syncable service named "archive",
"removable" or "oem" appears..

[kinuko, 2013-01-18 07:30:36]

Very cool, I think the change looks much nicer now!

On 2013/01/17 22:57:18, tbarzic wrote:
> this also fixes http://crbug.com/160693 since it removed ChromeOS drive file
> system to FileSystemContext specific mount points, so there is no conflict
with
> syncable file system "drive" in system mount points (which was breaking the
> tests).

Oh I see, probably we should consider another internal name for the api...
sync-drive or something like that.

> we could have a similar problem if one day syncable service named "archive",
> "removable" or "oem" appears..

[kinuko, 2013-01-18 07:33:32]

Made some comments but nits only. I'll take another look but I think this looks
very good to me.

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:82: // |external_provider_| will take an
ownership of reference or
will take a reference of ?

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:329: 
nit: extra empty line

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_context.h (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.h:212: 
nit: extra empty line

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.cc (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.cc:105: is_valid_ = CrackFileSystemURL(url,
&origin_, &type_, &path_);
This overloaded verb 'crack' is now getting seriously confusing, can we rename
this private static one to 'ParseFileSystemURL' maybe?

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.h (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.h:145: static FileSystemURL
CreateForCrackedURL(const FileSystemURL& original,
nit: indent

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/isolated_c...
File webkit/fileapi/isolated_context.cc (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.cc:265: 
nit: extra line

[tbarzic, 2013-01-18 21:33:14]

Note that I've removed is_cracked property from FileSystemURL, since it now
seems redundant.

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:82: // |external_provider_| will take an
ownership of reference or
On 2013/01/18 07:33:32, kinuko wrote:
> will take a reference of ?

Done.

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:329: 
On 2013/01/18 07:33:32, kinuko wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_context.h (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.h:212: 
On 2013/01/18 07:33:32, kinuko wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.cc (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.cc:105: is_valid_ = CrackFileSystemURL(url,
&origin_, &type_, &path_);
On 2013/01/18 07:33:32, kinuko wrote:
> This overloaded verb 'crack' is now getting seriously confusing, can we rename
> this private static one to 'ParseFileSystemURL' maybe?

Done.

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.h (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.h:145: static FileSystemURL
CreateForCrackedURL(const FileSystemURL& original,
On 2013/01/18 07:33:32, kinuko wrote:
> nit: indent

Done.

(jeez, Eclipse is killing me with its auto-formatting when I move blocks of code
:/)

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/isolated_c...
File webkit/fileapi/isolated_context.cc (right):

https://codereview.chromium.org/11787028/diff/50001/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.cc:265: 
On 2013/01/18 07:33:32, kinuko wrote:
> nit: extra line

Done.

[kinuko, 2013-01-21 06:57:33]

https://codereview.chromium.org/11787028/diff/55003/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_task_executor.cc (right):

https://codereview.chromium.org/11787028/diff/55003/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_task_executor.cc:51: url != file_urls.end();
++url) {
nit: indent

https://codereview.chromium.org/11787028/diff/55003/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/11787028/diff/55003/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:244: 
nit: extra empty line

https://codereview.chromium.org/11787028/diff/55003/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.h (right):

https://codereview.chromium.org/11787028/diff/55003/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.h:76: const FileSystemURL& url) const
OVERRIDE;
Now these two methods (CanHandleURL and CrackFileSystemURL) should we expose
these methods only to trusted parties (e.g. ExternalMountPointsTest and
FileSystemContext)?  Because we no longer have is_cracked flag in FileSystemURL
having these public methods look confusing.
(Ditto for Isolated ones)

[tbarzic, 2013-01-22 21:30:56]

https://chromiumcodereview.appspot.com/11787028/diff/55003/chrome/browser/chr...
File chrome/browser/chromeos/drive/drive_task_executor.cc (right):

https://chromiumcodereview.appspot.com/11787028/diff/55003/chrome/browser/chr...
chrome/browser/chromeos/drive/drive_task_executor.cc:51: url != file_urls.end();
++url) {
On 2013/01/21 06:57:33, kinuko wrote:
> nit: indent

Done.

https://chromiumcodereview.appspot.com/11787028/diff/55003/webkit/fileapi/ext...
File webkit/fileapi/external_mount_points.h (right):

https://chromiumcodereview.appspot.com/11787028/diff/55003/webkit/fileapi/ext...
webkit/fileapi/external_mount_points.h:76: const FileSystemURL& url) const
OVERRIDE;
On 2013/01/21 06:57:33, kinuko wrote:
> Now these two methods (CanHandleURL and CrackFileSystemURL) should we expose
> these methods only to trusted parties (e.g. ExternalMountPointsTest and
> FileSystemContext)?  Because we no longer have is_cracked flag in
FileSystemURL
> having these public methods look confusing.
> (Ditto for Isolated ones)

ok, I got rid of CrackFileSystemURL (and use CreateCrackedFileSystem instead).
Also removed FileSystemURL::CreateForCrackedURL and started using ctor irectly.

Changed CanHandleURL to HandlesFielSystemMountType (which seems closer to what
the method actually does)

[kinuko, 2013-01-23 03:43:53]

There might still be some oversight but I think this lgtm.

Thanks for working on this big refactoring!

https://codereview.chromium.org/11787028/diff/73055/webkit/fileapi/mount_poin...
File webkit/fileapi/mount_points.h (right):

https://codereview.chromium.org/11787028/diff/73055/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.h:55: virtual bool HandlesFileSystemMountType(const
FileSystemType& type) const = 0;
nit: it doesn't need to be const-ref

[tbarzic, 2013-01-23 18:02:54]

+benwells for chrome/browser/extensions/
+brettw for webkit/tools/test_shell/ (and content/content_tests.gypi)

[benwells, 2013-01-24 01:03:46]

c/b/e/ lgtm

[brettw, 2013-01-24 05:31:19]

test shell lgtm rubberstamp

[commit-bot: I haz the power, 2013-01-24 18:26:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/11787028/74013

[commit-bot: I haz the power, 2013-01-24 22:01:53]

Change committed as 178664