Bug 54051 – plugins/invalidate_rect.html fails on linux ports

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

[Sailesh Agrawal]

I'm checking in a new layout test that fails for chromium-mac. See:
https://bugs.webkit.org/show_bug.cgi?id=53117

[Shinichiro Hamaji]

Created attachment 81789 [details]
Looks like linux and win are failing, too.

[Shinichiro Hamaji]

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...

[Shinichiro Hamaji]

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.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 81789 [details]:

http/tests/websocket/tests/httponly-cookie.pl bug 54097 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Kenneth Russell]

It looks like the Linux and Win expectations patch landed in http://trac.webkit.org/changeset/78048 .

[Sailesh Agrawal]

Created attachment 82061 [details]
Patch

[Sailesh Agrawal]

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

[James Robinson]

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

[Kenneth Russell]

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.

[WebKit Review Bot]

Attachment 82061 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7886479

[Sailesh Agrawal]

Created attachment 82067 [details]
Patch

[Sailesh Agrawal]

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.

[Kenneth Russell]

Comment on attachment 82067 [details]
Patch

Looks fine. Let's wait for it to pass the EWS bots before cq+'ing it.

[Build Bot]

Attachment 82061 [details] did not build on win:
Build output: http://queues.webkit.org/results/7869505

[WebKit Commit Bot]

Comment on attachment 82067 [details]
Patch

Clearing flags on attachment: 82067

Committed r78359: <http://trac.webkit.org/changeset/78359>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Robert Hogan]

Created attachment 91834 [details]
Patch

[Robert Hogan]

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

[Eric Seidel (no email)]

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).

[Robert Hogan]

Reopening for qt fix

[Girish Ramakrishnan]

(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.

[Girish Ramakrishnan]

 #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.

[Noam Rosenthal]

(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.

[Noam Rosenthal]

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.

[Robert Hogan]

Created attachment 93694 [details]
Patch

[Robert Hogan]

(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.

[Girish Ramakrishnan]

Looks good to me. Can some reviewer r+?

[Kenneth Rohde Christiansen]

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?

[Robert Hogan]

(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.

[Robert Hogan]

Committed r86706: <http://trac.webkit.org/changeset/86706>

[Ademar Reis]

Revision r86706 cherry-picked into qtwebkit-2.2 with commit 5866e6f <http://gitorious.org/webkit/qtwebkit/commit/5866e6f>