Closed Bug 1262358 Opened 4 years ago Closed 4 years ago

Simplify logic in HTMLMediaElement::CanActivateAutoplay()

Categories

(Core :: Audio/Video, defect)

44 Branch
All
Unspecified
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: kamidphish, Assigned: kamidphish)

References

Details

Attachments

(1 file)

Replace huge && and || condition with explicit statements.
Comment on attachment 8738825 [details]
MozReview Request: Bug 1262358 - Part 1: Split autoplay condition into statements. r?cpearce

https://reviewboard.mozilla.org/r/44713/#review41465
Attachment #8738825 - Flags: review?(cpearce) → review+
https://reviewboard.mozilla.org/r/44713/#review41467

::: dom/html/HTMLMediaElement.cpp:3995
(Diff revision 1)
>    // this element itself might be blocking the stream from making progress by
>    // being paused. We also activate autopaly when playing a media source since
>    // the data download is controlled by the script and there is no way to
>    // evaluate MediaDecoder::CanPlayThrough().
> -  return !mPausedForInactiveDocumentOrChannel &&
> -         mAutoplaying &&
> +
> +  if (!HasAttr(kNameSpaceID_None, nsGkAtoms::autoplay) || !mAutoplayEnabled) {

It would be nice to add comments to each condition so we have a clear understanding when not to activate autoplay for each condition.
https://reviewboard.mozilla.org/r/44713/#review41467

> It would be nice to add comments to each condition so we have a clear understanding when not to activate autoplay for each condition.

JW, I had a version that logged the reason why false was returned. Would that be more useful? I just feel that commenting what the if condition is just duplication and I'd rather read the code instead of a comment describing the same thing the code says.
(In reply to Dan Glastonbury :kamidphish from comment #4)
> JW, I had a version that logged the reason why false was returned. Would
> that be more useful?
As long as it it not so verbose. Spamming log messages sometimes do not provide information but hide them.

> I just feel that commenting what the if condition is
> just duplication and I'd rather read the code instead of a comment
> describing the same thing the code says.
Take MediaDecoderStateMachine::NeedToSkipToNextKeyframe() as an example. We need comments when there are deeper rationales behind the simple if statement.

E.g.

// Don't skip key frames when the media sink is not started. -- Bad!
if (!mMediaSink->IsStarted())

// When the media sink is not started, it is not consuming media data and
// we will never fall behind playback. So there is no need to skip key frames. -- Good!
if (!mMediaSink->IsStarted())
https://hg.mozilla.org/mozilla-central/rev/7035dda309c8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.