Make WebFileSystemCallbacks not self-destruct, deprecate AsyncFileSystem

Make WebFileSystemCallbacks not self-destruct, deprecate AsyncFileSystem

The last piece of cleanup.

Depends on chromium-side change (with ifdef):
https://codereview.chromium.org/23762002/

BUG=257349
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157378

[kinuko, 2013-08-30 08:08:13]

The last piece in this series. Can you review?

[abarth-chromium, 2013-08-30 16:18:25]

LGTM

My one concern is that I don't think your change to hasPendingActivity is
correct.

https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
File Source/modules/filesystem/DOMFileSystem.cpp (right):

https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
Source/modules/filesystem/DOMFileSystem.cpp:107: return false;
This doesn't seem right.  Don't we have pending activity while the callback is
in flight?

https://codereview.chromium.org/23704004/diff/6001/public/platform/WebFileSys...
File public/platform/WebFileSystemCallbacks.h (right):

https://codereview.chromium.org/23704004/diff/6001/public/platform/WebFileSys...
public/platform/WebFileSystemCallbacks.h:40: #if WEBKIT_IMPLEMENTATION
You don;t need to wrap these forward declarations in WEBKIT_IMPLEMENTATION if
you don't want it.

[kinuko, 2013-09-03 09:32:48]

https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
File Source/modules/filesystem/DOMFileSystem.cpp (right):

https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
Source/modules/filesystem/DOMFileSystem.cpp:107: return false;
On 2013/08/30 16:18:25, abarth wrote:
> This doesn't seem right.  Don't we have pending activity while the callback is
> in flight?

We do, but we also hold reference to DOMFileSystem in callbacks so that we're
sure it's alive while there're inflight callbacks so I suspect we can just
remove these overrides... so I did so in the new patch.

https://codereview.chromium.org/23704004/diff/6001/public/platform/WebFileSys...
File public/platform/WebFileSystemCallbacks.h (right):

https://codereview.chromium.org/23704004/diff/6001/public/platform/WebFileSys...
public/platform/WebFileSystemCallbacks.h:40: #if WEBKIT_IMPLEMENTATION
On 2013/08/30 16:18:25, abarth wrote:
> You don;t need to wrap these forward declarations in WEBKIT_IMPLEMENTATION if
> you don't want it.

Done.

[abarth-chromium, 2013-09-03 16:21:02]

On 2013/09/03 09:32:48, kinuko wrote:
>
https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
> File Source/modules/filesystem/DOMFileSystem.cpp (right):
> 
>
https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
> Source/modules/filesystem/DOMFileSystem.cpp:107: return false;
> On 2013/08/30 16:18:25, abarth wrote:
> > This doesn't seem right.  Don't we have pending activity while the callback
is
> > in flight?
> 
> We do, but we also hold reference to DOMFileSystem in callbacks so that we're
> sure it's alive while there're inflight callbacks so I suspect we can just
> remove these overrides... so I did so in the new patch.

That's not enough because the JavaScript wrapper can fall off.  The
hasPendingAcivity approach keeps the JavasScript wrapper as well as the CPP
object alive.

[kinuko, 2013-09-03 22:21:19]

On 2013/09/03 16:21:02, abarth wrote:
> On 2013/09/03 09:32:48, kinuko wrote:
> >
>
https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
> > File Source/modules/filesystem/DOMFileSystem.cpp (right):
> > 
> >
>
https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
> > Source/modules/filesystem/DOMFileSystem.cpp:107: return false;
> > On 2013/08/30 16:18:25, abarth wrote:
> > > This doesn't seem right.  Don't we have pending activity while the
callback
> is
> > > in flight?
> > 
> > We do, but we also hold reference to DOMFileSystem in callbacks so that
we're
> > sure it's alive while there're inflight callbacks so I suspect we can just
> > remove these overrides... so I did so in the new patch.
> 
> That's not enough because the JavaScript wrapper can fall off.  The
> hasPendingAcivity approach keeps the JavasScript wrapper as well as the CPP
> object alive.

I thought the same, but couldn't repro the case where JS wrapper goes away while
callback is around (with forced gc()'s, no leaks after that). I am probably
missing something in my tests, will re-add hasPendingActivity and confirm it
works.

[kinuko, 2013-09-05 11:16:56]

On 2013/09/03 22:21:19, kinuko wrote:
> On 2013/09/03 16:21:02, abarth wrote:
> > On 2013/09/03 09:32:48, kinuko wrote:
> > >
> >
>
https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
> > > File Source/modules/filesystem/DOMFileSystem.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23704004/diff/6001/Source/modules/filesystem/...
> > > Source/modules/filesystem/DOMFileSystem.cpp:107: return false;
> > > On 2013/08/30 16:18:25, abarth wrote:
> > > > This doesn't seem right.  Don't we have pending activity while the
> callback
> > is
> > > > in flight?
> > > 
> > > We do, but we also hold reference to DOMFileSystem in callbacks so that
> we're
> > > sure it's alive while there're inflight callbacks so I suspect we can just
> > > remove these overrides... so I did so in the new patch.
> > 
> > That's not enough because the JavaScript wrapper can fall off.  The
> > hasPendingAcivity approach keeps the JavasScript wrapper as well as the CPP
> > object alive.
> 
> I thought the same, but couldn't repro the case where JS wrapper goes away
while
> callback is around (with forced gc()'s, no leaks after that). I am probably
> missing something in my tests, will re-add hasPendingActivity and confirm it
> works.

Ok, updated. Now we keep the # of pending callbacks internally, and returns true
for hasPendingActivity() if there're any inflight operations which may return an
object which has .filesystem field to refer the DOMFileSystem object.  I've also
tested the code to see if it introduces no leaks (at least in regular flow).

[abarth-chromium, 2013-09-06 04:56:31]

In any case, LGTM.

https://codereview.chromium.org/23704004/diff/21001/Source/modules/filesystem...
File Source/modules/filesystem/DOMFileSystem.cpp (right):

https://codereview.chromium.org/23704004/diff/21001/Source/modules/filesystem...
Source/modules/filesystem/DOMFileSystem.cpp:110: --m_pendingCallbacks;
You don't need to create your own version of this int.  ActiveDOMObject has one
you can use.  You can just call setPendingActivity and unsetPendingActivity to
increment and decerement the counter.  I think the counter is wired up to the
default implementation of hasPendingActivity.

[kinuko, 2013-09-06 05:44:19]

On 2013/09/06 04:56:31, abarth wrote:
> In any case, LGTM.
> 
>
https://codereview.chromium.org/23704004/diff/21001/Source/modules/filesystem...
> File Source/modules/filesystem/DOMFileSystem.cpp (right):
> 
>
https://codereview.chromium.org/23704004/diff/21001/Source/modules/filesystem...
> Source/modules/filesystem/DOMFileSystem.cpp:110: --m_pendingCallbacks;
> You don't need to create your own version of this int.  ActiveDOMObject has
one
> you can use.  You can just call setPendingActivity and unsetPendingActivity to
> increment and decerement the counter.  I think the counter is wired up to the
> default implementation of hasPendingActivity.

I didn't feel great to add extra references there, but it'll simplify the code
(yes default impl of hasPendingActivity uses the counter)-- k, will replace
those lines, and will submit after chrome side lands.  Thanks!

[commit-bot: I haz the power, 2013-09-06 15:48:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/23704004/46001

[commit-bot: I haz the power, 2013-09-06 16:30:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/23704004/51001

[commit-bot: I haz the power, 2013-09-06 17:53:46]

Change committed as 157378