Closed
Bug 1037213
Opened 11 years ago
Closed 11 years ago
imgIRequest should hold the actual error code when imageStatus == STATUS_ERROR
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: geko1702+bugzilla, Assigned: geko1702+bugzilla)
References
Details
Attachments
(1 file, 3 obsolete files)
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → gkontaxis
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 8454096 [details] [diff] [review]
extended imgIRequest with an imageErrorCode attribute and modified RecordStopRequest in imgStatusTracker to preserve the actual error code on STATUS_ERROR
This supports figuring out why an image has failed to load in nsImageLoadingContent::Notify(). Right now we only get STATUS_ERROR which is vague. For the purposes of https://bugzilla.mozilla.org/show_bug.cgi?id=1029887 we want to add some logic in there based on the error code.
Attachment #8454096 -
Flags: review?(seth)
Comment 3•11 years ago
|
||
Comment on attachment 8454096 [details] [diff] [review]
extended imgIRequest with an imageErrorCode attribute and modified RecordStopRequest in imgStatusTracker to preserve the actual error code on STATUS_ERROR
Review of attachment 8454096 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/public/imgIRequest.idl
@@ +82,5 @@
> + /*
> + * Actual error code that generated a STATUS_ERROR imageStatus
> + * (see xpcom/base/ErrorList.h)
> + */
> + readonly attribute nsresult imageErrorCode;
Is it a requirement that we expose this to add-ons?
If not, please mark this noscript and notxpcom. I generally try to avoid exposing things to add-ons unless someone actually asks for it.
::: image/src/imgStatusTracker.cpp
@@ +886,3 @@
> mImageStatus |= imgIRequest::STATUS_LOAD_COMPLETE;
> + }
> + else {
Use |} else {| please.
::: image/src/imgStatusTracker.h
@@ +191,5 @@
> uint32_t GetImageStatus() const;
>
> + // Returns actual error code that generated a STATUS_ERROR imageStatus.
> + // (see xpcom/base/ErrorList.h)
> + nsresult GetImageErrorCode() const;
As I mentioned to you on IRC, it seems more logical to me to store this on imgRequest, which is after all the class that represents the actual network request. Is there a reason to prefer imgStatusTracker?
Assignee | ||
Comment 4•11 years ago
|
||
This complements the image status. Do we really want them to be in different places?
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8459016 -
Flags: review?(seth)
Assignee | ||
Updated•11 years ago
|
Attachment #8454096 -
Attachment is obsolete: true
Attachment #8454096 -
Flags: review?(seth)
Comment 7•11 years ago
|
||
(In reply to gkontaxis from comment #4)
> This complements the image status. Do we really want them to be in different
> places?
As it stands now, at least, I would say that this is more a property of the request than of the image's state.
Comment 8•11 years ago
|
||
Comment on attachment 8459016 [details] [diff] [review]
extended imgIRequest with an imageErrorCode attribute and modified imgRequest::OnStopRequest to preserve the actual error code when NS_FAILED(status)
Review of attachment 8459016 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good! r+ with the problems below resolved.
::: image/src/imgRequest.cpp
@@ +750,5 @@
> // if the error isn't "just" a partial transfer
> // stops animations, removes from cache
> this->Cancel(status);
> +
> + mImageErrorCode = status;
imgRequest::Cancel causes notifications to be sent. You should set mImageErrorCode before calling Cancel so the listeners of those notifications can get an accurate error code.
::: image/src/imgRequest.h
@@ +254,5 @@
> bool mIsInCache : 1;
> bool mBlockingOnload : 1;
> bool mResniffMimeType : 1;
> +
> + nsresult mImageErrorCode;
Move this before the boolean fields or you run the risk of increasing the size of an imgRequest object more than necessary.
Attachment #8459016 -
Flags: review?(seth) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8459016 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8459067 -
Attachment is obsolete: true
Comment 11•11 years ago
|
||
Comment 12•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•