Bug 72893 – [Qt][WK2] Add (experimental) viewport attributes view to MiniBrowser/qt

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Michael Brüning]

It has been requested to be able to view the viewport attributes in the Qt version of MiniBrowser. 

This should be implemented using the experimental extensions of QQuickWebView.

[Michael Brüning]

Created attachment 116191 [details]
Proposed patch

[Michael Brüning]

Adding Tor Arne as he has been working with the experimental extensions.

[WebKit Review Bot]

Comment on attachment 116191 [details]
Proposed patch

Attachment 116191 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10569048

New failing tests:
css2.1/20110323/abspos-containing-block-initial-004b.htm
fast/repaint/scroll-fixed-reflected-layer.html
fast/repaint/fixed-scale.html
fast/replaced/width100percent-textarea.html
http/tests/inspector/network-preflight-options.html
fast/repaint/fixed-tranformed.html
css2.1/20110323/abspos-containing-block-initial-004d.htm
fast/repaint/fixed-table-overflow-zindex.html

[Tor Arne Vestbø]

Comment on attachment 116191 [details]
Proposed patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:168
> +    Q_PROPERTY(QWebViewportAttributes* attributes READ attributes CONSTANT FINAL)

I think viewportAttributes is better, since attributes is a bit too generic.

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:60
> +        viewportAttributesItem.initialScale = webView.experimental.attributes.initialScale
> +        viewportAttributesItem.minimumScale = webView.experimental.attributes.minimumScale
> +        viewportAttributesItem.maximumScale = webView.experimental.attributes.maximumScale
> +
> +        viewportAttributesItem.isScalable = webView.experimental.attributes.isScalable
> +        viewportAttributesItem.layoutWidth = webView.experimental.attributes.layoutWidth
> +        viewportAttributesItem.layoutHeight = webView.experimental.attributes.layoutHeight
> +    }

Why are you not using bindings for these?

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:242
> +        onHeightChanged: updateViewportAttributes()
> +        onWidthChanged: updateViewportAttributes()
> +        onLoadSucceeded: updateViewportAttributes()

The viewport attributes themselves should change in response to these events, and the bindings then updated.

> Tools/MiniBrowser/qt/qml/ViewportAttributesItem.qml:5
> +    color: "#000000"

black

> Tools/MiniBrowser/qt/qml/ViewportAttributesItem.qml:19
> +        color: "#ffffff"

white

> Tools/MiniBrowser/qt/qml/ViewportAttributesItem.qml:25
> +       color: "#ffffff"

white

> Tools/MiniBrowser/qt/qml/ViewportAttributesItem.qml:31
> +       color: "#ffffff"

style

[Michael Brüning]

Created attachment 116234 [details]
Updated proposed patch

[Tor Arne Vestbø]

Comment on attachment 116234 [details]
Updated proposed patch

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

lgtm, a few fixups before landing please

> ChangeLog:9
> +        attributes, which contains the viewport scalability and layout

viewportAttributes

> Source/WebKit/qt/ChangeLog:9
> +        attributes, which contains the viewport scalability and layout

viewportAttributes

> Source/WebKit2/ChangeLog:9
> +        attributes, which contains the viewport scalability and layout

viewportAttributes

> Tools/ChangeLog:9
> +        attributes, which contains the viewport scalability and layout

viewportAttributes

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:37
> +    property alias viewportattributesitem: viewportAttributesItem

normal lower-case+title-case naming apply (i see that the webview variable does not follow this, it should)

[Michael Brüning]

(In reply to comment #6)
> (From update of attachment 116234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116234&action=review
> 
> lgtm, a few fixups before landing please
> 
> > ChangeLog:9
> > +        attributes, which contains the viewport scalability and layout
> 
> viewportAttributes
> 
> > Source/WebKit/qt/ChangeLog:9
> > +        attributes, which contains the viewport scalability and layout
> 
> viewportAttributes
> 
> > Source/WebKit2/ChangeLog:9
> > +        attributes, which contains the viewport scalability and layout
> 
> viewportAttributes
> 
> > Tools/ChangeLog:9
> > +        attributes, which contains the viewport scalability and layout
> 
> viewportAttributes
> 
> > Tools/MiniBrowser/qt/qml/BrowserWindow.qml:37
> > +    property alias viewportattributesitem: viewportAttributesItem
> 
> normal lower-case+title-case naming apply (i see that the webview variable does not follow this, it should)

Thanks for the review, will put in the comments...

[Michael Brüning]

Created attachment 116491 [details]
Updated proposed patch

Added a comment about the use of the "webview" property workaround for the experimental extension. 
Changed layout of info page.

[Tor Arne Vestbø]

Comment on attachment 116491 [details]
Updated proposed patch

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

r=me, with comments

> ChangeLog:6
> +        Reviewed by Tor Arne Vestbo.

ø

> Source/WebKit/qt/ChangeLog:6
> +        Reviewed by Tor Arne Vestbo.

ø

> Source/WebKit2/ChangeLog:6
> +        Reviewed by Tor Arne Vestbo.

ø

> Tools/ChangeLog:6
> +        Reviewed by Tor Arne Vestbo.

ø

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:-189
> -

Don't include this hunk

[Michael Brüning]

Created attachment 116495 [details]
Updated proposed patch.

Put in the "ø"'s and "ü"'s at the appropriate places. 
Inserted accidentally removed blank line.

[WebKit Review Bot]

Comment on attachment 116495 [details]
Updated proposed patch.

Rejecting attachment 116495 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
cripts/webkitpy/common/system/executive.py", line 420, in run_command
    close_fds=self._should_close_fds())
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/system/executive.py", line 476, in popen
    return subprocess.Popen(*args, **kwargs)
  File "/usr/lib/python2.6/subprocess.py", line 623, in __init__
    errread, errwrite)
  File "/usr/lib/python2.6/subprocess.py", line 1141, in _execute_child
    raise child_exception
TypeError: execv() arg 2 must contain only strings

Full output: http://queues.webkit.org/results/10618280

[Kenneth Rohde Christiansen]

Comment on attachment 116495 [details]
Updated proposed patch.

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

> Source/WebKit2/UIProcess/API/qt/qwebviewportattributes.cpp:40
> +qreal QWebViewportAttributes::initialScale() const
> +{
> +    return m_initialScale;
> +}

What about currentScale? Isnt that pretty useful for debugging?

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:240
> +        // the issue is resovled.

spelling error. Bug url?

[Michael Brüning]

(In reply to comment #12)
> (From update of attachment 116495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116495&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qwebviewportattributes.cpp:40
> > +qreal QWebViewportAttributes::initialScale() const
> > +{
> > +    return m_initialScale;
> > +}
> 
> What about currentScale? Isnt that pretty useful for debugging?
> 
True. Will add it.
> > Tools/MiniBrowser/qt/qml/BrowserWindow.qml:240
> > +        // the issue is resovled.
> 
> spelling error. Bug url?
Not clear where the bug is at the moment, so no bug to add yet.

[Michael Brüning]

Created attachment 116517 [details]
Updated patch.

Renamed viewportAttributes to viewportInfo (also in the cpp part), added currentScale, devicePixelRatio and contentsWidth and contentsHeight properties.

[Tor Arne Vestbø]

(In reply to comment #14)
> Created an attachment (id=116517) [details]
> Updated patch.
> 
> Renamed viewportAttributes to viewportInfo (also in the cpp part), added currentScale, devicePixelRatio and contentsWidth and contentsHeight properties.

Sorry to keep nitpicking, but it should be contentWidth and contentHeight :/ (not plural)

[Michael Brüning]

(In reply to comment #15)
> (In reply to comment #14)
> > Created an attachment (id=116517) [details] [details]
> > Updated patch.
> > 
> > Renamed viewportAttributes to viewportInfo (also in the cpp part), added currentScale, devicePixelRatio and contentsWidth and contentsHeight properties.
> 
> Sorry to keep nitpicking, but it should be contentWidth and contentHeight :/ (not plural)

As discussed in #qtwebkit, the size that is referred to in the properties is the contentsSize, so plural matches the wording in the QQuickWebView handlers...

[WebKit Review Bot]

Comment on attachment 116517 [details]
Updated patch.

Rejecting attachment 116517 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
cripts/webkitpy/common/system/executive.py", line 420, in run_command
    close_fds=self._should_close_fds())
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/system/executive.py", line 476, in popen
    return subprocess.Popen(*args, **kwargs)
  File "/usr/lib/python2.6/subprocess.py", line 623, in __init__
    errread, errwrite)
  File "/usr/lib/python2.6/subprocess.py", line 1141, in _execute_child
    raise child_exception
TypeError: execv() arg 2 must contain only strings

Full output: http://queues.webkit.org/results/10644579

[Michael Brüning]

Created attachment 118584 [details]
Updated patch after js fix.

Updated patch because the qml extended type fix has been released. 

Added missing update of contentsSize in viewportInfo if the event was received during a page transition. 
To achieve that, I moved the implementation PostTransitionState::apply from qquickwebview_p_p.h to qquickwebview.cpp.

[Kenneth Rohde Christiansen]

Comment on attachment 118584 [details]
Updated patch after js fix.

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

why r? when it is already reviewed?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:210
> +    q->m_experimental->viewportInfo()->setCurrentScale(static_cast<qreal>(scale));

Why are all these value "copied" and not queried? ie queries from the interaction engine.

[Michael Brüning]

Created attachment 119018 [details]
Updated patch

Removed the caching of viewport constraint data in QWebViewportInfo and made it use the QQuickWebViewPrivate data instead.
Added layoutSize as a member of QtViewportInteractionEngine::Constraints in order to not cache it in QWebViewportInfo. 
Changed layoutWidth and layoutHeight to layoutSize and contentsWidth/contentsHeight to contentsSize.

[Kenneth Rohde Christiansen]

Comment on attachment 119018 [details]
Updated patch

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

> Source/WebKit2/ChangeLog:6
> +
> +        Reviewed by Tor Arne Vestbø.

I guess such a big change requires you to null this line

> Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:52
> +    if (!m_webViewPrivate->interactionEngine)
> +        return -1.0;

it would be 1.0 actually :-)

> Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:60
> +    if (!m_webViewPrivate->interactionEngine)
> +        return -1.0;

I guess it would be 1.0 :-) as well

> Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:68
> +    if (!m_webViewPrivate->interactionEngine)
> +        return -1.0;

same here

[Michael Brüning]

Created attachment 119181 [details]
Updated patch.

Moved the values that depend on the interactionEngine to be QVariants

[Kenneth Rohde Christiansen]

Comment on attachment 119181 [details]
Updated patch.

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

> Source/WebKit2/UIProcess/API/qt/qwebviewportinfo_p.h:39
> +    Q_PROPERTY(QSize contentsSize READ contentsSize NOTIFY contentsSizeUpdated)

I miss contentsPosition :-)

[WebKit Review Bot]

Comment on attachment 119181 [details]
Updated patch.

Rejecting attachment 119181 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
UIProcess/qt/QtViewportInteractionEngine.h
patching file Source/qtwebkit-export.map
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/MiniBrowser/qt/MiniBrowser.pro
patching file Tools/MiniBrowser/qt/MiniBrowser.qrc
patching file Tools/MiniBrowser/qt/qml/BrowserWindow.qml
patching file Tools/MiniBrowser/qt/qml/ViewportInfoItem.qml

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christia..." exit_code: 1

Full output: http://queues.webkit.org/results/10871232

[Michael Brüning]

Created attachment 119408 [details]
Patch rebased to current HEAD.

Commit queue seems to have been unable to apply die to merge conflicts. 
Rebased patch locally and uploaded that version.

[Kenneth Rohde Christiansen]

Comment on attachment 119408 [details]
Patch rebased to current HEAD.

You should have filled in my name and set cq? but now let me r+ and cq+

[WebKit Review Bot]

Comment on attachment 119408 [details]
Patch rebased to current HEAD.

Clearing flags on attachment: 119408

Committed r102922: <http://trac.webkit.org/changeset/102922>

[Michael Brüning]

Comment on attachment 119181 [details]
Updated patch.

Clearing flags from obsolete patch.

[Michael Brüning]

Marked Resolved Fixed as the patch has been landed via cq...