Summary: | WebKitTestRunner needs testRunner.queueLoad | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||||||
Component: | WebKit2 | Assignee: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | andersca, ap, gustavo, gyuyoung.kim, kenneth, menard, mikhail.pozdnyakov, philn, rakuco, webkit.review.bot, xan.lopez | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||
Bug Depends on: | 99317 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2010-07-20 14:35:29 PDT
Taking it. Created attachment 166884 [details]
preliminary patch.
Will not build on MAC. However, think, it's worth reviewing already.
Comment on attachment 166884 [details] preliminary patch. Attachment 166884 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14124853 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 on attachment 166884 [details] preliminary patch. Attachment 166884 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14134597 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.. 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 on attachment 166957 [details] patch v2 Attachment 166957 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14134716 (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 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. Created attachment 167823 [details]
patch v3
Took comments from Alexey into consideration. Thanks, Alexey.
Comment on attachment 167823 [details] patch v3 Attachment 167823 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14249078 Created attachment 168206 [details]
patch v4
Added new files to xproj.
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 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 Created attachment 168262 [details]
patch v5
Took comments from Chris into consideration
Comment on attachment 168262 [details] patch v5 Attachment 168262 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14249889 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 on attachment 168262 [details] patch v5 Attachment 168262 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14249894 Created attachment 168369 [details]
patch v6
Should make the bots happy.
Created attachment 168455 [details]
rebased
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? Having an IRC discussion with Kenneth, moved WKURL API modification to a separate bug: https://bugs.webkit.org/show_bug.cgi?id=99317 Created attachment 168913 [details]
patch v8
Rebased. Took comments from Kenneth into consideration.
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? (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. > > 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.
Created attachment 169106 [details]
patch v9
Took Kenneth feedback into consideration
Comment on attachment 169106 [details] patch v9 Clearing flags on attachment: 169106 Committed r131560: <http://trac.webkit.org/changeset/131560> All reviewed patches have been landed. Closing bug. |