Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873213003:
Move WebFrame to use WebLocalFrameBase instead of WebLocalFrameImpl. (Closed)
DescriptionMove WebFrame to use WebLocalFrameBase instead of WebLocalFrameImpl.
This requires WebLocalFrameBase become garbage collected, so moved the
declaration of GarbageCollectedFinalized<> from WebLocalFrameImpl to
WebLocalFrameBase, and added Tracing to WebLocalFrameBase.
BUG=708879
Review-Url: https://codereview.chromium.org/2873213003
Cr-Commit-Position: refs/heads/master@{#470903}
Committed: https://chromium.googlesource.com/chromium/src/+/481eb1f44f0db7dbfd321be80dccb3e0e8cbe314
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase #
Messages
Total messages: 23 (11 generated)
slangley@chromium.org changed reviewers: + haraken@chromium.org
The CQ bit was checked by slangley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1634: WebFrame::TraceFrames(visitor, this); Why can't we move this to WebLocalFrameBase's DEFINE_INLINE_VIRTUAL_TRACE now?
On 2017/05/11 at 01:01:28, haraken wrote: > LGTM > > https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): > > https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1634: WebFrame::TraceFrames(visitor, this); > > Why can't we move this to WebLocalFrameBase's DEFINE_INLINE_VIRTUAL_TRACE now? It will not link on all platforms - calling a static method in web/ from core/frame/.
The CQ bit was checked by slangley@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/11 01:18:53, slangley wrote: > On 2017/05/11 at 01:01:28, haraken wrote: > > LGTM > > > > > https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... > > File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): > > > > > https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1634: > WebFrame::TraceFrames(visitor, this); > > > > Why can't we move this to WebLocalFrameBase's DEFINE_INLINE_VIRTUAL_TRACE now? > > It will not link on all platforms - calling a static method in web/ from > core/frame/. Ah, got it. Why do we need to make WebLocalFrameBase a GarbageCollected? It looks strange that you are tracing a parent class (WebFrame) from a class that is not a direct child of WebFrame (WebLocalFrameImpl).
On 2017/05/11 at 01:24:59, haraken wrote: > On 2017/05/11 01:18:53, slangley wrote: > > On 2017/05/11 at 01:01:28, haraken wrote: > > > LGTM > > > > > > > > https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... > > > File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): > > > > > > > > https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... > > > third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1634: > > WebFrame::TraceFrames(visitor, this); > > > > > > Why can't we move this to WebLocalFrameBase's DEFINE_INLINE_VIRTUAL_TRACE now? > > > > It will not link on all platforms - calling a static method in web/ from > > core/frame/. > > Ah, got it. Why do we need to make WebLocalFrameBase a GarbageCollected? It looks strange that you are tracing a parent class (WebFrame) from a class that is not a direct child of WebFrame (WebLocalFrameImpl). I agree it is strange - but that's why the TODO exists. This state should be temporary while we untangle all of these links in the code, both explicit and implicit. In this case WebFrame is calling Trace on ToWebLocalFrameImpl, which we changed to ToWebLocalFrameBase.
On 2017/05/11 01:29:51, slangley wrote: > On 2017/05/11 at 01:24:59, haraken wrote: > > On 2017/05/11 01:18:53, slangley wrote: > > > On 2017/05/11 at 01:01:28, haraken wrote: > > > > LGTM > > > > > > > > > > > > https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... > > > > File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/2873213003/diff/1/third_party/WebKit/Source/w... > > > > third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1634: > > > WebFrame::TraceFrames(visitor, this); > > > > > > > > Why can't we move this to WebLocalFrameBase's DEFINE_INLINE_VIRTUAL_TRACE > now? > > > > > > It will not link on all platforms - calling a static method in web/ from > > > core/frame/. > > > > Ah, got it. Why do we need to make WebLocalFrameBase a GarbageCollected? It > looks strange that you are tracing a parent class (WebFrame) from a class that > is not a direct child of WebFrame (WebLocalFrameImpl). > > I agree it is strange - but that's why the TODO exists. This state should be > temporary while we untangle all of these links in the code, both explicit and > implicit. > > In this case WebFrame is calling Trace on ToWebLocalFrameImpl, which we changed > to ToWebLocalFrameBase. ah, makes sense.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by slangley@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for third_party/WebKit/Source/web/WebLocalFrameImpl.h:
While running git apply --index -3 -p1;
error: patch failed: third_party/WebKit/Source/web/WebLocalFrameImpl.h:433
Falling back to three-way merge...
Applied patch to 'third_party/WebKit/Source/web/WebLocalFrameImpl.h' with
conflicts.
U third_party/WebKit/Source/web/WebLocalFrameImpl.h
Patch: third_party/WebKit/Source/web/WebLocalFrameImpl.h
Index: third_party/WebKit/Source/web/WebLocalFrameImpl.h
diff --git a/third_party/WebKit/Source/web/WebLocalFrameImpl.h
b/third_party/WebKit/Source/web/WebLocalFrameImpl.h
index
9ac95a81c2735e8e6c211891b0cf2b3070bb4c12..a5dc770ba7355a51e9ea4dfa56ea50670f86de5b
100644
--- a/third_party/WebKit/Source/web/WebLocalFrameImpl.h
+++ b/third_party/WebKit/Source/web/WebLocalFrameImpl.h
@@ -84,8 +84,7 @@ class WebVector;
// Implementation of WebFrame, note that this is a reference counted object.
class WEB_EXPORT WebLocalFrameImpl final
- : public GarbageCollectedFinalized<WebLocalFrameImpl>,
- NON_EXPORTED_BASE(public WebLocalFrameBase) {
+ : NON_EXPORTED_BASE(public WebLocalFrameBase) {
public:
// WebFrame methods:
// TODO(dcheng): Fix sorting here; a number of method have been moved to
@@ -319,7 +318,9 @@ class WEB_EXPORT WebLocalFrameImpl final
WebString& clip_text,
WebString& clip_html) override;
- void InitializeCoreFrame(Page&, FrameOwner*, const AtomicString& name);
+ void InitializeCoreFrame(Page&,
+ FrameOwner*,
+ const AtomicString& name) override;
LocalFrame* GetFrame() const override { return frame_.Get(); }
void WillBeDetached();
@@ -433,7 +434,7 @@ class WEB_EXPORT WebLocalFrameImpl final
void SetContextMenuNode(Node* node) override { context_menu_node_ = node; }
void ClearContextMenuNode() override { context_menu_node_.Clear(); }
- DECLARE_TRACE();
+ DECLARE_VIRTUAL_TRACE();
private:
friend class LocalFrameClientImpl;
The CQ bit was checked by slangley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2873213003/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494491352084910,
"parent_rev": "5788c3d4d26446572f032baa70ba82fdf39e789d", "commit_rev":
"481eb1f44f0db7dbfd321be80dccb3e0e8cbe314"}
Message was sent while issue was closed.
Description was changed from ========== Move WebFrame to use WebLocalFrameBase instead of WebLocalFrameImpl. This requires WebLocalFrameBase become garbage collected, so moved the declaration of GarbageCollectedFinalized<> from WebLocalFrameImpl to WebLocalFrameBase, and added Tracing to WebLocalFrameBase. BUG=708879 ========== to ========== Move WebFrame to use WebLocalFrameBase instead of WebLocalFrameImpl. This requires WebLocalFrameBase become garbage collected, so moved the declaration of GarbageCollectedFinalized<> from WebLocalFrameImpl to WebLocalFrameBase, and added Tracing to WebLocalFrameBase. BUG=708879 Review-Url: https://codereview.chromium.org/2873213003 Cr-Commit-Position: refs/heads/master@{#470903} Committed: https://chromium.googlesource.com/chromium/src/+/481eb1f44f0db7dbfd321be80dcc... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/481eb1f44f0db7dbfd321be80dcc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
