Closed Bug 1312594 Opened 5 years ago Closed 5 years ago

src/currentSrc is cleared when an error occurred

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(1 file)

Attempting to troubleshoot bug 1311876.

One issue is that when the error event is fired, the element's src is cleared ; making it impossible to tell which URL caused the problem.

Adding the URL to the resource that caused the error would help.
Comment on attachment 8804098 [details]
Bug 1312594: Do not clear resource when error occurs.

https://reviewboard.mozilla.org/r/88242/#review87202

I know you are still working on this, so here are my review comments so far, so you may fix them in the next version.

::: dom/html/HTMLMediaElement.cpp:4444
(Diff revision 2)
>    if (mError) {
>      return;
>    }
> -  mError = new MediaError(this, aErrorCode, aErrorDetails);
>  
> +  // Get currentSrc

This comment is redundant I think.
Or did you intend to write more, e.g.: "Add currentSrc (if present) to error details"?

::: dom/html/HTMLMediaElement.cpp:4448
(Diff revision 2)
> +                          src.Length()
> +                            ? (src + NS_LITERAL_CSTRING(":") + aErrorDetails)
> +                            : nsCString(aErrorDetails));

Style: left-align '?' and ':' with 'src.Length()' above.
Comment on attachment 8804098 [details]
Bug 1312594: Do not clear resource when error occurs.

https://reviewboard.mozilla.org/r/88242/#review87202

> Style: left-align '?' and ':' with 'src.Length()' above.

I have taken the decision to ignore this coding-style "rule" from now on for two reasons:
1- I find it particularly hard to read, as it's not obvious at a glance that the block is related to the test above
2- clang-format in Mozilla mode adds an indentation. As my IDE reformat things as I type or when I copy/paste having to manually go against clang-format is a pain.

I could add a 3 in that no-one seems to follow the rule when it comes to test ? a : b, may as well change the rule.
It's more often than not written like throughout gecko code:
A ? B
  : C
Changing the title of this bug as it's now clear to me that this is an invalid implementation of the spec.

When an error occurs, we call void HTMLMediaElement::DecodeError(const MediaResult& aError)
which https://dxr.mozilla.org/mozilla-central/rev/215f9686117673a2c914ed207bc7da9bb8d741ad/dom/html/HTMLMediaElement.cpp#4400

will clear mLoadingSrc
as such, currentSrc is cleared.

per spec, media.currentSrc is never cleared, except when the object is created.
Otherwise, it's only ever set:
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-load-algorithm
Summary: Save URL in MediaError::message when an error occurs → src/currentSrc is cleared when an error occurred
(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Comment on attachment 8804098 [details]
> https://reviewboard.mozilla.org/r/88242/#review87202
> > Style: left-align '?' and ':' with 'src.Length()' above.
> I have taken the decision to ignore this coding-style "rule" from now on for
> two reasons: [...]

And I'll keep reporting this in reviews, as our team's "Vision and Culture" document says we should "Enforce Mozilla coding style".
(And I personally prefer the left-aligned style, it's consistent with binary operators.)

But it's true the operator-related guidelines are probably the least-followed ones, it's quite a mess out there. :-(
Maybe you should start discussing changing the guidelines, and/or using auto-formatting tools, in mozilla.dev.platform?
Comment on attachment 8804098 [details]
Bug 1312594: Do not clear resource when error occurs.

https://reviewboard.mozilla.org/r/88242/#review87230
Attachment #8804098 - Flags: review?(jwwang) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99efd9af4a01
Do not clear resource when error occurs. r=jwwang
https://hg.mozilla.org/mozilla-central/rev/99efd9af4a01
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.