Closed
Bug 1438956
Opened 5 years ago
Closed 5 years ago
Remove nsIDOMTimeRanges
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: adrian17, Assigned: adrian17)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
After nsIDOMHTMLMediaElement was removed in bug 1407040, this should be a simple cleanup.
Assignee | ||
Updated•5 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•5 years ago
|
||
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 4•5 years ago
|
||
mozreview-review |
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 5•5 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•5 years ago
|
Priority: -- → P2
![]() |
||
Comment 8•5 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment 10•5 years ago
|
||
hg error in cmd: hg rebase -s e995b4d76186032fc048bc7b63f4e1e2aee046b4 -d 66b561299185: abort: can't rebase public changeset e995b4d76186 (see 'hg help phases' for details)
Comment 11•5 years ago
|
||
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)
Comment 12•5 years ago
|
||
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
Comment 14•5 years ago
|
||
bugherder |
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: 5 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•