593891 - Fennec nsICapturePicker implementation

Status: VERIFIED FIXED

Description

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Created attachment 472508 [details] [diff] [review]
Patch

Needs some pretty XUL, but the actual XPCOM component is solid.

Comment 1

[[:fabrice] Fabrice Desré]

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)]

camera is not blocking fennec 2.0, sadly.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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é]

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é]

Created attachment 531404 [details]
screenshot

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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é]

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

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

Comment 8

[[:fabrice] Fabrice Desré]

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é]

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?)]

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?)]

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?)]

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é]

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?)]

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]

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]]

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)]

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]]

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]]

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