Closed Bug 780286 Opened 12 years ago Closed 12 years ago

Add a basic video test for the Camera API to the mozqa.com

Categories

(Mozilla QA Graveyard :: Infrastructure, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jsmith, Assigned: jsmith)

Details

Attachments

(1 file, 6 obsolete files)

I'm looking to add a basic test case for the camera API for desktop firefox. See the attached patch for what I'm adding.

Docs for the unprefixed version are here:

http://dev.w3.org/2011/webrtc/editor/getusermedia.html

The prefixed version isn't documented yet.
Attached file Basic Video Test Patch (obsolete) —
Attachment #648861 - Flags: review?(hskupin)
Comment on attachment 648861 [details]
Basic Video Test Patch

Actually, there's an error in the format of this patch. Let me fix that first.
Attachment #648861 - Attachment is obsolete: true
Attachment #648861 - Attachment is patch: false
Attachment #648861 - Flags: review?(hskupin)
Attached file Basic Video Test (obsolete) —
Attachment #648864 - Flags: review?(hskupin)
Attachment #648864 - Attachment is obsolete: true
Attachment #648864 - Attachment is patch: false
Attachment #648864 - Flags: review?(hskupin)
Attached file Basic Video Test (obsolete) —
Comment on attachment 648865 [details]
Basic Video Test

Hopefully I built the changeset correctly this time...
Attachment #648865 - Flags: review?(hskupin)
Attachment #648865 - Attachment is obsolete: true
Attachment #648865 - Attachment is patch: false
Attachment #648865 - Flags: review?(hskupin)
Attached patch Basic Video Test (obsolete) — Splinter Review
Comment on attachment 648869 [details] [diff] [review]
Basic Video Test

Fixed the tabbing usage (converted it into spaces).
Attachment #648869 - Flags: review?(hskupin)
Assignee: nobody → jsmith
Status: NEW → ASSIGNED
Component: General → Infrastructure
Comment on attachment 648869 [details] [diff] [review]
Basic Video Test

># Parent  32769bdd3ffa0afaf176d75977ba72301a9f8a2a
>Added a basic video test for the camera API.

Please also add the reviewer to the commit message. Same format as for Firefox core development.

>+++ b/firefox/camera/Basic Video Test.html	Fri Aug 03 14:59:57 2012 -0700

This is definitely not the right path to the test. I would propose we stick everything under /webapi/ or /webrtc/ because those tests apply to more products than only Firefox. If you will have more than a single test for camera feel free to even add another sub folder.

>+<!doctype html>
>+<html>
>+    <head>

HTML documents should have an indentation of 2 chars.

>+            function streamCameraToVideo() {

Where does this method come from? Is it a valid source or self-written?

>+                if(navigator.mozGetUserMedia) {

nit: please put a blank between if and the opening bracket.

>+                        stream.onended = noStream;

Why do you want to show a failure when the stream ended? Because it's not an user action?

>+                    navigator.mozGetUserMedia({video: true}, gotStream,
>+                                              noStream);

Would you mind to make those callbacks global in the JS scope? It would make the code better readable.

>+                } else {
>+
>+                    alert("Error: mozGetUserMedia is not enabled");
>+
>+                }

You shouldn't put up an alert on this page. Instead show it as highlighted text inside the monitor node. Also I would prefer a try/catch here. Additionally please kill those empty lines.

>+        <p>
>+            If your camera becomes active after this page loads and you see
>+            yourself in this page, then this test verifies.
>+        </p>

nit: just put everything into a single line. I don't see a reason for new lines here.
Attachment #648869 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 648869 [details] [diff] [review]
> Basic Video Test
> 
> ># Parent  32769bdd3ffa0afaf176d75977ba72301a9f8a2a
> >Added a basic video test for the camera API.
> 
> Please also add the reviewer to the commit message. Same format as for
> Firefox core development.
> 
> >+++ b/firefox/camera/Basic Video Test.html	Fri Aug 03 14:59:57 2012 -0700
> 
> This is definitely not the right path to the test. I would propose we stick
> everything under /webapi/ or /webrtc/ because those tests apply to more
> products than only Firefox. If you will have more than a single test for
> camera feel free to even add another sub folder.

So would /webapi/camera/Basic Video Test.html work then?

> 
> >+<!doctype html>
> >+<html>
> >+    <head>
> 
> HTML documents should have an indentation of 2 chars.
> 
> >+            function streamCameraToVideo() {
> 
> Where does this method come from? Is it a valid source or self-written?
> 

Self-Written. It mainly exists as a callback when the page loads to get the user's camera video stream and also show it to them.

> 
> >+                        stream.onended = noStream;
> 
> Why do you want to show a failure when the stream ended? Because it's not an
> user action?
> 

Right. It would be unexpected if the stream stopped, as the duration of a MediaStream from mozGetUserMedia is infinity. 

See http://www.w3.org/TR/mediacapture-streams/#mediastreams-as-media-elements for more information. 

Note - The spec isn't finalized, so if you have feedback on it, feel free to give some feedback.
(In reply to Jason Smith [:jsmith] from comment #9)
> So would /webapi/camera/Basic Video Test.html work then?

I would take /webapi/camera/video_basic.html so we can have tests sorted by type of media.

> > >+                        stream.onended = noStream;
> > 
> > Why do you want to show a failure when the stream ended? Because it's not an
> > user action?
> > 
> 
> Right. It would be unexpected if the stream stopped, as the duration of a
> MediaStream from mozGetUserMedia is infinity. 
> 
> See
> http://www.w3.org/TR/mediacapture-streams/#mediastreams-as-media-elements
> for more information. 
> 
> Note - The spec isn't finalized, so if you have feedback on it, feel free to
> give some feedback.

Sounds fine then. So in this case do the same as for an initial error.
Attachment #648869 - Attachment is obsolete: true
Attached patch Basic Video Test (obsolete) — Splinter Review
Updated based on review comments.
Attachment #651220 - Flags: review?(hskupin)
Attachment #651220 - Attachment is obsolete: true
Attachment #651220 - Flags: review?(hskupin)
Attached patch Basic Video Test (obsolete) — Splinter Review
Attachment #651244 - Flags: review?(hskupin)
Comment on attachment 651244 [details] [diff] [review]
Basic Video Test

>Added a basic video test for mozGetUserMedia. r=hskupin

nit: Next time please add the bug id like 'Bug XYZ - ' as prefix for the commit message for reference.

>+    /**
>+     * Gets a reference to the video node on the page.
>+     *
>+     * @return The DOM object referencing the video page node
>+     */
>+    function getVideoNode() {
>+      return document.getElementById('monitor');
>+    }

Why do we need this as an extra method? Just move it back to 'gotStream()' and we are fine.

>+        noStream(new Error("Stream unexpectedly stopped"));

Why do we need a full exception error instance here? You should simply pass in the string and assign it to the textContent property. Probably you should call the method onError().

>+    $(document).ready(function() {

nit: for all noname functions please add a blank before the parameters opening bracket: 'function ('

Otherwise looks fine and should be ready to checkin after the next update.
Attachment #651244 - Flags: review?(hskupin) → review-
Attachment #651244 - Attachment is obsolete: true
Attached patch Basic Video TestSplinter Review
Attachment #651384 - Flags: review?(hskupin)
Attachment #651384 - Flags: review?(hskupin) → review+
http://hg.mozilla.org/qa/litmus-data/rev/46b942c1040f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: