Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
324483002:
Move LocalFileSystem to hang off LocalFrame instead of Page. (Closed)
|
Created:
6 years, 6 months ago by jam Modified:
6 years, 6 months ago CC:
blink-reviews, kinuko+fileapi, nhiroki, tzik, site-isolation-reviews_chromium.org, oilpan-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionMove LocalFileSystem to hang off LocalFrame instead of Page.
BUG=304341
TBR=darin
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175970
Patch Set 1 #Patch Set 2 #Patch Set 3 : #
Total comments: 5
Patch Set 4 : update per haraken #
Total comments: 2
Patch Set 5 : #
Total comments: 6
Patch Set 6 : update #Patch Set 7 : rebase #
Messages
Total messages: 33 (0 generated)
haraken: modules. in particular, since LocalFrame doesn't use oilpan yet I've removed that code. I tried to base it like how you fixed up my geolocation change in https://codereview.chromium.org/314873002/. jamesr: web
+site-isolation-reviews
+oilpan-reviews@chromium.org I'm not sure why it's giving this build error: ../../third_party/WebKit/Source/modules/filesystem/LocalFileSystem.h:63:5: error: [blink-gc] Base class 'Supplement<WebCore::LocalFrame>' of derived class 'LocalFileSystem' requires tracing. virtual void trace(Visitor*) OVERRIDE { } I do have an empty trace method, is it checking that it has a body?
On 2014/06/06 17:37:49, jam wrote:
> mailto:+oilpan-reviews@chromium.org
>
> I'm not sure why it's giving this build error:
>
> ../../third_party/WebKit/Source/modules/filesystem/LocalFileSystem.h:63:5:
> error: [blink-gc] Base class 'Supplement<WebCore::LocalFrame>' of derived
class
> 'LocalFileSystem' requires tracing.
> virtual void trace(Visitor*) OVERRIDE { }
>
>
> I do have an empty trace method, is it checking that it has a body?
It's trying to tell you that you're not invoking the Supplement<LocalFrame>'s
trace() method.
i.e.,
virtual void trace(Visitor* visitor) OVERRIDE {
Supplement<LocalFrame>::trace(visitor); }
On 2014/06/06 17:45:36, sof wrote:
> On 2014/06/06 17:37:49, jam wrote:
> > mailto:+oilpan-reviews@chromium.org
> >
> > I'm not sure why it's giving this build error:
> >
> > ../../third_party/WebKit/Source/modules/filesystem/LocalFileSystem.h:63:5:
> > error: [blink-gc] Base class 'Supplement<WebCore::LocalFrame>' of derived
> class
> > 'LocalFileSystem' requires tracing.
> > virtual void trace(Visitor*) OVERRIDE { }
> >
> >
> > I do have an empty trace method, is it checking that it has a body?
>
> It's trying to tell you that you're not invoking the Supplement<LocalFrame>'s
> trace() method.
>
> i.e.,
>
> virtual void trace(Visitor* visitor) OVERRIDE {
> Supplement<LocalFrame>::trace(visitor); }
oh, how come I need that here, while for geolocation it's not needed, i.e.
https://codereview.chromium.org/314873002/ ?
On 2014/06/06 17:51:28, jam wrote:
> On 2014/06/06 17:45:36, sof wrote:
> > On 2014/06/06 17:37:49, jam wrote:
> > > mailto:+oilpan-reviews@chromium.org
> > >
> > > I'm not sure why it's giving this build error:
> > >
> > > ../../third_party/WebKit/Source/modules/filesystem/LocalFileSystem.h:63:5:
> > > error: [blink-gc] Base class 'Supplement<WebCore::LocalFrame>' of derived
> > class
> > > 'LocalFileSystem' requires tracing.
> > > virtual void trace(Visitor*) OVERRIDE { }
> > >
> > >
> > > I do have an empty trace method, is it checking that it has a body?
> >
> > It's trying to tell you that you're not invoking the
Supplement<LocalFrame>'s
> > trace() method.
> >
> > i.e.,
> >
> > virtual void trace(Visitor* visitor) OVERRIDE {
> > Supplement<LocalFrame>::trace(visitor); }
>
> oh, how come I need that here, while for geolocation it's not needed, i.e.
> https://codereview.chromium.org/314873002/ ?
I think that works without a trace() (to the clang GC static checker) because
the Supplement<> has a trace(), but PageLifecycleObserver doesn't.
In your case, you have a trace() in either Supplement<>, so the static checker
insists that the derived class has an overridden trace() that delegates to both.
On 2014/06/06 17:59:22, sof wrote:
> On 2014/06/06 17:51:28, jam wrote:
> > On 2014/06/06 17:45:36, sof wrote:
> > > On 2014/06/06 17:37:49, jam wrote:
> > > > mailto:+oilpan-reviews@chromium.org
> > > >
> > > > I'm not sure why it's giving this build error:
> > > >
> > > >
../../third_party/WebKit/Source/modules/filesystem/LocalFileSystem.h:63:5:
> > > > error: [blink-gc] Base class 'Supplement<WebCore::LocalFrame>' of
derived
> > > class
> > > > 'LocalFileSystem' requires tracing.
> > > > virtual void trace(Visitor*) OVERRIDE { }
> > > >
> > > >
> > > > I do have an empty trace method, is it checking that it has a body?
> > >
> > > It's trying to tell you that you're not invoking the
> Supplement<LocalFrame>'s
> > > trace() method.
> > >
> > > i.e.,
> > >
> > > virtual void trace(Visitor* visitor) OVERRIDE {
> > > Supplement<LocalFrame>::trace(visitor); }
> >
> > oh, how come I need that here, while for geolocation it's not needed, i.e.
> > https://codereview.chromium.org/314873002/ ?
>
> I think that works without a trace() (to the clang GC static checker) because
> the Supplement<> has a trace(), but PageLifecycleObserver doesn't.
>
> In your case, you have a trace() in either Supplement<>, so the static checker
> insists that the derived class has an overridden trace() that delegates to
both.
I see, thanks for the explanation. done.
https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right): https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... Source/modules/filesystem/InspectorFileSystemAgent.cpp:44: #include "core/page/Page.h" Why is this needed now? (just trying to understand.) https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... File Source/modules/filesystem/LocalFileSystem.h (left): https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.h:66: WillBeHeapSupplement<WorkerClients>::trace(visitor); s/WillBeHeapSupplement/Supplement/ https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... File Source/modules/filesystem/LocalFileSystem.h (right): https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.h:65: WillBeHeapSupplement<LocalFrame>::trace(visitor); s/WillBeHeapSupplement/Supplement/
https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... File Source/modules/filesystem/LocalFileSystem.h (right): https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.h:48: class LocalFileSystem FINAL : public Supplement<LocalFrame>, public Supplement<WorkerClients> { Ian (or oilpan experts): WorkerClients is already on-heap. Is it safe for an off-heap object (i.e., LocalFileSystem) to derive WorkerClients as Supplement<WorkerClients>, not HeapSupplement<WorkerClients> ? In my understanding, if WorkerClients is on-heap, all supplementing objects need to be on-heap. If my understanding is correct, it's tricky to make this CL workable, because LocalFileSystem supplements both LocalFrame (off-heap) and WorkerClients (on-heap). Probably do we need to move LocalFrame to the heap so that we can make LocalFileSystem on-heap?
On 2014/06/06 20:32:41, sof wrote: > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right): > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > Source/modules/filesystem/InspectorFileSystemAgent.cpp:44: #include > "core/page/Page.h" > Why is this needed now? (just trying to understand.) > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > File Source/modules/filesystem/LocalFileSystem.h (left): > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > Source/modules/filesystem/LocalFileSystem.h:66: > WillBeHeapSupplement<WorkerClients>::trace(visitor); > s/WillBeHeapSupplement/Supplement/ > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > File Source/modules/filesystem/LocalFileSystem.h (right): > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > Source/modules/filesystem/LocalFileSystem.h:65: > WillBeHeapSupplement<LocalFrame>::trace(visitor); > s/WillBeHeapSupplement/Supplement/ haraken had indicated otherwise in a thread on oilpan-team@google.com, so I've tried that for now. I really don't know what's the answer here.
https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right): https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... Source/modules/filesystem/InspectorFileSystemAgent.cpp:44: #include "core/page/Page.h" On 2014/06/06 20:32:41, sof wrote: > Why is this needed now? (just trying to understand.) because I removed it from LocalFileSystem.h
On 2014/06/09 18:33:13, jam wrote: > On 2014/06/06 20:32:41, sof wrote: > > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > > File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right): > > > > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > > Source/modules/filesystem/InspectorFileSystemAgent.cpp:44: #include > > "core/page/Page.h" > > Why is this needed now? (just trying to understand.) > > > > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > > File Source/modules/filesystem/LocalFileSystem.h (left): > > > > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > > Source/modules/filesystem/LocalFileSystem.h:66: > > WillBeHeapSupplement<WorkerClients>::trace(visitor); > > s/WillBeHeapSupplement/Supplement/ > > > > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > > File Source/modules/filesystem/LocalFileSystem.h (right): > > > > > https://codereview.chromium.org/324483002/diff/40001/Source/modules/filesyste... > > Source/modules/filesystem/LocalFileSystem.h:65: > > WillBeHeapSupplement<LocalFrame>::trace(visitor); > > s/WillBeHeapSupplement/Supplement/ > > haraken had indicated otherwise in a thread on mailto:oilpan-team@google.com, so I've > tried that for now. I really don't know what's the answer here. You still need to trace off-heap supplements, but the mixture of heap and non-heap supplements here is a problem. (My bad for thinking that WorkerClients was off-heap, it isn't.)
https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... File Source/modules/filesystem/LocalFileSystem.cpp (right): https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.cpp:179: return static_cast<LocalFileSystem*>(Supplement<WorkerClients>::from(toWorkerGlobalScope(context).clients(), supplementName())); You need to revert this back to what it was before,i.e., make it again WillBeHeapSupplement<WorkerClients>.
https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... File Source/modules/filesystem/LocalFileSystem.cpp (right): https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.cpp:179: return static_cast<LocalFileSystem*>(Supplement<WorkerClients>::from(toWorkerGlobalScope(context).clients(), supplementName())); On 2014/06/09 21:12:27, sof wrote: > You need to revert this back to what it was before,i.e., make it again > WillBeHeapSupplement<WorkerClients>. Done.
On 2014/06/09 22:09:54, jam wrote: > https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... > File Source/modules/filesystem/LocalFileSystem.cpp (right): > > https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... > Source/modules/filesystem/LocalFileSystem.cpp:179: return > static_cast<LocalFileSystem*>(Supplement<WorkerClients>::from(toWorkerGlobalScope(context).clients(), > supplementName())); > On 2014/06/09 21:12:27, sof wrote: > > You need to revert this back to what it was before,i.e., make it again > > WillBeHeapSupplement<WorkerClients>. > > Done. Well, the bigger problem remains of how to support having such a (heap) supplement for WorkerClients. Either we need to move WorkerClients back to being off the heap, support persistent heap supplementables somehow (for LocalFrame), or move LocalFrame to the heap.
On 2014/06/10 07:00:55, sof wrote: > On 2014/06/09 22:09:54, jam wrote: > > > https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... > > File Source/modules/filesystem/LocalFileSystem.cpp (right): > > > > > https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... > > Source/modules/filesystem/LocalFileSystem.cpp:179: return > > > static_cast<LocalFileSystem*>(Supplement<WorkerClients>::from(toWorkerGlobalScope(context).clients(), > > supplementName())); > > On 2014/06/09 21:12:27, sof wrote: > > > You need to revert this back to what it was before,i.e., make it again > > > WillBeHeapSupplement<WorkerClients>. > > > > Done. > > Well, the bigger problem remains of how to support having such a (heap) > supplement for WorkerClients. > > Either we need to move WorkerClients back to being off the heap, support > persistent heap supplementables somehow (for LocalFrame), or move LocalFrame to > the heap. Ian started to look into this issue. We're planning to make LocalFrame's supplements traceable with keeping the LocalFrame off-heap or if it's hard we need to move the LocalFrame to the heap.
On 2014/06/10 07:34:18, haraken wrote: > On 2014/06/10 07:00:55, sof wrote: > > On 2014/06/09 22:09:54, jam wrote: > > > > > > https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... > > > File Source/modules/filesystem/LocalFileSystem.cpp (right): > > > > > > > > > https://codereview.chromium.org/324483002/diff/60001/Source/modules/filesyste... > > > Source/modules/filesystem/LocalFileSystem.cpp:179: return > > > > > > static_cast<LocalFileSystem*>(Supplement<WorkerClients>::from(toWorkerGlobalScope(context).clients(), > > > supplementName())); > > > On 2014/06/09 21:12:27, sof wrote: > > > > You need to revert this back to what it was before,i.e., make it again > > > > WillBeHeapSupplement<WorkerClients>. > > > > > > Done. > > > > Well, the bigger problem remains of how to support having such a (heap) > > supplement for WorkerClients. > > > > Either we need to move WorkerClients back to being off the heap, support > > persistent heap supplementables somehow (for LocalFrame), or move LocalFrame > to > > the heap. > > Ian started to look into this issue. We're planning to make LocalFrame's > supplements traceable with keeping the LocalFrame off-heap or if it's hard we > need to move the LocalFrame to the heap. Good, thanks for the update.
CL https://codereview.chromium.org/323873007/ makes LocalFrame HeapSupplementable again using a persistent/GC-root. This means that all of the supplements are on-heap GC managed objects. My inlined comments should be the necessary changes to make this CL compatible with the change to LocalFrame. https://codereview.chromium.org/324483002/diff/80001/Source/modules/filesyste... File Source/modules/filesystem/LocalFileSystem.cpp (left): https://codereview.chromium.org/324483002/diff/80001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.cpp:77: PassOwnPtrWillBeRawPtr<LocalFileSystem> LocalFileSystem::create(PassOwnPtr<FileSystemClient> client) revert https://codereview.chromium.org/324483002/diff/80001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.cpp:79: return adoptPtrWillBeNoop(new LocalFileSystem(client)); revert https://codereview.chromium.org/324483002/diff/80001/Source/modules/filesyste... File Source/modules/filesystem/LocalFileSystem.cpp (right): https://codereview.chromium.org/324483002/diff/80001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.cpp:176: return static_cast<LocalFileSystem*>(Supplement<LocalFrame>::from(toDocument(context).frame(), supplementName())); WillBeHeapSupplement<LocalFrame> https://codereview.chromium.org/324483002/diff/80001/Source/modules/filesyste... File Source/modules/filesystem/LocalFileSystem.h (left): https://codereview.chromium.org/324483002/diff/80001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.h:47: class LocalFileSystem FINAL : public NoBaseWillBeGarbageCollectedFinalized<LocalFileSystem>, public WillBeHeapSupplement<Page>, public WillBeHeapSupplement<WorkerClients> { revert https://codereview.chromium.org/324483002/diff/80001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.h:51: static PassOwnPtrWillBeRawPtr<LocalFileSystem> create(PassOwnPtr<FileSystemClient>); revert https://codereview.chromium.org/324483002/diff/80001/Source/modules/filesyste... Source/modules/filesystem/LocalFileSystem.h:65: WillBeHeapSupplement<Page>::trace(visitor); revert
Thanks, I've updated the cl to not change any of the oilpan stuff. Really appreciate the quick fixes :)
On 2014/06/10 17:27:57, jam wrote: > Thanks, I've updated the cl to not change any of the oilpan stuff. Really > appreciate the quick fixes :) Confirmed that it is indeed so, lgtm oilpan-wise. Sorry about the back&forth here.
oilpan lgtm2.
LGTM. Hope you can move other modules off from a Page to a LocalFrame smoothly.
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jam@chromium.org/324483002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/web/WebLocalFrameImpl.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/web/WebLocalFrameImpl.cpp
Hunk #1 succeeded at 182 (offset 1 line).
Hunk #2 FAILED at 1621.
1 out of 2 hunks FAILED -- saving rejects to file
Source/web/WebLocalFrameImpl.cpp.rej
Patch: Source/web/WebLocalFrameImpl.cpp
Index: Source/web/WebLocalFrameImpl.cpp
diff --git a/Source/web/WebLocalFrameImpl.cpp b/Source/web/WebLocalFrameImpl.cpp
index
48a17adfc6bd43d1d37e162fd1a13fd2304a2e08..17cad02eba14abc4135cacdcf640b0e445fee494
100644
--- a/Source/web/WebLocalFrameImpl.cpp
+++ b/Source/web/WebLocalFrameImpl.cpp
@@ -181,6 +181,7 @@
#include "web/EventListenerWrapper.h"
#include "web/FindInPageCoordinates.h"
#include "web/GeolocationClientProxy.h"
+#include "web/LocalFileSystemClient.h"
#include "web/MIDIClientProxy.h"
#include "web/PageOverlay.h"
#include "web/SharedWorkerRepositoryClientImpl.h"
@@ -1620,6 +1621,7 @@ void
WebLocalFrameImpl::setWebCoreFrame(PassRefPtr<WebCore::LocalFrame> frame)
provideGeolocationTo(*m_frame, m_geolocationClientProxy.get());
m_geolocationClientProxy->setController(GeolocationController::from(m_frame.get()));
provideMIDITo(*m_frame, MIDIClientProxy::create(m_client ?
m_client->webMIDIClient() : 0));
+ provideLocalFileSystemTo(*m_frame, LocalFileSystemClient::create());
}
}
rebase
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jam@chromium.org/324483002/120001
+darin for web
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7451)
Message was sent while issue was closed.
Change committed as 175970
Message was sent while issue was closed.
Please see https://codereview.chromium.org/319633007/ -- seeing Oilpan issues with these new persistent supplements for LocalFrame. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
