Bug 54051 - plugins/invalidate_rect.html fails on linux ports
Summary: plugins/invalidate_rect.html fails on linux ports
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Sailesh Agrawal
URL:
Keywords:
Depends on: 53117
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-08 17:38 PST by Sailesh Agrawal
Modified: 2011-07-13 14:58 PDT (History)
14 users (show)

See Also:


Attachments
Looks like linux and win are failing, too. (1.27 KB, patch)
2011-02-09 04:10 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch (6.77 KB, patch)
2011-02-10 15:31 PST, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch (6.13 KB, patch)
2011-02-10 16:00 PST, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch (13.25 KB, patch)
2011-05-01 12:44 PDT, Robert Hogan
noam: review-
Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2011-05-16 14:34 PDT, Robert Hogan
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sailesh Agrawal 2011-02-08 17:38:51 PST
I'm checking in a new layout test that fails for chromium-mac. See:
https://bugs.webkit.org/show_bug.cgi?id=53117
Comment 1 Shinichiro Hamaji 2011-02-09 04:10:05 PST
Created attachment 81789 [details]
Looks like linux and win are failing, too.
Comment 2 Shinichiro Hamaji 2011-02-09 04:10:56 PST
Comment on attachment 81789 [details]
Looks like linux and win are failing, too.

I'd like to use commit queue to land this unreviewed fix as currently my local repository is broken...
Comment 3 Shinichiro Hamaji 2011-02-09 05:57:21 PST
Comment on attachment 81789 [details]
Looks like linux and win are failing, too.

Clearing flags as I could land this by myself before cq lands this.
Comment 4 WebKit Commit Bot 2011-02-09 06:43:07 PST
The commit-queue encountered the following flaky tests while processing attachment 81789 [details]:

http/tests/websocket/tests/httponly-cookie.pl bug 54097 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 5 Kenneth Russell 2011-02-09 10:32:38 PST
It looks like the Linux and Win expectations patch landed in http://trac.webkit.org/changeset/78048 .
Comment 6 Sailesh Agrawal 2011-02-10 15:31:45 PST
Created attachment 82061 [details]
Patch
Comment 7 Sailesh Agrawal 2011-02-10 15:34:11 PST
This patch fixes the layout test on chromium Windows.

It should also fixes the test for Safari Windows. Unfortunately I have no way to tell if it does. I'm using Visual Studio 2008 and I can't build the Safari port of WebKit. Is there any way to tell for sure?

Thanks,
Sailesh
Comment 8 James Robinson 2011-02-10 15:43:41 PST
Comment on attachment 82061 [details]
Patch

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

Could someone with a working Safari/win build test this out?  They are somewhat difficult to set up (and require VS2005).

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:1000
> +    rect.left = (int)NPVARIANT_TO_DOUBLE(args[0]);
> +    rect.top = (int)NPVARIANT_TO_DOUBLE(args[1]);
> +    rect.right = (int)NPVARIANT_TO_DOUBLE(args[2]);
> +    rect.bottom = (int)NPVARIANT_TO_DOUBLE(args[3]);

we typically use static_cast<> for this rather than c-style casts

> LayoutTests/platform/chromium/test_expectations.txt:3260
> +BUGWK54051 MAc LINUX : plugins/invalidate_rect.html = FAIL

typo: MAC
Comment 9 Kenneth Russell 2011-02-10 15:45:21 PST
Comment on attachment 82061 [details]
Patch

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

Mostly looks good. A few minor issues. Looks like James caught all but one. Regarding passing on Safari Win, you could file a separate bug or submit a separate patch to enable the test there, we could commit it and see whether it passes on the bots.

>> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:1000
>> +    rect.left = (int)NPVARIANT_TO_DOUBLE(args[0]);
>> +    rect.top = (int)NPVARIANT_TO_DOUBLE(args[1]);
>> +    rect.right = (int)NPVARIANT_TO_DOUBLE(args[2]);
>> +    rect.bottom = (int)NPVARIANT_TO_DOUBLE(args[3]);
> 
> we typically use static_cast<> for this rather than c-style casts

Use static_cast<int>(...).

> Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:267
> +                windowed = (void*)(1);

Use static_cast<void*>(...).

>> LayoutTests/platform/chromium/test_expectations.txt:3260
>> +BUGWK54051 MAc LINUX : plugins/invalidate_rect.html = FAIL
> 
> typo: MAC

MAC, not MAc.
Comment 10 WebKit Review Bot 2011-02-10 15:48:44 PST
Attachment 82061 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7886479
Comment 11 Sailesh Agrawal 2011-02-10 16:00:00 PST
Created attachment 82067 [details]
Patch
Comment 12 Sailesh Agrawal 2011-02-10 16:02:17 PST
Addressed review comments.

> Regarding passing on Safari Win, you could file a separate bug or submit a separate patch to enable the test there, we could commit it and see whether it passes on the bots.

Sounds good. I dropped LayoutTests/platform/win/plugins/invalidate_rect-expected.txt from this patch. I'll send it out separately in bug 54122.
Comment 13 Kenneth Russell 2011-02-10 16:05:52 PST
Comment on attachment 82067 [details]
Patch

Looks fine. Let's wait for it to pass the EWS bots before cq+'ing it.
Comment 14 Build Bot 2011-02-10 16:14:12 PST
Attachment 82061 [details] did not build on win:
Build output: http://queues.webkit.org/results/7869505
Comment 15 WebKit Commit Bot 2011-02-11 12:41:43 PST
Comment on attachment 82067 [details]
Patch

Clearing flags on attachment: 82067

Committed r78359: <http://trac.webkit.org/changeset/78359>
Comment 16 WebKit Commit Bot 2011-02-11 12:41:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Robert Hogan 2011-05-01 12:44:18 PDT
Created attachment 91834 [details]
Patch
Comment 18 Robert Hogan 2011-05-01 12:51:26 PDT
Comment on attachment 91834 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/PageClientQt.h:101
>      virtual bool allowsAcceleratedCompositing() const { return false; }

Girish/Ariya,

Can you comment on this part? Am I right in thinking a platformPageClient() that is a PageClientQWidget() will always have a parent QWebView, rather than a QGraphicsWebView, and so will never propagate accelerated compositing events? And that a page client widget created from a QGraphicsWebView will always have a PageClientQGrapicsWidget ?

Testing drt with 'QT_DRT_WEBVIEW_MODE=graphics' to toggle the use of QGraphicsWebView and QWebView suggests I am right, but I would be happy if you could make sure you're happy with the change here and in PluginViewQt.cpp
Comment 19 Eric Seidel 2011-05-09 20:57:27 PDT
Comment on attachment 91834 [details]
Patch

Cleared review? from attachment 91834 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 20 Robert Hogan 2011-05-10 10:28:22 PDT
Reopening for qt fix
Comment 21 Girish Ramakrishnan 2011-05-10 23:35:55 PDT
(In reply to comment #18)
> (From update of attachment 91834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91834&action=review
> 
> > Source/WebKit/qt/WebCoreSupport/PageClientQt.h:101
> >      virtual bool allowsAcceleratedCompositing() const { return false; }
> 
> Girish/Ariya,
> 
> Can you comment on this part? Am I right in thinking a platformPageClient() that is a PageClientQWidget() will always have a parent QWebView, rather than a QGraphicsWebView, and so will never propagate accelerated compositing events? And that a page client widget created from a QGraphicsWebView will always have a PageClientQGrapicsWidget ?
> 

That is correct.
Comment 22 Girish Ramakrishnan 2011-05-11 00:03:30 PDT
 #if USE(ACCELERATED_COMPOSITING) && !USE(TEXTURE_MAPPER)
-        if (m_parentFrame->page()->chrome()->client()->allowsAcceleratedCompositing()
+        if (m_parentFrame->view()->hostWindow()->platformPageClient()->allowsAcceleratedCompositing()

The above two should really have the same effect, no? Isn't the actual problem that we have not reimplemented ChromeClientQt::allowsAcceleratedCompositing() to return platformPageClient()->allowsAcceleratedCompositing()?

-#if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER)
-    virtual bool allowsAcceleratedCompositing() const { return true; }
-#else
+    // Accelerated compositing is supported by QGraphicsWebView, so any QGraphicsWidgets
+    // implemented under a QWebPageClient will never receive the appropriate paint events
     virtual bool allowsAcceleratedCompositing() const { return false; }
-#endif

I think QWebView actually support AC when compiled with TEXTURE_MAPPER. If that is the case, the above change is not correct.
Comment 23 Noam Rosenthal 2011-05-12 12:57:19 PDT
(In reply to comment #22)
>  #if USE(ACCELERATED_COMPOSITING) && !USE(TEXTURE_MAPPER)
> -        if (m_parentFrame->page()->chrome()->client()->allowsAcceleratedCompositing()
> +        if (m_parentFrame->view()->hostWindow()->platformPageClient()->allowsAcceleratedCompositing()
> 
> The above two should really have the same effect, no? Isn't the actual problem that we have not reimplemented ChromeClientQt::allowsAcceleratedCompositing() to return platformPageClient()->allowsAcceleratedCompositing()?

Sounds right. That would be the right fix.
Comment 24 Noam Rosenthal 2011-05-12 12:59:00 PDT
Comment on attachment 91834 [details]
Patch

This would break texture-mapper. texture-mapper allows accelerated compositing with QWebView, you're essentially disabling it with no good reason.
Comment 25 Robert Hogan 2011-05-16 14:34:36 PDT
Created attachment 93694 [details]
Patch
Comment 26 Robert Hogan 2011-05-16 14:36:19 PDT
(In reply to comment #24)
> (From update of attachment 91834 [details])
> This would break texture-mapper. texture-mapper allows accelerated compositing with QWebView, you're essentially disabling it with no good reason.

Right enough, I was misreading the compile guards. (i.e. mistaking USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER) and USE(ACCELERATED_COMPOSITING) && !USE(TEXTURE_MAPPER) as the same).

Updated patch.
Comment 27 Girish Ramakrishnan 2011-05-16 23:46:52 PDT
Looks good to me. Can some reviewer r+?
Comment 28 Kenneth Rohde Christiansen 2011-05-17 01:47:30 PDT
Comment on attachment 93694 [details]
Patch

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

> Tools/DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp:108
> +                    assert(false);

why not the ASSERT macro?

> Tools/DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp:298
> +            if (obj->eventLogging)
> +                pluginLog(instance, "mouseUp at (%d, %d)", evt->xbutton.x, evt->xbutton.y);

What about putting that if inside the pluginLog? is that possible?
Comment 29 Robert Hogan 2011-05-17 11:06:06 PDT
(In reply to comment #28)
> (From update of attachment 93694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93694&action=review
> 
> > Tools/DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp:108
> > +                    assert(false);
> 
> why not the ASSERT macro?

Both of these are stock code from the windows test plugin. assert() seems to be the standard both there and in the unix plugin.
> 
> > Tools/DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp:298
> > +            if (obj->eventLogging)
> > +                pluginLog(instance, "mouseUp at (%d, %d)", evt->xbutton.x, evt->xbutton.y);
> 
> What about putting that if inside the pluginLog? is that possible?

Yes, I think so but not without some heavy refactoring - there's lots of common code that could be removed in both the unix and windows plugins.
Comment 30 Robert Hogan 2011-05-17 13:58:41 PDT
Committed r86706: <http://trac.webkit.org/changeset/86706>
Comment 31 Ademar Reis 2011-07-13 14:58:08 PDT
Revision r86706 cherry-picked into qtwebkit-2.2 with commit 5866e6f <http://gitorious.org/webkit/qtwebkit/commit/5866e6f>