Some ChannelMediaResource/MediaCacheStream code cleanup

RESOLVED FIXED in Firefox 57

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(12 attachments)

59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
59 bytes, text/x-review-board-request
gerald
: review+
Details | Review
Comment hidden (empty)
(Assignee)

Updated

3 months ago
Assignee: nobody → jwwang
Blocks: 1382978
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8906453 - Flags: review?(gsquelart)
Attachment #8906454 - Flags: review?(gsquelart)
Attachment #8906455 - Flags: review?(gsquelart)
Attachment #8906456 - Flags: review?(gsquelart)
Attachment #8906457 - Flags: review?(gsquelart)
Attachment #8906458 - Flags: review?(gsquelart)
Attachment #8906459 - Flags: review?(gsquelart)
Attachment #8906460 - Flags: review?(gsquelart)
Attachment #8906461 - Flags: review?(gsquelart)
Attachment #8906462 - Flags: review?(gsquelart)
Attachment #8906463 - Flags: review?(gsquelart)
Attachment #8906464 - Flags: review?(gsquelart)

Comment 13

3 months ago
mozreview-review
Comment on attachment 8906453 [details]
Bug 1398659. P1 - tighten up the assertions in InitAsClone().

https://reviewboard.mozilla.org/r/178186/#review183104
Attachment #8906453 - Flags: review?(gsquelart) → review+

Comment 14

3 months ago
mozreview-review
Comment on attachment 8906454 [details]
Bug 1398659. P2 - remove the if statement since new() is infallible.

https://reviewboard.mozilla.org/r/178188/#review183106
Attachment #8906454 - Flags: review?(gsquelart) → review+

Comment 15

3 months ago
mozreview-review
Comment on attachment 8906455 [details]
Bug 1398659. P3 - ChannelMediaResource::Open() is a no-op for a cloned resource.

https://reviewboard.mozilla.org/r/178190/#review183110

Indeed.
Attachment #8906455 - Flags: review?(gsquelart) → review+

Comment 16

3 months ago
mozreview-review
Comment on attachment 8906456 [details]
Bug 1398659. P4 - remove unused code and add some assertions.

https://reviewboard.mozilla.org/r/178192/#review183114
Attachment #8906456 - Flags: review?(gsquelart) → review+

Comment 17

3 months ago
mozreview-review
Comment on attachment 8906457 [details]
Bug 1398659. P5 - let ChannelMediaResource::Open() set mListener without calling OpenChannel().

https://reviewboard.mozilla.org/r/178194/#review183120
Attachment #8906457 - Flags: review?(gsquelart) → review+

Comment 18

3 months ago
mozreview-review
Comment on attachment 8906458 [details]
Bug 1398659. P6 - remove the nsIStreamListener** parameter from OpenChannel().

https://reviewboard.mozilla.org/r/178196/#review183122

::: commit-message-dada9:3
(Diff revision 1)
> +Bug 1398659. P6 - remove the nsIStreamListener** parameter from OpenChannel().
> +
> +The only caller is CacheClientSeek() which always pass nullptr to it.

'pass' -> 'passes'
Attachment #8906458 - Flags: review?(gsquelart) → review+

Comment 19

3 months ago
mozreview-review
Comment on attachment 8906459 [details]
Bug 1398659. P7 - tighten up the assertions in OpenChannel().

https://reviewboard.mozilla.org/r/178198/#review183124
Attachment #8906459 - Flags: review?(gsquelart) → review+

Comment 20

3 months ago
mozreview-review
Comment on attachment 8906460 [details]
Bug 1398659. P8 - remove the call to mCacheStream.NotifyDataLength() from OpenChannel().

https://reviewboard.mozilla.org/r/178200/#review183126
Attachment #8906460 - Flags: review?(gsquelart) → review+

Comment 21

3 months ago
mozreview-review
Comment on attachment 8906461 [details]
Bug 1398659. P9 - remove unused member.

https://reviewboard.mozilla.org/r/178202/#review183128
Attachment #8906461 - Flags: review?(gsquelart) → review+

Comment 22

3 months ago
mozreview-review
Comment on attachment 8906462 [details]
Bug 1398659. P10 - remove ChannelMediaResource::mIgnoreClose.

https://reviewboard.mozilla.org/r/178204/#review183130

::: commit-message-12a82:4
(Diff revision 1)
> +Bug 1398659. P10 - remove ChannelMediaResource::mIgnoreClose.
> +
> +mIgnoreClose is always set in conjunction with a call to CloseChannel().
> +Sincce mListener->Revoke() will prevent future OnStopRequest() calls from coming,

'Sincce' -> 'Since'
Attachment #8906462 - Flags: review?(gsquelart) → review+

Comment 23

3 months ago
mozreview-review
Comment on attachment 8906463 [details]
Bug 1398659. P11 - replace mCacheStream.GetLength() with GetLength().

https://reviewboard.mozilla.org/r/178206/#review183132
Attachment #8906463 - Flags: review?(gsquelart) → review+

Comment 24

3 months ago
mozreview-review
Comment on attachment 8906464 [details]
Bug 1398659. P12 - remove unused MediaCacheStream::mHasHadUpdate.

https://reviewboard.mozilla.org/r/178208/#review183134
Attachment #8906464 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 25

3 months ago
Thanks for the reviews!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

3 months ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3563eaa019d3
P1 - tighten up the assertions in InitAsClone(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/3928ea56c1d3
P2 - remove the if statement since new() is infallible. r=gerald
https://hg.mozilla.org/integration/autoland/rev/2a71c9276c31
P3 - ChannelMediaResource::Open() is a no-op for a cloned resource. r=gerald
https://hg.mozilla.org/integration/autoland/rev/bd93f2e8986b
P4 - remove unused code and add some assertions. r=gerald
https://hg.mozilla.org/integration/autoland/rev/aae0f0941029
P5 - let ChannelMediaResource::Open() set mListener without calling OpenChannel(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/020a6054e228
P6 - remove the nsIStreamListener** parameter from OpenChannel(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/44e400e8dc12
P7 - tighten up the assertions in OpenChannel(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/9692fc0d7937
P8 - remove the call to mCacheStream.NotifyDataLength() from OpenChannel(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/efd3ef5ae6f9
P9 - remove unused member. r=gerald
https://hg.mozilla.org/integration/autoland/rev/665409f64b70
P10 - remove ChannelMediaResource::mIgnoreClose. r=gerald
https://hg.mozilla.org/integration/autoland/rev/fadb12f8f604
P11 - replace mCacheStream.GetLength() with GetLength(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/130a5e787ba6
P12 - remove unused MediaCacheStream::mHasHadUpdate. r=gerald
https://hg.mozilla.org/mozilla-central/rev/3563eaa019d3
https://hg.mozilla.org/mozilla-central/rev/3928ea56c1d3
https://hg.mozilla.org/mozilla-central/rev/2a71c9276c31
https://hg.mozilla.org/mozilla-central/rev/bd93f2e8986b
https://hg.mozilla.org/mozilla-central/rev/aae0f0941029
https://hg.mozilla.org/mozilla-central/rev/020a6054e228
https://hg.mozilla.org/mozilla-central/rev/44e400e8dc12
https://hg.mozilla.org/mozilla-central/rev/9692fc0d7937
https://hg.mozilla.org/mozilla-central/rev/efd3ef5ae6f9
https://hg.mozilla.org/mozilla-central/rev/665409f64b70
https://hg.mozilla.org/mozilla-central/rev/fadb12f8f604
https://hg.mozilla.org/mozilla-central/rev/130a5e787ba6
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.