Bug 111526 – Adding id attribute test for MediaStream

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Li Yin]

Spec: http://dev.w3.org/2011/webrtc/editor/getusermedia.html#widl-MediaStream-id
When a LocalMediaStream object is created, the user agent must generate a globally unique identifier string, and must initialize the object’s id attribute to that string. Such strings must only use characters in the ranges U+0021, U+0023 to U+0027, U+002A to U+002B, U+002D to U+002E, U+0030 to U+0039, U+0041 to U+005A, U+005E to U+007E, and must be 36 characters long.

Currently, DRT can't support id verification correctly, and the related test is missed also.

[Li Yin]

Hi Tommy,
I justed prepared a patch to fixed the MediaStream.id issue for DumpRenderTree.
I am copying the fucntion RandomLabel() from media_stream_manager.cc file.

It seems your commit http://trac.webkit.org/changeset/144748 has moved the producing id code to WebMediaStream.cpp

Maybe use RandomLabel function is better for verifing LocalMediaStream.id attribute.

[Tommy Widenflycht]

I am much more comfortable in using createCanonicalUUIDString() since it is already used within the mediastream part of WebCore.

[Li Yin]

Created attachment 191679 [details]
Patch

[Li Yin]

I just saw your commit after the patch is almost finished.
So I still post it to have a review.

In real environment, browser used RandomLabel function to generate an id string, when the mediastream is generated through getUserMedia().

And function createCanonicalUUIDString() still can be verified through "new MediaStream(stream)".

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Li Yin]

So if DRT used the same code like chromoum browser, more code branches can be verified, including RandomLabel and createCanonicalUUIDString.

[Tommy Widenflycht]

Comment on attachment 191679 [details]
Patch

I don't particularly like the changes to DRT and WebMediaStream since they makes your tests test data from DRT.
The RandomLabel function is only used internally within chromium and its output is never visible in WebKit.

[Li Yin]

(In reply to comment #7)
> (From update of attachment 191679 [details])
> I don't particularly like the changes to DRT and WebMediaStream since they makes your tests test data from DRT.
> The RandomLabel function is only used internally within chromium and its output is never visible in WebKit.

OK, I have no very strong expectation about the change.
And it seems the test should be reasonable. What is your opinion?

[Li Yin]

Created attachment 191868 [details]
Patch

[Li Yin]

The new patch just added a test, no C++ code changes, of cause no changes of Chromium public API.

[Tommy Widenflycht]

The test is just fine.

Btw I have noticed that you like to work on WebRTC. Why don't you ping me offline so that we can sync our efforts?

[Kentaro Hara]

Comment on attachment 191868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191868&action=review

LGTM too, with some nits.

> LayoutTests/fast/mediastream/MediaStreamConstructor.html:31
> +        if (charCode == 0x21 ||
> +            (charCode >= 0x23 && charCode <= 0x27) ||
> +            (charCode >= 0x2A && charCode <= 0x2B) ||
> +            (charCode >= 0x2D && charCode <= 0x2E) ||
> +            (charCode >= 0x30 && charCode <= 0x39) ||
> +            (charCode >= 0x41 && charCode <= 0x5A) ||
> +            (charCode >= 0x5E && charCode <= 0x7E))

Nit: || should come at the head of the next line, instead of the tail of the current line.

http://www.webkit.org/coding/coding-style.html#indentation-case-label

> LayoutTests/fast/mediastream/MediaStreamConstructor.html:36
> +        if (id == idArray[i])

Nit: For testing purpose, let's use === instead of ==.

[Li Yin]

Committed r145153: <http://trac.webkit.org/changeset/145153>

[Li Yin]

(In reply to comment #11)
> The test is just fine.
> 
> Btw I have noticed that you like to work on WebRTC. Why don't you ping me offline so that we can sync our efforts?

Okay, that is a good idea, and I just sent an email to you about my work, please have a check.
And my name is liyin in WebKit IRC, you can ping me, it seems you are offline when I an trying to ping you :)