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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: geko1702+bugzilla, Assigned: geko1702+bugzilla)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Blocks: 1029887
Assignee: nobody → gkontaxis
Status: NEW → ASSIGNED
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 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?
This complements the image status. Do we really want them to be in different places?
Attachment #8459016 - Flags: review?(seth)
Attachment #8454096 - Attachment is obsolete: true
Attachment #8454096 - Flags: review?(seth)
(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 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+
Attachment #8459016 - Attachment is obsolete: true
Attachment #8459067 - Attachment is obsolete: true
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.

Attachment

General

Created:
Updated:
Size: