Bug 42674 – WebKitTestRunner needs testRunner.queueLoad

• Component: WebKit2
• Status: RESOLVED FIXED

[Sam Weinig]

WebKitTestRunner needs layoutTestController.queueLoad

[Sam Weinig]

<rdar://problem/8213871>

[Mikhail Pozdnyakov]

Taking it.

[Mikhail Pozdnyakov]

Created attachment 166884 [details]
preliminary patch.

Will not build on MAC. However, think, it's worth reviewing already.

[Build Bot]

Comment on attachment 166884 [details]
preliminary patch.

Attachment 166884 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14124853

[Early Warning System Bot]

Comment on attachment 166884 [details]
preliminary patch.

Attachment 166884 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14125781

[Build Bot]

Comment on attachment 166884 [details]
preliminary patch.

Attachment 166884 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14134597

[Mikhail Pozdnyakov]

Comment on attachment 166884 [details]
preliminary patch.

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

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:815
> +    KURL absoluteURL(baseURL, url->string());

Might be a good idea to hide this logic in a WKURL function, so that there is no need to access WebCore::KURL from here..

[Mikhail Pozdnyakov]

Created attachment 166957 [details]
patch v2

Added new constructor function WKURLCreateWithBaseURL for WKURL which resolves the relative URL with the given base URL.
So that there is no need to use WebCore data types from TestRunner directly.

[Build Bot]

Comment on attachment 166957 [details]
patch v2

Attachment 166957 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14134716

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (From update of attachment 166957 [details])
> Attachment 166957 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14134716

Need MAC machine to include newly added files to MAC project file.

[Alexey Proskuryakov]

Comment on attachment 166957 [details]
patch v2

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

Only doing a shallow review, didn't attempt to check logic.

> Source/WebKit2/Shared/WebURL.h:57
> +        KURL* absoluteURL = new KURL(*baseURL->m_parsedURL.get(), string);
> +
> +        return adoptRef(new WebURL(absoluteURL, absoluteURL->string()));

I think that you should use OwnPtr/PassOwnPtr here. Adopting inside constructor is opaque and error-prone.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:810
> +    WKRetainPtr<WKURLRef> baseURLWK = WKBundleFrameCopyURL(WKBundlePageGetMainFrame(InjectedBundle::shared().page()->page()));
> +    WKRetainPtr<WKURLRef> urlWK = WKURLCreateWithBaseURL(baseURLWK.get(), toWTFString(toWK(url)).utf8().data());
> +    WKRetainPtr<WKStringRef> urlStringWK = WKURLCopyString(urlWK.get());

These all leak. Results of "copy" and "create" functions should be adopted.

> Tools/WebKitTestRunner/TestInvocation.cpp:395
> +        const uint64_t stepCount = WKUInt64GetValue(static_cast<WKUInt64Ref>(messageBody));

As a matter of coding style, we don't use const local variables.

> Tools/WebKitTestRunner/TestInvocation.cpp:427
> +        const bool isEmpty = TestController::shared().workQueueManager().isWorkQueueEmpty();

As a matter of coding style, we don't use const local variables.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:37
> +namespace {

As a matter of coding style, we don't use anonymous namespaces. These make debugging harder, and name clashes that they facilitate make refactoring harder.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:89
> +        { }

These braces should be on separate lines.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:94
> +                // FIXME: Use target.

This could use a little more verbosity. What is broken because of us not using it?

> Tools/WebKitTestRunner/WorkQueueManager.cpp:103
> +        WKRetainPtr<WKURLRef> mUrl;
> +        String mTarget;

Coding style: m_url, m_target.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:113
> +        BackNavigationItem(unsigned howFarBackward): mHowFarBackward(howFarBackward) { }

There should be a space before colon.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:117
> +        unsigned mHowFarBackward;

m_howFarBackward.

> Tools/WebKitTestRunner/WorkQueueManager.h:59
> +    WorkQueue mWorkQueue;
> +    bool mProcessing;

Coding style: m_workQueue, m_processing.

> Tools/WebKitTestRunner/win/WebKitTestRunner.vcproj:519
> -			>
> +                        >

Looks like this file should use tabs.

> Tools/WebKitTestRunner/win/WebKitTestRunner.vcproj:536
> +                <File
> +                        RelativePath="..\WorkQueueManager.cpp"
> +                        >
> +                </File>
> +                <File
> +                        RelativePath="..\WorkQueueManager.h"
> +                        >
> +                </File>

Looks like this file should use tabs.

[Mikhail Pozdnyakov]

Created attachment 167823 [details]
patch v3

Took comments from Alexey into consideration. Thanks, Alexey.

[Build Bot]

Comment on attachment 167823 [details]
patch v3

Attachment 167823 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14249078

[Mikhail Pozdnyakov]

Created attachment 168206 [details]
patch v4

Added new files to xproj.

[Chris Dumez]

Comment on attachment 168206 [details]
patch v4

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

> Source/WebKit2/Shared/WebURL.h:49
> +    static PassRefPtr<WebURL> create(const WebURL* baseURL, const String& string)

The factory methods usually don't do anything except calling the private constructor. Shouldn't we move all this code to the constructor?

"string" -> "relativeURL" ?

> Source/WebKit2/Shared/WebURL.h:51
> +        using WebCore::KURL;

Not that commonly used in WebKit.

> Source/WebKit2/Shared/WebURL.h:55
> +        WebCore::KURL* absoluteURL = new KURL(*baseURL->m_parsedURL.get(), string);

One KURL with namespace and the other without?
You should should probably use OwnPtr<WebCore::KURL> instead of raw pointer here.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:463
> +    WKTypeRef resultToPass = 0;

Can we use directly WKBooleanRef to avoid casting below?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:104
> +    void queueBackNavigation(uint64_t howFarBackward);

TestRunner seems to use "unsigned" type for howFarBackward.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:844
> +    WKRetainPtr<WKURLRef> urlWK(AdoptWK, WKURLCreateWithBaseURL(baseURLWK.get(), toWTFString(toWK(url)).utf8().data()));

toWTFString(toWK(url)) -> url->string() ?

> Tools/WebKitTestRunner/WorkQueueManager.cpp:91
> +                // FIXME: Use target. Some layout tests cannot pas as rely on this functionality.

"pass as they rely"

> Tools/WebKitTestRunner/WorkQueueManager.cpp:100
> +        String m_target;

Why store the url as WK type but not the target?

[Mikhail Pozdnyakov]

Comment on attachment 168206 [details]
patch v4

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

>> Source/WebKit2/Shared/WebURL.h:49
>> +    static PassRefPtr<WebURL> create(const WebURL* baseURL, const String& string)
> 
> The factory methods usually don't do anything except calling the private constructor. Shouldn't we move all this code to the constructor?
> 
> "string" -> "relativeURL" ?

Why do we need factory functions than? :) If we put it into constructor, the code would be less efficient as class data members would have to be initialized by default values first.

>> Source/WebKit2/Shared/WebURL.h:51
>> +        using WebCore::KURL;
> 
> Not that commonly used in WebKit.

It is good C++ practice. So far I haven't seen anything against it in WebKit coding guidelines. Have you?

>> Source/WebKit2/Shared/WebURL.h:55
>> +        WebCore::KURL* absoluteURL = new KURL(*baseURL->m_parsedURL.get(), string);
> 
> One KURL with namespace and the other without?
> You should should probably use OwnPtr<WebCore::KURL> instead of raw pointer here.

Do not see any benefit from having OwnPtr here, moreover further we have to pass both absoluteURL as PassOwnPtr (that would take the pointer from OwnPtr) and its member absoluteURL->string() at the same time.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:463
>> +    WKTypeRef resultToPass = 0;
> 
> Can we use directly WKBooleanRef to avoid casting below?

Unfortunately no, WKBundlePostSynchronousMessage accepts WKTypeRef*. (WKTypeRef is defined as const void*, and WKBooleanRef is defined as const struct OpaqueWKBoolean* )

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:104
>> +    void queueBackNavigation(uint64_t howFarBackward);
> 
> TestRunner seems to use "unsigned" type for howFarBackward.

we need uint64_t to be used further in WKUInt64Create

>> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:844
>> +    WKRetainPtr<WKURLRef> urlWK(AdoptWK, WKURLCreateWithBaseURL(baseURLWK.get(), toWTFString(toWK(url)).utf8().data()));
> 
> toWTFString(toWK(url)) -> url->string() ?

Indeed. Thanks.

>> Tools/WebKitTestRunner/WorkQueueManager.cpp:100
>> +        String m_target;
> 
> Why store the url as WK type but not the target?

Further we use m_url in WK api unlike m_target

[Mikhail Pozdnyakov]

Created attachment 168262 [details]
patch v5

Took comments from Chris into consideration

[Build Bot]

Comment on attachment 168262 [details]
patch v5

Attachment 168262 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14249889

[Early Warning System Bot]

Comment on attachment 168262 [details]
patch v5

Attachment 168262 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14260543

[Build Bot]

Comment on attachment 168262 [details]
patch v5

Attachment 168262 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14249894

[Mikhail Pozdnyakov]

Created attachment 168369 [details]
patch v6

Should make the bots happy.

[Mikhail Pozdnyakov]

Created attachment 168455 [details]
rebased

[Kenneth Rohde Christiansen]

Comment on attachment 168455 [details]
rebased

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

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:185
> +    // Work queue

Dot at end

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:965
> +    locationChangeForFrame(frame, true /*dump*/);

/* shouldDump */

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1867
> +void InjectedBundlePage::locationChangeForFrame(WKBundleFrameRef frame, bool dump)

shouldDump?

[Mikhail Pozdnyakov]

Having an IRC discussion with Kenneth, moved WKURL API modification to a separate bug: https://bugs.webkit.org/show_bug.cgi?id=99317

[Mikhail Pozdnyakov]

Created attachment 168913 [details]
patch v8

Rebased. Took comments from Kenneth into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 168913 [details]
patch v8

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

> Tools/ChangeLog:10
> +        Work Queue implementation. Work Queue is managed by WorkQueueManager which belongs to UI process
> +        and exchanges messages with Injected bundle.

Maybe you should write why that makes the most sense, even if obvious

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:965
> +    locationChangeForFrame(frame, true /*shouldDump*/);

Don't we do /* shouldDump */ true?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1867
> +void InjectedBundlePage::locationChangeForFrame(WKBundleFrameRef frame, bool shouldDump)

frameDidChangeLocation ?

> Tools/WebKitTestRunner/WorkQueueManager.cpp:37
> +static inline WKPageRef mainPage()

What is a main page? isn't it not just page() ?

> Tools/WebKitTestRunner/WorkQueueManager.cpp:93
> +            if (!m_target.isEmpty()) {
> +                // FIXME: Use target. Some layout tests cannot pass as they rely on this functionality.
> +                fprintf(stderr, "queueLoad for a specific target is not implemented.\n");
> +                return false;

IS this hard to fix?

[Mikhail Pozdnyakov]

(In reply to comment #26)
> 
> > Tools/WebKitTestRunner/WorkQueueManager.cpp:93
> > +            if (!m_target.isEmpty()) {
> > +                // FIXME: Use target. Some layout tests cannot pass as they rely on this functionality.
> > +                fprintf(stderr, "queueLoad for a specific target is not implemented.\n");
> > +                return false;
> 
> IS this hard to fix?

This functionality requires 1) finding child frame by name (can traverse through WKFrameCopyChildFrames result) 2) loading url for a specific frame (this API is missing). To my mind it's worth having a separate task.

[Mikhail Pozdnyakov]

> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1867
> > +void InjectedBundlePage::locationChangeForFrame(WKBundleFrameRef frame, bool shouldDump)
> 
> frameDidChangeLocation ?
> 
> > Tools/WebKitTestRunner/WorkQueueManager.cpp:37
> > +static inline WKPageRef mainPage()
> 
> What is a main page? isn't it not just page() ?
> 
it returns page from TestController::shared().mainWebView() so it's main page I guess.

[Mikhail Pozdnyakov]

Created attachment 169106 [details]
patch v9

Took Kenneth feedback into consideration

[WebKit Review Bot]

Comment on attachment 169106 [details]
patch v9

Clearing flags on attachment: 169106

Committed r131560: <http://trac.webkit.org/changeset/131560>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.