Bug 137156 - HTMLPlugInElement::isUserObservable() is causing layout
Summary: HTMLPlugInElement::isUserObservable() is causing layout
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
Depends on:
Reported: 2014-09-26 13:54 PDT by Chris Dumez
Modified: 2014-09-27 22:58 PDT (History)
7 users (show)

See Also:

Patch (11.71 KB, patch)
2014-09-26 14:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2014-09-26 15:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.91 KB, patch)
2014-09-27 21:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.91 KB, patch)
2014-09-27 22:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-09-26 13:54:14 PDT
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.
Comment 1 Chris Dumez 2014-09-26 14:34:36 PDT
Created attachment 238738 [details]
Comment 2 Ryosuke Niwa 2014-09-26 14:42:20 PDT
Comment on attachment 238738 [details]

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_.
Comment 3 Chris Dumez 2014-09-26 15:49:05 PDT
Created attachment 238743 [details]
Comment 4 Ryosuke Niwa 2014-09-27 13:24:03 PDT
Comment on attachment 238743 [details]

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 =.
Comment 5 Chris Dumez 2014-09-27 21:58:49 PDT
Created attachment 238802 [details]
Comment 6 Chris Dumez 2014-09-27 22:04:51 PDT
Created attachment 238803 [details]
Comment 7 WebKit Commit Bot 2014-09-27 22:57:54 PDT
Comment on attachment 238803 [details]

Clearing flags on attachment: 238803

Committed r174040: <http://trac.webkit.org/changeset/174040>
Comment 8 WebKit Commit Bot 2014-09-27 22:58:00 PDT
All reviewed patches have been landed.  Closing bug.