Summary: | [chromium] move remaining resource load related methods to TestRunner library | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||
Component: | New Bugs | Assignee: | jochen | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, jamesr, tkent+wkapi, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
jochen
2013-01-30 05:47:27 PST
Created attachment 185477 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Comment on attachment 185477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185477&action=review > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:410 > + return WebTestProxyBase::cannotHandleRequestError(frame, request); There's no call to Base here? > Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:772 > + return !url.SchemeIs("spaceballs"); I probably would have combined this line with 769. Comment on attachment 185477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185477&action=review >> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:410 >> + return WebTestProxyBase::cannotHandleRequestError(frame, request); > > There's no call to Base here? RenderViewImpl has NOTREACHED() in cannotHandleRequestError because it claims to be able to handle all requests. >> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:772 >> + return !url.SchemeIs("spaceballs"); > > I probably would have combined this line with 769. done Comment on attachment 185477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185477&action=review >>> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:772 >>> + return !url.SchemeIs("spaceballs"); >> >> I probably would have combined this line with 769. > > done Actually, we need to cast the WebURL to GURL to get access to SchemeIs (In reply to comment #4) > (From update of attachment 185477 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185477&action=review > > >> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:410 > >> + return WebTestProxyBase::cannotHandleRequestError(frame, request); > > > > There's no call to Base here? > > RenderViewImpl has NOTREACHED() in cannotHandleRequestError because it claims to be able to handle all requests. It seems odd for this code to know that detail about RenderViewImpl. Perhaps we should ASSERT that we're in some sort of testing mode? > >>> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:772 > >>> + return !url.SchemeIs("spaceballs"); > >> > >> I probably would have combined this line with 769. > > > > done > > Actually, we need to cast the WebURL to GURL to get access to SchemeIs Ok. Comment on attachment 185477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185477&action=review >>>> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:410 >>>> + return WebTestProxyBase::cannotHandleRequestError(frame, request); >>> >>> There's no call to Base here? >> >> RenderViewImpl has NOTREACHED() in cannotHandleRequestError because it claims to be able to handle all requests. > > It seems odd for this code to know that detail about RenderViewImpl. Perhaps we should ASSERT that we're in some sort of testing mode? there are already a bunch of places that don't call Base, e.g. createView doesn't call base if the proxy decides not to create a view. Other places don't call Base because the API guarantees certain conditions which we violate during tests, e.g. combinations smart insert/delete and selecting trailing whitespace. I think this falls into both categories: the API guarantees that cannotHandleRequestError is not invoked unless canHandleRequest returns false. And since it's the Proxy that returned false, it would violate the API if we forwarded this call to the embedder. Also, I can only return one header, so even if I called out to Base, I had to discard the result. Comment on attachment 185477 [details] Patch Attachment 185477 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16252080 ok Comment on attachment 185477 [details] Patch Clearing flags on attachment: 185477 Committed r141381: <http://trac.webkit.org/changeset/141381> All reviewed patches have been landed. Closing bug. |