Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843693003:
Move methods from FrameViewBase to FrameView. (Closed)
|
Created:
3 years, 8 months ago by joelhockey Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-frames_chromium.org, chromium-reviews, kinuko+watch Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove methods from FrameViewBase to FrameView.
Remove FrameViewBase.cpp file.
This CL is in preparation to move core/paint/ScrollbarManager
to platform/scroll and use it as replacement for FrameViewBase.
BUG=637460
Review-Url: https://codereview.chromium.org/2843693003
Cr-Commit-Position: refs/heads/master@{#467303}
Committed: https://chromium.googlesource.com/chromium/src/+/e9592f40c7a6e6959be58a19af9f1483c148ed3b
Patch Set 1 #Patch Set 2 : revert inadvertent Scrollbar changes #Patch Set 3 : more scrollbar revert #
Total comments: 7
Patch Set 4 : address comments #
Total comments: 2
Messages
Total messages: 27 (10 generated)
The CQ bit was checked by joelhockey@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...
Description was changed from ========== Move methods from FrameViewBase to FrameView. BUG=637460 ========== to ========== Move methods from FrameViewBase to FrameView. Remove FrameViewBase.cpp file. This CL is in preparation to move core/paint/ScrollbarManager to platform/scroll and use it as replacement for FrameViewBase. BUG=637460 ==========
joelhockey@chromium.org changed reviewers: + dcheng@chromium.org, haraken@chromium.org, szager@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> parent_; Are we going to have some way of sharing this between FrameView/RemoteFrameView?
On 2017/04/25 at 23:38:51, dcheng wrote: > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> parent_; > Are we going to have some way of sharing this between FrameView/RemoteFrameView? RemoteFrameView needs only ConvertFromRootFrame. My feeling right now is to duplicate it into RemoteFrameView. I'm also thinking that RemoteFrameView may become a specialized child of FrameView just like Plugin and Scrollbar since it can only be a child.
On 2017/04/26 at 00:11:25, joelhockey wrote: > On 2017/04/25 at 23:38:51, dcheng wrote: > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): > > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> parent_; > > Are we going to have some way of sharing this between FrameView/RemoteFrameView? > > RemoteFrameView needs only ConvertFromRootFrame. My feeling right now is to duplicate it into RemoteFrameView. I'm also thinking that RemoteFrameView may become a specialized child of FrameView just like Plugin and Scrollbar since it can only be a child. Hang on, first I should clarify that as of this CL, RemoteFrameView is still inheriting from FrameViewBase, and FrameViewBase still has everything required by RemoteFrameView. My intention is to stop RemoteFrameView from inheriting from FrameViewBase (soon to be replaced by ScrollbarManager). At that point, there are say half a dozen lines of code that need to be duplicated into RemoteFrameView. And then I also need to think about what its relationship with FrameView should be.
On 2017/04/26 00:15:59, joelhockey wrote: > On 2017/04/26 at 00:11:25, joelhockey wrote: > > On 2017/04/25 at 23:38:51, dcheng wrote: > > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): > > > > > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> > parent_; > > > Are we going to have some way of sharing this between > FrameView/RemoteFrameView? > > > > RemoteFrameView needs only ConvertFromRootFrame. My feeling right now is to > duplicate it into RemoteFrameView. I'm also thinking that RemoteFrameView may > become a specialized child of FrameView just like Plugin and Scrollbar since it > can only be a child. > > Hang on, first I should clarify that as of this CL, RemoteFrameView is still > inheriting from FrameViewBase, and FrameViewBase still has everything required > by RemoteFrameView. My intention is to stop RemoteFrameView from inheriting > from FrameViewBase (soon to be replaced by ScrollbarManager). At that point, > there are say half a dozen lines of code that need to be duplicated into > RemoteFrameView. And then I also need to think about what its relationship with > FrameView should be. Duplicating code between FrameView and RemoteFrameView doesn't sound nice to me. Would it be an option to introduce a base class for them? My proposal is to rename: - FrameView => LocalFrameLayoutController - RemoteFrameView => RemoteFrameLayoutController - PluginView => PluginLayoutController - FrameOrPlugin => LayoutController and then make XXXLayoutController inherit from the LayoutController. We can add common functionalities to the LayoutController.
On 2017/04/26 at 06:01:42, haraken wrote: > On 2017/04/26 00:15:59, joelhockey wrote: > > On 2017/04/26 at 00:11:25, joelhockey wrote: > > > On 2017/04/25 at 23:38:51, dcheng wrote: > > > > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): > > > > > > > > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> > > parent_; > > > > Are we going to have some way of sharing this between > > FrameView/RemoteFrameView? > > > > > > RemoteFrameView needs only ConvertFromRootFrame. My feeling right now is to > > duplicate it into RemoteFrameView. I'm also thinking that RemoteFrameView may > > become a specialized child of FrameView just like Plugin and Scrollbar since it > > can only be a child. > > > > Hang on, first I should clarify that as of this CL, RemoteFrameView is still > > inheriting from FrameViewBase, and FrameViewBase still has everything required > > by RemoteFrameView. My intention is to stop RemoteFrameView from inheriting > > from FrameViewBase (soon to be replaced by ScrollbarManager). At that point, > > there are say half a dozen lines of code that need to be duplicated into > > RemoteFrameView. And then I also need to think about what its relationship with > > FrameView should be. > > Duplicating code between FrameView and RemoteFrameView doesn't sound nice to me. Would it be an option to introduce a base class for them? > > My proposal is to rename: > > - FrameView => LocalFrameLayoutController > - RemoteFrameView => RemoteFrameLayoutController > - PluginView => PluginLayoutController > - FrameOrPlugin => LayoutController > > and then make XXXLayoutController inherit from the LayoutController. We can add common functionalities to the LayoutController. I like that proposal. I do already have the rename you suggested earlier on my TODO list, and the separation of Local vs Remote matches what I was suggesting today. This Cl is still consistent with that path, so if anyone can give approval, I will continue with changing the inheritance hierarchies and location of methods. I will do the rename to *LayoutController as one of the last things.
On 2017/04/26 06:16:57, joelhockey wrote: > On 2017/04/26 at 06:01:42, haraken wrote: > > On 2017/04/26 00:15:59, joelhockey wrote: > > > On 2017/04/26 at 00:11:25, joelhockey wrote: > > > > On 2017/04/25 at 23:38:51, dcheng wrote: > > > > > > > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: > Member<FrameView> > > > parent_; > > > > > Are we going to have some way of sharing this between > > > FrameView/RemoteFrameView? > > > > > > > > RemoteFrameView needs only ConvertFromRootFrame. My feeling right now is > to > > > duplicate it into RemoteFrameView. I'm also thinking that RemoteFrameView > may > > > become a specialized child of FrameView just like Plugin and Scrollbar since > it > > > can only be a child. > > > > > > Hang on, first I should clarify that as of this CL, RemoteFrameView is still > > > inheriting from FrameViewBase, and FrameViewBase still has everything > required > > > by RemoteFrameView. My intention is to stop RemoteFrameView from inheriting > > > from FrameViewBase (soon to be replaced by ScrollbarManager). At that > point, > > > there are say half a dozen lines of code that need to be duplicated into > > > RemoteFrameView. And then I also need to think about what its relationship > with > > > FrameView should be. > > > > Duplicating code between FrameView and RemoteFrameView doesn't sound nice to > me. Would it be an option to introduce a base class for them? > > > > My proposal is to rename: > > > > - FrameView => LocalFrameLayoutController > > - RemoteFrameView => RemoteFrameLayoutController > > - PluginView => PluginLayoutController > > - FrameOrPlugin => LayoutController > > > > and then make XXXLayoutController inherit from the LayoutController. We can > add common functionalities to the LayoutController. > > I like that proposal. I do already have the rename you suggested earlier on my > TODO list, and the separation of Local vs Remote matches what I was suggesting > today. > > This Cl is still consistent with that path, so if anyone can give approval, I > will continue with changing the inheritance hierarchies and location of methods. > I will do the rename to *LayoutController as one of the last things. However, once we introduce LayoutController, won't we end up with moving the duplicated parent_, X(), Y(), isVisible() etc to LayoutController? Then I guess it would make more sense to introduce LayoutController first and move methods in FrameViewBase to LayoutController.
> However, once we introduce LayoutController, won't we end up with moving the duplicated parent_, X(), Y(), isVisible() etc to LayoutController? Then I guess it would make more sense to introduce LayoutController first and move methods in FrameViewBase to LayoutController. No. There is only 1 method that RemoteFrameView calls from FrameViewBase - ConvertFromRootFrame. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Rem... This method then has a callback which RemoteFrameView implements for ConvertFromContainingFrameViewBase. It is 5 lines of code. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Rem... RemoteFrameView does not use any of the other methods you list. So when I remove FrameViewBase as base class of RemoteFrameView, my current plan is to implement ConvertFromRootFrame (5 lines of code) directly into RemoteFrameView. This is all that will be needed. I expect that LayoutController will be a pure virtual class.
Thanks for the clarification! That totally makes sense. LGTM. https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:131: void Resize(int w, int h) { width height https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:134: void Resize(const IntSize& s) { SetFrameRect(IntRect(Location(), s)); } size
The CQ bit was checked by joelhockey@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/2843693003/#ps60001 (title: "address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:131: void Resize(int w, int h) { On 2017/04/26 at 07:42:14, haraken wrote: > width > height Done https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:134: void Resize(const IntSize& s) { SetFrameRect(IntRect(Location(), s)); } On 2017/04/26 at 07:42:14, haraken wrote: > size Done https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> parent_; On 2017/04/25 at 23:38:51, dcheng (OOO through May 2) wrote: > Are we going to have some way of sharing this between FrameView/RemoteFrameView? FrameOrPlugin (LayoutController) could end up holding parent. It is base class for PluginView, FrameView and RemoteFrameView. But I'm more inclined for FrameOrPlugin to be pure virtual and define SetParent/Parent methods and each subclass can hold its own fields.
https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> parent_; On 2017/04/26 07:48:37, joelhockey wrote: > On 2017/04/25 at 23:38:51, dcheng (OOO through May 2) wrote: > > Are we going to have some way of sharing this between > FrameView/RemoteFrameView? > > FrameOrPlugin (LayoutController) could end up holding parent. It is base class > for PluginView, FrameView and RemoteFrameView. But I'm more inclined for > FrameOrPlugin to be pure virtual and define SetParent/Parent methods and each > subclass can hold its own fields. I think the long-term idea is to get rid of the parent_ field, isn't it? My concern is with the visibility propagation; I'd rather not duplicate that logic if possible (which it currently is in this CL).
On 2017/04/26 at 08:16:15, dcheng wrote: > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> parent_; > On 2017/04/26 07:48:37, joelhockey wrote: > > On 2017/04/25 at 23:38:51, dcheng (OOO through May 2) wrote: > > > Are we going to have some way of sharing this between > > FrameView/RemoteFrameView? > > > > FrameOrPlugin (LayoutController) could end up holding parent. It is base class > > for PluginView, FrameView and RemoteFrameView. But I'm more inclined for > > FrameOrPlugin to be pure virtual and define SetParent/Parent methods and each > > subclass can hold its own fields. > > I think the long-term idea is to get rid of the parent_ field, isn't it? My concern is with the visibility propagation; I'd rather not duplicate that logic if possible (which it currently is in this CL). Yes, I have heard it expressed that it would be good to get rid of parent. I'm not sure if that is possible. I think it is possible to get rid of the FrameView->FrameView parent relationship, but I think that Scrollbar, Plugin and RemoteFrameView will likely need to keep the parent field of FrameView. I assume for 'visibility propagation' you mean the IsVisible / IsSelfVisible / IsParentVisible, etc. I have moved this from FrameViewBase into FrameView. This required the self_visible_ and parent_visible_ fields to also be copied into RemoteFrameView. This is very small duplication (of fields, but not really logic) that I don't have a problem with. My opinion has formed quite strongly that RemoteFrameView is just not very much at all like FrameView and I prefer to have them not share a base class.
On 2017/04/26 08:26:57, joelhockey wrote: > On 2017/04/26 at 08:16:15, dcheng wrote: > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): > > > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> > parent_; > > On 2017/04/26 07:48:37, joelhockey wrote: > > > On 2017/04/25 at 23:38:51, dcheng (OOO through May 2) wrote: > > > > Are we going to have some way of sharing this between > > > FrameView/RemoteFrameView? > > > > > > FrameOrPlugin (LayoutController) could end up holding parent. It is base > class > > > for PluginView, FrameView and RemoteFrameView. But I'm more inclined for > > > FrameOrPlugin to be pure virtual and define SetParent/Parent methods and > each > > > subclass can hold its own fields. > > > > I think the long-term idea is to get rid of the parent_ field, isn't it? My > concern is with the visibility propagation; I'd rather not duplicate that logic > if possible (which it currently is in this CL). > > Yes, I have heard it expressed that it would be good to get rid of parent. I'm > not sure if that is possible. I think it is possible to get rid of the > FrameView->FrameView parent relationship, but I think that Scrollbar, Plugin and > RemoteFrameView will likely need to keep the parent field of FrameView. Yeah, this is my understanding as well. The FrameView->FrameView parent relationship should be removed by using the Frame::parent(), but Scrollbar and Plugin will need to keep the parent field. > I assume for 'visibility propagation' you mean the IsVisible / IsSelfVisible / > IsParentVisible, etc. I have moved this from FrameViewBase into FrameView. > This required the self_visible_ and parent_visible_ fields to also be copied > into RemoteFrameView. This is very small duplication (of fields, but not really > logic) that I don't have a problem with. My opinion has formed quite strongly > that RemoteFrameView is just not very much at all like FrameView and I prefer to > have them not share a base class.
On 2017/04/26 08:26:57, joelhockey wrote: > On 2017/04/26 at 08:16:15, dcheng wrote: > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): > > > > > https://codereview.chromium.org/2843693003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/RemoteFrameView.h:65: Member<FrameView> > parent_; > > On 2017/04/26 07:48:37, joelhockey wrote: > > > On 2017/04/25 at 23:38:51, dcheng (OOO through May 2) wrote: > > > > Are we going to have some way of sharing this between > > > FrameView/RemoteFrameView? > > > > > > FrameOrPlugin (LayoutController) could end up holding parent. It is base > class > > > for PluginView, FrameView and RemoteFrameView. But I'm more inclined for > > > FrameOrPlugin to be pure virtual and define SetParent/Parent methods and > each > > > subclass can hold its own fields. > > > > I think the long-term idea is to get rid of the parent_ field, isn't it? My > concern is with the visibility propagation; I'd rather not duplicate that logic > if possible (which it currently is in this CL). > > Yes, I have heard it expressed that it would be good to get rid of parent. I'm > not sure if that is possible. I think it is possible to get rid of the > FrameView->FrameView parent relationship, but I think that Scrollbar, Plugin and > RemoteFrameView will likely need to keep the parent field of FrameView. Why can't RemoteFrameView use frame->Parent()? > > I assume for 'visibility propagation' you mean the IsVisible / IsSelfVisible / > IsParentVisible, etc. I have moved this from FrameViewBase into FrameView. > This required the self_visible_ and parent_visible_ fields to also be copied > into RemoteFrameView. This is very small duplication (of fields, but not really > logic) that I don't have a problem with. My opinion has formed quite strongly > that RemoteFrameView is just not very much at all like FrameView and I prefer to > have them not share a base class. I personally find the visibility calculations to be a little confusing to reason about, hence the reason I prefer we keep logic that is reused in a common location, and use some sort of delegate pattern to allow RemoteFrameView/LocalFrameView the necessary customization hooks (e.g. for SetParentIsVisible/SetParent). I think I would feel less strongly about this if we got rid of the parent_visible_ field though. https://codereview.chromium.org/2843693003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2843693003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:171: FrameView::FrameView(LocalFrame& frame, IntRect frame_rect) Btw, this should be passed by const ref (followup is OK if that's easier)
https://codereview.chromium.org/2843693003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2843693003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:171: FrameView::FrameView(LocalFrame& frame, IntRect frame_rect) On 2017/04/26 at 08:42:59, dcheng (OOO through May 2) wrote: > Btw, this should be passed by const ref (followup is OK if that's easier) OK, I'll follow up with this
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493192700869720,
"parent_rev": "333e43abd4855a0ae2912fb1e517e0813bd00005", "commit_rev":
"e9592f40c7a6e6959be58a19af9f1483c148ed3b"}
Message was sent while issue was closed.
Description was changed from ========== Move methods from FrameViewBase to FrameView. Remove FrameViewBase.cpp file. This CL is in preparation to move core/paint/ScrollbarManager to platform/scroll and use it as replacement for FrameViewBase. BUG=637460 ========== to ========== Move methods from FrameViewBase to FrameView. Remove FrameViewBase.cpp file. This CL is in preparation to move core/paint/ScrollbarManager to platform/scroll and use it as replacement for FrameViewBase. BUG=637460 Review-Url: https://codereview.chromium.org/2843693003 Cr-Commit-Position: refs/heads/master@{#467303} Committed: https://chromium.googlesource.com/chromium/src/+/e9592f40c7a6e6959be58a19af9f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e9592f40c7a6e6959be58a19af9f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
