Bug 82163 - Scrollable plugins not registered properly in ScrollingCoordinator
Summary: Scrollable plugins not registered properly in ScrollingCoordinator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
: 82164 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-03-25 19:36 PDT by Nat Duca
Modified: 2012-03-27 15:24 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.87 KB, patch)
2012-03-26 10:37 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (11.04 KB, patch)
2012-03-26 11:40 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (5.75 KB, patch)
2012-03-26 12:32 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (12.63 KB, patch)
2012-03-26 13:36 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2012-03-25 19:36:29 PDT
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/
Comment 1 James Robinson 2012-03-26 10:13:14 PDT
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.
Comment 2 James Robinson 2012-03-26 10:37:19 PDT
Created attachment 133844 [details]
Patch
Comment 3 James Robinson 2012-03-26 10:40:03 PDT
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.
Comment 4 James Robinson 2012-03-26 11:40:00 PDT
Created attachment 133854 [details]
Patch
Comment 5 Philippe Normand 2012-03-26 11:56:59 PDT
Comment on attachment 133854 [details]
Patch

Attachment 133854 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12134951
Comment 6 Build Bot 2012-03-26 12:07:07 PDT
Comment on attachment 133854 [details]
Patch

Attachment 133854 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12128978
Comment 7 Anders Carlsson 2012-03-26 12:11:51 PDT
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)
Comment 8 James Robinson 2012-03-26 12:24:15 PDT
(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).
Comment 9 Build Bot 2012-03-26 12:30:15 PDT
Comment on attachment 133854 [details]
Patch

Attachment 133854 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12140014
Comment 10 James Robinson 2012-03-26 12:32:36 PDT
Created attachment 133866 [details]
Patch
Comment 11 James Robinson 2012-03-26 12:33:21 PDT
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.
Comment 12 Anders Carlsson 2012-03-26 12:36:48 PDT
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.
Comment 13 James Robinson 2012-03-26 12:43:29 PDT
Committed r112142: <http://trac.webkit.org/changeset/112142>
Comment 14 James Robinson 2012-03-26 13:33:10 PDT
Reopening since this doesn't properly handle a plugin becoming scrollable and could be done in a more straightforward way.
Comment 15 James Robinson 2012-03-26 13:36:34 PDT
Created attachment 133880 [details]
Patch
Comment 16 James Robinson 2012-03-26 16:12:43 PDT
*** Bug 82164 has been marked as a duplicate of this bug. ***
Comment 17 WebKit Review Bot 2012-03-27 15:24:51 PDT
Comment on attachment 133880 [details]
Patch

Clearing flags on attachment: 133880

Committed r112325: <http://trac.webkit.org/changeset/112325>
Comment 18 WebKit Review Bot 2012-03-27 15:24:59 PDT
All reviewed patches have been landed.  Closing bug.