Chia-hung Tai [:ctai] Reporter
	Description • 8 years ago

Once Bug 1201363 landed, there will no video buffer in MSG. Rename Bug 1201363 first.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 1 • 8 years ago

Carry r+ from pehrsons. See https://bugzilla.mozilla.org/show_bug.cgi?id=1201363#c87

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 3 • 8 years ago

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

	Andreas Pehrson [:pehrsons]
	Comment 4 • 8 years ago

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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 5 • 8 years ago

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.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 6 • 8 years ago

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.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 7 • 8 years ago

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7d36f091567&selectedJob=19842387

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 8 • 8 years ago

Carry r+

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 9 • 8 years ago

Carry r+.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 10 • 8 years ago

Carry r+.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 11 • 8 years ago

Carry r+.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 12 • 8 years ago

Carry r+.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 13 • 8 years ago

Carry r+.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 14 • 8 years ago

Carry r+.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 15 • 8 years ago

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.

	Paul Adenot (:padenot)
	Comment 16 • 8 years ago

Comment on attachment 8744802 [details] [diff] [review]
bug-1266644-5.diff

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

Yep, thanks for fixing this comment !

	Pulsebot
	Comment 17 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd16a818e34e
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0d98b53876
https://hg.mozilla.org/integration/mozilla-inbound/rev/41f21e3f7cbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bbea4dc90c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdb01638397
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ccb82588607
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f43cfbd45ca

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fd16a818e34e
https://hg.mozilla.org/mozilla-central/rev/eb0d98b53876
https://hg.mozilla.org/mozilla-central/rev/41f21e3f7cbd
https://hg.mozilla.org/mozilla-central/rev/5bbea4dc90c0
https://hg.mozilla.org/mozilla-central/rev/8cdb01638397
https://hg.mozilla.org/mozilla-central/rev/0ccb82588607
https://hg.mozilla.org/mozilla-central/rev/5f43cfbd45ca

	Cameron McCormack (:heycam)
	Comment 19 • 8 years ago

ctai, did you mean to move StreamTracks.h/cpp into the root directory?

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 20 • 8 years ago

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?

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Assignee
	Comment 21 • 8 years ago

Correct the hg history. Thanks for JW's help.

	Pulsebot
	Comment 23 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f873753780e
https://hg.mozilla.org/integration/mozilla-inbound/rev/58ea1b949927

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7f873753780e
https://hg.mozilla.org/mozilla-central/rev/58ea1b949927