1373577 - Devritualize MediaResource::MediaReadAt()

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[JW Wang [:jwwang] [:jw_wang]]

The difference is the sub-class override doesn't call DispatchBytesConsumed(). It doesn't make sense not to call that after you do consume some data. For callers that want silent reads, they should call ReadFromCache() instead.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1373577. P1 - devritualize MediaResource::MediaReadAt().

The difference is the sub-class override doesn't call DispatchBytesConsumed().
It doesn't make sense not to call that after you do consume some data.
For callers that want silent reads, they should call ReadFromCache() instead.

Review commit: https://reviewboard.mozilla.org/r/149730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149730/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1373577. P2 - move the code of MediaResource::MediaReadAt() into MediaResourceIndex.

MediaReadAt() accesses only public members of MediaResource. It doesn't has
to be a member of MediaResource.

Review commit: https://reviewboard.mozilla.org/r/149732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149732/

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1373577. P3 - fix the comment since MediaResource::ReatAt() does update the offset of MediaResource.

Review commit: https://reviewboard.mozilla.org/r/149734/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149734/

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8878390 [details]
Bug 1373577. P1 - devritualize MediaResource::MediaReadAt().

https://reviewboard.mozilla.org/r/149730/#review154398

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8878391 [details]
Bug 1373577. P2 - move the code of MediaResource::MediaReadAt() into MediaResourceIndex.

https://reviewboard.mozilla.org/r/149732/#review154400

::: commit-message-127e4:4
(Diff revision 1)
> +Bug 1373577. P2 - move the code of MediaResource::MediaReadAt() into MediaResourceIndex.
> +
> +MediaReadAt() accesses only public members of MediaResource. It doesn't has
> +to be a member of MediaResource.

it doesn't have to be

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8878392 [details]
Bug 1373577. P3 - fix the comment since MediaResource::ReatAt() does update the offset of MediaResource.

https://reviewboard.mozilla.org/r/149734/#review154402

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Thanks!

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8878391 [details]
Bug 1373577. P2 - move the code of MediaResource::MediaReadAt() into MediaResourceIndex.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149732/diff/1-2/

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8878392 [details]
Bug 1373577. P3 - fix the comment since MediaResource::ReatAt() does update the offset of MediaResource.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149734/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0eacbd673e5a
P1 - devritualize MediaResource::MediaReadAt(). r=jya
https://hg.mozilla.org/integration/autoland/rev/5f983c055532
P2 - move the code of MediaResource::MediaReadAt() into MediaResourceIndex. r=jya
https://hg.mozilla.org/integration/autoland/rev/c8d764a48d58
P3 - fix the comment since MediaResource::ReatAt() does update the offset of MediaResource. r=jya

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0eacbd673e5a
https://hg.mozilla.org/mozilla-central/rev/5f983c055532
https://hg.mozilla.org/mozilla-central/rev/c8d764a48d58