Bug 86474 - [Qt] Add infra for testing double-tap to zoom functionality etc
Summary: [Qt] Add infra for testing double-tap to zoom functionality etc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-15 06:12 PDT by Kenneth Rohde Christiansen
Modified: 2020-08-24 21:49 PDT (History)
6 users (show)

See Also:


Attachments
Patch (36.24 KB, patch)
2012-05-15 06:20 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (36.48 KB, patch)
2012-05-15 06:28 PDT, Kenneth Rohde Christiansen
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2012-05-15 06:12:15 PDT
SSIA
Comment 1 Kenneth Rohde Christiansen 2012-05-15 06:20:28 PDT
Created attachment 141945 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-15 06:22:57 PDT
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.
Comment 3 Simon Hausmann 2012-05-15 06:27:08 PDT
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? :)
Comment 4 Kenneth Rohde Christiansen 2012-05-15 06:28:29 PDT
Created attachment 141949 [details]
Patch
Comment 5 Kenneth Rohde Christiansen 2012-05-15 06:31:16 PDT
(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?
Comment 6 Simon Hausmann 2012-05-15 06:33:34 PDT
(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?
Comment 7 Simon Hausmann 2012-05-15 06:34:15 PDT
Comment on attachment 141949 [details]
Patch

Now without style errors :).
Comment 8 Kenneth Rohde Christiansen 2012-05-15 06:54:32 PDT
Fixed patch landed in 117061