Closed Bug 1438956 Opened 2 years ago Closed 2 years ago

Remove nsIDOMTimeRanges

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: adrian17, Assigned: adrian17)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1438956 - Remove nsIDOMTimeRanges.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1438956 - Add inline Start/End TimeRanges methods for C++ users.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1438956 - Modify HTMLMediaElement::Seek to avoid undefined behavior.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
After nsIDOMHTMLMediaElement was removed in bug 1407040, this should be a simple cleanup.
Assignee: nobody → adrian.wielgosik
Blocks: 1387169
Status: NEW → ASSIGNED
The second commit is optional - I noticed that IgnoreErrors() generated additional calls, and all C++ callers used safe ranges  anyway, so this felt like a good thing to add.
Comment on attachment 8951718 [details]
Bug 1438956 - Remove nsIDOMTimeRanges.

https://reviewboard.mozilla.org/r/220990/#review227018

::: dom/html/HTMLMediaElement.cpp:2710
(Diff revision 1)
> -  aIsInRanges = false;
> -  uint32_t length;
> +  uint32_t length = aRanges.Length();
> +
> -  nsresult rv = aRanges.GetLength(&length);
> -  NS_ENSURE_SUCCESS(rv, rv);
>    for (uint32_t i = 0; i < length; i++) {
> -    double start, end;
> +    double start = aRanges.Start(i, IgnoreErrors());

It would be better to actually have an IgnoredErrorResult for the Start() call and assert that it doesn't fail because our index is < length.

::: dom/html/HTMLMediaElement.cpp:2712
(Diff revision 1)
>    for (uint32_t i = 0; i < length; i++) {
> -    double start, end;
> +    double start = aRanges.Start(i, IgnoreErrors());
> -    rv = aRanges.Start(i, &start);
> -    NS_ENSURE_SUCCESS(rv, rv);
>      if (start > aValue) {
>        aIntervalIndex = i - 1;

This is preexisting, but nasty, because it is technically undefined behavior when i == 0...

Might be better to just do:

    aIntervalIndex = i > 0 ? i - 1 : -1;

::: dom/html/HTMLMediaElement.cpp:2715
(Diff revision 1)
> -    NS_ENSURE_SUCCESS(rv, rv);
>      if (start > aValue) {
>        aIntervalIndex = i - 1;
> -      return NS_OK;
> +      return false;
>      }
> -    rv = aRanges.End(i, &end);
> +    double end = aRanges.End(i, IgnoreErrors());

Again, should assert why this can't fail.

::: dom/html/HTMLMediaElement.cpp:2721
(Diff revision 1)
>        aIntervalIndex = i;
> -      aIsInRanges = true;
> +      return true;
> -      return NS_OK;
>      }
>    }
>    aIntervalIndex = length - 1;

This is again undefined behavior when length == 0...

::: dom/html/HTMLMediaElement.cpp:2804
(Diff revision 1)
>    if (!isInRange) {
>      if (range != -1) {
>        // |range + 1| can't be negative, because the only possible negative value
>        // for |range| is -1.
>        if (uint32_t(range + 1) < length) {
> -        double leftBound, rightBound;
> +        double leftBound = seekable->End(range, IgnoreErrors());

Again, should assert or document why ignoring errors is ok.

::: dom/html/HTMLMediaElement.cpp:2817
(Diff revision 1)
>          }
>          aTime = (distanceLeft < distanceRight) ? leftBound : rightBound;
>        } else {
>          // Seek target is after the end last range in seekable data.
>          // Clamp the seek target to the end of the last seekable range.
> -        if (NS_FAILED(seekable->End(length - 1, &aTime))) {
> +        aTime = seekable->GetEndTime();

Nice!

::: dom/html/HTMLMediaElement.cpp:2886
(Diff revision 1)
>    uint32_t timeRangeCount = 0;
>    if (mPlayed) {
> -    mPlayed->GetLength(&timeRangeCount);
> +    timeRangeCount = mPlayed->Length();
>    }
>    for (uint32_t i = 0; i < timeRangeCount; i++) {
> -    double begin;
> +    double begin = mPlayed->Start(i, IgnoreErrors());

Again, document or assert why can't fail.

::: dom/html/HTMLVideoElement.cpp:358
(Diff revision 1)
>    if (mPlayed) {
> -    uint32_t timeRangeCount = 0;
> +    uint32_t timeRangeCount = mPlayed->Length();
> -    mPlayed->GetLength(&timeRangeCount);
>  
>      for (uint32_t i = 0; i < timeRangeCount; i++) {
> -      double begin;
> +      double begin = mPlayed->Start(i, IgnoreErrors());

Document or assert why can't fail.
Attachment #8951718 - Flags: review?(bzbarsky) → review+
Comment on attachment 8951719 [details]
Bug 1438956 - Add inline Start/End TimeRanges methods for C++ users.

https://reviewboard.mozilla.org/r/220992/#review227020

::: dom/html/HTMLMediaElement.cpp:2710
(Diff revision 1)
>             int32_t& aIntervalIndex)
>  {
>    uint32_t length = aRanges.Length();
>  
>    for (uint32_t i = 0; i < length; i++) {
> -    double start = aRanges.Start(i, IgnoreErrors());
> +    double start = aRanges.Start(i);

Ah, I see.  OK, ignore the comments about asserting/documenting from part 1.  ;)
Attachment #8951719 - Flags: review?(bzbarsky) → review+
Priority: -- → P2
Comment on attachment 8951885 [details]
Bug 1438956 - Modify HTMLMediaElement::Seek to avoid undefined behavior.

https://reviewboard.mozilla.org/r/221160/#review227262

Thank you this is much nicer.  ;)

::: dom/html/HTMLMediaElement.cpp:2699
(Diff revision 2)
>  
>  /**
>   * Check if aValue is inside a range of aRanges, and if so returns true
>   * and puts the range index in aIntervalIndex. If aValue is not
>   * inside a range, returns false, and aIntervalIndex
> - * is set to the index of the range which ends immediately before aValue
> + * is set to the index of the range which ends immediately after aValue

Ends or starts?  It's the same thing if the ranges are disjoint, and reurning the index of the range which _starts_ after aValue would be a bit clearer...

Looking at the code, "starts" is closer to what we really check, and the ranges are in fact disjoint.
Attachment #8951885 - Flags: review?(bzbarsky) → review+
hg error in cmd: hg rebase -s e995b4d76186032fc048bc7b63f4e1e2aee046b4 -d 66b561299185: abort: can't rebase public changeset e995b4d76186
(see 'hg help phases' for details)
https://mozilla.logbot.info/ateam/20180220#c14333234-c14333982 from #ateam suggested attempting to reland the change. bz, could you make that happen?
Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86e9709e94b3
Remove nsIDOMTimeRanges. r=bz
https://hg.mozilla.org/integration/autoland/rev/63e144e6ac83
Add inline Start/End TimeRanges methods for C++ users. r=bz
https://hg.mozilla.org/integration/autoland/rev/bfc24ecd4246
Modify HTMLMediaElement::Seek to avoid undefined behavior. r=bz
Fun transient autoland failures...
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/86e9709e94b3
https://hg.mozilla.org/mozilla-central/rev/63e144e6ac83
https://hg.mozilla.org/mozilla-central/rev/bfc24ecd4246
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.