Move plugins to be stored in HTMLPlugInElement.

third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
index 9eb65ef0dbf4bb2bf62f5aafb4e2c845cc1624d4..c83f2002c7431079a3a2668c4822226ea6c2be4c 100644
--- a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
@@ -92,25 +92,56 @@ HTMLPlugInElement::~HTMLPlugInElement() {
 
 DEFINE_TRACE(HTMLPlugInElement) {
   visitor->trace(m_imageLoader);
+  visitor->trace(m_plugin);
   visitor->trace(m_persistedPlugin);
   HTMLFrameOwnerElement::trace(visitor);
 }
 
-// TODO(joelhockey): Move implementation of HTMLFrameOwnerElement
-// setWidget/releaseWidget/ownedWidget that relates to plugins to here and
-// remove inheritance from PluginView to FrameViewBase.
-void HTMLPlugInElement::setPlugin(PluginView* pluginView) {
-  setWidget(pluginView);
-}
+void HTMLPlugInElement::setPlugin(PluginView* plugin) {
+  if (plugin == m_plugin)
+    return;
 
-PluginView* HTMLPlugInElement::releasePlugin() {
-  FrameViewBase* plugin = releaseWidget();
-  return plugin && plugin->isPluginView() ? toPluginView(plugin) : nullptr;
+  // Remove and dispose the old plugin if we had one.
+  if (m_plugin) {

[dcheng, 2017/03/27 22:30:42]

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?

[joelhockey, 2017/03/28 22:56:23]

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.

+    document().view()->removePlugin(m_plugin);

[haraken, 2017/03/28 07:39:41]

Don't you need to do something like moveWidgetToParentSoon? i.e., take care of
s_updateSuspendCount.

[joelhockey, 2017/03/28 22:56:23]

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.

[haraken, 2017/03/29 11:04:46]

I'll defer this part to dcheng@, who should be more familiar with the subtlety.

[dcheng, 2017/03/30 04:43:07]

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.

[joelhockey, 2017/03/30 06:40:56]

Ack

[haraken, 2017/03/30 14:29:12]

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.

[joelhockey, 2017/03/31 01:03:55]

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.

+    disposeWidgetSoon(m_plugin);
+  }
+  m_plugin = plugin;
+
+  // TODO(joelhockey): I copied the rest of this method from
+  // HTMLFrameOwnerElement.  I don't really know what it does.
+  // Are the layoutPartItem.isNull check and DCHECKs required?
+  LayoutPart* layoutPart = toLayoutPart(layoutObject());
+  LayoutPartItem layoutPartItem = LayoutPartItem(layoutPart);
+  if (layoutPartItem.isNull())
+    return;
+
+  // Update layout and frame with new plugin.
+  if (m_plugin) {
+    layoutPartItem.updateOnWidgetChange();
+
+    DCHECK_EQ(document().view(), layoutPartItem.frameView());
+    DCHECK(layoutPartItem.frameView());
+    plugin->setParent(layoutPartItem.frameView());
+    document().view()->addPlugin(plugin);
+  }
+
+  // Apparently accessibility objects might have been modified if plugin
+  // was removed.
+  if (AXObjectCache* cache = document().existingAXObjectCache())
+    cache->childrenChanged(layoutPart);
 }
 
-PluginView* HTMLPlugInElement::ownedPlugin() const {
-  FrameViewBase* plugin = ownedWidget();
-  return plugin && plugin->isPluginView() ? toPluginView(plugin) : nullptr;
+PluginView* HTMLPlugInElement::releasePlugin() {
+  if (!m_plugin)
+    return nullptr;
+  document().view()->removePlugin(m_plugin);

[haraken, 2017/03/28 07:39:41]

Don't we need to do something like temporarilyRemoveWidgetFromParentSoon? i.e.,
take care of s_updateSuspendCount.

[joelhockey, 2017/03/28 22:56:23]

same as above

+  LayoutPart* layoutPart = toLayoutPart(layoutObject());
+  if (layoutPart) {
+    if (AXObjectCache* cache = document().existingAXObjectCache())
+      cache->childrenChanged(layoutPart);
+  }
+  return m_plugin.release();
 }
 
 void HTMLPlugInElement::setPersistedPlugin(PluginView* plugin) {
@@ -153,8 +184,7 @@ bool HTMLPlugInElement::requestObjectInternal(
 }
 
 bool HTMLPlugInElement::canProcessDrag() const {
-  return pluginWidget() && pluginWidget()->isPluginView() &&
-         toPluginView(pluginWidget())->canProcessDrag();
+  return pluginWidget() && pluginWidget()->canProcessDrag();
 }
 
 bool HTMLPlugInElement::canStartSelection() const {
@@ -170,9 +200,8 @@ bool HTMLPlugInElement::willRespondToMouseClickEvents() {
 
 void HTMLPlugInElement::removeAllEventListeners() {
   HTMLFrameOwnerElement::removeAllEventListeners();
-  if (LayoutPart* layoutObject = existingLayoutPart()) {
-    if (FrameViewBase* frameViewBase = layoutObject->frameViewBase())
-      frameViewBase->eventListenersRemoved();
+  if (m_plugin) {
+    m_plugin->eventListenersRemoved();
   }
 }
 
@@ -264,7 +293,7 @@ void HTMLPlugInElement::createPluginWithoutLayoutObject() {
 }
 
 bool HTMLPlugInElement::shouldAccelerate() const {
-  return ownedPlugin() && ownedPlugin()->platformLayer();
+  return m_plugin && m_plugin->platformLayer();
 }
 
 void HTMLPlugInElement::detachLayoutTree(const AttachContext& context) {
@@ -280,8 +309,7 @@ void HTMLPlugInElement::detachLayoutTree(const AttachContext& context) {
   }
 
   // Only try to persist a plugin we actually own.
-  PluginView* plugin = ownedPlugin();
-  if (plugin && context.performingReattach) {
+  if (m_plugin && context.performingReattach) {
     setPersistedPlugin(releasePlugin());
   } else {
     // Clear the plugin; will trigger disposal of it with Oilpan.
@@ -347,12 +375,20 @@ SharedPersistent<v8::Object>* HTMLPlugInElement::pluginWrapper() {
   return m_pluginWrapper.get();
 }
 
-FrameViewBase* HTMLPlugInElement::pluginWidget() const {
+// TODO(joelhockey): Remove this and use m_plugin.
+// Maybe just pluginWrapper function should check if m_plugin is null,
+// and call document().updateStyleAndLayoutIgnorePendingStylesheets as per
+// layoutPartForJSBindings.
+PluginView* HTMLPlugInElement::pluginWidget() const {
   if (LayoutPart* layoutPart = layoutPartForJSBindings())
-    return layoutPart->frameViewBase();
+    return layoutPart->plugin();
   return nullptr;
 }
 
+PluginView* HTMLPlugInElement::plugin() const {
+  return m_plugin.get();
+}
+
 bool HTMLPlugInElement::isPresentationAttribute(
     const QualifiedName& name) const {
   if (name == widthAttr || name == heightAttr || name == vspaceAttr ||
@@ -402,10 +438,9 @@ void HTMLPlugInElement::defaultEventHandler(Event* event) {
             .showsUnavailablePluginIndicator())
       return;
   }
-  FrameViewBase* frameViewBase = toLayoutPart(r)->frameViewBase();
-  if (!frameViewBase)
+  if (!m_plugin)
     return;
-  frameViewBase->handleEvent(event);
+  m_plugin->handleEvent(event);
   if (event->defaultHandled())
     return;
   HTMLFrameOwnerElement::defaultEventHandler(event);
@@ -424,8 +459,7 @@ bool HTMLPlugInElement::isKeyboardFocusable() const {
   if (HTMLFrameOwnerElement::isKeyboardFocusable())
     return true;
   return document().isActive() && pluginWidget() &&
-         pluginWidget()->isPluginView() &&
-         toPluginView(pluginWidget())->supportsKeyboardFocus();
+         pluginWidget()->supportsKeyboardFocus();
 }
 
 bool HTMLPlugInElement::hasCustomFocusLogic() const {