Bug 42674 - WebKitTestRunner needs testRunner.queueLoad
Summary: WebKitTestRunner needs testRunner.queueLoad
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords: InRadar
Depends on: 99317
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-20 14:35 PDT by Sam Weinig
Modified: 2012-10-17 00:54 PDT (History)
11 users (show)

See Also:


Attachments
preliminary patch. (34.37 KB, patch)
2012-10-03 06:47 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch v2 (36.85 KB, patch)
2012-10-03 14:08 PDT, Mikhail Pozdnyakov
ap: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch v3 (36.96 KB, patch)
2012-10-09 12:46 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch v4 (41.83 KB, patch)
2012-10-11 06:21 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v5 (42.09 KB, patch)
2012-10-11 12:43 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch v6 (deleted)
2012-10-12 00:19 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
rebased (41.81 KB, patch)
2012-10-12 11:50 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v8 (38.91 KB, patch)
2012-10-16 03:58 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v9 (38.94 KB, patch)
2012-10-17 00:24 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-07-20 14:35:29 PDT
WebKitTestRunner needs layoutTestController.queueLoad
Comment 1 Sam Weinig 2010-07-20 15:33:16 PDT
<rdar://problem/8213871>
Comment 2 Mikhail Pozdnyakov 2012-09-30 23:58:46 PDT
Taking it.
Comment 3 Mikhail Pozdnyakov 2012-10-03 06:47:37 PDT
Created attachment 166884 [details]
preliminary patch.

Will not build on MAC. However, think, it's worth reviewing already.
Comment 4 Build Bot 2012-10-03 06:50:49 PDT
Comment on attachment 166884 [details]
preliminary patch.

Attachment 166884 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14124853
Comment 5 Early Warning System Bot 2012-10-03 06:53:45 PDT
Comment on attachment 166884 [details]
preliminary patch.

Attachment 166884 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14125781
Comment 6 Build Bot 2012-10-03 07:08:18 PDT
Comment on attachment 166884 [details]
preliminary patch.

Attachment 166884 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14134597
Comment 7 Mikhail Pozdnyakov 2012-10-03 08:00:44 PDT
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..
Comment 8 Mikhail Pozdnyakov 2012-10-03 14:08:34 PDT
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.
Comment 9 Build Bot 2012-10-03 14:35:12 PDT
Comment on attachment 166957 [details]
patch v2

Attachment 166957 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14134716
Comment 10 Mikhail Pozdnyakov 2012-10-05 05:50:19 PDT
(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.
Comment 11 Alexey Proskuryakov 2012-10-05 09:42:31 PDT
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.
Comment 12 Mikhail Pozdnyakov 2012-10-09 12:46:57 PDT
Created attachment 167823 [details]
patch v3

Took comments from Alexey into consideration. Thanks, Alexey.
Comment 13 Build Bot 2012-10-09 13:38:19 PDT
Comment on attachment 167823 [details]
patch v3

Attachment 167823 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14249078
Comment 14 Mikhail Pozdnyakov 2012-10-11 06:21:38 PDT
Created attachment 168206 [details]
patch v4

Added new files to xproj.
Comment 15 Chris Dumez 2012-10-11 08:04:55 PDT
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?
Comment 16 Mikhail Pozdnyakov 2012-10-11 12:03:16 PDT
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
Comment 17 Mikhail Pozdnyakov 2012-10-11 12:43:20 PDT
Created attachment 168262 [details]
patch v5

Took comments from Chris into consideration
Comment 18 Build Bot 2012-10-11 12:52:33 PDT
Comment on attachment 168262 [details]
patch v5

Attachment 168262 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14249889
Comment 19 Early Warning System Bot 2012-10-11 13:16:25 PDT
Comment on attachment 168262 [details]
patch v5

Attachment 168262 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14260543
Comment 20 Build Bot 2012-10-11 13:17:12 PDT
Comment on attachment 168262 [details]
patch v5

Attachment 168262 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14249894
Comment 21 Mikhail Pozdnyakov 2012-10-12 00:19:26 PDT
Created attachment 168369 [details]
patch v6

Should make the bots happy.
Comment 22 Mikhail Pozdnyakov 2012-10-12 11:50:05 PDT
Created attachment 168455 [details]
rebased
Comment 23 Kenneth Rohde Christiansen 2012-10-15 01:47:39 PDT
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?
Comment 24 Mikhail Pozdnyakov 2012-10-15 06:15:43 PDT
Having an IRC discussion with Kenneth, moved WKURL API modification to a separate bug: https://bugs.webkit.org/show_bug.cgi?id=99317
Comment 25 Mikhail Pozdnyakov 2012-10-16 03:58:49 PDT
Created attachment 168913 [details]
patch v8

Rebased. Took comments from Kenneth into consideration.
Comment 26 Kenneth Rohde Christiansen 2012-10-16 09:18:11 PDT
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?
Comment 27 Mikhail Pozdnyakov 2012-10-17 00:05:28 PDT
(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.
Comment 28 Mikhail Pozdnyakov 2012-10-17 00:13:53 PDT
> > 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.
Comment 29 Mikhail Pozdnyakov 2012-10-17 00:24:07 PDT
Created attachment 169106 [details]
patch v9

Took Kenneth feedback into consideration
Comment 30 WebKit Review Bot 2012-10-17 00:54:28 PDT
Comment on attachment 169106 [details]
patch v9

Clearing flags on attachment: 169106

Committed r131560: <http://trac.webkit.org/changeset/131560>
Comment 31 WebKit Review Bot 2012-10-17 00:54:35 PDT
All reviewed patches have been landed.  Closing bug.