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)
Mozilla QA Graveyard
Infrastructure
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: jsmith, Assigned: jsmith)
Details
Attachments
(1 file, 6 obsolete files)
2.54 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #648861 -
Flags: review?(hskupin)
Assignee | ||
Comment 2•12 years ago
|
||
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)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #648864 -
Flags: review?(hskupin)
Assignee | ||
Updated•12 years ago
|
Attachment #648864 -
Attachment is obsolete: true
Attachment #648864 -
Attachment is patch: false
Attachment #648864 -
Flags: review?(hskupin)
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 648865 [details]
Basic Video Test
Hopefully I built the changeset correctly this time...
Attachment #648865 -
Flags: review?(hskupin)
Assignee | ||
Updated•12 years ago
|
Attachment #648865 -
Attachment is obsolete: true
Attachment #648865 -
Attachment is patch: false
Attachment #648865 -
Flags: review?(hskupin)
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 648869 [details] [diff] [review] Basic Video Test Fixed the tabbing usage (converted it into spaces).
Attachment #648869 -
Flags: review?(hskupin)
Updated•12 years ago
|
Assignee: nobody → jsmith
Status: NEW → ASSIGNED
Component: General → Infrastructure
Comment 8•12 years ago
|
||
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-
Assignee | ||
Comment 9•12 years ago
|
||
(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.
Comment 10•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Attachment #648869 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
Updated based on review comments.
Attachment #651220 -
Flags: review?(hskupin)
Assignee | ||
Updated•12 years ago
|
Attachment #651220 -
Attachment is obsolete: true
Attachment #651220 -
Flags: review?(hskupin)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #651244 -
Flags: review?(hskupin)
Comment 13•12 years ago
|
||
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-
Assignee | ||
Updated•12 years ago
|
Attachment #651244 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #651384 -
Flags: review?(hskupin)
Updated•12 years ago
|
Attachment #651384 -
Flags: review?(hskupin) → review+
Comment 15•12 years ago
|
||
http://hg.mozilla.org/qa/litmus-data/rev/46b942c1040f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•