Bug 108334 – [chromium] move remaining resource load related methods to TestRunner library

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[chromium] move remaining resource load related methods to TestRunner library

Created attachment 185477 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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

[Adam Barth]

(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.

[Build Bot]

Comment on attachment 185477 [details]
Patch

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

[Adam Barth]

ok

[WebKit Review Bot]

Comment on attachment 185477 [details]
Patch

Clearing flags on attachment: 185477

Committed r141381: <http://trac.webkit.org/changeset/141381>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.