Bug 51330 – [Qt] Improve the readability of FrameLoaderClientQt::createPlugin()

• Component: Plug-ins
• Status: RESOLVED FIXED

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 76995 [details]
Patch

[Andreas Kling]

Comment on attachment 76995 [details]
Patch

r=me

[WebKit Commit Bot]

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  <[e-mail]>

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

[Benjamin Poulain]

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.

[WebKit Commit Bot]

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  <[e-mail]>

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

[Benjamin Poulain]

Committed r74344: <http://trac.webkit.org/changeset/74344>

[Noam Rosenthal]

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.

[Noam Rosenthal]

Created attachment 77732 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Noam Rosenthal]

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

[Andreas Kling]

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.

[Noam Rosenthal]

Created attachment 77748 [details]
Patch

[Noam Rosenthal]

Created attachment 77750 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Noam Rosenthal]

Created attachment 77751 [details]
Patch

[WebKit Review Bot]

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.

[Noam Rosenthal]

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

[Benjamin Poulain]

I effectively broke that. Thanks Noam for noticing and fixing the bug.

[Andreas Kling]

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.

[Noam Rosenthal]

Created attachment 78051 [details]
Patch

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 78051 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7284399

[Ademar Reis]

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.

[Noam Rosenthal]

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.

[Antonio Gomes]

it is strange a but titled "improve readability" blocking a release ... :(

[Noam Rosenthal]

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

[Ademar Reis]

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

[Ademar Reis]

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

[Andreas Kling]

Comment on attachment 78051 [details]
Patch

This does not even build on Qt, we'll need a new patch.

[Benjamin Poulain]

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.

[Andreas Kling]

I've scheduled a rollout of this patch (as bug 52290), let's do new stuff on new bugs. This frankentask needs to die.

[Ademar Reis]

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