Bug 111526 - Adding id attribute test for MediaStream
Summary: Adding id attribute test for MediaStream
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Li Yin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-05 23:25 PST by Li Yin
Modified: 2013-03-07 16:52 PST (History)
13 users (show)

See Also:


Attachments
Patch (13.28 KB, patch)
2013-03-06 01:04 PST, Li Yin
no flags Details | Formatted Diff | Diff
Patch (7.31 KB, patch)
2013-03-06 16:55 PST, Li Yin
haraken: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Yin 2013-03-05 23:25:13 PST
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.
Comment 1 Li Yin 2013-03-05 23:48:05 PST
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.
Comment 2 Tommy Widenflycht 2013-03-06 00:52:10 PST
I am much more comfortable in using createCanonicalUUIDString() since it is already used within the mediastream part of WebCore.
Comment 3 Li Yin 2013-03-06 01:04:02 PST
Created attachment 191679 [details]
Patch
Comment 4 Li Yin 2013-03-06 01:12:10 PST
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)".
Comment 5 WebKit Review Bot 2013-03-06 01:15:46 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 6 Li Yin 2013-03-06 01:16:48 PST
So if DRT used the same code like chromoum browser, more code branches can be verified, including RandomLabel and createCanonicalUUIDString.
Comment 7 Tommy Widenflycht 2013-03-06 06:48:31 PST
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.
Comment 8 Li Yin 2013-03-06 14:16:39 PST
(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?
Comment 9 Li Yin 2013-03-06 16:55:38 PST
Created attachment 191868 [details]
Patch
Comment 10 Li Yin 2013-03-06 16:57:58 PST
The new patch just added a test, no C++ code changes, of cause no changes of Chromium public API.
Comment 11 Tommy Widenflycht 2013-03-07 00:45:33 PST
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?
Comment 12 Kentaro Hara 2013-03-07 01:08:03 PST
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 ==.
Comment 13 Li Yin 2013-03-07 16:46:27 PST
Committed r145153: <http://trac.webkit.org/changeset/145153>
Comment 14 Li Yin 2013-03-07 16:52:18 PST
(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 :)