Summary: | HTMLPlugInElement::isUserObservable() is causing layout | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | Plug-ins | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | barraclough, commit-queue, esprehn+autocc, gyuyoung.kim, kling, koivisto, rniwa | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Chris Dumez
2014-09-26 13:54:14 PDT
Created attachment 238738 [details]
Patch
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_. Created attachment 238743 [details]
Patch
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 =. Created attachment 238802 [details]
Patch
Created attachment 238803 [details]
Patch
Comment on attachment 238803 [details] Patch Clearing flags on attachment: 238803 Committed r174040: <http://trac.webkit.org/changeset/174040> All reviewed patches have been landed. Closing bug. |