Closed
Bug 1037213
Opened 8 years ago
Closed 8 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•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gkontaxis
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 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•8 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•8 years ago
|
||
This complements the image status. Do we really want them to be in different places?
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9022eb55c931
Assignee | ||
Updated•8 years ago
|
Attachment #8459016 -
Flags: review?(seth)
Assignee | ||
Updated•8 years ago
|
Attachment #8454096 -
Attachment is obsolete: true
Attachment #8454096 -
Flags: review?(seth)
Comment 7•8 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•8 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•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8459016 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8459067 -
Attachment is obsolete: true
Comment 12•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/df134a5be8fb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•