FileSystem mods: Changes to snapshot file creation to remove dependencies on blobs.

FileSystem mods: Changes to snapshot file creation to remove dependencies on blobs.

* Implementats of a new WebFileSystem::createSnapshotFileAndReadMetadata(...)
method that does not take a blobURL as input and that invokes a new
WebFileSystemCallbacks::didCreateSnapshotFile(...) in response instead of
overloading the didReadMetadata callback for that purpose.

* Leaves the existing handling in place since its still in use by WK.

This CL is the 2nd in a series of changes that straddle webkit vs chromium repositories.
1) WK: Declare new virtual createSnapshotFile/didCreateSnapshotFile public api methods.
2) CR: Implement the new create method such that the didCreate method is invoked in response.
3) WK: Use the new create method and implement the new didCreate method.
3) CR: Cleanup the obsolete/deprecated blocks of code.

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

[michaeln, 2013-01-30 23:32:23]

ptal

https://codereview.chromium.org/12084077/diff/19005/content/common/fileapi/we...
File content/common/fileapi/webfilesystem_callback_dispatcher.cc (right):

https://codereview.chromium.org/12084077/diff/19005/content/common/fileapi/we...
content/common/fileapi/webfilesystem_callback_dispatcher.cc:53:
//callbacks_->didCreateSnapshotFile(web_file_info);
I need to land the first in the series of patches first, and then uncomment this
line.

https://codereview.chromium.org/12084077/diff/19005/webkit/tools/test_shell/s...
File webkit/tools/test_shell/simple_file_system.cc (right):

https://codereview.chromium.org/12084077/diff/19005/webkit/tools/test_shell/s...
webkit/tools/test_shell/simple_file_system.cc:421:
//callbacks->didCreateSnapshotFile(web_file_info);
ditto

[michaeln, 2013-01-30 23:46:12]

I'm dusting off a mac to prepare the first CL in the series and will put links
to the webkit patch here when something is uploaded there.

[kinuko, 2013-01-31 06:37:03]

lgtm, assuming we'll have the WK patch landed before this goes.

Meta-comment: can we file a new issue so that others can also easily track
what's going on around blob implementation?

https://codereview.chromium.org/12084077/diff/6008/content/common/fileapi/web...
File content/common/fileapi/webfilesystem_impl.h (right):

https://codereview.chromium.org/12084077/diff/6008/content/common/fileapi/web...
content/common/fileapi/webfilesystem_impl.h:64:
WebKit::WebFileSystemCallbacks*);
is this going to have OVERRIDE once WK patch lands?

[michaeln, 2013-01-31 21:41:09]

https://codereview.chromium.org/12084077/diff/6008/content/common/fileapi/web...
File content/common/fileapi/webfilesystem_impl.h (right):

https://codereview.chromium.org/12084077/diff/6008/content/common/fileapi/web...
content/common/fileapi/webfilesystem_impl.h:64:
WebKit::WebFileSystemCallbacks*);
> is this going to have OVERRIDE once WK patch lands?

I see all of the other methods in this class do, so this one probably should
too.

In general though, I'm not sure that's the best practice when it comes to coding
to the webkit API since we have this repository split with no ability to commit
changes together. On the flip-side, we've taken to providing default
{NOTREACHED();} impls of what would/should be pure virtual methods defined in
the webkit api. Seems like the decision to declare OVERRIDE vs not on
derivations is similar in nature. I'm for anything that removes 'friction' from
work that spans the two repositories, but also am for having OVERRIDE there to
catch a base class change such that derivatives aren't actually overriding as
intended.

How about I add the OVERRIDE in the cleanup CL (the 4th in the series) after the
dust has settled. That way if anything is reverted on either side prior to that
cleanup... things will still build and run (less friction).

[michaeln, 2013-02-02 02:26:45]

Dusting off that mac involved updating the OS to mountain lion :) Here's the
first CL in the series... https://bugs.webkit.org/show_bug.cgi?id=108733

Here's the webkit meta bug... https://bugs.webkit.org/show_bug.cgi?id=108733.
I'll add one for crbug too.

[kinuko, 2013-02-04 06:16:52]

https://codereview.chromium.org/12084077/diff/6008/content/common/fileapi/web...
File content/common/fileapi/webfilesystem_impl.h (right):

https://codereview.chromium.org/12084077/diff/6008/content/common/fileapi/web...
content/common/fileapi/webfilesystem_impl.h:64:
WebKit::WebFileSystemCallbacks*);
On 2013/01/31 21:41:10, michaeln wrote:
> > is this going to have OVERRIDE once WK patch lands?
> 
> I see all of the other methods in this class do, so this one probably should
> too.
> 
> In general though, I'm not sure that's the best practice when it comes to
coding
> to the webkit API since we have this repository split with no ability to
commit
> changes together. On the flip-side, we've taken to providing default
> {NOTREACHED();} impls of what would/should be pure virtual methods defined in
> the webkit api. Seems like the decision to declare OVERRIDE vs not on
> derivations is similar in nature. I'm for anything that removes 'friction'
from
> work that spans the two repositories, but also am for having OVERRIDE there to
> catch a base class change such that derivatives aren't actually overriding as
> intended.
> 
> How about I add the OVERRIDE in the cleanup CL (the 4th in the series) after
the
> dust has settled. That way if anything is reverted on either side prior to
that
> cleanup... things will still build and run (less friction).

I'm fine with this approach unless no buildbots complain (I'm afraid some clang
bots may), and mildly agree that having OVERRIDE across different modules could
make code management difficult.

[michaeln, 2013-02-04 23:00:24]

@yzshen for pepper owners

[michaeln, 2013-02-05 01:13:02]

@brettw for pepper owners (since yshen is out sick)

[brettw, 2013-02-07 04:52:58]

lgtm

https://codereview.chromium.org/12084077/diff/52001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/12084077/diff/52001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.cc:66: const FilePath&
platform_path) OVERRIDE {
Use base:: for FilePath.

https://codereview.chromium.org/12084077/diff/52001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://codereview.chromium.org/12084077/diff/52001/content/renderer/pepper/p...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:279: const FilePath&
platform_path) OVERRIDE {
Ditto

https://codereview.chromium.org/12084077/diff/52001/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/12084077/diff/52001/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:70: const FilePath& path) {
Ditto

https://codereview.chromium.org/12084077/diff/52001/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/12084077/diff/52001/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:52: const FilePath& path);
Please use base::FilePath now.

[michaeln, 2013-02-13 00:47:09]

thnx for looking brett, base::FilePath changes are done

[michaeln, 2013-02-13 01:08:46]

On 2013/02/13 00:47:09, michaeln wrote:
> thnx for looking brett, base::FilePath changes are done

Bah... i missed (at least) one. Why does it still compile with it the wrong way?
Would be easier to find these if the compiler would point them out.

[commit-bot: I haz the power, 2013-02-13 01:17:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/12084077/60007

[commit-bot: I haz the power, 2013-02-13 01:17:52]

Presubmit check for 12084077-60007 failed and returned exit status 1.

INFO:root:Found 17 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common/fileapi/file_system_messages.h

Presubmit checks took 2.0s to calculate.

[michaeln, 2013-02-13 01:39:48]

That's odd, i'm listed as an OWNER of that directory? I'll stick with gcl commit
i guess. Was just using the cq to run tryjobs anyway.

[kinuko, 2013-02-13 03:24:24]

*_messages.h files seem to need further security review. (By the reviewers
listed in OWNERS with per-file *_messages.h prefix)

[brettw, 2013-02-13 04:41:35]

On Tue, Feb 12, 2013 at 5:08 PM,  <michaeln@chromium.org> wrote:
> On 2013/02/13 00:47:09, michaeln wrote:
>>
>> thnx for looking brett, base::FilePath changes are done
>
>
> Bah... i missed (at least) one. Why does it still compile with it the wrong
> way?
> Would be easier to find these if the compiler would point them out.

I'm still in the process of converting everybody over. There's a
"using" in there now while I work on it.

Brett

[michaeln, 2013-02-13 22:47:52]

@cevans, can you take a look at the _messages.h file per OWNERS

[Chris Evans, 2013-02-13 23:00:33]

[+aedla] -- I think @aedla recently audited this area, so maybe he would have
solid feedback for any changes in this sensitive area.

[michaeln, 2013-02-14 01:46:27]

https://codereview.chromium.org/12084077/diff/60007/content/common/fileapi/fi...
File content/common/fileapi/file_system_messages.h (right):

https://codereview.chromium.org/12084077/diff/60007/content/common/fileapi/fi...
content/common/fileapi/file_system_messages.h:38:
IPC_MESSAGE_CONTROL3(FileSystemMsg_DidCreateSnapshotFile,
@aedla, in some cases where the existing FileSystemMsg_DidReadMetadata is sent
in response to an ipc, we'll send this new more specific message instead... same
payload with a different msgtype to invoke more specific processing on the other
side.

from a security pov, i believe things are identical before and after the change

[aedla, 2013-02-14 14:53:03]

Changes to IPC LGTM.

New messages are pretty much like the old. A blob is not created anymore to hold
the snapshot.

[commit-bot: I haz the power, 2013-02-15 00:55:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/12084077/57011

[commit-bot: I haz the power, 2013-02-15 00:55:57]

Presubmit check for 12084077-57011 failed and returned exit status 1.

INFO:root:Found 18 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common/fileapi/file_system_messages.h

Presubmit checks took 2.1s to calculate.

[Chris Evans, 2013-02-15 01:01:23]

On 2013/02/15 00:55:57, I haz the power (commit-bot) wrote:
> Presubmit check for 12084077-57011 failed and returned exit status 1.
> 
> INFO:root:Found 18 file(s).
> 
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     content/common/fileapi/file_system_messages.h
> 
> Presubmit checks took 2.1s to calculate.

rubber stamp LGTM because someone else from security looked at it (thanks
@aedla!)

[commit-bot: I haz the power, 2013-02-15 01:47:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/12084077/57011

[commit-bot: I haz the power, 2013-02-15 10:00:58]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-02-15 21:06:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/12084077/57011

[commit-bot: I haz the power, 2013-02-15 21:07:59]

Change committed as 182822