SSIA
Created attachment 141945 [details] Patch
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 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? :)
Created attachment 141949 [details] Patch
(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?
(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 on attachment 141949 [details] Patch Now without style errors :).
Fixed patch landed in 117061