Bug 62396 – Web Inspector: [Chromium] Successfully prefetched page shows up as an error in console

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Vsevolod Vlasov]

Successfully prefetched page shows up as an error in console.

[Vsevolod Vlasov]

Created attachment 96623 [details]
patch

[Vsevolod Vlasov]

Downstream bug: crbug.com/82422

Here is a quick and naive attempt to implement this.
I'd like to hear some feedback on using CANCELED to avoid getting console error.

The chromium part of the patch is here: http://codereview.chromium.org/6995118

[Adam Barth]

Comment on attachment 96623 [details]
patch

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

> Source/WebKit/chromium/src/WebURLError.cpp:67
> +    resourceError.setIsCancellation(isCanceling);

Why isn't this part of the constructor?

[Adam Barth]

Comment on attachment 96623 [details]
patch

Needs ChangeLog and test.

[Adam Barth]

+fishd for WebKit API review.

[Vsevolod Vlasov]

This doesn't have tests and changelog intentionally. 

I just want to get some feedback on using CANCELED for that in general.
And on setting isCancellation flag for all the re

[Vsevolod Vlasov]

This doesn't have tests and changelog intentionally. 

I just want to get some feedback on using URLRequestStatus::CANCELED and setting isCancellation flag for all the requests having URLRequestStatus::CANCELED status  in general.

[Adam Barth]

+gavinp

He's likely to give you the best quality feedback.

[Gavin Peters]

This approach seems fine to me.  The error in the debug console was really counterintuitive.  See my comments in the corresponding chromium review at http://codereview.chromium.org/6995118

[Darin Fisher (:fishd, Google)]

Comment on attachment 96623 [details]
patch

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

> Source/WebKit/chromium/public/WebURLError.h:54
> +    bool isCanceling;

This struct is supposed to correspond to WebCore::ResourceError.  Unless you are plumbing this field all the way through ResourceError, I don't think it belongs here.

[Darin Fisher (:fishd, Google)]

Comment on attachment 96623 [details]
patch

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

>> Source/WebKit/chromium/public/WebURLError.h:54
>> +    bool isCanceling;
> 
> This struct is supposed to correspond to WebCore::ResourceError.  Unless you are plumbing this field all the way through ResourceError, I don't think it belongs here.

OK, I missed the fact that ResourceError has an m_isCancellation field.  Hmm...

[Darin Fisher (:fishd, Google)]

It looks like ResourceError::m_isCancellation was added in http://trac.webkit.org/changeset/33559, and the layout test for that change is one that we skip for Chromium :-(

It definitely seems like we need WebURLError to have its own isCancellation field, and we need to update both the WebURLError::operator=(const ResourceError&) and WebURLError::operator ResourceError().

[Vsevolod Vlasov]

Created attachment 97475 [details]
patch

[Vsevolod Vlasov]

I have added isCancellation field to WebURLError and updated WebURLError::operator=(const ResourceError&) and WebURLError::operator ResourceError() accordingly.

I did not add a test for that. Unfortunately DumpRenderTree doesn't seem to put this error in console (looks like it treats prefetch differently overall), so it gives false positive on my test even without this change.

[Vsevolod Vlasov]

Created attachment 97476 [details]
patch (forgot to add a changelog)

[Vsevolod Vlasov]

Created attachment 97782 [details]
Patch with new field renamed

[Darin Fisher (:fishd, Google)]

Comment on attachment 97782 [details]
Patch with new field renamed

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

> Source/WebKit/chromium/public/WebURLError.h:55
> +    // A flag showing whether this error should be trated as a cancellation,

"trated" -> "treated"

> Source/WebKit/chromium/src/WebURLError.cpp:65
> +    ResourceError resourceError = ResourceError(domain, reason,

note: the old code aligned the parameters in a right-aligned column.
you should probably indent the 2nd and 3rd params to preserve that
style.

[Vsevolod Vlasov]

Created attachment 97858 [details]
Patch with fixes

[WebKit Review Bot]

Comment on attachment 97858 [details]
Patch with fixes

Clearing flags on attachment: 97858

Committed r89658: <http://trac.webkit.org/changeset/89658>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.