Bug 54138

Created attachment 81861 [details] gdb backtrace when running client_crash test app

Created attachment 81863 [details] Check webPageClient before referencing it

Comment on attachment 81863 [details] Check webPageClient before referencing it This should really have an autotest.

Created attachment 81911 [details] Check webPageClient before referencing it Add unit test

Comment on attachment 81911 [details] Check webPageClient before referencing it View in context: https://bugs.webkit.org/attachment.cgi?id=81911&action=review > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2482 > +#if defined(ENABLE_WEBGL) && ENABLE_WEBGL this looks odd... > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2483 > +// https://bugs.webkit.org/show_bug.cgi?id=54138 usually git blame and the changelog are enough for reference bugs like this.

(In reply to comment #5) > (From update of attachment 81911 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81911&action=review > > > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2482 > > +#if defined(ENABLE_WEBGL) && ENABLE_WEBGL > > this looks odd... I know the conventional way is: #if ENABLE(WEBGL) but the ENABLE macro isn't available in tst_qwebpage.cpp - not sure what header I need to get it. Should I just remove the ifdefs altogether?

(In reply to comment #6) > I know the conventional way is: > #if ENABLE(WEBGL) > but the ENABLE macro isn't available in tst_qwebpage.cpp - not sure what header I need to get it. I doubt ENABLE_WEBGL is available for the tests, this is WebKit internal. Maybe you should check if WebGL is available at runtime, like I did for geolocation?

Why is there no QWebPageClient set?

(In reply to comment #7) > I doubt ENABLE_WEBGL is available for the tests, this is WebKit internal. Well, the test builds and runs (when passing --3d-canvas to build-webkit) so it must be available. > Maybe you should check if WebGL is available at runtime, like I did for geolocation? OK, I'll do that.

(In reply to comment #9) > (In reply to comment #7) > > I doubt ENABLE_WEBGL is available for the tests, this is WebKit internal. > > Well, the test builds and runs (when passing --3d-canvas to build-webkit) so it must be available. That particular function runs? Ok.

(In reply to comment #8) > Why is there no QWebPageClient set? It looks like the client is set when you set the view (in QWebPage::setView in Source/WebKit/qt/Api/qwebpage.cpp) and we don't have a view in this case.

(In reply to comment #10) > (In reply to comment #9) > > Well, the test builds and runs (when passing --3d-canvas to build-webkit) so it must be available. > > That particular function runs? Ok. Yes: ./tst_qwebpage webGLScreenshotWithoutView ********* Start testing of tst_QWebPage ********* Config: Using QTest library 4.7.1, Qt 4.7.1 PASS : tst_QWebPage::initTestCase() PASS : tst_QWebPage::webGLScreenshotWithoutView() PASS : tst_QWebPage::cleanupTestCase() Totals: 3 passed, 0 failed, 0 skipped ********* Finished testing of tst_QWebPage ********* LEAK: 7 WebCoreNode LEAK: 291 Structure

Hi, what's the status of this? I have implemented couple of fixes to upstream that you have to take into account: - https://bugs.webkit.org/show_bug.cgi?id=56555 - https://bugs.webkit.org/show_bug.cgi?id=56339 I looked at your fix and these changes must be done: - In the test case enable QWebSettings::AcceleratedCompositing. We support (at least for the first releasable WebGL support) creation only when accelerated compositing is enabled. - Put the fix into getViewportGLWidget(). Either submit a revised fix *or* inform me if you don't want to do it. Thank you.

This is also now listed in https://trac.webkit.org/wiki/QtWebKitWebGL

Created attachment 86288 [details] Check webPageClient before referencing it Fix patch to take into account upstream changes. Enable QWebSettings::AcceleratedCompositingEnabled in testcase.

Looks good to me. Thanks.

Comment on attachment 86288 [details] Check webPageClient before referencing it View in context: https://bugs.webkit.org/attachment.cgi?id=86288&action=review The change make sense to me, I would just like better testing. > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2482 > +void tst_QWebPage::webGLScreenshotWithoutView() I would like this test, with and without AcceleratedCompositingEnabled. Just to have good coverage. > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2484 > + QWebPage* page = new QWebPage; You should allocate the QWebPage on the stack to avoid dealing with the memory manually. > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2492 > + ::waitForSignal(mainFrame, SIGNAL(loadFinished(bool)), 2000); Is loadFinished() called for setHtml? Otherwise the test would always spend 2 seconds waiting.

Created attachment 86462 [details] Check webPageClient before referencing it Changed testcase to: * Test with and without AcceleratedCompositing enabled. * Allocate QWebPage on stack. * Do not wait for loadFinished signal, it is not called for setHtml().

Comment on attachment 86462 [details] Check webPageClient before referencing it Clearing flags on attachment: 86462 Committed r81669: <http://trac.webkit.org/changeset/81669>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/81669 might have broken GTK Linux 32-bit Debug The following tests are not passing: inspector/debugger/debug-inlined-scripts.html