Comment 1 [:fabrice] Fabrice Desré 2010-09-08 01:40:52 PDT

hi Kyle,
Can you add the dependencies on this bug? For instance, I'd like to see what it would take to port capturepicker.xul to fennec but can't find it in mxr.

Comment 2 Doug Turner (:dougt) 2010-09-28 22:12:45 PDT

camera is not blocking fennec 2.0, sadly.

Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-11 22:33:55 PDT

Comment on attachment 472508 [details] [diff] [review]
Patch

beyond the obvious bit rot, we don't want to use |  parent.openDialog("chrome://global/content/capturepicker.xul" | approaches in Fennec. We use importDialog to pull XUL into the main window instead of opening new windows.

Comment 4 [:fabrice] Fabrice Desré 2011-05-10 12:15:54 PDT

Created attachment 531403 [details] [diff] [review]
patch

This patch splits the implementation in two parts due to e10s.

A simple test page is available here : http://people.mozilla.com/~fdesre/capture/

Comment 5 [:fabrice] Fabrice Desré 2011-05-10 12:16:37 PDT

Created attachment 531404 [details]
screenshot

Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-19 21:34:43 PDT

Comment on attachment 531403 [details] [diff] [review]
patch

>diff --git a/mobile/chrome/content/CapturePicker.js b/mobile/chrome/content/CapturePicker.js

Too many scripts named CapturePicker*.js (3)

Can you name this one CapturePickerUI.js

>+var CapturePicker = {

var CapturePickerUI = {

>+  receiveMessage: function(aMessage) {
>+    switch(aMessage.name) {

switch (

>+        let params = new Object();
>+        params.result = true;

let params = { result: true };

>+        let dialog = importDialog(null, "chrome://browser/content/CapturePicker.xul", params);
>+        document.getElementById("capturepicker-title").appendChild(document.createTextNode(aMessage.json.title));
>+        document.getElementById("capturepicker-video").play();

autoplay="true" in the dialog XUL. do we need this?

>diff --git a/mobile/chrome/content/CapturePicker.xul b/mobile/chrome/content/CapturePicker.xul

name this one CaptureDialog.xul

>+      <video xmlns="http://www.w3.org/1999/xhtml" id="capturepicker-video"
>+             src1="moz-device:?width=640&amp;height=480&amp;type=video/x-raw-yuv"
>+             src="https://developer.mozilla.org/samples/video/chroma-key/video.ogv" autoplay="true"> </video>

Does this actually work on Android? Or are you faking it with this video?

>diff --git a/mobile/chrome/content/CapturePickerScript.js b/mobile/chrome/content/CapturePickerScript.js

name this one CaptureDialog.js


>+var Picker = {

var CaptureDialog = {

>+  saveFrame: function() {

>+    let source = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService).newURI(url, "UTF8", null);
>+    persist.saveURI(source, null, null, null, null, file);
>+    let tm = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
>+    while (!mDone)
>+      tm.currentThread.processNextEvent(true);

* Line break after the "persist.saveURI" call
* Spinning an event loop might not be popular here. What if we are never done? We at least an escape hatch. add a timeout by not allowing the save to take longer than a few seconds. If it fails, return null or something.

>diff --git a/mobile/components/CapturePicker.js b/mobile/components/CapturePicker.js
>+CapturePicker.prototype = {

>+    init: function(window, title, mode) {
>+        this._window = window;
>+        this._title = title;
>+        this._mode = mode;

Indent is off

>+    show: function() {
>+        if (this._shown) {
>+            throw Cr.NS_ERROR_UNEXPECTED;
>+        }

More indent problems (fix the whole file)
No {} around a one-liner

>+        let res = this.messageManager.sendSyncMessage("CapturePicker:Show",
>+                                                      { title: this._title, mode: this._mode, type: this._type })[0];

No wrap

>+        if (res.value) {
>+          this._file = res.path;
>+        }

No {}

>+    set file(a) {
>+        throw "readonly property";
>+    },

Remove this. If you don't define a setter, JS will complain if someone tries to set it

>+    get file() {
>+        Cu.reportError("get file() path=" + this._file);

Remove dump

>+    set type(newType) {
>+        if (this._shown) {
>+            throw Cr.NS_ERROR_UNEXPECTED;
>+        } else {
>+            this._type = newType;
>+        }

No {} around either block

>diff --git a/mobile/components/Makefile.in b/mobile/components/Makefile.in

>         BlocklistPrompt.js \
>+	CapturePicker.js \
>         $(NULL)

Indent issue

>diff --git a/mobile/themes/core/browser.css b/mobile/themes/core/browser.css

>+#capturepicker-video {
>+  border: 2px solid white;

Use the @@ defines to specify the border width

>+  width: 320px;
>+  height: 240px;

I guess you need to specify this, and it needs to be in pixels. However, I wouldn't need moving it to the XUL file, since you hard code the size of the canvas in code too.

>+  margin: 5px;

Use the @@ defines to specify the margin

r- for the tweak, see new patch and to find if this works on Android for real.

Comment 7 [:fabrice] Fabrice Desré 2011-09-09 19:00:00 PDT

Created attachment 559648 [details] [diff] [review]
part 1

Adding a function we'll need in the main patch.

Comment 8 [:fabrice] Fabrice Desré 2011-09-09 19:01:02 PDT

Created attachment 559650 [details] [diff] [review]
Part 2

Change the label of the dialog from "File upload" to "Media upload"

Comment 9 [:fabrice] Fabrice Desré 2011-09-09 19:02:02 PDT

Created attachment 559651 [details] [diff] [review]
part 3

Main part: XUL dialog with orientation support, and saving to a DOMFile

Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-09 21:19:01 PDT

Comment on attachment 559648 [details] [diff] [review]
part 1

Very handy for crossing chrome and web files wrappers.

Looks OK to me, but you'll need someone else to review too. Probably a DOM peer. sicking or jst maybe?

Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-09 21:24:52 PDT

Comment on attachment 559650 [details] [diff] [review]
Part 2

Handling the title separate from "file upload" is probably wise. We can always tweak the string if we get negative feedback on "Media Upload" being too generic.

Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-09 22:06:49 PDT

Comment on attachment 559651 [details] [diff] [review]
part 3

>diff --git a/mobile/chrome/content/CaptureDialog.js b/mobile/chrome/content/CaptureDialog.js

>+var CaptureDialog = {

>+  setPreviewOrientation: function(e) {
>+  handleEvent: function(e) {

nit: e -> aEvent

>+    if (e.type == "deviceorientation") {
>+      //document.getElementById("capturepicker-info").value = "a=" + e.alpha + " b=" + e.beta + " g=" + e.gamma;
>+      //this.setPreviewOrientation(e);

Remove the commented code in this function

>+      this._event = e;

Since this holds the last orientation event, maybe we should be more specific in the naming, so others don't get confused: "_lastOrientationEvent

>+  saveFrame: function() {

>+    let tmpDir = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);

      let tmpDir = Services.dirsvc.get("TmpD", Ci.nsIFile);

(import Services.jsm)

>+    persist.persistFlags = Ci.nsIWebBrowserPersist.PERSIST_FLAGS_REPLACE_EXISTING_FILES;
>+    persist.persistFlags |= Ci.nsIWebBrowserPersist.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION;

      persist.persistFlags = Ci.nsIWebBrowserPersist.PERSIST_FLAGS_REPLACE_EXISTING_FILES | Ci.nsIWebBrowserPersist.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION;

Both on one line is fine

>+    let source = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService).newURI(url, "UTF8", null);

      let source = Services.ios.newURI(url, "UTF8", null);

>+    let tm = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
>+    while (!mDone)
>+      tm.currentThread.processNextEvent(true);

        Services.tm.currentThread.processNextEvent(true);

>diff --git a/mobile/chrome/content/CaptureDialog.xul b/mobile/chrome/content/CaptureDialog.xul

>+        onload="document.getElementsByAttribute('command', 'cmd_ok')[0].focus(); document.getElementById('capturepicker-dialog').CaptureDialog.init()"

document.getElementById('capturepicker-dialog').CaptureDialog.init()

won't | CaptureDialog.init() | work?

>+    <!-- <label id="capturepicker-info" value="..."/> -->

Remove this too

>diff --git a/mobile/components/CapturePicker.js b/mobile/components/CapturePicker.js
>+  init: function(window, title, mode) {

nit: aWindow, aTitle, aMode 

>+  modeMayBeAvailable: function (mode) {

nit: aMode and remove the space after "function"

>+  set type(newType) {

nit: aNewType

>diff --git a/mobile/themes/core/gingerbread/browser.css b/mobile/themes/core/gingerbread/browser.css
> /* Text selection handles */
> 
>+/* Capture picker ------------------------------------------------------------- */

> #selectionhandle-start,
> #selectionhandle-end {

Looks like you jammed this inside the text selection CSS block

r+ with the minor fixups

Comment 13 [:fabrice] Fabrice Desré 2011-09-19 18:11:08 PDT

Created attachment 561092 [details] [diff] [review]
part 3

Mark,

I addressed you comments but I added code to actually rotate the result canvas when doing the capture according to the orientation. This starts in CaptureDialog.js at line 105

Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-19 19:29:11 PDT

Comment on attachment 561092 [details] [diff] [review]
part 3

>+  saveFrame: function() {

>+    let tm = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
>+    while (!mDone)
>+      tm.currentThread.processNextEvent(true);

        Services.tm.currentThread.processNextEvent(true);

r+ with that fixed

Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-09-21 14:03:37 PDT

Comment on attachment 559648 [details] [diff] [review]
part 1

Review of attachment 559648 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed

::: dom/base/nsDOMWindowUtils.cpp
@@ +1837,5 @@
>  
> +NS_IMETHODIMP
> +nsDOMWindowUtils::WrapDOMFile(nsIFile *aFile,
> +                              nsIDOMFile **aDOMFile) {
> +  *aDOMFile = new nsDOMFileFile(aFile);

You need to wrap that in NS_ADDREF. As in

NS_ADDREF(*aDOMFile = new ...);

Comment 16 Ed Morley [:emorley] 2011-09-27 03:35:22 PDT

https://hg.mozilla.org/mozilla-central/rev/0df8c6fcea9e
https://hg.mozilla.org/mozilla-central/rev/8ecc8009b371
https://hg.mozilla.org/mozilla-central/rev/78b3f8faf4e3

Comment 17 Matt Brubeck (:mbrubeck) 2011-09-28 16:34:51 PDT

Adding dev-doc-needed keyword.  This is a new feature in Firefox 9 for Android that allows users to capture images from the camera and upload them, without leaving the browser.  Web developers can use this just by using <input type="file" accept="image/*"> in their HTML.

See also bug 567323 which added some of the underlying interfaces and widgets, and bug 621915 which I think is the same thing but for desktop platforms.

Comment 18 Eric Shepherd [:sheppy] 2011-11-07 12:52:52 PST

Documentation updated:

https://developer.mozilla.org/en/HTML/Element/Input#Image_capture_from_cameras
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils#wrapDOMFile%28%29

Also mentioned on Firefox 9 for developers.

Comment 19 Catalin Suciu [:csuciu] 2011-12-08 04:26:11 PST

Verified fixed on Firefox 9 Beta 5 using http://people.mozilla.com/~fdesre/capture/
Mozilla/5.0(Android;Linux armv7l;rv:9.0) Gecko/20111206 Firefox/9.0 Fennec/9.0
Samsung GalaxyS, Android 2.2