this patch needs to be localized and probably needs some UI love, but it works.

We only want to do this if accept="image/*" is there, right? Also, can we query to check if camera is really available. Seems silly to display the button when it's not. We possibly also don't want to change the size of the <input> when displaying the camera button. That will likely break a lot of pages unfortunately.

Comment on attachment 446682 [details] [diff] [review] patch Clearing this request for now. See comment #2

This patch creates two interfaces, one to tell layout whether or not to display the Capture button (nsICaptureService) and another to display the actual capture dialog (nsICapturePicker). This keeps the button from displaying on platforms where there is no capture support or where the capture service determines that there's no mic/camera attached. Roc, can you sr the interfaces here?

Doh! forgot to qrefresh.

This appears to pass tests. There was a lot of noise on try :-(

I think these interfaces should not live in widget. They don't really have anything to do with widgets. How about putting the implementations into toolkit/system or dom/system and the interfaces into xpcom/system or even layout/forms? Can nsICapturePicker describe what sort of URI will be returned? addParameter is very vague. Can we at least make it take the parameter name and value as separate string parameters? And document what some allowed parameters are? Ideally we would have proper attributes for setting parameters, e.g. a "type" attribute and a "frameRate" attribute. Would it make sense to combine nsICaptureService with nsICapturePicker, and just instantiate nsICapturePicker and then interrogate it by asking what modes it supports, e.g. with a supportsMode(mode) method?

The reason I put them in widget is because that's where nsIFilePicker lives. I'm fine with moving them somewhere else. We want nsICapturePicker to be able to return at least data, file, and moz-filedata URIs. I can add that to the interface comments. I'll see if I can come up with a list of relevant parameters we'd want. I think at first all we're really interested in is the type. The reason I have them as two separate interfaces at the moment is because determining whether or not you can capture content is different from actually capturing it, but I don't really have strong feelings either way.

Roc, I've addressed the comments on the interface. Mind taking another look at it? The actual code here needs some work, in particular we need to parse accept="" in an HTML 5 compliant manner.

Looks good, just one comment: + readonly attribute boolean stillsMaybeAvailable; + readonly attribute boolean audioMaybeAvailable; + readonly attribute boolean videoMaybeAvailable; Why not just one method, "boolean modeMaybeAvailable(long mode)"?

Because those modes would be different than the other modes (for determining what to capture). Other than that I have no problem changing it.

+ * @param flags Mode and type flags for what to capture + void init(in nsIDOMWindow parent, in AString title, in long flags); Shouldn't we call flags 'mode' instead? Why would the mode in modeMaybeAvailable be different to the modes you pass into 'init'? Seems to me modeMaybeAvailable(mode) is going to return false when init(mode) is guaranteed to fail, right? Your modes are + const long modeStill = 0; // Capture a still (image) + const long modeAudioClip = 1; // Capture a clip of audio + const long modeVideoClip = 2; // Capture a clip of video + const long modeVideoNoSoundClip = 3; // Capture a clip of video (no sound) and your attributes are + readonly attribute boolean stillsMaybeAvailable; + readonly attribute boolean audioMaybeAvailable; + readonly attribute boolean videoMaybeAvailable; So the only 'missing' attribute is videoNoSoundClipMaybeAvailable. Why we wouldn't we want to be able to ask about that? Surely there could be devices that don't have audio input but can capture video?

Yeah ok it does make more sense to do it that way.

I spun off parsing the accept attribute properly (as the HTML 5 spec says to) into Bug 574570.

+ boolean modeMaybeAvailable(in unsigned long mode); 2-space indent, and probably should be modeMayBeAvailable. + const long modeStill = 0; // Capture a still (image) + const long modeAudioClip = 1; // Capture a clip of audio + const long modeVideoClip = 2; // Capture a clip of video + const long modeVideoNoSoundClip = 3; // Capture a clip of video (no sound) + + // Return codes from the dialog + const long returnOK = 0; + const long returnCancel = 1; Should probably be MODE_STIL, etc, and "OK" and "CANCEL". + NS_LITERAL_STRING("Capture"), PR_FALSE); "capture" + // only allow the left button + nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aMouseEvent); + nsCOMPtr<nsIDOMNSUIEvent> uiEvent = do_QueryInterface(aMouseEvent); + NS_ENSURE_STATE(uiEvent); + PRBool defaultPrevented = PR_FALSE; + uiEvent->GetPreventDefault(&defaultPrevented); + if (defaultPrevented) { + return NS_OK; + } + + PRUint16 whichButton; + if (NS_FAILED(mouseEvent->GetButton(&whichButton)) || whichButton != 0) { + return NS_OK; + } + + PRInt32 clickCount; + if (NS_FAILED(mouseEvent->GetDetail(&clickCount)) || clickCount > 1) { + return NS_OK; + } Move this into a helper function that both mouse listeners can share? + nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(uri); + if (!fileURL) + return NS_ERROR_UNEXPECTED; Is there some plan for follow-up work to support non-file URLs?

Comment on attachment 454111 [details] [diff] [review] Patch This is looking great, though definitely want a layout person to look over the frame related changes. Though I would *love* it if we could get someone in UX to design a better UI so that we can enable 'capture' on all input elements. Another thing to keep in mind is that I think that on most desktop platforms, it's possible to extend the standard filepicker to add buttons. We could use that to add a "take picture" button in the normal filepicker. This would definitely involve a lot of platform specific hacking though, so something for a separate bug.

Comment on attachment 454111 [details] [diff] [review] Patch This needs some minor updates for my new patch on the blocking bug.

Carrying forward sicking's r+ because the changes are minor. Addressed roc's comments. There is a plan to support non-file URIs but that requires teaching the related code in content how do deal with things that aren't nsIFiles.

FWIW, as new is infallible, lines like NS_ENSURE_TRUE(mMouseListener, NS_ERROR_OUT_OF_MEMORY); NS_ENSURE_TRUE(mCaptureMouseListener, NS_ERROR_OUT_OF_MEMORY); aren't necessary. (Asserting that could be useful, of course.)

(In reply to comment #20) > FWIW, as new is infallible, lines like > > NS_ENSURE_TRUE(mMouseListener, NS_ERROR_OUT_OF_MEMORY); > NS_ENSURE_TRUE(mCaptureMouseListener, NS_ERROR_OUT_OF_MEMORY); > > aren't necessary. (Asserting that could be useful, of course.) Hit these. http://hg.mozilla.org/mozilla-central/rev/c765493e9530

Backed out due to failures. http://hg.mozilla.org/mozilla-central/rev/6c7a682b3faf

Relanded with simple changes. Factoring out the common code removed the abort early for non-left buttons, so I changed that method to return a boolean indicating whether or not to process the click. http://hg.mozilla.org/mozilla-central/rev/8ec5010204bc