Bug 54051

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

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

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.

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

Created attachment 82061 [details] Patch

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

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

Created attachment 82067 [details] Patch

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 on attachment 82067 [details] Patch Looks fine. Let's wait for it to pass the EWS bots before cq+'ing it.

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

Comment on attachment 82067 [details] Patch Clearing flags on attachment: 82067 Committed r78359: <http://trac.webkit.org/changeset/78359>

All reviewed patches have been landed. Closing bug.

Created attachment 91834 [details] Patch

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

Reopening for qt fix

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

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

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

Created attachment 93694 [details] Patch

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

Looks good to me. Can some reviewer r+?

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?

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

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

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