Bug 65975 – [Qt/WK2] Add initial support for viewport meta tag

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 103465 [details]
Patch

[WebKit Review Bot]

Attachment 103465 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/qt/ViewInterface.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 103465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103465&action=review

> Source/WebKit2/ChangeLog:8
> +        Redirect the viewport arguments thru to the view.

You might want to be more informal than "thru"?

"Redirect da viewport stuff thru da view, yo." ;)

> Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:75
> +        d->viewInterface.updateViewportState();

If the viewport is handling the state, this call would not be done on the view interface but on this. I think that would be more right, the viewport handle...the viewport state :)

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:108
> +    // FIXME: Make these properties on the view?

You can remove that, we'll modify that when the time come.
I would rather have
// FIXME: adapt those value dynamically to the target device.

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:116
> +    viewport.layoutSize = QSize(attr.layoutSize);

Why does the viewport needs that information? Shouldn't it only rely on the item size?

> Source/WebKit2/UIProcess/qt/TouchViewInterface.h:68
> +    struct ViewportState {
> +        ViewportState()
> +            : layoutSize(QSize())
> +            , initialScale(1.0)
> +            , minimumScale(0.25)
> +            , maximumScale(1.8)
> +            , pixelRatio(1.0)
> +            , isUserScalable(true)
> +        { }
> +
> +        QSize layoutSize;
> +        qreal initialScale;
> +        qreal minimumScale;
> +        qreal maximumScale;
> +        qreal pixelRatio;
> +        bool isUserScalable;
> +    } viewport;
> +
> +    void updateViewportState();
> +

I wonder if the right place for this is not QTouchWebView.
The viewport is the class responsible for enforcing this state.

[Kenneth Rohde Christiansen]

Created attachment 103609 [details]
Patch 2

[Benjamin Poulain]

Comment on attachment 103609 [details]
Patch 2

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.

[Kenneth Rohde Christiansen]

Created attachment 103616 [details]
Patch 3

[Benjamin Poulain]

Comment on attachment 103616 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=103616&action=review

Simple, clean...awesome :)

> Source/WebKit2/UIProcess/API/qt/qtouchwebview_p.h:63
> +
> +    WebCore::ViewportArguments viewportArguments;

Could be private I guess but that does not help much.

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:28
> +#include <WebCore/ViewportArguments.h>

I think you can get away with forward declaration.

> Source/WebKit2/UIProcess/qt/TouchViewInterface.h:25
> +#include <WebCore/ViewportArguments.h>

You have a forward declaration in ViewInterface.

[Kenneth Rohde Christiansen]

Created attachment 103618 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 103618 [details]
Patch for landing

Clearing flags on attachment: 103618

Committed r92851: <http://trac.webkit.org/changeset/92851>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.