Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2764313002:
Move plugins to be stored in HTMLPlugInElement. (Closed)
|
Created:
3 years, 9 months ago by joelhockey Modified:
3 years, 8 months ago CC:
blink-layers+watch_chromium.org, blink-reviews, blink-reviews-dom_chromium.org, 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, kenneth.christiansen, kinuko+watch, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, sof, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove plugins to be stored in HTMLPlugInElement.
Move plugins from being part of the widget tree in
HTMLFrameOwnerElement to being stored directly on the related
HTMLPlugInElement DOM node.
Plugins are also stored in FrameView as a list of plugin children
to be used in updateStyleAndLayoutIfNeededRecursiveInternal.
Now all calls to HTMLFrameOwnerElement::ownedWidget, or to
LayoutPart::frameViewBase (which calls onwedWidget) will no longer
return the plugin. Callers of these methods should call
HTMLPlugInElement::plugin or LayoutPart::plugin if they require
the plugin.
I have checked all callers. Many were obvious about whether they
expect/require FrameView, PluginView or Scrollbar. But I have left
comments in places where I am not sure.
The next step is to stop PluginView from inheriting from FrameViewBase
and duplicate any required methods into PluginView. This will complete
removing Plugins from the widget tree.
BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2764313002
Cr-Commit-Position: refs/heads/master@{#462243}
Committed: https://chromium.googlesource.com/chromium/src/+/d927e98fb228fb205e54152165e238432372df85
Patch Set 1 #
Total comments: 22
Patch Set 2 : Set parent for plugin #Patch Set 3 : Check for Plugin in Document::frameViewBaseForElement #Patch Set 4 : Fix PartPainter CHECK to !plugin rather than must be frame. #
Total comments: 17
Patch Set 5 : Fix places in LayoutPart where plugin and frame must be considered together. #Patch Set 6 : Add DCHECK and modify comments as per CL feedback. #Patch Set 7 : Remove unneeded TODOs. #Patch Set 8 : Remove invalid check #Patch Set 9 : Use plugin and frame in updateGeometries #
Total comments: 6
Patch Set 10 : Don't forget plugins when processing children in FrameView. #Patch Set 11 : Rebase and merge #
Total comments: 10
Patch Set 12 : Update comments about duplicating code #Patch Set 13 : Update comments about duplicating code #
Total comments: 6
Messages
Total messages: 75 (46 generated)
Description was changed from ========== Move plugins to be stored in HTMLPlugInElement. Move plugins from being part of the widget tree in HTMLFrameOwnerElement to being stored directly on the related HTMLPlugInElement DOM node. Plugins are also stored in FrameView as a list of plugin children to be used in updateStyleAndLayoutIfNeededRecursiveInternal. Now all calls to HTMLFrameOwnerElement::ownedWidget, or to LayoutPart::frameViewBase (which calls onwedWidget) will no longer return the plugin. Callers of these methods should call HTMLPlugInElement::plugin or LayoutPart::plugin if they require the plugin. BUG=637460 ========== to ========== Move plugins to be stored in HTMLPlugInElement. Move plugins from being part of the widget tree in HTMLFrameOwnerElement to being stored directly on the related HTMLPlugInElement DOM node. Plugins are also stored in FrameView as a list of plugin children to be used in updateStyleAndLayoutIfNeededRecursiveInternal. Now all calls to HTMLFrameOwnerElement::ownedWidget, or to LayoutPart::frameViewBase (which calls onwedWidget) will no longer return the plugin. Callers of these methods should call HTMLPlugInElement::plugin or LayoutPart::plugin if they require the plugin. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Move plugins to be stored in HTMLPlugInElement. Move plugins from being part of the widget tree in HTMLFrameOwnerElement to being stored directly on the related HTMLPlugInElement DOM node. Plugins are also stored in FrameView as a list of plugin children to be used in updateStyleAndLayoutIfNeededRecursiveInternal. Now all calls to HTMLFrameOwnerElement::ownedWidget, or to LayoutPart::frameViewBase (which calls onwedWidget) will no longer return the plugin. Callers of these methods should call HTMLPlugInElement::plugin or LayoutPart::plugin if they require the plugin. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move plugins to be stored in HTMLPlugInElement. Move plugins from being part of the widget tree in HTMLFrameOwnerElement to being stored directly on the related HTMLPlugInElement DOM node. Plugins are also stored in FrameView as a list of plugin children to be used in updateStyleAndLayoutIfNeededRecursiveInternal. Now all calls to HTMLFrameOwnerElement::ownedWidget, or to LayoutPart::frameViewBase (which calls onwedWidget) will no longer return the plugin. Callers of these methods should call HTMLPlugInElement::plugin or LayoutPart::plugin if they require the plugin. I have checked all callers. Many were obvious about whether they expect/require FrameView, PluginView or Scrollbar. But I have left comments in places where I am not sure. The next step is to stop PluginView from inheriting from FrameViewBase and duplicate any required methods into PluginView. This will complete removing Plugins from the widget tree. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
joelhockey@chromium.org changed reviewers: + dcheng@chromium.org, haraken@chromium.org
This is quite a big change to move Plugins to be stored on HTMLPlugInElement and have callers access the plugin via HTMLPlugInElement::plugin rather than HTMLFrameOwnerElement::frameViewBase. But it is the smallest atomic change that I could see to make. I have looked at all classes that handle FrameViewBase to see if they are expecting a FrameView or a Plugin. I have left comments in the places where I am unsure. These places might need the caller to process both FrameViews and Plugins.
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_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.h:1145: PluginsSet m_plugins; You don't need to address this in this CL but it looks weird that "FrameView" owns a set of "plugin"s. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/PluginDocument.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/PluginDocument.cpp:179: return m_pluginNode && isHTMLPlugInElement(m_pluginNode) Is it possible that m_pluginNode is not a plugin element? https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutEmbeddedObject.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutEmbeddedObject.cpp:139: // If so, need to access plugin differently to this. It shouldn't, since this is using frameView()->addPartToUpdate(), not frameViewBase()->addPartToUpdate(). https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutPart.cpp:258: // TODO(joelhockey): Does this need separate code for plugins? I guess yes. Can you add CHECK(!(node() && isHTMLPlugInElement(node()))) and check if your guess is correct? The same comment for other TODOs in this CL. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutPart.cpp:260: frameViewBase->hide(); One way to handle both the FrameViewBase case and the plugin case would be to implement the show/hide method on node() (i.e., HTMLFrameOwnerElement and HTMLPluginElement). Then HTMLFrameOwnerElement and HTMLPluginElement can dispatch those methods to proper things (i.e., m_frameViewBase or m_plugin).
https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:4047: // TODO(joelhockey): Do we need spcial handling for plugins? Unfortunately, I'm pretty sure the answer here is yes: things like browser plugin expect to be notified about focus changes. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:203: m_plugin.get()->eventListenersRemoved(); Minor nit: no .get() is needed here https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:381: if (LayoutPart* layoutPart = layoutPartForJSBindings()) This will synchronously trigger initialization of the plugin if script tries to access it, so it's important to keep =( https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutPart.cpp:313: // TODO(joelhockey): Does this need special handling for plugin? I think this also needs to be handled: the main subclass of PluginView is WebPluginContainerImpl, which forwards the visibility change to the plugin itself. =/
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by joelhockey@chromium.org
I think this CL is ready to go. I have noted a further few CLs that I can follow up with along with the major next change to remove inheritance from PluginView to FrameViewBase. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:4047: // TODO(joelhockey): Do we need spcial handling for plugins? On 2017/03/23 at 07:21:42, dcheng wrote: > Unfortunately, I'm pretty sure the answer here is yes: things like browser plugin expect to be notified about focus changes. I can confirm that plugin is expecting a call here. For now I have modified frameViewBaseForElement to return either PluginView or FrameView and use the FrameViewBase polymorphism to call setFocused. My plan once I remove FrameViewBase class is to create a Focusable ABC that FrameView and PluginView will both implement. Then frameViewBaseForElement will be renamed to getFocusableFromElement and return Focusable class. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.h:1145: PluginsSet m_plugins; On 2017/03/22 at 15:32:33, haraken wrote: > You don't need to address this in this CL but it looks weird that "FrameView" owns a set of "plugin"s. I'm curious what you think is weird. I don't really know the code well enough to get that sort of sense. It doesn't seem so strange to me that a frame can contain a set of other frames (ChildrenSet) and also a set of plugins (PluginsSet). The PluginSet is required in FrameView::updateStyleAndLayoutIfNeededRecursiveInternal to call PluginView::updateAllLifecyclePhases. If there is a better way to achieve this, I can definitely do it in another CL. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:203: m_plugin.get()->eventListenersRemoved(); On 2017/03/23 at 07:21:42, dcheng wrote: > Minor nit: no .get() is needed here Thanks. Do let me know when my code is not idiomatic. I'm still getting up to speed on C++. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:381: if (LayoutPart* layoutPart = layoutPartForJSBindings()) On 2017/03/23 at 07:21:42, dcheng wrote: > This will synchronously trigger initialization of the plugin if script tries to access it, so it's important to keep =( Ack. In another CL I might see if I can remove this method pluginWidget, and also layoutPartForJSBindings and put the document().updateStyleAndLayoutIgnorePendingStylesheets call into pluginWrapper which looks like the only entry point where JS calls to plugin. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/PluginDocument.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/PluginDocument.cpp:179: return m_pluginNode && isHTMLPlugInElement(m_pluginNode) On 2017/03/22 at 15:32:33, haraken wrote: > Is it possible that m_pluginNode is not a plugin element? No. m_pluginNode is always HTMLPlugInElement, or more specifically, it is HTMLEmbedElement (which extends HTMLPlugInElement). https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/Plug... Maybe in a followup CL, I can change type of m_pluginNode to HTMLPlugInElement. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutEmbeddedObject.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutEmbeddedObject.cpp:139: // If so, need to access plugin differently to this. On 2017/03/22 at 15:32:33, haraken wrote: > It shouldn't, since this is using frameView()->addPartToUpdate(), not frameViewBase()->addPartToUpdate(). Thanks. Yes, that makes sense. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutPart.cpp:258: // TODO(joelhockey): Does this need separate code for plugins? On 2017/03/22 at 15:32:33, haraken wrote: > I guess yes. > > Can you add CHECK(!(node() && isHTMLPlugInElement(node()))) and check if your guess is correct? The same comment for other TODOs in this CL. Thanks, that is a helpful approach. It showed that plugins do indeed get called from here. For now, I am setting FrameViewBase to PluginView if not null, else FrameView. Once I remove FrameViewBase, my plan is to have separate version of updateOnWidgetChange and updateGeometry: - updateOnFrameChange - updateFrameGeometry - updateOnPluginChange - updatePluginGeometry. It is clear to see where each of these are called separately. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutPart.cpp:260: frameViewBase->hide(); On 2017/03/22 at 15:32:33, haraken wrote: > One way to handle both the FrameViewBase case and the plugin case would be to implement the show/hide method on node() (i.e., HTMLFrameOwnerElement and HTMLPluginElement). Then HTMLFrameOwnerElement and HTMLPluginElement can dispatch those methods to proper things (i.e., m_frameViewBase or m_plugin). This function will be ok to take FrameViewBase as input and use polymorphism to call on either Frame or Plugin. I will create a common ABC with hide/show methods. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutPart.cpp:313: // TODO(joelhockey): Does this need special handling for plugin? On 2017/03/23 at 07:21:42, dcheng wrote: > I think this also needs to be handled: the main subclass of PluginView is WebPluginContainerImpl, which forwards the visibility change to the plugin itself. =/ Thanks. I just noticed how the impl forwards the change through to itself. Well worthy of a wry smily =/. As per comment above, for now I will get either PluginView or FrameView here, but my longer plan is to duplicate updateOnWidgetChange to have separate versions for Frame and Plugin.
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
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_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
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_...)
Also, I'm kind of curious: do you have a long-term vision of what you see this code looking like when it's de-Widgetized? I ask this, because while we've wanted to remove Widgets for a long time, we've also wanted to make Plugins/Frames less different as well (and try to consolidate any common code paths). It seems like that for removing Widget, we're adding more-plugin and more-frame specific paths to the code (which at face value, might be opposed to the goal of unifying frame/plugin architecture). https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:373: // Check if we have a plugin. Nit: I think this comment could be reworded to be a bit more helpful (as the check for plugin part is somewhat obvious from isPluginElement()). Alternatively, why can't LayoutPart::frameViewBase() just return the focusable ABC? https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:105: if (m_plugin) { I haven't looked at this part in detail, but I looked over the rest of the CL. Would we eventually be able to clean up more of HTMLFrameOwnerElement::setWidget()? Is that blocked on something else atm?
Mostly looks good. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.h:1145: PluginsSet m_plugins; On 2017/03/27 06:42:21, joelhockey wrote: > On 2017/03/22 at 15:32:33, haraken wrote: > > You don't need to address this in this CL but it looks weird that "FrameView" > owns a set of "plugin"s. > > I'm curious what you think is weird. I don't really know the code well enough > to get that sort of sense. > > It doesn't seem so strange to me that a frame can contain a set of other frames > (ChildrenSet) and also a set of plugins (PluginsSet). The PluginSet is required > in FrameView::updateStyleAndLayoutIfNeededRecursiveInternal to call > PluginView::updateAllLifecyclePhases. If there is a better way to achieve this, > I can definitely do it in another CL. Ah, thanks! That makes sense :) https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutPart.cpp:260: frameViewBase->hide(); On 2017/03/27 06:42:21, joelhockey wrote: > On 2017/03/22 at 15:32:33, haraken wrote: > > One way to handle both the FrameViewBase case and the plugin case would be to > implement the show/hide method on node() (i.e., HTMLFrameOwnerElement and > HTMLPluginElement). Then HTMLFrameOwnerElement and HTMLPluginElement can > dispatch those methods to proper things (i.e., m_frameViewBase or m_plugin). > > This function will be ok to take FrameViewBase as input and use polymorphism to > call on either Frame or Plugin. I will create a common ABC with hide/show > methods. However, you're planning to remove the inheritance between FrameViewBase and Plugin, right? At that point, you can no longer rely on the polymorphism. https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:3786: m_plugins.erase(plugin); DCHECK(m_plugins.contains(plugin)); https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:3790: m_plugins.insert(plugin); DCHECK(!m_plugins.contains(plugin)); https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: document().view()->removePlugin(m_plugin); Don't you need to do something like moveWidgetToParentSoon? i.e., take care of s_updateSuspendCount. https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:138: document().view()->removePlugin(m_plugin); Don't we need to do something like temporarilyRemoveWidgetFromParentSoon? i.e., take care of s_updateSuspendCount.
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...
On Tue, Mar 28, 2017 at 9:30 AM, <dcheng@chromium.org> wrote: > > Also, I'm kind of curious: do you have a long-term vision of what you see this > code looking like when it's de-Widgetized? > > I ask this, because while we've wanted to remove Widgets for a long time, we've > also wanted to make Plugins/Frames less different as well (and try to > consolidate any common code paths). It seems like that for removing Widget, > we're adding more-plugin and more-frame specific paths to the code (which at > face value, might be opposed to the goal of unifying frame/plugin architecture). > I don't mind admitting that I'm only learning this code as I'm changing it. But I have formed some ideas for what I'm hoping to get to quite soon (a few days or weeks). * frame, plugin and scrollbar do not share any implementation inheritance * frame and plugin (not sure yet about scrollbar) will share some interfaces (abstract base classes) but each will have independent implementations * Wherever we want to treat frame and plugin (mabye scrollbar) the same, we use the appropriate interface * Frame will be a member of HTMLFrameOwnerElement, Plugin will be a member of HTMLPlugInElement * maybe I can remove what I hear people call the 'widget tree'. This is the parent relationship in FrameView. I'm not sure about this yet. * Code should be clear to see which of frame, plugin, scrollbar is relevant in any section. It should either use the specific type for one of them, or an appropriate interface and use polymorphism but still be clear which of the types are possible. * Calls to widget->isFrameView, widget->isPlugin, widget-isScrollbar will be gone. I agree with your comments that we want to remove Widget, but we also want to have commonality between frames and plugins. I had made some comments in the code about duplicating some methods for frames and plugins, but I don't expect to do that now unless there is a clear difference between how frames and plugins are handled. -- 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.
On Tue, Mar 28, 2017 at 9:30 AM, <dcheng@chromium.org> wrote: > > Also, I'm kind of curious: do you have a long-term vision of what you see this > code looking like when it's de-Widgetized? > > I ask this, because while we've wanted to remove Widgets for a long time, we've > also wanted to make Plugins/Frames less different as well (and try to > consolidate any common code paths). It seems like that for removing Widget, > we're adding more-plugin and more-frame specific paths to the code (which at > face value, might be opposed to the goal of unifying frame/plugin architecture). > I don't mind admitting that I'm only learning this code as I'm changing it. But I have formed some ideas for what I'm hoping to get to quite soon (a few days or weeks). * frame, plugin and scrollbar do not share any implementation inheritance * frame and plugin (not sure yet about scrollbar) will share some interfaces (abstract base classes) but each will have independent implementations * Wherever we want to treat frame and plugin (mabye scrollbar) the same, we use the appropriate interface * Frame will be a member of HTMLFrameOwnerElement, Plugin will be a member of HTMLPlugInElement * maybe I can remove what I hear people call the 'widget tree'. This is the parent relationship in FrameView. I'm not sure about this yet. * Code should be clear to see which of frame, plugin, scrollbar is relevant in any section. It should either use the specific type for one of them, or an appropriate interface and use polymorphism but still be clear which of the types are possible. * Calls to widget->isFrameView, widget->isPlugin, widget-isScrollbar will be gone. I agree with your comments that we want to remove Widget, but we also want to have commonality between frames and plugins. I had made some comments in the code about duplicating some methods for frames and plugins, but I don't expect to do that now unless there is a clear difference between how frames and plugins are handled. -- 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.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/03/27 at 22:30:42, dcheng wrote: > Also, I'm kind of curious: do you have a long-term vision of what you see this code looking like when it's de-Widgetized? > > I ask this, because while we've wanted to remove Widgets for a long time, we've also wanted to make Plugins/Frames less different as well (and try to consolidate any common code paths). It seems like that for removing Widget, we're adding more-plugin and more-frame specific paths to the code (which at face value, might be opposed to the goal of unifying frame/plugin architecture). FWIW, when I see the "if plugin do something else" code all over RenderFrameImpl, RenderViewImpl, et al, I am worried about the specifics of the approach of unifying frame/plugin architecture. dcheng@, do you have any sketches on how this would look?
On 2017/03/28 16:01:54, dglazkov wrote: > On 2017/03/27 at 22:30:42, dcheng wrote: > > Also, I'm kind of curious: do you have a long-term vision of what you see this > code looking like when it's de-Widgetized? > > > > I ask this, because while we've wanted to remove Widgets for a long time, > we've also wanted to make Plugins/Frames less different as well (and try to > consolidate any common code paths). It seems like that for removing Widget, > we're adding more-plugin and more-frame specific paths to the code (which at > face value, might be opposed to the goal of unifying frame/plugin architecture). > > FWIW, when I see the "if plugin do something else" code all over > RenderFrameImpl, RenderViewImpl, et al, I am worried about the specifics of the > approach of unifying frame/plugin architecture. dcheng@, do you have any > sketches on how this would look? I don't have any sketches yet. In the past, we talked about having a PluginFrame, but I'm not entirely sure that's the best solution to this either. (We're hitting some of these issues elsewhere as well: we're working on migrating the legacy browser plugin architecture to use OOPIF, and there's an open question of the best way to handle the PDF plugin, which is actually a component extension...)
It turns out that in all of the places particularly in LayoutPart where I wasn't sure whether plugins needed to be considered as well as frames - they do. I have created LayoutPart::frameOrPlugin function for now which is used where the code must have one or the other. I will likely change this a little (change the name, change the return type) once I remove the FrameViewBase as parent class of PluginView. https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutPart.cpp:260: frameViewBase->hide(); On 2017/03/28 at 07:39:41, haraken wrote: > On 2017/03/27 06:42:21, joelhockey wrote: > > On 2017/03/22 at 15:32:33, haraken wrote: > > > One way to handle both the FrameViewBase case and the plugin case would be to > > implement the show/hide method on node() (i.e., HTMLFrameOwnerElement and > > HTMLPluginElement). Then HTMLFrameOwnerElement and HTMLPluginElement can > > dispatch those methods to proper things (i.e., m_frameViewBase or m_plugin). > > > > This function will be ok to take FrameViewBase as input and use polymorphism to > > call on either Frame or Plugin. I will create a common ABC with hide/show > > methods. > > However, you're planning to remove the inheritance between FrameViewBase and Plugin, right? At that point, you can no longer rely on the polymorphism. Yes, I will remove the inheritance from FrameView -> FrameViewBase and PluginView -> FrameViewBase. One of my current thoughts is to create a new common abstract base class named Focusable which has functions hide and show. FrameView and PluginView will both extend this. LayoutPart can then have a single way to get a handle on Focusable and use polymorphism to call hide/show on either Frame or Plugin. I do also like your suggestion to defer this method into Node and then have HTMLPlugInElement and HTMLFrameOwnerElement provide override implementations. I might try them both and see what I think looks the best design. There are also some other geometry methods that are common to both classes. I'm reluctant to put even more methods on Node, although maybe a few more can't hurt when its already got so many :-) https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:373: // Check if we have a plugin. On 2017/03/27 at 22:30:42, dcheng wrote: > Nit: I think this comment could be reworded to be a bit more helpful (as the check for plugin part is somewhat obvious from isPluginElement()). > > Alternatively, why can't LayoutPart::frameViewBase() just return the focusable ABC? Slightly changed wording. I didn't want this CL to get too big, but I will add Focusable (and other appropriate) ABC in next CL. This method will return Focusable type. https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:3786: m_plugins.erase(plugin); On 2017/03/28 at 07:39:41, haraken wrote: > DCHECK(m_plugins.contains(plugin)); Done https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:3790: m_plugins.insert(plugin); On 2017/03/28 at 07:39:41, haraken wrote: > DCHECK(!m_plugins.contains(plugin)); Done https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:105: if (m_plugin) { On 2017/03/27 at 22:30:42, dcheng wrote: > I haven't looked at this part in detail, but I looked over the rest of the CL. > > Would we eventually be able to clean up more of HTMLFrameOwnerElement::setWidget()? Is that blocked on something else atm? There is definitely a cleaner way that I can handle some of that function. Rather than 2 calls to moveToParentSoon with some funny inputs like nullptr, I can make calls to methods like add/remove. But from what I can tell, the code in HTMLFrameOwnerElement::setWidget is required. https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: document().view()->removePlugin(m_plugin); On 2017/03/28 at 07:39:41, haraken wrote: > Don't you need to do something like moveWidgetToParentSoon? i.e., take care of s_updateSuspendCount. You might be right, but I thought that it is only the call on dispose that must be careful to wait until after any suspends since it may trigger calls to JS. I think changing the frame/plugin parent/child references is fine to happen at any time since the purpose of this parent/child relationship is for FrameView::updateStyleAndLayoutIfNeededRecursiveInternal to call WebPluginContainerImpl::updateAllLifecyclePhases for all children plugins. My guess (and it is not very well informed, I admit) was that a failure to call updateAllLifecyclePhases on a plugin which is just about to be disposed will not be a problem. And likewise, calling it on a newly created plugin is not an issue. Please do correct me if this is wrong, or if you want me to delay the calls to document.view()-.[add|remove]Plugin to be outside the suspend, I can make that change. https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:138: document().view()->removePlugin(m_plugin); On 2017/03/28 at 07:39:41, haraken wrote: > Don't we need to do something like temporarilyRemoveWidgetFromParentSoon? i.e., take care of s_updateSuspendCount. same as above
https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2764313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutPart.cpp:260: frameViewBase->hide(); On 2017/03/28 22:56:23, joelhockey wrote: > On 2017/03/28 at 07:39:41, haraken wrote: > > On 2017/03/27 06:42:21, joelhockey wrote: > > > On 2017/03/22 at 15:32:33, haraken wrote: > > > > One way to handle both the FrameViewBase case and the plugin case would be > to > > > implement the show/hide method on node() (i.e., HTMLFrameOwnerElement and > > > HTMLPluginElement). Then HTMLFrameOwnerElement and HTMLPluginElement can > > > dispatch those methods to proper things (i.e., m_frameViewBase or m_plugin). > > > > > > This function will be ok to take FrameViewBase as input and use polymorphism > to > > > call on either Frame or Plugin. I will create a common ABC with hide/show > > > methods. > > > > However, you're planning to remove the inheritance between FrameViewBase and > Plugin, right? At that point, you can no longer rely on the polymorphism. > > Yes, I will remove the inheritance from FrameView -> FrameViewBase and > PluginView -> FrameViewBase. One of my current thoughts is to create a new > common abstract base class named Focusable which has functions hide and show. > FrameView and PluginView will both extend this. LayoutPart can then have a > single way to get a handle on Focusable and use polymorphism to call hide/show > on either Frame or Plugin. I do also like your suggestion to defer this method > into Node and then have HTMLPlugInElement and HTMLFrameOwnerElement provide > override implementations. I might try them both and see what I think looks the > best design. > > There are also some other geometry methods that are common to both classes. I'm > reluctant to put even more methods on Node, although maybe a few more can't hurt > when its already got so many :-) Sounds like a nice plan. https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: document().view()->removePlugin(m_plugin); On 2017/03/28 22:56:23, joelhockey wrote: > On 2017/03/28 at 07:39:41, haraken wrote: > > Don't you need to do something like moveWidgetToParentSoon? i.e., take care of > s_updateSuspendCount. > > You might be right, but I thought that it is only the call on dispose that must > be careful to wait until after any suspends since it may trigger calls to JS. > > I think changing the frame/plugin parent/child references is fine to happen at > any time since the purpose of this parent/child relationship is for > FrameView::updateStyleAndLayoutIfNeededRecursiveInternal to call > WebPluginContainerImpl::updateAllLifecyclePhases for all children plugins. My > guess (and it is not very well informed, I admit) was that a failure to call > updateAllLifecyclePhases on a plugin which is just about to be disposed will not > be a problem. And likewise, calling it on a newly created plugin is not an > issue. Please do correct me if this is wrong, or if you want me to delay the > calls to document.view()-.[add|remove]Plugin to be outside the suspend, I can > make that change. I'll defer this part to dcheng@, who should be more familiar with the subtlety.
sorry for the slow review, i've been feeling a bit under the weather. https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: document().view()->removePlugin(m_plugin); On 2017/03/29 11:04:46, haraken wrote: > On 2017/03/28 22:56:23, joelhockey wrote: > > On 2017/03/28 at 07:39:41, haraken wrote: > > > Don't you need to do something like moveWidgetToParentSoon? i.e., take care > of > > s_updateSuspendCount. > > > > You might be right, but I thought that it is only the call on dispose that > must > > be careful to wait until after any suspends since it may trigger calls to JS. > > > > I think changing the frame/plugin parent/child references is fine to happen at > > any time since the purpose of this parent/child relationship is for > > FrameView::updateStyleAndLayoutIfNeededRecursiveInternal to call > > WebPluginContainerImpl::updateAllLifecyclePhases for all children plugins. My > > guess (and it is not very well informed, I admit) was that a failure to call > > updateAllLifecyclePhases on a plugin which is just about to be disposed will > not > > be a problem. And likewise, calling it on a newly created plugin is not an > > issue. Please do correct me if this is wrong, or if you want me to delay the > > calls to document.view()-.[add|remove]Plugin to be outside the suspend, I can > > make that change. > > I'll defer this part to dcheng@, who should be more familiar with the subtlety. I'm willing to try this and see what happens--if it doesn't break anything, I agree that it would probably be simpler overall. https://codereview.chromium.org/2764313002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:101: if (plugin == m_plugin) One other concern is that because we don't use HTMLFrameOwnerElement::setWidget() anymore, we don't add this to |m_children|. I wonder if this is a problem for visibility notifications -- for example, see FrameView::setParentVisible(). https://codereview.chromium.org/2764313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:381: // layoutPartForJSBindings. The difference between these is pretty subtle. Is it necessary to expose both of these? I think this makes it easy to use the wrong one. I think a few things want to synchronously force plugin init using this, but it's not really clear right now which one should be used when. https://codereview.chromium.org/2764313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:443: m_plugin->handleEvent(event); Hm... if this is an <object> element with a subframe loaded, will we still forward events to it? Is that handled elsewhere?
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
Thanks for the reviews haraken and dcheng. Latest code updates are to make sure that plugins set FrameView as parent correctly and for FrameView to include plugins when processing m_children. https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: document().view()->removePlugin(m_plugin); > I'm willing to try this and see what happens--if it doesn't break anything, I agree that it would probably be simpler overall. Ack https://codereview.chromium.org/2764313002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:101: if (plugin == m_plugin) On 2017/03/30 at 04:43:07, dcheng wrote: > One other concern is that because we don't use HTMLFrameOwnerElement::setWidget() anymore, we don't add this to |m_children|. I wonder if this is a problem for visibility notifications -- for example, see FrameView::setParentVisible(). Yes, I've been trying to figure out a failing test for the last 2 days that was exactly about this issue. I need to remember to call out to plugins when FrameView is doing things to m_children. In my case it was frameRectsChanged. So I've also included plugins in the other calls to setParentVisible. https://codereview.chromium.org/2764313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:381: // layoutPartForJSBindings. On 2017/03/30 at 04:43:07, dcheng wrote: > The difference between these is pretty subtle. Is it necessary to expose both of these? I think this makes it easy to use the wrong one. > > I think a few things want to synchronously force plugin init using this, but it's not really clear right now which one should be used when. I will look at this in a follow up CL. If I can't remove one of them, I will try and name them more clearly. https://codereview.chromium.org/2764313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:443: m_plugin->handleEvent(event); On 2017/03/30 at 04:43:07, dcheng wrote: > Hm... if this is an <object> element with a subframe loaded, will we still forward events to it? Is that handled elsewhere? I'm not sure if I really understand you, but I'm pretty sure that I don't know the answer ;-) Do you mean a frame nested inside object? <object><frame>...</frame></object> Would that only load the frame if the plugin doesn't load, and the document falls back to the inner tags?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: document().view()->removePlugin(m_plugin); On 2017/03/30 06:40:56, joelhockey wrote: > > I'm willing to try this and see what happens--if it doesn't break anything, I > agree that it would probably be simpler overall. > > Ack If this breaks something, what will break? Actually I don't fully understand what the moveWidgetToParentSoon is doing. i.e., why we had to introduce s_updateSuspendCount.
https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: document().view()->removePlugin(m_plugin); > If this breaks something, what will break? > > Actually I don't fully understand what the moveWidgetToParentSoon is doing. i.e., why we had to introduce s_updateSuspendCount. I assume this question is for dcheng. I see some description from dcheng at https://codereview.chromium.org/1444183003 It looks like this is where the code was first added. https://chromium.googlesource.com/chromium/src/+/102e6f3cf3e6e849f2e7ef6ebecf... I'm going to check if any tests fail if I remove the suspend.
On 2017/03/31 01:03:55, joelhockey wrote: > https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): > > https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: > document().view()->removePlugin(m_plugin); > > If this breaks something, what will break? > > > > Actually I don't fully understand what the moveWidgetToParentSoon is doing. > i.e., why we had to introduce s_updateSuspendCount. > > I assume this question is for dcheng. > > I see some description from dcheng at https://codereview.chromium.org/1444183003 > > It looks like this is where the code was first added. > https://chromium.googlesource.com/chromium/src/+/102e6f3cf3e6e849f2e7ef6ebecf... > > I'm going to check if any tests fail if I remove the suspend. I haven't had a time to review the rest of the CL, but just to provide some background: - Unlike frames, plugins can be unloaded when they are hidden by display:none - However, it is unsafe to dispose the plugin in the middle of style update (because disposing Flash plugin can synchronously run script) - Similarly, it is unsafe to dispose plugins in the middle of DOM mutation (e.g. when removing child nodes from a ContainerNode) [1] The solution to this problem is UpdateSuspendScope. When there is any instance of this on the stack, any widget updates are deferred: once the last UpdateSuspendScope goes out of scope, we do all the parent-child updates / disposes that were deferred. I'm honestly not sure what will break if the widget is removed from the widget tree before calling dispose(), since this code has been in WebKit/Blink for a very long time. My assumption is it would show up as test failures (but not crashes)... and if nothing breaks, we may as well try it: having deferred widget tree updates is very confusing and leads to weird code like https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...
On 2017/03/31 01:16:43, dcheng wrote: > On 2017/03/31 01:03:55, joelhockey wrote: > > > https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): > > > > > https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: > > document().view()->removePlugin(m_plugin); > > > If this breaks something, what will break? > > > > > > Actually I don't fully understand what the moveWidgetToParentSoon is doing. > > i.e., why we had to introduce s_updateSuspendCount. > > > > I assume this question is for dcheng. > > > > I see some description from dcheng at > https://codereview.chromium.org/1444183003 > > > > It looks like this is where the code was first added. > > > https://chromium.googlesource.com/chromium/src/+/102e6f3cf3e6e849f2e7ef6ebecf... > > > > I'm going to check if any tests fail if I remove the suspend. > > I haven't had a time to review the rest of the CL, but just to provide some > background: > - Unlike frames, plugins can be unloaded when they are hidden by display:none > - However, it is unsafe to dispose the plugin in the middle of style update > (because disposing Flash plugin can synchronously run script) > - Similarly, it is unsafe to dispose plugins in the middle of DOM mutation (e.g. > when removing child nodes from a ContainerNode) [1] > > The solution to this problem is UpdateSuspendScope. When there is any instance > of this on the stack, any widget updates are deferred: once the last > UpdateSuspendScope goes out of scope, we do all the parent-child updates / > disposes that were deferred. Makes sense. Would there be any option to unconditionally defer disposing frames / plugins to a separate task (for simplicity)?
On 2017/03/31 01:43:05, haraken wrote: > On 2017/03/31 01:16:43, dcheng wrote: > > On 2017/03/31 01:03:55, joelhockey wrote: > > > > > > https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): > > > > > > > > > https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: > > > document().view()->removePlugin(m_plugin); > > > > If this breaks something, what will break? > > > > > > > > Actually I don't fully understand what the moveWidgetToParentSoon is > doing. > > > i.e., why we had to introduce s_updateSuspendCount. > > > > > > I assume this question is for dcheng. > > > > > > I see some description from dcheng at > > https://codereview.chromium.org/1444183003 > > > > > > It looks like this is where the code was first added. > > > > > > https://chromium.googlesource.com/chromium/src/+/102e6f3cf3e6e849f2e7ef6ebecf... > > > > > > I'm going to check if any tests fail if I remove the suspend. > > > > I haven't had a time to review the rest of the CL, but just to provide some > > background: > > - Unlike frames, plugins can be unloaded when they are hidden by display:none > > - However, it is unsafe to dispose the plugin in the middle of style update > > (because disposing Flash plugin can synchronously run script) > > - Similarly, it is unsafe to dispose plugins in the middle of DOM mutation > (e.g. > > when removing child nodes from a ContainerNode) [1] > > > > The solution to this problem is UpdateSuspendScope. When there is any instance > > of this on the stack, any widget updates are deferred: once the last > > UpdateSuspendScope goes out of scope, we do all the parent-child updates / > > disposes that were deferred. > > Makes sense. Would there be any option to unconditionally defer disposing frames > / plugins to a separate task (for simplicity)? Yes, though with some complexity. See the plan proposed at https://bugs.chromium.org/p/chromium/issues/detail?id=395902#c5. In short, we cannot always unconditionally defer: when DOM nodes are being detached, we need to do so "early enough" or else we will break random script. This is why I changed detachChildren() to always use ChildFrameDisconnector: I want this to also handle disposing plugins with the right timing. For style updates, we should unconditionally defer, with some mechanism to flush it synchronously if needed (e.g. JS tries to access a plugin widget).
On 2017/03/31 02:02:26, dcheng wrote: > On 2017/03/31 01:43:05, haraken wrote: > > On 2017/03/31 01:16:43, dcheng wrote: > > > On 2017/03/31 01:03:55, joelhockey wrote: > > > > > > > > > > https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2764313002/diff/60001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:106: > > > > document().view()->removePlugin(m_plugin); > > > > > If this breaks something, what will break? > > > > > > > > > > Actually I don't fully understand what the moveWidgetToParentSoon is > > doing. > > > > i.e., why we had to introduce s_updateSuspendCount. > > > > > > > > I assume this question is for dcheng. > > > > > > > > I see some description from dcheng at > > > https://codereview.chromium.org/1444183003 > > > > > > > > It looks like this is where the code was first added. > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/102e6f3cf3e6e849f2e7ef6ebecf... > > > > > > > > I'm going to check if any tests fail if I remove the suspend. > > > > > > I haven't had a time to review the rest of the CL, but just to provide some > > > background: > > > - Unlike frames, plugins can be unloaded when they are hidden by > display:none > > > - However, it is unsafe to dispose the plugin in the middle of style update > > > (because disposing Flash plugin can synchronously run script) > > > - Similarly, it is unsafe to dispose plugins in the middle of DOM mutation > > (e.g. > > > when removing child nodes from a ContainerNode) [1] > > > > > > The solution to this problem is UpdateSuspendScope. When there is any > instance > > > of this on the stack, any widget updates are deferred: once the last > > > UpdateSuspendScope goes out of scope, we do all the parent-child updates / > > > disposes that were deferred. > > > > Makes sense. Would there be any option to unconditionally defer disposing > frames > > / plugins to a separate task (for simplicity)? > > Yes, though with some complexity. See the plan proposed at > https://bugs.chromium.org/p/chromium/issues/detail?id=395902#c5. > > In short, we cannot always unconditionally defer: when DOM nodes are being > detached, we need to do so "early enough" or else we will break random script. > This is why I changed detachChildren() to always use ChildFrameDisconnector: I > want this to also handle disposing plugins with the right timing. > > For style updates, we should unconditionally defer, with some mechanism to flush > it synchronously if needed (e.g. JS tries to access a plugin widget). Also see https://bugs.chromium.org/p/chromium/issues/detail?id=561683#c6 for some brief context on what we've already tried.
https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:3783: CHECK(child->parent() == this); Nit: DCHECK is the equivalent of ASSERT https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:113: // such as the layoutPartItem.isNull check and DCHECKs. I feel a bit uneasy about this code duplication. If it mostly ends up looking similar, can we consider merging them back together? (I think one of the reasons I'm not entirely comfortable with this change is one of our goals is get rid of widget trees, and in some ways, we're actually creating a parallel tree of plugins atm. But maybe that's OK because we're going to get rid of the original widget tree (m_children) eventually?) https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (right): https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.h:54: PluginView* plugin() const; Please add a TODO to clean this up (and note that pluginWidget() triggers sync creation of the plugin) Edit: I see the comment is in the .cpp file, let's move it to the header, since that's where people will probably see it. https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutPart.cpp:110: FrameViewBase* result = plugin(); Nit: I would personally invert the order here (as frames are more common than plugins) https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutPart.cpp:332: // make separate versions of this function for plugins and frames. For line 308 and here, I have a bit of concern about code duplication here: it's not clear to me how duplicating this code helps us in the long run. Is there a path to unduplicating it?
https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:3783: CHECK(child->parent() == this); On 2017/04/04 at 07:20:12, dcheng wrote: > Nit: DCHECK is the equivalent of ASSERT Done, also changed CHECK below in removePlugin to be DCHECK. https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:113: // such as the layoutPartItem.isNull check and DCHECKs. On 2017/04/04 at 07:20:12, dcheng wrote: > I feel a bit uneasy about this code duplication. If it mostly ends up looking similar, can we consider merging them back together? > > (I think one of the reasons I'm not entirely comfortable with this change is one of our goals is get rid of widget trees, and in some ways, we're actually creating a parallel tree of plugins atm. But maybe that's OK because we're going to get rid of the original widget tree (m_children) eventually?) My primary focus has bene to remove the common base class (Widget/FrameViewBase). The primary difference between this method and HTMLFrameOwnerElement::setWidget is using m_plugins rather than m_children. So I agree that I have created something parallel, but technically it is not a tree. It is a set of plugins that belong to a frame. Once I have removed FrameView::m_children, hopefully a lot of the duplication can go away. But I couldn't say right now exactly how it will all turn out. Again, I don't mind to abandon this CL and maybe I should focus first to remove m_children. However everything is all quite tangled and I think it is easier to remove m_children if I can first pull plugin and scrollbar out. https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (right): https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.h:54: PluginView* plugin() const; On 2017/04/04 at 07:20:12, dcheng wrote: > Please add a TODO to clean this up (and note that pluginWidget() triggers sync creation of the plugin) > > Edit: I see the comment is in the .cpp file, let's move it to the header, since that's where people will probably see it. Done. https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right): https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutPart.cpp:110: FrameViewBase* result = plugin(); On 2017/04/04 at 07:20:12, dcheng wrote: > Nit: I would personally invert the order here (as frames are more common than plugins) Good point. Done https://codereview.chromium.org/2764313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutPart.cpp:332: // make separate versions of this function for plugins and frames. On 2017/04/04 at 07:20:12, dcheng wrote: > For line 308 and here, I have a bit of concern about code duplication here: it's not clear to me how duplicating this code helps us in the long run. Is there a path to unduplicating it? I've deleted these comments. I expect right now to still maintain 1 or more common abstract base classes between FrameView and PluginView, so I will not duplicate these methods.
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: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:376: return toHTMLPlugInElement(focusedElement).plugin(); Btw, I would suggest just changing 381 to use pluginOrFrame() and remove this for now. If/when we get around to changing pluginOrFrame(), we'll have a natural way of addressing the issue here then as well
LGTM You're working on a very complex thing! Amazing :) https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:139: document().view()->removePlugin(m_plugin); Not related to this CL, do you know why we don't need to call disposeWidgetSoon(m_plugin) here? https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/PluginDocument.cpp (right): https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/PluginDocument.cpp:179: return m_pluginNode && isHTMLPlugInElement(m_pluginNode) Add TODO: Change the type of m_pluginNode to HTMLPluginElement and remove this check.
https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:376: return toHTMLPlugInElement(focusedElement).plugin(); On 2017/04/05 at 09:12:24, dcheng wrote: > Btw, I would suggest just changing 381 to use pluginOrFrame() and remove this for now. > > If/when we get around to changing pluginOrFrame(), we'll have a natural way of addressing the issue here then as well I'll stick with this for right now in the hope of landing this quickly. But I've found out from my investigations yesterday that only plugin is relevant here in the calls to setFocus, and this code will go away when I use the dispatchBlur/dispatchFocus directly on HTMLPlugInElement. https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:139: document().view()->removePlugin(m_plugin); On 2017/04/05 at 12:34:00, haraken wrote: > Not related to this CL, do you know why we don't need to call disposeWidgetSoon(m_plugin) here? You are right that we do not dispose plugin here. This method is called when we persist the plugin and to object is held in the plugin element. I can't say that I really know what that means and why we do it. This is on my radar to see if I can remove or simplify. Especially now that we are always holding the plugin on the element, I suspect persisting the plugin already happens by default. https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/PluginDocument.cpp (right): https://codereview.chromium.org/2764313002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/PluginDocument.cpp:179: return m_pluginNode && isHTMLPlugInElement(m_pluginNode) On 2017/04/05 at 12:34:00, haraken wrote: > Add TODO: Change the type of m_pluginNode to HTMLPluginElement and remove this check. Ack
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": 240001, "attempt_start_ts": 1491430236490430,
"parent_rev": "b6d12780972f3d9711de6d11c6dd6e0536020339", "commit_rev":
"d927e98fb228fb205e54152165e238432372df85"}
Message was sent while issue was closed.
Description was changed from ========== Move plugins to be stored in HTMLPlugInElement. Move plugins from being part of the widget tree in HTMLFrameOwnerElement to being stored directly on the related HTMLPlugInElement DOM node. Plugins are also stored in FrameView as a list of plugin children to be used in updateStyleAndLayoutIfNeededRecursiveInternal. Now all calls to HTMLFrameOwnerElement::ownedWidget, or to LayoutPart::frameViewBase (which calls onwedWidget) will no longer return the plugin. Callers of these methods should call HTMLPlugInElement::plugin or LayoutPart::plugin if they require the plugin. I have checked all callers. Many were obvious about whether they expect/require FrameView, PluginView or Scrollbar. But I have left comments in places where I am not sure. The next step is to stop PluginView from inheriting from FrameViewBase and duplicate any required methods into PluginView. This will complete removing Plugins from the widget tree. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move plugins to be stored in HTMLPlugInElement. Move plugins from being part of the widget tree in HTMLFrameOwnerElement to being stored directly on the related HTMLPlugInElement DOM node. Plugins are also stored in FrameView as a list of plugin children to be used in updateStyleAndLayoutIfNeededRecursiveInternal. Now all calls to HTMLFrameOwnerElement::ownedWidget, or to LayoutPart::frameViewBase (which calls onwedWidget) will no longer return the plugin. Callers of these methods should call HTMLPlugInElement::plugin or LayoutPart::plugin if they require the plugin. I have checked all callers. Many were obvious about whether they expect/require FrameView, PluginView or Scrollbar. But I have left comments in places where I am not sure. The next step is to stop PluginView from inheriting from FrameViewBase and duplicate any required methods into PluginView. This will complete removing Plugins from the widget tree. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2764313002 Cr-Commit-Position: refs/heads/master@{#462243} Committed: https://chromium.googlesource.com/chromium/src/+/d927e98fb228fb205e54152165e2... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/d927e98fb228fb205e54152165e2... |
