Bug 82163 – Scrollable plugins not registered properly in ScrollingCoordinator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nat Duca]

With threadeded compositing on, we process mousewheels on the impl thread. So, mouse scrolling never reaches the PDF plugin. ~cries~

To reproduce
chrome --force-compositing-mode --enable-threaded-compositing --show-composited-layer-borders --register-pepper-plugins="/opt/google/chrome/libpdf.so;application/pdf"

Then go to here:
http://www.corp.google.com/~pavanv/no_crawl/pdf-samples/flatform.pdf

If you dont have these applied, the screen wont update (although the scrolls will behave incorrectly):
https://bugs.webkit.org/show_bug.cgi?id=82154
http://codereview.chromium.org/9856005/

[James Robinson]

Plugins are PluginViewBases, which are Widgets and thus scroll, but they aren't WebCore::ScrollableAreas so the computeNonFastScrollableRegion() test in ScrollingCoordinator ignores them.

The fix is to check for scrollable plugins when iterating over Widgets.

[James Robinson]

Created attachment 133844 [details]
Patch

[James Robinson]

This appears to work - any ideas on how to test? I was thinking of adding a DRT hook to dump the nonFastScrollableRegion as text on somewhere.

[James Robinson]

Created attachment 133854 [details]
Patch

[Philippe Normand]

Comment on attachment 133854 [details]
Patch

Attachment 133854 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12134951

[Build Bot]

Comment on attachment 133854 [details]
Patch

Attachment 133854 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12128978

[Anders Carlsson]

Comment on attachment 133844 [details]
Patch

I don't really like the idea of making all plug-ins be non-scrollable. Can we add a member function to PluginViewBase to let subclasses control this instead? (I'm really tempted to not send wheel events to some plug-ins that we know never handle them correctly)

[James Robinson]

(In reply to comment #7)
> (From update of attachment 133844 [details])
> I don't really like the idea of making all plug-ins be non-scrollable. Can we add a member function to PluginViewBase to let subclasses control this instead? (I'm really tempted to not send wheel events to some plug-ins that we know never handle them correctly)

I can do that, but then it means I can't make a layout test (since I don't know how to instantiate a plugin that will hook up to the correct subclass).

[Build Bot]

Comment on attachment 133854 [details]
Patch

Attachment 133854 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12140014

[James Robinson]

Created attachment 133866 [details]
Patch

[James Robinson]

How about this? Other PluginViewBase subclasses would have to register themselves in the proper way, but I'm not familiar with how they all work.

[Anders Carlsson]

Comment on attachment 133866 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:142
> +Region ScrollingCoordinator::nonFastScrollableRegion()
> +{
> +    return computeNonFastScrollableRegion(m_page->mainFrame()->view());
> +}

This change seems unrelated.

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:118
> +    Region nonFastScrollableRegion();

Ditto.

> Source/WebCore/plugins/PluginViewBase.h:56
> +    virtual bool scrollable() { return false; }

I think this should be called wantsWheelEvents instead, since a plug-in may want wheel events without being scrollable.

[James Robinson]

Committed r112142: <http://trac.webkit.org/changeset/112142>

[James Robinson]

Reopening since this doesn't properly handle a plugin becoming scrollable and could be done in a more straightforward way.

[James Robinson]

Created attachment 133880 [details]
Patch

[James Robinson]

*** Bug 82164 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 133880 [details]
Patch

Clearing flags on attachment: 133880

Committed r112325: <http://trac.webkit.org/changeset/112325>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.