[webkit-reviews] review denied: [Bug 65975] [Qt/WK2] Add initial support for viewport meta tag : [Attachment 103609] Patch 2

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Kenneth Rohde Christiansen
<kenneth at webkit.org>'s request for review:
Bug 65975: [Qt/WK2] Add initial support for viewport meta tag
https://bugs.webkit.org/show_bug.cgi?id=65975

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103609&action=review


All good for me appart from the comment.
I don't r+ because I would like to see the final version :-D

> Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:63
> +    WebPreferences* wkPrefs = wkPage->pageGroup()->preferences();
> +
> +    WebCore::ViewportAttributes attr =
WebCore::computeViewportAttributes(rawViewportData,
wkPrefs->layoutFallbackWidth(), wkPrefs->deviceWidth(),
wkPrefs->deviceHeight(), wkPrefs->deviceDPI(), availableSize);

Nice change, using wkPref

> Source/WebKit2/UIProcess/API/qt/qtouchwebview.h:51
> +    friend class QTouchWebViewPrivate;

That is pretty uncommon. Like very very very uncommon :)
Why do you need that?

> Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:52
> +	   QSize layoutSize;

I think that is too much info for the viewport. It would be better if the
viewport can be written without knowing that.

> Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:65
> +    WebCore::ViewportArguments rawViewportData;

I am not a fan of the name. pageViewportArguments?

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:152
> +    viewPrivate->rawViewportData = args;
> +    viewPrivate->updateViewportState();

I would prefer a ->setRawViewportData(args); and its implementation would call
updateViewportState(). That way you cannot forget to call updateViewportState.