Bug 86474 – [Qt] Add infra for testing double-tap to zoom functionality etc

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

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 141945 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/declarative/experimental/plugin.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 16 files


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

[Simon Hausmann]

Comment on attachment 141945 [details]
Patch

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

r=me, I like the step of moving the viewport info stuff into something that can be re-used for unit testing. But it doesn't belong into experimental (because we know this experiment is never going to succeed to become public API) and it should only be instantiated when run as part of the unit tests.

I suggest private C++ API to expose (and thus lazily create) the testing object and then make it available in TestWebView.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:114
> +    emit d->viewportItem->experimental()->test()->contentsSizeChanged();

So even "production" code instantiates the test object and emits signals. Wouldn't it be better to avoid that?

> Source/WebKit2/UIProcess/API/qt/qwebkittest.cpp:53
> +    // if (delay > 0)
> +    //     QTest::qWait(delay);

Forgot to remove this? :)

[Kenneth Rohde Christiansen]

Created attachment 141949 [details]
Patch

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> (From update of attachment 141945 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141945&action=review
> 
> r=me, I like the step of moving the viewport info stuff into something that can be re-used for unit testing. But it doesn't belong into experimental (because we know this experiment is never going to succeed to become public API) and it should only be instantiated when run as part of the unit tests.
> 
> I suggest private C++ API to expose (and thus lazily create) the testing object and then make it available in TestWebView.

Let's do this in a follow up.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:114
> > +    emit d->viewportItem->experimental()->test()->contentsSizeChanged();
> 
> So even "production" code instantiates the test object and emits signals. Wouldn't it be better to avoid that?

Any suggestion on how to do this?

> 
> > Source/WebKit2/UIProcess/API/qt/qwebkittest.cpp:53
> > +    // if (delay > 0)
> > +    //     QTest::qWait(delay);
> 
> Forgot to remove this? :)

I wanted to link to QTest later, but I dont want to do that unless it becomes a separate module. I can remove it, what do you think?

[Simon Hausmann]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 141945 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=141945&action=review
> > 
> > r=me, I like the step of moving the viewport info stuff into something that can be re-used for unit testing. But it doesn't belong into experimental (because we know this experiment is never going to succeed to become public API) and it should only be instantiated when run as part of the unit tests.
> > 
> > I suggest private C++ API to expose (and thus lazily create) the testing object and then make it available in TestWebView.
> 
> Let's do this in a follow up.

Okay.
 
> > 
> > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:114
> > > +    emit d->viewportItem->experimental()->test()->contentsSizeChanged();
> > 
> > So even "production" code instantiates the test object and emits signals. Wouldn't it be better to avoid that?
> 
> Any suggestion on how to do this?

For example by lazy allocation of the testing harness in QQuickWebView, i.e. not allocated by default unless somebody calls QQuickWebView::testingHarness().
 
> > 
> > > Source/WebKit2/UIProcess/API/qt/qwebkittest.cpp:53
> > > +    // if (delay > 0)
> > > +    //     QTest::qWait(delay);
> > 
> > Forgot to remove this? :)
> 
> I wanted to link to QTest later, but I dont want to do that unless it becomes a separate module. I can remove it, what do you think?

Yeah. What would you use QTest for beyond qWait?

[Simon Hausmann]

Comment on attachment 141949 [details]
Patch

Now without style errors :).

[Kenneth Rohde Christiansen]

Fixed patch landed in 117061