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

Status: RESOLVED FIXED

Comment 1

[Georgios Kontaxis]

Attachment: extended imgIRequest with an imageErrorCode attribute and modified RecordStopRequest in imgStatusTracker to preserve the actual error code on STATUS_ERROR

Comment 2

[Georgios Kontaxis]

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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

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?

Comment 4

[Georgios Kontaxis]

This complements the image status. Do we really want them to be in different places?

Comment 5

[Georgios Kontaxis]

Attachment: extended imgIRequest with an imageErrorCode attribute and modified imgRequest::OnStopRequest to preserve the actual error code when NS_FAILED(status)

Comment 6

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=9022eb55c931

Comment 7

[Seth Fowler [:seth] [:s2h]]

(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

[Seth Fowler [:seth] [:s2h]]

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.

Comment 9

[Georgios Kontaxis]

Attachment: extended imgIRequest with an imageErrorCode attribute and modified imgRequest::OnStopRequest to preserve the actual error code when NS_FAILED(status)

Comment 10

[Georgios Kontaxis]

Attachment: extended imgIRequest with an imageErrorCode attribute and modified imgRequest::OnStopRequest to preserve the actual error code when NS_FAILED(status)

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df134a5be8fb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df134a5be8fb