Last Comment Bug 593891 - Fennec nsICapturePicker implementation
: Fennec nsICapturePicker implementation
Status: VERIFIED FIXED
: dev-doc-complete
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: Firefox 9
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
Depends on: 567323 659188
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-06 18:02 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2012-12-14 11:56 PST (History)
11 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (5.85 KB, patch)
2010-09-06 18:02 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
mark.finkle: review-
Details | Diff | Splinter Review
patch (14.81 KB, patch)
2011-05-10 12:15 PDT, [:fabrice] Fabrice Desré
mark.finkle: review-
Details | Diff | Splinter Review
screenshot (48.28 KB, image/png)
2011-05-10 12:16 PDT, [:fabrice] Fabrice Desré
no flags Details
part 1 (1.75 KB, patch)
2011-09-09 19:00 PDT, [:fabrice] Fabrice Desré
jonas: review+
Details | Diff | Splinter Review
Part 2 (1.27 KB, patch)
2011-09-09 19:01 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Splinter Review
part 3 (17.90 KB, patch)
2011-09-09 19:02 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Splinter Review
part 3 (18.05 KB, patch)
2011-09-19 18:11 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-09-06 18:02:30 PDT
Created attachment 472508 [details] [diff] [review]
Patch

Needs some pretty XUL, but the actual XPCOM component is solid.
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 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 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

Note You need to log in before you can comment on or make changes to this bug.