Bug 137156 – HTMLPlugInElement::isUserObservable() is causing layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Chris Dumez]

While profiling the page load of nytimes.com, I noticed that we were spending ~4-5% of cpu time in HTMLPlugInElement::isUserObservable(), the reason being that the function calls pluginWidget(), which causes a layout update in HTMLObjectElement::renderWidgetForJSBindings().
HTMLPlugInElement::isUserObservable() shouldn't need to do a layout.

[Chris Dumez]

Created attachment 238738 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 238738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238738&action=review

> Source/WebCore/html/HTMLAppletElement.cpp:86
> -RenderWidget* HTMLAppletElement::renderWidgetForJSBindings() const
> +RenderWidget* HTMLAppletElement::renderWidgetForJSBindings(LayoutPolicy shouldUpdateLayout) const

I don't think shouldUpdateLayout is a good argument name in that it doesn't tell us why we would like to update the layout in the first place.
What's misleading is that this function is called renderWidgetForJSBindings.
If we're obtaining the render widget for JS bindings, we definitely need to update the layout per comment below.

> Source/WebCore/html/HTMLPlugInElement.cpp:169
> -    RenderWidget* renderWidget = renderWidgetForJSBindings();
> +    RenderWidget* renderWidget = renderWidgetForJSBindings(shouldUpdateLayout);

It's weird that this function always calls renderWidgetForJSBindings even if it's not for JS bindings.
We should either rename the function or add a new function that's used for other purposes than JS bindings.

> Source/WebCore/html/HTMLPlugInElement.h:55
> +    enum LayoutPolicy { LayoutPolicyDoNotUpdate, LayoutPolicyUpdate };
> +    WEBCORE_EXPORT Widget* pluginWidget(LayoutPolicy = LayoutPolicyUpdate) const;

It's not clear to someone reading this code why anyone would like to update the layout in order to get Widget*
The enum value names should signify _why_.

[Chris Dumez]

Created attachment 238743 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 238743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238743&action=review

> Source/WebCore/html/HTMLPlugInElement.h:54
> +    enum PluginLoadingPolicy  { PluginLoadingPolicyDoNotLoad, PluginLoadingPolicyLoad };

I don't think there is a value in prefixing the enum value with PluginLoadingPolicy.
If anything, use enum class instead.

> Source/WebCore/html/HTMLPlugInElement.h:55
> +    WEBCORE_EXPORT Widget* pluginWidget(PluginLoadingPolicy  = PluginLoadingPolicyLoad) const;

Nit: Two spaces before =.

[Chris Dumez]

Created attachment 238802 [details]
Patch

[Chris Dumez]

Created attachment 238803 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 238803 [details]
Patch

Clearing flags on attachment: 238803

Committed r174040: <http://trac.webkit.org/changeset/174040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.