[webkit-reviews] review denied: [Bug 109422] [Qt] Add Page Visibility API support : [Attachment 208173] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (6)

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Benjamin Dupont
<bdupont at nds.com>'s request for review:
Bug 109422: [Qt] Add Page Visibility API support
https://bugs.webkit.org/show_bug.cgi?id=109422

Attachment 208173: New QWebPage API (with test): void
QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (6)
https://bugs.webkit.org/attachment.cgi?id=208173&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=208173&action=review


Looks great! Three small finishing-touches type of nitpicks. But otherwise I
love it. Small, compact but straight to the point.

> Source/WebKit/qt/WidgetApi/qwebpage.cpp:3152
> +/*!
> +    This method is used to change the page visibility state.

You can combine the docs for the setter and the getter into one snippet of
documentation for the entire property. See the use of the \property tag in
other places in the same file.

> Source/WebKit/qt/WidgetApi/qwebpage.cpp:3164
> +void QWebPage::setVisibilityState(QWebPage::VisibilityState state)

I think that you can remove the QWebPage:: prefix for the parameter

> Source/WebKit/qt/WidgetApi/qwebpage.h:220
> +    enum VisibilityState {

This new enum needs documentation that explains what the values mean :)