[webkit-reviews] review denied: [Bug 30118] [Qt] QWebPage autotest crash: createViewlessPlugin : [Attachment 40861] patch v2

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied jedrzej.nowacki at nokia.com's
request for review:
Bug 30118: [Qt] QWebPage autotest crash: createViewlessPlugin
https://bugs.webkit.org/show_bug.cgi?id=30118

Attachment 40861: patch v2
https://bugs.webkit.org/attachment.cgi?id=40861&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
> diff --git a/WebKit/qt/ChangeLog b/WebKit/qt/ChangeLog
> index dbaa5a2..bd956ed 100644
> --- a/WebKit/qt/ChangeLog
> +++ b/WebKit/qt/ChangeLog
> @@ -1,3 +1,28 @@
> +2009-10-08  Jedrzej Nowacki	<jedrzej.nowacki at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   QWebPage's createViewlessPlugin autotest crash fix.
> +	   
> +	   A plug-ins returning widgets (QWidget or QGraphicsWidget) might be
> +	   created even in a viewless applications. The plug-ins won't be fully


I suggest to start the first sentence with "It is possible that plugins that
are QWidgets or QGraphicsWidgets
are created before a view has been assigned to a QWebPage".

> +	   functional, as by design, they should visualise something, but they
> +	   won't crash and will stay, work in memory.
> +	   
> +	   Autotest was developped to cover a viewless applications that create

> +	   a plug-ins based on the QGraphicsWidget class.

Typo: developed with one p. How about "An autotest is included that covers
this use-case." ? :)



> -class PluginTrackedPage : public QWebPage
> +class PluginTrackedPageWidget : public QWebPage
>  {
>  public:
>  
>      int count;
>      QPointer<QWidget> widget;
>  
> -    PluginTrackedPage(QWidget *parent = 0) : QWebPage(parent), count(0) {
> +    PluginTrackedPageWidget(QWidget *parent = 0) : QWebPage(parent),
count(0) {
>	  settings()->setAttribute(QWebSettings::PluginsEnabled, true);
>      }
>  
> @@ -640,9 +641,28 @@ public:
>      }
>  };
>  
> +class PluginTrackedPageGraphicsWidget : public QWebPage
> +{
> +public:
> +
> +    int count;
> +    QPointer<QGraphicsWidget> widget;
> +
> +    PluginTrackedPageGraphicsWidget(QWidget *parent = 0) : QWebPage(parent),
count(0) {
> +	  settings()->setAttribute(QWebSettings::PluginsEnabled, true);
> +    }

Coding style: In function definitions place each brace on its own line.

> +    virtual QObject* createPlugin(const QString&, const QUrl&, const
QStringList&, const QStringList&) {
> +	  count++;
> +	  QGraphicsWidget *w = new QGraphicsWidget;

Coding style: * placement

> +	  widget = w;
> +	  return w;

Why not simply write it in two lines? :)

    widget = new QGraphicsWidget;
    return widget;