Bug 62396 - Web Inspector: [Chromium] Successfully prefetched page shows up as an error in console
Summary: Web Inspector: [Chromium] Successfully prefetched page shows up as an error i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-09 13:06 PDT by Vsevolod Vlasov
Modified: 2011-06-24 01:26 PDT (History)
14 users (show)

See Also:


Attachments
patch (1.52 KB, patch)
2011-06-09 13:12 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
patch (1.87 KB, patch)
2011-06-16 11:57 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
patch (forgot to add a changelog) (2.68 KB, patch)
2011-06-16 12:08 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch with new field renamed (2.67 KB, patch)
2011-06-20 05:17 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch with fixes (2.82 KB, patch)
2011-06-20 14:33 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-06-09 13:06:48 PDT
Successfully prefetched page shows up as an error in console.
Comment 1 Vsevolod Vlasov 2011-06-09 13:12:27 PDT
Created attachment 96623 [details]
patch
Comment 2 Vsevolod Vlasov 2011-06-09 13:16:06 PDT
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
Comment 3 Adam Barth 2011-06-09 13:52:24 PDT
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?
Comment 4 Adam Barth 2011-06-09 13:52:40 PDT
Comment on attachment 96623 [details]
patch

Needs ChangeLog and test.
Comment 5 Adam Barth 2011-06-09 13:52:59 PDT
+fishd for WebKit API review.
Comment 6 Vsevolod Vlasov 2011-06-09 14:22:26 PDT
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
Comment 7 Vsevolod Vlasov 2011-06-09 14:23:41 PDT
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.
Comment 8 Adam Barth 2011-06-09 14:32:20 PDT
+gavinp

He's likely to give you the best quality feedback.
Comment 9 Gavin Peters 2011-06-15 08:30:08 PDT
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
Comment 10 Darin Fisher (:fishd, Google) 2011-06-15 09:30:46 PDT
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.
Comment 11 Darin Fisher (:fishd, Google) 2011-06-15 09:37:12 PDT
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...
Comment 12 Darin Fisher (:fishd, Google) 2011-06-15 09:43:07 PDT
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().
Comment 13 Vsevolod Vlasov 2011-06-16 11:57:31 PDT
Created attachment 97475 [details]
patch
Comment 14 Vsevolod Vlasov 2011-06-16 12:03:42 PDT
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.
Comment 15 Vsevolod Vlasov 2011-06-16 12:08:41 PDT
Created attachment 97476 [details]
patch (forgot to add a changelog)
Comment 16 Vsevolod Vlasov 2011-06-20 05:17:16 PDT
Created attachment 97782 [details]
Patch with new field renamed
Comment 17 Darin Fisher (:fishd, Google) 2011-06-20 13:52:02 PDT
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.
Comment 18 Vsevolod Vlasov 2011-06-20 14:33:58 PDT
Created attachment 97858 [details]
Patch with fixes
Comment 19 WebKit Review Bot 2011-06-24 01:26:02 PDT
Comment on attachment 97858 [details]
Patch with fixes

Clearing flags on attachment: 97858

Committed r89658: <http://trac.webkit.org/changeset/89658>
Comment 20 WebKit Review Bot 2011-06-24 01:26:08 PDT
All reviewed patches have been landed.  Closing bug.