One curly braket of FrameLoaderClientQt::createPlugin() is at a weird place, making the identation incorrect later and the object scope hard to define. This has been introduced in 6674d2a2919aefd4850c7a1b73f487c1483ec456.
Created attachment 76995 [details] Patch
Comment on attachment 76995 [details] Patch r=me
Comment on attachment 76995 [details] Patch Rejecting attachment 76995 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 76995]" exit_code: 2 Last 500 characters of output: /ChangeLog Failed to merge in the changes. Patch failed at 0001 2010-12-20 Yury Semikhatsky <yurys@chromium.org> When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 132. Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2 Full output: http://queues.webkit.org/results/7283067
Comment on attachment 76995 [details] Patch Looks like the commit bot has issues with changelogs recently. Trying again, I'll merge manually if that fails.
Comment on attachment 76995 [details] Patch Rejecting attachment 76995 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 76995]" exit_code: 2 Last 500 characters of output: /ChangeLog Failed to merge in the changes. Patch failed at 0001 2010-12-20 Yury Semikhatsky <yurys@chromium.org> When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 132. Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2 Full output: http://queues.webkit.org/results/7209069
Committed r74344: <http://trac.webkit.org/changeset/74344>
I think this patch needs to be reverted. It breaks usage of QWebPluginFactory with any mime-type other than application/x-qt-plugin or application/x-qt-styled-widget.
Created attachment 77732 [details] Patch
Comment on attachment 77732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77732&action=review > WebKit/qt/ChangeLog:10 > + Reverted part of the originally patch that broke the plugin functionality. > + The plugin factory should have a chance to create a plugin for any mime-type, not just for > + application/x-qt-plugin Could you do this as two patches? It is a bit confusing what is reverting and what is new functionality. And the original patch was quite simple. > WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1514 > + if (parentWidget) // don't reparent to nothing (i.e. keep whatever parent QWebPage::createPlugin() chose. If you do this we should fix the comments to start with capital letter
> Could you do this as two patches? It is a bit confusing what is reverting and what is new functionality. And the original patch was quite simple. This patch is as small as I can make it. It's actually super simple, it just moves the brackets to a different place. But since it changes indentations of several lines it looks big. > > > WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1514 > > + if (parentWidget) // don't reparent to nothing (i.e. keep whatever parent QWebPage::createPlugin() chose. > > If you do this we should fix the comments to start with capital letter And maybe remove the double-negative :)
Comment on attachment 77732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77732&action=review > WebKit/qt/ChangeLog:8 > + Reverted part of the originally patch that broke the plugin functionality. This needs a regression test.
Created attachment 77748 [details] Patch
Created attachment 77750 [details] Patch
Attachment 77748 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'WebKit/qt/tests/qwebpluginfactory/qwebpluginfactory.pro', u'WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp', u'WebKit/qt/tests/tests.pro']" exit_code: 1 WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:20: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 77750 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'WebKit/qt/tests/qwebpluginfactory/qwebpluginfactory.pro', u'WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp', u'WebKit/qt/tests/tests.pro']" exit_code: 1 WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:22: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 77751 [details] Patch
Attachment 77751 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'WebKit/qt/tests/qwebpluginfactory/qwebpluginfactory.pro', u'WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp', u'WebKit/qt/tests/tests.pro']" exit_code: 1 WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:22: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #17) > Attachment 77751 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'WebKit/qt/tests/qwebpluginfactory/qwebpluginfactory.pro', u'WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp', u'WebKit/qt/tests/tests.pro']" exit_code: 1 > WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:22: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > Total errors found: 1 in 5 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. This file should be exempt from the style check. I think a false positive about that was already posted.
I effectively broke that. Thanks Noam for noticing and fixing the bug.
Comment on attachment 77751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77751&action=review > WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:2 > + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies) Wrong year. ;) > WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:144 > + QVERIFY(plugins.size() == 1); Please use QCOMPARE() instead where it makes sense. The output will be a lot more readable when/if something starts failing.
Created attachment 78051 [details] Patch
Attachment 78051 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'WebKit/qt/tests/qwebpluginfactory/qwebpluginfactory.pro', u'WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp', u'WebKit/qt/tests/tests.pro']" exit_code: 1 WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:22: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 78051 [details] did not build on qt: Build output: http://queues.webkit.org/results/7284399
The fix for bug 51464 has been added to trunk and cherry-picked into QtWebKit-2.1 (and therefore is present in 2.2 as well) and according to a comment there, that fix needs this bug fixed in order to work properly, so I'm adding this one as a release blocker for 2.1.
It's only a blocker if the original patch got into 2.1, since the second patch reverts some of it due to a regression.
it is strange a but titled "improve readability" blocking a release ... :(
(In reply to comment #26) > it is strange a but titled "improve readability" blocking a release ... :( Sometimes a readability bug causes a regression :) Maybe I should have opened a second bug for the regression.
(In reply to comment #25) > It's only a blocker if the original patch got into 2.1, since the second patch reverts some of it due to a regression. Yep, the original patch (6674d2a2919aefd4850c7a1b73f487c1483ec456) is quite old (from August, 2008)
(In reply to comment #27) > (In reply to comment #26) > > it is strange a but titled "improve readability" blocking a release ... :( > > Sometimes a readability bug causes a regression :) Maybe I should have opened a second bug for the regression. I guess it would be easier to review as well, but since there's a commit already (see Comment #6 and Comment #7) I'm not sure it makes sense on trunk. Anyway, would you mind attaching the patch that fixes only the regression so that I can apply it to qtwebkit-2.1, avoiding the risk of this refactoring at this point?
Comment on attachment 78051 [details] Patch This does not even build on Qt, we'll need a new patch.
This is a patch changing code style, no feature, in trunk, landed the 20 of December. How can that be blocking a release? My original patch is _NOT_ in the 2.1 branch. The patch of #51464 is depending on this for _trunk_. I start to understand why this release is taking so long.
I've scheduled a rollout of this patch (as bug 52290), let's do new stuff on new bugs. This frankentask needs to die.
(In reply to comment #31) > This is a patch changing code style, no feature, in trunk, landed the 20 of December. How can that be blocking a release? > > My original patch is _NOT_ in the 2.1 branch. The patch of #51464 is depending on this for _trunk_. The bug description says the problem was introduced by 6674d2a2919aefd4850c7a1b73f487c1483ec456, which in my git tree is a commit from 2008. > > I start to understand why this release is taking so long. C'mon... You know better than me that these technicalities have nothing to do with the release schedule.