Closed Bug 593891 Opened 9 years ago Closed 8 years ago

Fennec nsICapturePicker implementation

Categories

(Firefox for Android Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(firefox9 fixed, fennec+)

VERIFIED FIXED
Firefox 9
Tracking Status
firefox9 --- fixed
fennec + ---

People

(Reporter: khuey, Assigned: fabrice)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 3 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Needs some pretty XUL, but the actual XPCOM component is solid.
Attachment #472508 - Flags: review?(mark.finkle)
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.
tracking-fennec: --- → ?
camera is not blocking fennec 2.0, sadly.
tracking-fennec: ? → 2.0-
tracking-fennec: 2.0- → 2.0next+
Assignee: khuey → blassey.bugs
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.
Attachment #472508 - Flags: review?(mark.finkle) → review-
Assignee: blassey.bugs → fabrice
tracking-fennec: 2.0next+ → 6+
Attached patch patch (obsolete) — Splinter Review
This patch splits the implementation in two parts due to e10s.

A simple test page is available here : http://people.mozilla.com/~fdesre/capture/
Attachment #472508 - Attachment is obsolete: true
Attachment #531403 - Flags: review?(mark.finkle)
Attached image screenshot
Attachment #531404 - Flags: ui-review?(madhava)
tracking-fennec: 6+ → 7+
tracking-fennec: 7+ → 6+
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.
Attachment #531403 - Flags: review?(mark.finkle) → review-
tracking-fennec: 6+ → 7+
Depends on: 659188
tracking-fennec: 7+ → +
Attached patch part 1Splinter Review
Adding a function we'll need in the main patch.
Attachment #531403 - Attachment is obsolete: true
Attachment #559648 - Flags: review?(mark.finkle)
Attached patch Part 2Splinter Review
Change the label of the dialog from "File upload" to "Media upload"
Attachment #559650 - Flags: review?(mark.finkle)
Attached patch part 3 (obsolete) — Splinter Review
Main part: XUL dialog with orientation support, and saving to a DOMFile
Attachment #559651 - Flags: review?(mark.finkle)
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?
Attachment #559648 - Flags: review?(mark.finkle) → review+
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.
Attachment #559650 - Flags: review?(mark.finkle) → review+
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
Attachment #559651 - Flags: review?(mark.finkle) → review+
Attachment #559648 - Flags: review+ → review?(jonas)
Attached patch part 3Splinter Review
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
Attachment #559651 - Attachment is obsolete: true
Attachment #561092 - Flags: review?(mark.finkle)
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
Attachment #561092 - Flags: review?(mark.finkle) → review+
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 ...);
Attachment #559648 - Flags: review?(jonas) → review+
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.
Depends on: 567323
Keywords: dev-doc-needed
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
Status: RESOLVED → VERIFIED
Attachment #531404 - Flags: ui-review?(madhava)
You need to log in before you can comment on or make changes to this bug.