Bug 55242 – Plugins needs a way to trigger style recalc

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

[Alok Priyadarshi]

The plugins may dynamically switch between 2D and 3D rendering. Plugins need a way to inform the browser of such changes such that accelerated compositing can be switched on or off. More information in the chromium bug:

http://code.google.com/p/chromium/issues/detail?id=72784

[Alok Priyadarshi]

Created attachment 83854 [details]
Adds a hook to WebPluginContainer to mark the containing element that it needs style recalc.

[Darin Fisher (:fishd, Google)]

Comment on attachment 83854 [details]
Adds a hook to WebPluginContainer to mark the containing element that it needs style recalc.

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

> WebKit/chromium/public/WebPluginContainer.h:57
> +    virtual void setNeedsStyleRecalc() = 0;

It is really unfortunate to have to expose such a low-level detail to the embedder.
Can you explain why you need this?

Also, if we really need to expose this, then it seems like it should be exposed
on WebElement as that is the wrapper for WebCore::Element methods.

[Antoine Labour]

I don't think the method should be named as such.

Fundamentally, the WebPluginContainer asks the WebPlugin for its backing texture ID for compositing when it makes layout decisions. What needs to be exposed is a way for the WebPlugin to tell the WebPluginContainer that it has changed. The notion of style is internal to WebKit.

I would call it something like "changedBackingTextureID". Or maybe better yet, change the API to remove WebPlugin::getBackingTextureID and add WebPluginContainer::setBackingTextureID which would do the proper invalidation. Thoughts ?

[Alok Priyadarshi]

PPAPI plugins may dynamically switch between 2D (non-accelerated) and 3D rendering (accelerated). The only way to switch on/off accelerated compositing is to trigger a recalcStyle on the containing element, which in turn reevaluates whether accelerated compositing is required or not. WebPluginContainer::setNeedsStyleRecalc() seems similar to WebPluginContainer::invalidateRect(), which is used by the embedder to notify the containing element that something needs to be updated. May be there are better ways to do this. I am not too familiar with WebKit, so please suggest.

I am fine with adding this function to WebElement, but it seemed like a const-wrapper to WebCore::Element.

More details are provided in the chromium bug:
http://code.google.com/p/chromium/issues/detail?id=72784

[Darin Adler]

Comment on attachment 83854 [details]
Adds a hook to WebPluginContainer to mark the containing element that it needs style recalc.

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

>> WebKit/chromium/public/WebPluginContainer.h:57
>> +    virtual void setNeedsStyleRecalc() = 0;
> 
> It is really unfortunate to have to expose such a low-level detail to the embedder.
> Can you explain why you need this?
> 
> Also, if we really need to expose this, then it seems like it should be exposed
> on WebElement as that is the wrapper for WebCore::Element methods.

I don’t understand why a plug-in would have to ask for this explicitly. If the real issue is switching between 2D and 3D rendering, then the call should be about that. Not about “style recalc”, which is an internal detail of the web engine that could be subject to change.

Specifically what is going to change about the plug-in? Presumably something that WebKit queries through the plug-in API, so what? That’s what the call should be named after.

[Alok Priyadarshi]

Created attachment 84612 [details]
Added WebPluginContainer::setBackingTextureId as suggested by Antoine

I liked Antoine's suggestion of adding WebPluginContainer::setBackingTextureId(). I think it is much cleaner and at a higher level than my original solution. Please note that WebPlugin::getBackingTextureId() is now obsolete. I will remove it after this CL lands into chromium tree and I have made corresponding changes in chromium.

[WebKit Review Bot]

Attachment 84612 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/public/WebPluginContainer.h:61:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/WebPluginContainerImpl.h:87:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:321:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alok Priyadarshi]

Created attachment 84769 [details]
Fixed style issues

[Alok Priyadarshi]

Ping! I know Darin Fisher is on paternity leave. Who else can look at it?

[Dimitri Glazkov (Google)]

Comment on attachment 84769 [details]
Fixed style issues

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

ok, but please fix the ChangeLog.

> WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This is usually replaced with either a reference to new test, existing test that covers the change, or an explanation why the test is not necessary or possible.

[Alok Priyadarshi]

Created attachment 85060 [details]
Fixed ChangeLog

[Dimitri Glazkov (Google)]

Comment on attachment 85060 [details]
Fixed ChangeLog

activate Iconian gateway!

[WebKit Commit Bot]

Comment on attachment 85060 [details]
Fixed ChangeLog

Clearing flags on attachment: 85060

Committed r80584: <http://trac.webkit.org/changeset/80584>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.