imgIRequest should hold the actual error code when imageStatus == STATUS_ERROR

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

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

Tracking

unspecified
mozilla33
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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
https://hg.mozilla.org/mozilla-central/rev/df134a5be8fb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.