Bug 51330 - [Qt] Improve the readability of FrameLoaderClientQt::createPlugin()
Summary: [Qt] Improve the readability of FrameLoaderClientQt::createPlugin()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P5 Minor
Assignee: Benjamin Poulain
URL:
Keywords: Qt, QtTriaged
Depends on: 52290
Blocks: 51464
  Show dependency treegraph
 
Reported: 2010-12-20 06:31 PST by Benjamin Poulain
Modified: 2011-01-12 04:23 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.18 KB, patch)
2010-12-20 06:33 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (5.77 KB, patch)
2010-12-31 14:11 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (12.05 KB, patch)
2011-01-01 12:04 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (12.11 KB, patch)
2011-01-01 12:06 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (13.29 KB, patch)
2011-01-01 12:16 PST, Noam Rosenthal
kling: review-
Details | Formatted Diff | Diff
Patch (13.29 KB, patch)
2011-01-05 15:02 PST, Noam Rosenthal
kling: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2010-12-20 06:31:09 PST
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.
Comment 1 Benjamin Poulain 2010-12-20 06:33:52 PST
Created attachment 76995 [details]
Patch
Comment 2 Andreas Kling 2010-12-20 06:35:40 PST
Comment on attachment 76995 [details]
Patch

r=me
Comment 3 WebKit Commit Bot 2010-12-20 06:37:22 PST
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 4 Benjamin Poulain 2010-12-20 06:41:53 PST
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 5 WebKit Commit Bot 2010-12-20 06:43:20 PST
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
Comment 6 Benjamin Poulain 2010-12-20 06:58:43 PST
Committed r74344: <http://trac.webkit.org/changeset/74344>
Comment 7 Noam Rosenthal 2010-12-31 12:32:10 PST
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.
Comment 8 Noam Rosenthal 2010-12-31 14:11:52 PST
Created attachment 77732 [details]
Patch
Comment 9 Kenneth Rohde Christiansen 2010-12-31 15:31:02 PST
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
Comment 10 Noam Rosenthal 2010-12-31 15:36:36 PST
> 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 11 Andreas Kling 2010-12-31 17:44:12 PST
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.
Comment 12 Noam Rosenthal 2011-01-01 12:04:23 PST
Created attachment 77748 [details]
Patch
Comment 13 Noam Rosenthal 2011-01-01 12:06:47 PST
Created attachment 77750 [details]
Patch
Comment 14 WebKit Review Bot 2011-01-01 12:06:53 PST
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.
Comment 15 WebKit Review Bot 2011-01-01 12:09:07 PST
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.
Comment 16 Noam Rosenthal 2011-01-01 12:16:37 PST
Created attachment 77751 [details]
Patch
Comment 17 WebKit Review Bot 2011-01-01 12:20:06 PST
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.
Comment 18 Noam Rosenthal 2011-01-01 14:17:29 PST
(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.
Comment 19 Benjamin Poulain 2011-01-03 07:18:28 PST
I effectively broke that. Thanks Noam for noticing and fixing the bug.
Comment 20 Andreas Kling 2011-01-05 09:08:45 PST
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.
Comment 21 Noam Rosenthal 2011-01-05 15:02:01 PST
Created attachment 78051 [details]
Patch
Comment 22 WebKit Review Bot 2011-01-05 15:05:07 PST
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.
Comment 23 Early Warning System Bot 2011-01-05 15:15:05 PST
Attachment 78051 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7284399
Comment 24 Ademar Reis 2011-01-11 14:20:04 PST
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.
Comment 25 Noam Rosenthal 2011-01-11 14:40:32 PST
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.
Comment 26 Antonio Gomes 2011-01-11 14:42:53 PST
it is strange a but titled "improve readability" blocking a release ... :(
Comment 27 Noam Rosenthal 2011-01-11 14:44:47 PST
(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.
Comment 28 Ademar Reis 2011-01-11 14:49:02 PST
(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)
Comment 29 Ademar Reis 2011-01-11 14:52:45 PST
(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 30 Andreas Kling 2011-01-12 03:51:05 PST
Comment on attachment 78051 [details]
Patch

This does not even build on Qt, we'll need a new patch.
Comment 31 Benjamin Poulain 2011-01-12 04:10:16 PST
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.
Comment 32 Andreas Kling 2011-01-12 04:16:57 PST
I've scheduled a rollout of this patch (as bug 52290), let's do new stuff on new bugs. This frankentask needs to die.
Comment 33 Ademar Reis 2011-01-12 04:23:11 PST
(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.