621915 - implement capture picker for camera input

Status: RESOLVED WONTFIX

(Toolkit :: UI Widgets, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP

this works for taking stills. Probably need some design love though

Comment 1

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

Comment on attachment 500211 [details] [diff] [review]
WIP


>diff --git a/browser/confvars.sh b/browser/confvars.sh

>+if test "$OS_ARCH" = "Linux"; then
>+MOZ_CAPTURE=1
>+fi
>\ No newline at end of file

Fix the newline

Can't comment too much on the C++ impl. The printf's should probably be changed to something more debug/release friendly

>+++ b/toolkit/components/capturepicker/content/capturepicker.js
>@@ -0,0 +1,76 @@
>+var retvals;

>+var Cc = Components.classes;
>+var Ci = Components.interfaces;
>+var Cu = Components.utils;

>+var cp_deck;
>+var cp_video;
>+var cp_image;

>+function capturepickerLoad() {

Add some line breaks to clean this up

>+  if (window.arguments) {
>+    var o = window.arguments[0];
>+    retvals = o.retvals;
>+    document.title = o.title;
>+
>+  }

Remove the blank line

>+function takePhoto() {
>+  let tmpDir = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);
>+  let file = tmpDir.clone();
>+  file.append("image.png");
>+  file.createUnique(file.NORMAL_FILE_TYPE, 0600);
>+  var io = Cc["@mozilla.org/network/io-service;1"]
>+    .getService(Ci.nsIIOService);
>+  var source = io.newURI("moz-device:camera?type=image/png", "UTF8", null);
>+  var persist = Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]
>+    .createInstance(Ci.nsIWebBrowserPersist);

You are mixing "let" and "var" a lot. "var" for globals and "let" for locals is a good rule. Also, the line wrapping is a bit loose ("io" lines could be all on one)

>+  var progressListener = {
>+  onProgressChange: function() {
>+      /* Ignore progress callback */
>+    },
>+  onStateChange: function(aProgress, aRequest, aStateFlag, aStatus) {
>+    if (aStateFlag & STATE_STOP) {
>+      cp_deck.selectedIndex = 1;
>+      cp_image.src = fileURL.spec;
>+    }
>+    }
>+  }

Indent the body of progressListener

>+function onOK() {
>+  retvals.buttonStatus = nsIFilePicker.returnOK;

Ci.nsIFilePicker (unless I missed something)

>+function onCancel()
>+{
>+  // Close the window.
>+  retvals.buttonStatus = nsIFilePicker.returnCancel;

Ci.nsIFilePicker (unless I missed something)

>diff --git a/toolkit/components/capturepicker/content/capturepicker.xul b/toolkit/components/capturepicker/content/capturepicker.xul

>+<dialog id="main-window"
>+	xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+	xmlns:html="http://www.w3.org/1999/xhtml"
>+	onload="capturepickerLoad();"
>+	width="320" height="320"
>+	persist="screenX screenY">

Align indent on the "id" attribute

>+      <vbox flex="10">
>+	<html:video id="capturepicker_video"  src="moz-device:camera?type=video/x-raw-yuv" width="320" height="240" autoplay="true"/>
>+	<button oncommand="takePhoto()" label="&takePhoto.label;"/>
>+      </vbox>

needs to be indented. <vbox> is a container

>+      <vbox flex="10">
>+	<hbox flex="0">

Same

>diff --git a/toolkit/components/capturepicker/jar.mn b/toolkit/components/capturepicker/jar.mn

>+toolkit.jar:
>+* content/global/capturepicker.xul                     (content/capturepicker.xul)
>+* content/global/capturepicker.js                      (content/capturepicker.js)

Do we need to preprocess these files? (the '*' means preprocess)

>diff --git a/toolkit/components/capturepicker/src/nsCapturePicker.js b/toolkit/components/capturepicker/src/nsCapturePicker.js

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.

Is this boilerplate really old?

>+const LOCAL_FILE_CONTRACTID = "@mozilla.org/file/local;1";
>+const APPSHELL_SERV_CONTRACTID  = "@mozilla.org/appshell/appShellService;1";
>+const STRBUNDLE_SERV_CONTRACTID = "@mozilla.org/intl/stringbundle;1";

You could use shorter const names here. They are almost as long as the real strings! Replace the "CONTRACTID" with "CID" and maybe drop the "SERV"

>+const nsIAppShellService    = Ci.nsIAppShellService;
>+const nsILocalFile          = Ci.nsILocalFile;
>+const nsIFileURL            = Ci.nsIFileURL;
>+const nsISupports           = Ci.nsISupports;
>+const nsIFactory            = Ci.nsIFactory;
>+const nsICapturePicker      = Ci.nsICapturePicker;
>+const nsIInterfaceRequestor = Ci.nsIInterfaceRequestor;
>+const nsIDOMWindow          = Ci.nsIDOMWindow;
>+const nsIStringBundleService= Ci.nsIStringBundleService;
>+const nsIWebNavigation      = Ci.nsIWebNavigation;
>+const nsIDocShellTreeItem   = Ci.nsIDocShellTreeItem;
>+const nsIBaseWindow         = Ci.nsIBaseWindow;

Remove all of this. Just add the darn "Ci." to the places where this is needed.

>+  QueryInterface: function(iid) {
>+    if (iid.equals(nsICapturePicker) ||
>+        iid.equals(nsISupports))
>+      return this;
>+
>+    throw Components.results.NS_ERROR_NO_INTERFACE;
>+  },


Use XPCOMUtils:

QueryInterface: XPCOMUtils.generateQI([Ci.nsICapturePicker]),

>+  show: function() {
<snip>
>+  },

The code in this method that tries to get the parentWin is almost scary. I assume it came from somewhere else?

>+  init: function (parent, title, mode) {
>+    this.mParentWindow = parent;
>+    this.mTitle = title;

No need to hold mode?

>+  modeMayBeAvailable: function(mode) {
>+    return true; mode == nsICapturePicker.MODE_STILL;

Which is it? "true" or "mode == Ci.nsICapturePicker.MODE_STILL"  ?

>+  /* readonly attribute nsILocalFile file; */
>+  set file(a) { throw "readonly property"; },

Just don't supply the setter

>+  get file()  {
>+    var utils = this.mParentWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+                                     getInterface(Ci.nsIDOMWindowUtils);
>+    return utils.wrapDOMFile(this.mFile);

wrapDOMFile exists somewhere?

>diff --git a/toolkit/locales/jar.mn b/toolkit/locales/jar.mn

>   locale/@AB_CD@/global/filefield.properties            (%chrome/global/filefield.properties)
>   locale/@AB_CD@/global/filepicker.dtd                  (%chrome/global/filepicker.dtd)
>+  locale/@AB_CD@/global/capturepicker.dtd                  (%chrome/global/capturepicker.dtd)
>   locale/@AB_CD@/global/filepicker.properties           (%chrome/global/filepicker.properties)

Fix spacing and maybe add after filepicker.dtd / .properties ?

If I were writing the JS parts i would use a more Object based approach instead of the global funcs and data style used here (and probably prevalent in the older file picker code)

I assume this won't work from e10s since the component will try to load in the child process.

Overall approach looks good to proceed.

Comment 2

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

:imelven - does this need reassignment since sec realignment?

Comment 3

[Ian Melven :imelven]

(In reply to Curtis Koenig [:curtisk] from comment #2)
> :imelven - does this need reassignment since sec realignment?

yes, but it looks like this bug isn't currently active ?

Comment 4

[Ian Melven :imelven]

I think this may have ended up being the getUserMedia work instead ?

sending to mgoodwin as it's mobile but don't think there's anything to do here..

Comment 5

[Mark Goodwin [:mgoodwin]]

I've spoken to blassey about this; he says:

"I think getUserMedia's impl has made that irrelevant".

Comment 6

[Mark Goodwin [:mgoodwin]]

See comment 5

Comment 7

[Tobie Langel]

getUserMedia and HTML media capture have a very different set of use cases, and implementing one doesn't obsolete the other.

As of now, getUserMedia doesn't have the necessary APIs to take high quality pictures, it also forces the developer to build his own API for taking pictures instead of being able to rely on the device's camera application when there is one (frequent on mobile devices).

Please reconsider closing this.

Comment 8

[Mark Goodwin [:mgoodwin]]

Ah, apologies; I though this was the secreview bug. Un-closing

Comment 9

[Tobie Langel]

The secreview bug!?

Comment 10

[Mark Goodwin [:mgoodwin]]

(In reply to Tobie Langel from comment #9)
> The secreview bug!?

Yeah; the security review for this feature (there doesn't seem to be a separate bug for this). Security reviews for bugs that seem dormant aren't necessarily the best use of time, which is why I was closing it out.

Comment 11

[Mounir Lamouri (:mounir)]

Isn't this bug related to the old and ugly capture button that was in Fennec XUL? I doubt this is needed for bug 741393. Bug 741393 could simply make the intent fired when the <input type='file'> is activated different and only use the capture handlers.

IMO, we should close this bug and work on bug 741393 instead.

Comment 12

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

This is now implemented via getUserMedia