Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814643003:
Remove FrameViewBase as base class of PluginView. (Closed)
|
Created:
3 years, 8 months ago by joelhockey Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-frames_chromium.org, blink-reviews-html_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dglazkov+blink, dshwang, eae+blinkwatch, jchaffraix+rendering, kinuko+watch, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove FrameViewBase as base class of PluginView.
Created new pure abstract class FrameOrPlugin to be used
in places where polymorphism of frames and plugins is required.
FrameView, RemoteFrameView and PluginView implement this class.
FrameOrPlugin class has the minimal common methods required:
SetFrameRect, FrameRect, Paint, Hide, Show, Dispose.
The parent_, frame_rect_, and visible_ vars from FrameViewBase
have now moved into WebPluginContainerImpl.
Next step is to remove FrameView, RemoteFrameView, and Scrollbar from
inheriting from FrameViewBase, and remove the FrameViewBase class.
BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2814643003
Cr-Commit-Position: refs/heads/master@{#465144}
Committed: https://chromium.googlesource.com/chromium/src/+/e4904c3670d6a8d4f88642ca7e18ca2a9878a684
Patch Set 1 #Patch Set 2 : Changed upstream to origin/master. #
Total comments: 2
Patch Set 3 : fix unused var #
Total comments: 14
Patch Set 4 : No need for child_frame_view var #Patch Set 5 : RemoteFrameView must also be FrameOrPlugin #Patch Set 6 : Added class comment #Patch Set 7 : Make FrameOrPlugin GarbageCollectedMixin #Patch Set 8 : Merge after dependency CL #
Total comments: 12
Patch Set 9 : Remove FrameViewBase Paint, Show, Hide. #
Total comments: 2
Patch Set 10 : Remove FrameViewBase Paint, Show, Hide. #Patch Set 11 : Address final comments #Dependent Patchsets: Messages
Total messages: 65 (40 generated)
Description was changed from ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. This class has the minimal methods required. PluginView and FrameView inherit from FrameOrPlugin. Next step is to remove FrameView and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 ========== to ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. This class has the minimal methods required. PluginView and FrameView inherit from FrameOrPlugin. Next step is to remove FrameView and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. This class has the minimal methods required. PluginView and FrameView inherit from FrameOrPlugin. Next step is to remove FrameView and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. FrameOrPlugin class has the minimal methods required: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. PluginView and FrameView inherit from FrameOrPlugin. Next step is to remove FrameView and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
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...
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...
joelhockey@chromium.org changed reviewers: + dcheng@chromium.org, haraken@chromium.org
This is the smallest change that I can do to remove PluginView from inheriting from FrameViewBase.
Description was changed from ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. FrameOrPlugin class has the minimal methods required: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. PluginView and FrameView inherit from FrameOrPlugin. Next step is to remove FrameView and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. FrameView and PluginView implement this class. FrameOrPlugin class has the minimal common methods required: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. The parent_, frame_rect_, and visible_ vars from FrameViewBase have now moved into WebPluginContainerImpl. Next step is to remove FrameView and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameOrPlugin.h:16: class CORE_EXPORT FrameOrPlugin { My personal feeling about this naming is that it's too directly tied to the subclasses and doesn't really describe what this interface is for (it's for common functionality, but what is the common functionality? Is there something we could name it that would better reflect its purpose? Etc) It's kind of hard to name though... but I think we should try to figure something out here. https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:59: static DisposeList& PendingDispose() { PendingDispose / DisposeList is a bit too generic -- what's pending dispose, etc. (This relates to my naming comment from earlier) https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutPart.cpp:270: if (frame_view_base && frame_view_base->IsFrameView()) { Maybe it would be more natural to write this as a downcast from FrameOrPlugin directly, rather than checking FrameViewBase? https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutPart.cpp:353: if (frame_view_base && frame_view_base->IsFrameView()) { Ditto -- it seems a bit unusual to have this null check here. https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:119: // Update visibility if plugin is visible and parent visibility changes. Can the parent change visibility without changing parents? Do we need to get notified in that case?
https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameOrPlugin.h:16: class CORE_EXPORT FrameOrPlugin { This class is pure virtual. I have not made it inherit from GarbageCollectedFinalized. Do I need to have it interacting with oilpan in some way? This seems most relevant in HTMLFrameOwnerElement where I hold a vector of this class for disposing after suspend finishes. https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right): https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:58: using DisposeList = Vector<FrameOrPlugin*>; I didn't know if this needs to interact with oilpan in some way? FrameOrPlugin class does not inherit from any oilpan or other classes. https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameOrPlugin.h:16: class CORE_EXPORT FrameOrPlugin { On 2017/04/12 at 00:06:39, dcheng wrote: > My personal feeling about this naming is that it's too directly tied to the subclasses and doesn't really describe what this interface is for (it's for common functionality, but what is the common functionality? Is there something we could name it that would better reflect its purpose? Etc) > > It's kind of hard to name though... but I think we should try to figure something out here. I totally agree that it is better to name based on what the thing does, not on what the things are that implement it, but I went with FrameOrPlugin since it was the best of the options I could think of. So what does this do?: It has methods: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. Suggestions: * View - I had this one first, but the word View is too general and becomes very confusing since we already have a lot of methods named View that return LayoutView or FrameView or some other views. * VisibleRectangle or FrameRect or FrameRectangle - this seems to describe what the thing is based on the methods that it has. * FrameView - rename existing FrameView to LocalFrameView. This makes sense since we also have RemoteFrameView. * FrameViewInterface * FrameViewBase - once existing FrameViewBase (old Widget) class is removed, then I could change FrameOrPlugin to be FrameViewBase. * Widget - ha, ha, this is a joke. But it goes to show that Widget is not a crazy name for this thing. https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:59: static DisposeList& PendingDispose() { On 2017/04/12 at 00:06:39, dcheng wrote: > PendingDispose / DisposeList is a bit too generic -- what's pending dispose, etc. > > (This relates to my naming comment from earlier) If you suggest a name, I'm happy to change it. But in this case, I would make an argument that this is a good name. I feel like the type of the Vector shows what's pending dispose - it is FrameOrPlugin. So I don't feel like the variable needs to echo the name of the type that is involved. If I was to pick another name, I guess it would be FrameOrPluginPendingDispose. Or <Whatever-name-we-agree-for-this-class>PendingDispose https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutPart.cpp:270: if (frame_view_base && frame_view_base->IsFrameView()) { On 2017/04/12 at 00:06:39, dcheng wrote: > Maybe it would be more natural to write this as a downcast from FrameOrPlugin directly, rather than checking FrameViewBase? My preferred solution is to make FrameView accessible directly from LayoutPart on its own method. I didn't want to mix up this CL with too many things though. I think I'll do it as a prequel CL right now. Keep your eyes peeeled. I don't like the explicit downcasts as a general pattern, so I don't want to introduce it into the FrameOrPlugin class. LayoutPart inherits indirectly from LayoutObject. LayoutObject already has a method GetFrameView which returns Document().Frame().View(). This is not the same as the FrameViewBase which is stored on HTMLFrameOwnerElement. So I need to think of a good name for this method. I will do my best and you will see it in a CL coming soon. https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutPart.cpp:353: if (frame_view_base && frame_view_base->IsFrameView()) { On 2017/04/12 at 00:06:39, dcheng wrote: > Ditto -- it seems a bit unusual to have this null check here. I'll clean this up as a prequel CL. https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:119: // Update visibility if plugin is visible and parent visibility changes. On 2017/04/12 at 00:06:39, dcheng wrote: > Can the parent change visibility without changing parents? Do we need to get notified in that case? I'm pretty sure that yes, parent can change vis. However, it seems that pepper does nothing in this case: https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_webplugin... And none of the tests failed when I removed the code to call plugin::SetParentVisible, so I did this CL 2 days ago: https://codereview.chromium.org/2808723002 I'm going to revert that CL, because I see that in browser_plugin, it probably does rely on getting those updates.
I had some questions that I had put as comments on the CL. It looks like they didn't get sent until now. Sorry, it has made this all a bit mixed up. On Wed, Apr 12, 2017 at 2:33 PM, <joelhockey@chromium.org> wrote: > > https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): > > https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameOrPlugin.h:16: class > CORE_EXPORT FrameOrPlugin { > This class is pure virtual. I have not made it inherit from > GarbageCollectedFinalized. Do I need to have it interacting with oilpan > in some way? > > This seems most relevant in HTMLFrameOwnerElement where I hold a vector > of this class for disposing after suspend finishes. > > https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp > (right): > > https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:58: using > DisposeList = Vector<FrameOrPlugin*>; > I didn't know if this needs to interact with oilpan in some way? > FrameOrPlugin class does not inherit from any oilpan or other classes. > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameOrPlugin.h:16: class > CORE_EXPORT FrameOrPlugin { > On 2017/04/12 at 00:06:39, dcheng wrote: >> My personal feeling about this naming is that it's too directly tied > to the subclasses and doesn't really describe what this interface is for > (it's for common functionality, but what is the common functionality? Is > there something we could name it that would better reflect its purpose? > Etc) >> >> It's kind of hard to name though... but I think we should try to > figure something out here. > > I totally agree that it is better to name based on what the thing does, > not on what the things are that implement it, but I went with > FrameOrPlugin since it was the best of the options I could think of. > > So what does this do?: > It has methods: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. > > Suggestions: > * View - I had this one first, but the word View is too general and > becomes very confusing since we already have a lot of methods named View > that return LayoutView or FrameView or some other views. > * VisibleRectangle or FrameRect or FrameRectangle - this seems to > describe what the thing is based on the methods that it has. > * FrameView - rename existing FrameView to LocalFrameView. This makes > sense since we also have RemoteFrameView. > * FrameViewInterface > * FrameViewBase - once existing FrameViewBase (old Widget) class is > removed, then I could change FrameOrPlugin to be FrameViewBase. > * Widget - ha, ha, this is a joke. But it goes to show that Widget is > not a crazy name for this thing. > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp > (right): > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:59: static > DisposeList& PendingDispose() { > On 2017/04/12 at 00:06:39, dcheng wrote: >> PendingDispose / DisposeList is a bit too generic -- what's pending > dispose, etc. >> >> (This relates to my naming comment from earlier) > > If you suggest a name, I'm happy to change it. But in this case, I > would make an argument that this is a good name. I feel like the type > of the Vector shows what's pending dispose - it is FrameOrPlugin. So I > don't feel like the variable needs to echo the name of the type that is > involved. > > If I was to pick another name, I guess it would be > FrameOrPluginPendingDispose. Or > <Whatever-name-we-agree-for-this-class>PendingDispose > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutPart.cpp:270: if > (frame_view_base && frame_view_base->IsFrameView()) { > On 2017/04/12 at 00:06:39, dcheng wrote: >> Maybe it would be more natural to write this as a downcast from > FrameOrPlugin directly, rather than checking FrameViewBase? > > My preferred solution is to make FrameView accessible directly from > LayoutPart on its own method. I didn't want to mix up this CL with too > many things though. I think I'll do it as a prequel CL right now. Keep > your eyes peeeled. > > I don't like the explicit downcasts as a general pattern, so I don't > want to introduce it into the FrameOrPlugin class. > > LayoutPart inherits indirectly from LayoutObject. LayoutObject already > has a method GetFrameView which returns Document().Frame().View(). This > is not the same as the FrameViewBase which is stored on > HTMLFrameOwnerElement. So I need to think of a good name for this > method. I will do my best and you will see it in a CL coming soon. > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutPart.cpp:353: if > (frame_view_base && frame_view_base->IsFrameView()) { > On 2017/04/12 at 00:06:39, dcheng wrote: >> Ditto -- it seems a bit unusual to have this null check here. > > I'll clean this up as a prequel CL. > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:119: // Update > visibility if plugin is visible and parent visibility changes. > On 2017/04/12 at 00:06:39, dcheng wrote: >> Can the parent change visibility without changing parents? Do we need > to get notified in that case? > > I'm pretty sure that yes, parent can change vis. > > However, it seems that pepper does nothing in this case: > https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_webplugin... > > And none of the tests failed when I removed the code to call > plugin::SetParentVisible, so I did this CL 2 days ago: > https://codereview.chromium.org/2808723002 > > I'm going to revert that CL, because I see that in browser_plugin, it > probably does rely on getting those updates. > > https://codereview.chromium.org/2814643003/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I had some questions that I had put as comments on the CL. It looks like they didn't get sent until now. Sorry, it has made this all a bit mixed up. On Wed, Apr 12, 2017 at 2:33 PM, <joelhockey@chromium.org> wrote: > > https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): > > https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameOrPlugin.h:16: class > CORE_EXPORT FrameOrPlugin { > This class is pure virtual. I have not made it inherit from > GarbageCollectedFinalized. Do I need to have it interacting with oilpan > in some way? > > This seems most relevant in HTMLFrameOwnerElement where I hold a vector > of this class for disposing after suspend finishes. > > https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp > (right): > > https://codereview.chromium.org/2814643003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:58: using > DisposeList = Vector<FrameOrPlugin*>; > I didn't know if this needs to interact with oilpan in some way? > FrameOrPlugin class does not inherit from any oilpan or other classes. > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameOrPlugin.h:16: class > CORE_EXPORT FrameOrPlugin { > On 2017/04/12 at 00:06:39, dcheng wrote: >> My personal feeling about this naming is that it's too directly tied > to the subclasses and doesn't really describe what this interface is for > (it's for common functionality, but what is the common functionality? Is > there something we could name it that would better reflect its purpose? > Etc) >> >> It's kind of hard to name though... but I think we should try to > figure something out here. > > I totally agree that it is better to name based on what the thing does, > not on what the things are that implement it, but I went with > FrameOrPlugin since it was the best of the options I could think of. > > So what does this do?: > It has methods: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. > > Suggestions: > * View - I had this one first, but the word View is too general and > becomes very confusing since we already have a lot of methods named View > that return LayoutView or FrameView or some other views. > * VisibleRectangle or FrameRect or FrameRectangle - this seems to > describe what the thing is based on the methods that it has. > * FrameView - rename existing FrameView to LocalFrameView. This makes > sense since we also have RemoteFrameView. > * FrameViewInterface > * FrameViewBase - once existing FrameViewBase (old Widget) class is > removed, then I could change FrameOrPlugin to be FrameViewBase. > * Widget - ha, ha, this is a joke. But it goes to show that Widget is > not a crazy name for this thing. > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp > (right): > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:59: static > DisposeList& PendingDispose() { > On 2017/04/12 at 00:06:39, dcheng wrote: >> PendingDispose / DisposeList is a bit too generic -- what's pending > dispose, etc. >> >> (This relates to my naming comment from earlier) > > If you suggest a name, I'm happy to change it. But in this case, I > would make an argument that this is a good name. I feel like the type > of the Vector shows what's pending dispose - it is FrameOrPlugin. So I > don't feel like the variable needs to echo the name of the type that is > involved. > > If I was to pick another name, I guess it would be > FrameOrPluginPendingDispose. Or > <Whatever-name-we-agree-for-this-class>PendingDispose > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutPart.cpp:270: if > (frame_view_base && frame_view_base->IsFrameView()) { > On 2017/04/12 at 00:06:39, dcheng wrote: >> Maybe it would be more natural to write this as a downcast from > FrameOrPlugin directly, rather than checking FrameViewBase? > > My preferred solution is to make FrameView accessible directly from > LayoutPart on its own method. I didn't want to mix up this CL with too > many things though. I think I'll do it as a prequel CL right now. Keep > your eyes peeeled. > > I don't like the explicit downcasts as a general pattern, so I don't > want to introduce it into the FrameOrPlugin class. > > LayoutPart inherits indirectly from LayoutObject. LayoutObject already > has a method GetFrameView which returns Document().Frame().View(). This > is not the same as the FrameViewBase which is stored on > HTMLFrameOwnerElement. So I need to think of a good name for this > method. I will do my best and you will see it in a CL coming soon. > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutPart.cpp:353: if > (frame_view_base && frame_view_base->IsFrameView()) { > On 2017/04/12 at 00:06:39, dcheng wrote: >> Ditto -- it seems a bit unusual to have this null check here. > > I'll clean this up as a prequel CL. > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): > > https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:119: // Update > visibility if plugin is visible and parent visibility changes. > On 2017/04/12 at 00:06:39, dcheng wrote: >> Can the parent change visibility without changing parents? Do we need > to get notified in that case? > > I'm pretty sure that yes, parent can change vis. > > However, it seems that pepper does nothing in this case: > https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_webplugin... > > And none of the tests failed when I removed the code to call > plugin::SetParentVisible, so I did this CL 2 days ago: > https://codereview.chromium.org/2808723002 > > I'm going to revert that CL, because I see that in browser_plugin, it > probably does rely on getting those updates. > > https://codereview.chromium.org/2814643003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Regarding oilpan, you should make FrameOrPlugin GarbageCollectedMixin. Then you can use HeapVector<Member<FrameOrPlugin>>.
A suggestion on naming: - FrameView => FrameLayoutController - PluginView => PluginLayoutController - Scrollbar => ScrollbarLayoutController - FrameOrPlugin => LayoutController Would it require too much renaming? https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameOrPlugin.h:16: class CORE_EXPORT FrameOrPlugin { Add a class-level comment. https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameOrPlugin.h:18: // FrameOrPlugin(); Remove this?
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 ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. FrameView and PluginView implement this class. FrameOrPlugin class has the minimal common methods required: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. The parent_, frame_rect_, and visible_ vars from FrameViewBase have now moved into WebPluginContainerImpl. Next step is to remove FrameView and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. FrameView, RemoteFrameView and PluginView implement this class. FrameOrPlugin class has the minimal common methods required: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. The parent_, frame_rect_, and visible_ vars from FrameViewBase have now moved into WebPluginContainerImpl. Next step is to remove FrameView, RemoteFrameView, and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
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...
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...
https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameOrPlugin.h:16: class CORE_EXPORT FrameOrPlugin { On 2017/04/12 at 06:32:31, haraken wrote: > Add a class-level comment. Done https://codereview.chromium.org/2814643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameOrPlugin.h:18: // FrameOrPlugin(); On 2017/04/12 at 06:32:31, haraken wrote: > Remove this? Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
I think the unresolved issues for this CL are: 1/ What to name FrameOrPlugin class 2/ What to name HTMLFrameOwnerElement::DisposeSoon static HeapVector. haraken@ has made a suggestion to rename a few different classes. To be honest, I'm not really excited about renaming so many things, but I don't mind doing it if you both think that is best. dcheng@, do you have any specific suggestions for names? Given that I've got this CL all done and passing on the bots, how does anyone feel about submitting this as-is and we can think about other names next week?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/13 09:12:27, joelhockey wrote: > I think the unresolved issues for this CL are: > > 1/ What to name FrameOrPlugin class > 2/ What to name HTMLFrameOwnerElement::DisposeSoon static HeapVector. > > haraken@ has made a suggestion to rename a few different classes. > > To be honest, I'm not really excited about renaming so many things, but I don't > mind doing it if you both think that is best. > > dcheng@, do you have any specific suggestions for names? > > Given that I've got this CL all done and passing on the bots, how does anyone > feel about submitting this as-is and we can think about other names next week? (I'll defer the naming decision to dcheng@.)
On 2017/04/13 at 16:16:22, haraken wrote: > On 2017/04/13 09:12:27, joelhockey wrote: > > Given that I've got this CL all done and passing on the bots, how does anyone > > feel about submitting this as-is and we can think about other names next week? > > (I'll defer the naming decision to dcheng@.) File a bug to rename, add a TODO linking to bug and land? WDYT?
szager@chromium.org changed reviewers: + szager@chromium.org
Sorry for the late drive-by, but: is there any reason not to make FrameViewBase inherit from FrameOrPlugin? Why push the inheritance down to FrameView and RemoteFrameView?
On 2017/04/13 at 17:28:10, szager wrote: > Sorry for the late drive-by, but: is there any reason not to make FrameViewBase inherit from FrameOrPlugin? Why push the inheritance down to FrameView and RemoteFrameView? One of the current goals is to remove the FrameViewBase (used to be called Widget) class. It wouldn't help to add any new supertype for it. platform/FrameViewBase is the base class for: * core/frame/FrameView * core/frame/RemoteFrameView * core/plugins/PluginView * platform/scroll/Scrollbar Scrollbar has no business having a common superclass with the others, but there is some legitimate polymorphism between the other 3, so it makes sense to have a common base class like core/frame/FrameOrPlugin. In some ways, FrameOrPlugin is not a lot different to FrameViewBase, so you might say that this isn't achieving much, but the 2 important differences are: 1/ FrameOrPlugin does not define SetParent / Parent / Root methods, and this will help with another of the current goals which is to remove the widget tree. 2/ FrameOrPlugin is in core/frame rather than platform which will help to isolate Scrollbar away from the others. As mentioned in CL description, next step from here is to remove the FrameViewBase class. Then after that, I plan to look at FrameView::children_ (widget tree) to see if it can be removed or improved.
https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/RemoteFrameView.h:43: void Paint(GraphicsContext&, const CullRect&) const override {} This is a bit tricky, because this overrides methods from both FrameViewBase *and* FrameOrPlugin. It might be worth calling this out, until we remove FrameViewBase? I would suggest that this means FrameOrPlugin should just be a base of FrameViewBase: I don't think this makes it harder to remove FrameViewBase in the long run, and it prevents the short-term awkwardness of having parallel virtual dispatch hierarchies. It also makes it clear which FrameViewBase methods are left to finish moving out before we can remove it altogether. WDYT? https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:109: void DisposeSoon(FrameOrPlugin*); Let's fix the naming for this method, since we're changing it in this CL anyway. I don't have a better suggestion than haraken's suggestion of LayoutController, so let's just go with that. https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutPart.cpp:122: return ToRemoteFrameView(frame_view_base); This is another example of something that would be simpler of FrameViewBase just subclasses FrameOrPlugin. https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/plugins/PluginView.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/plugins/PluginView.h:58: virtual bool IsPluginContainer() const { return false; } We may want to consider removing this as well (there is only one subclass of PluginView now, as far as I can tell): we basically have same situation as WebViewImpl here now: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp...
https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/RemoteFrameView.h:43: void Paint(GraphicsContext&, const CullRect&) const override {} On 2017/04/17 at 21:09:04, dcheng wrote: > This is a bit tricky, because this overrides methods from both FrameViewBase *and* FrameOrPlugin. It might be worth calling this out, until we remove FrameViewBase? > > I would suggest that this means FrameOrPlugin should just be a base of FrameViewBase: I don't think this makes it harder to remove FrameViewBase in the long run, and it prevents the short-term awkwardness of having parallel virtual dispatch hierarchies. > > It also makes it clear which FrameViewBase methods are left to finish moving out before we can remove it altogether. WDYT? I would rather have FrameOrPlugin class be in core/frame rather than platform (where FrameViewBase is). I have been able to easily remove Paint, Show, Hide from FrameViewBase right now which means less parallel virtual dispatch. It is only SetFrameRect, FrameRect, and Dispose that now exist in both. I could remove them in this CL, but it is a little bit involved, so I would rather do it in a separate CL. https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:109: void DisposeSoon(FrameOrPlugin*); On 2017/04/17 at 21:09:04, dcheng wrote: > Let's fix the naming for this method, since we're changing it in this CL anyway. I don't have a better suggestion than haraken's suggestion of LayoutController, so let's just go with that. I have renamed the method to DisposeFrameOrPluginSoon. Haraken's suggestion was to rename a few different classes. If we all agree that we want to do that, I can follow up with that in another CL, and I will also rename this method to the appropriate new name such as DisposeLayoutControllerSoon. https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutPart.cpp:122: return ToRemoteFrameView(frame_view_base); On 2017/04/17 at 21:09:05, dcheng wrote: > This is another example of something that would be simpler of FrameViewBase just subclasses FrameOrPlugin. That is true, but it will be even better when FrameViewBase is removed, and the common class of Plugin, FrameView, and RemoteFrameView is in /core/frame rather than platform. https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/plugins/PluginView.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/plugins/PluginView.h:58: virtual bool IsPluginContainer() const { return false; } On 2017/04/17 at 21:09:06, dcheng wrote: > We may want to consider removing this as well (there is only one subclass of PluginView now, as far as I can tell): we basically have same situation as WebViewImpl here now: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp... +1. There is only a single implementation, so I agree there is no need for this abstraction. I would then be inclined to rename web/WebPluginContainerImpl to just web/WebPluginContainer (drop the Impl suffix), but I see that many classes in web have Impl suffix.
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...
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...
LGTM w/comments addressed https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/RemoteFrameView.h:43: void Paint(GraphicsContext&, const CullRect&) const override {} On 2017/04/18 00:42:41, joelhockey wrote: > On 2017/04/17 at 21:09:04, dcheng wrote: > > This is a bit tricky, because this overrides methods from both FrameViewBase > *and* FrameOrPlugin. It might be worth calling this out, until we remove > FrameViewBase? > > > > I would suggest that this means FrameOrPlugin should just be a base of > FrameViewBase: I don't think this makes it harder to remove FrameViewBase in the > long run, and it prevents the short-term awkwardness of having parallel virtual > dispatch hierarchies. > > > > It also makes it clear which FrameViewBase methods are left to finish moving > out before we can remove it altogether. WDYT? > > I would rather have FrameOrPlugin class be in core/frame rather than platform > (where FrameViewBase is). I have been able to easily remove Paint, Show, Hide > from FrameViewBase right now which means less parallel virtual dispatch. It is > only SetFrameRect, FrameRect, and Dispose that now exist in both. I could > remove them in this CL, but it is a little bit involved, so I would rather do it > in a separate CL. Hmm. I guess this is fine for now given the reduced duplication, and that it's only temporary. (My one thought is it's a bit funny for core/plugins to implement something in core/frame, but it seems limited enough that it's probably OK) https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:109: void DisposeSoon(FrameOrPlugin*); On 2017/04/18 00:42:41, joelhockey wrote: > On 2017/04/17 at 21:09:04, dcheng wrote: > > Let's fix the naming for this method, since we're changing it in this CL > anyway. I don't have a better suggestion than haraken's suggestion of > LayoutController, so let's just go with that. > > I have renamed the method to DisposeFrameOrPluginSoon. Haraken's suggestion was > to rename a few different classes. If we all agree that we want to do that, I > can follow up with that in another CL, and I will also rename this method to the > appropriate new name such as DisposeLayoutControllerSoon. I think we should implement haraken's suggested names, as it provides a better hint of what this class hierarchy is used for. Separate CL is fine then I guess. https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/plugins/PluginView.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/plugins/PluginView.h:58: virtual bool IsPluginContainer() const { return false; } On 2017/04/18 00:42:41, joelhockey wrote: > On 2017/04/17 at 21:09:06, dcheng wrote: > > We may want to consider removing this as well (there is only one subclass of > PluginView now, as far as I can tell): we basically have same situation as > WebViewImpl here now: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp... > > +1. There is only a single implementation, so I agree there is no need for this > abstraction. I would then be inclined to rename web/WebPluginContainerImpl to > just web/WebPluginContainer (drop the Impl suffix), but I see that many classes > in web have Impl suffix. OK, please add a TODO since we're not doing it in this CL. https://codereview.chromium.org/2814643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): https://codereview.chromium.org/2814643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameOrPlugin.h:23: virtual void SetFrameRect(const IntRect& frameRect) = 0; Nit: frame_rect (or just remove the argument name completely here, since the purpose is implied by the name of the method: this is normally something that the presubmit would notice and enforce; not sure why it isn't catching this)
https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/plugins/PluginView.h (right): https://codereview.chromium.org/2814643003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/plugins/PluginView.h:58: virtual bool IsPluginContainer() const { return false; } On 2017/04/18 at 00:56:31, dcheng (OOO through May 2) wrote: > On 2017/04/18 00:42:41, joelhockey wrote: > > On 2017/04/17 at 21:09:06, dcheng wrote: > > > We may want to consider removing this as well (there is only one subclass of > > PluginView now, as far as I can tell): we basically have same situation as > > WebViewImpl here now: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp... > > > > +1. There is only a single implementation, so I agree there is no need for this > > abstraction. I would then be inclined to rename web/WebPluginContainerImpl to > > just web/WebPluginContainer (drop the Impl suffix), but I see that many classes > > in web have Impl suffix. > > OK, please add a TODO since we're not doing it in this CL. Done https://codereview.chromium.org/2814643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameOrPlugin.h (right): https://codereview.chromium.org/2814643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameOrPlugin.h:23: virtual void SetFrameRect(const IntRect& frameRect) = 0; On 2017/04/18 at 00:56:31, dcheng (OOO through May 2) wrote: > Nit: frame_rect (or just remove the argument name completely here, since the purpose is implied by the name of the method: this is normally something that the presubmit would notice and enforce; not sure why it isn't catching this) Done
The CQ bit was checked by joelhockey@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2814643003/#ps200001 (title: "Address final comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
haraken, could you give LGTM as OWNER in platform?
The CQ bit was checked by joelhockey@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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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...
LGTM
The CQ bit was checked by joelhockey@chromium.org
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": 200001, "attempt_start_ts": 1492490571782360,
"parent_rev": "c7466a84b6706007041fd3e5a348aa57ba550ba4", "commit_rev":
"e4904c3670d6a8d4f88642ca7e18ca2a9878a684"}
Message was sent while issue was closed.
Description was changed from ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. FrameView, RemoteFrameView and PluginView implement this class. FrameOrPlugin class has the minimal common methods required: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. The parent_, frame_rect_, and visible_ vars from FrameViewBase have now moved into WebPluginContainerImpl. Next step is to remove FrameView, RemoteFrameView, and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Remove FrameViewBase as base class of PluginView. Created new pure abstract class FrameOrPlugin to be used in places where polymorphism of frames and plugins is required. FrameView, RemoteFrameView and PluginView implement this class. FrameOrPlugin class has the minimal common methods required: SetFrameRect, FrameRect, Paint, Hide, Show, Dispose. The parent_, frame_rect_, and visible_ vars from FrameViewBase have now moved into WebPluginContainerImpl. Next step is to remove FrameView, RemoteFrameView, and Scrollbar from inheriting from FrameViewBase, and remove the FrameViewBase class. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2814643003 Cr-Commit-Position: refs/heads/master@{#465144} Committed: https://chromium.googlesource.com/chromium/src/+/e4904c3670d6a8d4f88642ca7e18... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/e4904c3670d6a8d4f88642ca7e18... |
