[webkit-reviews] review denied: [Bug 70141] [Qt][WK2] WebPreferences are impossible to use in QML. : [Attachment 111071] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Alexis Menard
<alexis.menard at openbossa.org>'s request for review:
Bug 70141: [Qt][WK2] WebPreferences are impossible to use in QML.
https://bugs.webkit.org/show_bug.cgi?id=70141

Attachment 111071: Patch
https://bugs.webkit.org/attachment.cgi?id=111071&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111071&action=review


Some nits

> Source/WebKit2/ChangeLog:8
> +	   Make possible to use qwkpreferences in QML. I renamed it

Make *it*. Don't use I. Renamed it to follow our new style regarding class
names.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:301
> +
> +

Double newline

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:60
> +    Q_PROPERTY(QWebPreferences* preferences READ preferences CONSTANT FINAL)


is preferences the right word to use in QML or should it be settings?

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:29
> +
> +

im not sure we use double newlines here

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:62
> +void
QWebPreferencesPrivate::setAttribute(QWebPreferencesPrivate::WebAttribute attr,
bool on)

I would have used "enable" and not "on".

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:94
> +
> +

double newline

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:153
> +
> +

again!

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:171
> +int QWebPreferencesPrivate::fontSize(QWebPreferencesPrivate::FontSize type)
const

unsigned? Can we have negative sizes?

Wha? FontSize type? :-) s/FontSize/FontSizeType/ ? Kind?

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:251
> +bool QWebPreferences::XSSAuditingEnabled() const

xssAuditing...

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:355
> +void QWebPreferences::setMinimumFontSize(int size)

In the future we need someting like this per column width (-webkit-text-adjust)


> Source/WebKit2/UIProcess/API/qt/qwebpreferences.h:21
> +#ifndef QWEBREFERENCES_H
> +#define QWEBPREFERENCES_H

Something is wrong here! Misses P!