Closed Bug 1266644 Opened 4 years ago Closed 3 years ago

Rename StreamBuffers to StreamTracks.

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: ctai, Assigned: ctai)

References

Details

Attachments

(9 files, 2 obsolete files)

72.69 KB, patch
ctai
: review+
Details | Diff | Splinter Review
1.77 KB, patch
ctai
: review+
Details | Diff | Splinter Review
2.36 KB, patch
ctai
: review+
Details | Diff | Splinter Review
8.37 KB, patch
ctai
: review+
Details | Diff | Splinter Review
2.26 KB, patch
ctai
: review+
padenot
: review+
Details | Diff | Splinter Review
932 bytes, patch
ctai
: review+
Details | Diff | Splinter Review
17.12 KB, patch
ctai
: review+
Details | Diff | Splinter Review
24.22 KB, patch
Details | Diff | Splinter Review
722 bytes, patch
Details | Diff | Splinter Review
Once Bug 1201363 landed, there will no video buffer in MSG. Rename Bug 1201363 first.
Assignee: nobody → ctai
Attachment #8744170 - Flags: review?(rjesup)
Attached patch bug-1266644-2.diff (obsolete) — Splinter Review
Attachment #8744171 - Flags: review?(rjesup)
Attachment #8744171 - Flags: review?(pehrsons)
Simple refactor.
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #2)
> Created attachment 8744171 [details] [diff] [review]
> bug-1266644-2.diff
Comment on attachment 8744171 [details] [diff] [review]
bug-1266644-2.diff

Review of attachment 8744171 [details] [diff] [review]:
-----------------------------------------------------------------

Please divide these into smaller commits before landing. Feel free to carry forward my r+, modulo the inline comment below.

::: dom/media/MediaStreamGraphImpl.h
@@ +92,5 @@
>   * be able to friend it.
>   *
> + * There can be multiple MediaStreamGraph per process: one per AudioChannel.
> + * Additionaly, each OfflineAudioContext object creates its own MediaStreamGraph
> + * object too.

padenot should take a look at this.
Attachment #8744171 - Flags: review?(pehrsons) → review+
Comment on attachment 8744170 [details] [diff] [review]
bug-1266644-1.diff

Review of attachment 8744170 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but PLEASE instead of hg remove and hg add for StreamBuffer* -> StreamTrack*, use hg rename for each file.  This keeps history across the rename.
Attachment #8744170 - Flags: review?(rjesup) → review+
Attachment #8744171 - Flags: review?(rjesup) → review+
Thanks for the reminder, but I did use hg rename for StreamBuffer* -> StreamTrack*.
roc already reminded me in his review(https://bugzilla.mozilla.org/show_bug.cgi?id=1201363#c56).
And I did that in previous patch. See https://reviewboard.mozilla.org/r/42457/diff/1#index_header 
Also in the link of the detail of the attached patch, it shows 

diff --git a/dom/media/StreamBuffer.cpp b/StreamTracks.cpp
rename from dom/media/StreamBuffer.cpp
rename to StreamTracks.cpp
diff --git a/dom/media/StreamBuffer.h b/StreamTracks.h
rename from dom/media/StreamBuffer.h
rename to StreamTracks.h

See https://bugzilla.mozilla.org/attachment.cgi?id=8744170&action=edit

I think that might be caused by the diff and review page of bugzilla won't reflect that but mozreview did.
Anyway, thanks for your review. :)

(In reply to Randell Jesup [:jesup] from comment #5)
> Comment on attachment 8744170 [details] [diff] [review]
> bug-1266644-1.diff
> 
> Review of attachment 8744170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ but PLEASE instead of hg remove and hg add for StreamBuffer* ->
> StreamTrack*, use hg rename for each file.  This keeps history across the
> rename.
Carry r+
Attachment #8744170 - Attachment is obsolete: true
Attachment #8744171 - Attachment is obsolete: true
Attachment #8744798 - Flags: review+
Carry r+.
Attachment #8744799 - Flags: review+
Carry r+.
Attachment #8744800 - Flags: review+
Carry r+.
Attachment #8744801 - Flags: review+
Carry r+.
Attachment #8744802 - Flags: review+
Carry r+.
Attachment #8744803 - Flags: review+
Carry r+.
Attachment #8744804 - Flags: review+
Comment on attachment 8744802 [details] [diff] [review]
bug-1266644-5.diff

Hi, Paul,
pehrsons suggest me that you might review these comments in comment 5. Thanks.
Attachment #8744802 - Flags: review?(padenot)
Comment on attachment 8744802 [details] [diff] [review]
bug-1266644-5.diff

Review of attachment 8744802 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, thanks for fixing this comment !
Attachment #8744802 - Flags: review?(padenot) → review+
Rank: 25
Priority: -- → P2
ctai, did you mean to move StreamTracks.h/cpp into the root directory?
Flags: needinfo?(ctai)
Oh, no. May bad.
I would like to move to /dom/media/StreamTracks.cpp
Thanks for pointing out.


(In reply to Cameron McCormack (:heycam) from comment #19)
> ctai, did you mean to move StreamTracks.h/cpp into the root directory?
Flags: needinfo?(ctai)
Correct the hg history. Thanks for JW's help.
You need to log in before you can comment on or make changes to this bug.