Re: Implements mediaStreamTrack.getSettings() (issue 1937893002 by hta@chromium.org)

0 views
Skip to first unread message

h...@chromium.org

unread,
May 19, 2016, 2:04:40 PM5/19/16
to esp...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
On 2016/05/09 17:35:25, esprehn wrote:
> Can you add some methods or usage here? This is just dead code, we shouldn't
add
> patches that are dead code. You want to implement part of the feature, it's
okay
> if it doesn't really work. You can also write tests that have the "wrong"
output
> and then iterate as you land more code.

Done, PTAL.

(fighting with include rules et cetera led to some code movement too)


https://codereview.chromium.org/1937893002/

esp...@chromium.org

unread,
May 20, 2016, 4:07:05 PM5/20/16
to h...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
no friends please :)


https://codereview.chromium.org/1937893002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp
(right):

https://codereview.chromium.org/1937893002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp#newcode187
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:187:
if (platformSettings.m_hasFrameRate) {
use getters

https://codereview.chromium.org/1937893002/diff/40001/third_party/WebKit/public/platform/WebMediaStreamTrack.h
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/1937893002/diff/40001/third_party/WebKit/public/platform/WebMediaStreamTrack.h#newcode64
third_party/WebKit/public/platform/WebMediaStreamTrack.h:64: friend
class MediaStreamTrack;
why friends? Can we use getters or a struct instead? Not sure why this
has setters and no getters, and then uses friends

https://codereview.chromium.org/1937893002/

h...@chromium.org

unread,
May 23, 2016, 4:58:38 AM5/23/16
to esp...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
PTAL.

I also renamed "extraData" to "trackData" since it now has operations that are
track-specific.




https://codereview.chromium.org/1937893002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp
(right):

https://codereview.chromium.org/1937893002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp#newcode187
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:187:
if (platformSettings.m_hasFrameRate) {
On 2016/05/20 20:07:04, esprehn wrote:
> use getters

Is now struct.


https://codereview.chromium.org/1937893002/diff/40001/third_party/WebKit/public/platform/WebMediaStreamTrack.h
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/1937893002/diff/40001/third_party/WebKit/public/platform/WebMediaStreamTrack.h#newcode64
third_party/WebKit/public/platform/WebMediaStreamTrack.h:64: friend
class MediaStreamTrack;
On 2016/05/20 20:07:05, esprehn wrote:
> why friends? Can we use getters or a struct instead? Not sure why this
has
> setters and no getters, and then uses friends

esp...@chromium.org

unread,
May 26, 2016, 12:35:54 AM5/26/16
to h...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
Your change description doesn't look right?


https://codereview.chromium.org/1937893002/diff/60001/third_party/WebKit/public/platform/WebMediaStreamTrack.h
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/1937893002/diff/60001/third_party/WebKit/public/platform/WebMediaStreamTrack.h#newcode46
third_party/WebKit/public/platform/WebMediaStreamTrack.h:46: :
hasFrameRate(false)
why not initialize all the other fields too? This means you can get
garbage sometimes

https://codereview.chromium.org/1937893002/diff/60001/third_party/WebKit/public/platform/WebMediaStreamTrack.h#newcode53
third_party/WebKit/public/platform/WebMediaStreamTrack.h:53: frameRate =
rate;
can we make frameRate be -1 by default? What does a negative frameRate
mean?

https://codereview.chromium.org/1937893002/

h...@chromium.org

unread,
May 26, 2016, 1:08:12 AM5/26/16
to esp...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
Fixed change description. Changing the settings class as suggested.
On 2016/05/26 04:35:54, esprehn wrote:
> why not initialize all the other fields too? This means you can get
garbage
> sometimes

Can do that. WebStrings are self-nulling, and if hasFrameRate is false,
I don't care about rate, but if following your suggestion below, need to
init frameRate, so might as well do them all.
On 2016/05/26 04:35:54, esprehn wrote:
> can we make frameRate be -1 by default? What does a negative frameRate
mean?

Can do that for frameRate, since it's semantically nonsense if negative.
Need to document the semantics in a comment, since it's not obvious from
the type.

https://codereview.chromium.org/1937893002/

esp...@chromium.org

unread,
May 26, 2016, 1:22:09 AM5/26/16
to h...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
Okay lgtm w/ the struct init stuff fixed.


https://codereview.chromium.org/1937893002/diff/60001/third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getSettings.html
File
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getSettings.html
(right):

https://codereview.chromium.org/1937893002/diff/60001/third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getSettings.html#newcode3
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getSettings.html:3:
<head>
I'd leave out html, head and body. They're not needed.

https://codereview.chromium.org/1937893002/

h...@chromium.org

unread,
May 26, 2016, 2:01:19 AM5/26/16
to esp...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
Thanks!

@jochen @guido @tommi ping
On 2016/05/26 05:22:09, esprehn wrote:
> I'd leave out html, head and body. They're not needed.

Done. This leaves it non-obvious whether the scripts occur in the head
or in the body (I assume it's well-defined); I assume that nobody cares.

https://codereview.chromium.org/1937893002/

esp...@chromium.org

unread,
May 26, 2016, 2:10:55 AM5/26/16
to h...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
On 2016/05/26 at 06:01:19, hta wrote:
>...

>
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getSettings.html:3:
<head>
> On 2016/05/26 05:22:09, esprehn wrote:
> > I'd leave out html, head and body. They're not needed.
>
> Done. This leaves it non-obvious whether the scripts occur in the head or in
the body (I assume it's well-defined); I assume that nobody cares.

The scripts are in the <head>, which is typically where they are in tests
anyway. The parser will insert a <head> for you, and it'll implicitly close it
upon encountering any element that isn't metadata:

See: https://html.spec.whatwg.org/multipage/semantics.html#the-head-element

and the list of allowed elements here:
https://html.spec.whatwg.org/multipage/dom.html#metadata-content-2

After closing it everything gets put into the <body>

So

<!doctype html>
<script>...</script>
<a></a>

makes the same document (ignoring whitespace) as:

<!doctype html>
<html>
<head>
<script>...</script>
</head>
<body>
<a></a>
</body>
</html>


https://codereview.chromium.org/1937893002/

gui...@chromium.org

unread,
May 26, 2016, 4:29:33 AM5/26/16
to h...@chromium.org, esp...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
lgtm % nits


https://codereview.chromium.org/1937893002/diff/100001/third_party/WebKit/public/platform/WebMediaStreamTrack.h
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/1937893002/diff/100001/third_party/WebKit/public/platform/WebMediaStreamTrack.h#newcode47
third_party/WebKit/public/platform/WebMediaStreamTrack.h:47: ,
deviceId()
nit: This one should be unnecessary as the default constructor applies
by default.

https://codereview.chromium.org/1937893002/diff/100001/third_party/WebKit/public/platform/WebMediaStreamTrack.h#newcode51
third_party/WebKit/public/platform/WebMediaStreamTrack.h:51: bool
hasFrameRate()
Since we have this one, shouldn't we have hasXXX methods for all
properties for consistency?

https://codereview.chromium.org/1937893002/

to...@chromium.org

unread,
May 26, 2016, 4:51:06 AM5/26/16
to h...@chromium.org, esp...@chromium.org, gui...@chromium.org, joc...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
https://codereview.chromium.org/1937893002/diff/100001/third_party/WebKit/public/platform/WebMediaStreamTrack.h#newcode46
third_party/WebKit/public/platform/WebMediaStreamTrack.h:46: :
frameRate(-1.0)
fyi - you could also initialize these members inline

https://codereview.chromium.org/1937893002/

h...@chromium.org

unread,
May 26, 2016, 5:52:35 AM5/26/16
to esp...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
Trying to submit now....
On 2016/05/26 08:51:05, tommi-chrömium wrote:
> remove {}

Done.
On 2016/05/26 08:51:06, tommi-chrömium wrote:
> fyi - you could also initialize these members inline

Done.
On 2016/05/26 08:29:33, Guido Urdaneta wrote:
> nit: This one should be unnecessary as the default constructor applies
by
> default.

Might as well list them all.
On 2016/05/26 08:29:33, Guido Urdaneta wrote:
> Since we have this one, shouldn't we have hasXXX methods for all
properties for
> consistency?

Esprehn suggested putting in the getter as a means of documenting that
negative values are treated as "not present". For values that already
have a "not present" value, it seems unnecessary.

https://codereview.chromium.org/1937893002/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 26, 2016, 5:53:06 AM5/26/16
to h...@chromium.org, esp...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 26, 2016, 7:27:08 AM5/26/16
to h...@chromium.org, esp...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
Committed patchset #7 (id:120001)

https://codereview.chromium.org/1937893002/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 26, 2016, 7:28:39 AM5/26/16
to h...@chromium.org, esp...@chromium.org, gui...@chromium.org, joc...@chromium.org, to...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, har...@chromium.org, j...@chromium.org, kinuko...@chromium.org, mcasas+watch...@chromium.org, mcasas+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, poscia...@chromium.org, tommyw+w...@chromium.org
Patchset 7 (id:??) landed as
https://crrev.com/aa40992364d32544f7301ad6791b302736f651d5
Cr-Commit-Position: refs/heads/master@{#396160}

https://codereview.chromium.org/1937893002/
Reply all
Reply to author
Forward
0 new messages