Closed
Bug 593891
Opened 11 years ago
Closed 10 years ago
Fennec nsICapturePicker implementation
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox9 fixed, fennec+)
VERIFIED
FIXED
Firefox 9
People
(Reporter: khuey, Assigned: fabrice)
References
Details
(Keywords: dev-doc-complete)
Attachments
(4 files, 3 obsolete files)
48.28 KB,
image/png
|
Details | |
1.75 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
1.27 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
18.05 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
Needs some pretty XUL, but the actual XPCOM component is solid.
Attachment #472508 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 1•11 years ago
|
||
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.
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•10 years ago
|
tracking-fennec: 2.0- → 2.0next+
Updated•10 years ago
|
Assignee: khuey → blassey.bugs
Comment 3•10 years ago
|
||
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-
Updated•10 years ago
|
Assignee: blassey.bugs → fabrice
Updated•10 years ago
|
tracking-fennec: 2.0next+ → 6+
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #531404 -
Flags: ui-review?(madhava)
Updated•10 years ago
|
tracking-fennec: 6+ → 7+
Updated•10 years ago
|
tracking-fennec: 7+ → 6+
Comment 6•10 years ago
|
||
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&height=480&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-
Updated•10 years ago
|
tracking-fennec: 6+ → 7+
Updated•10 years ago
|
tracking-fennec: 7+ → +
Assignee | ||
Comment 7•10 years ago
|
||
Adding a function we'll need in the main patch.
Attachment #531403 -
Attachment is obsolete: true
Attachment #559648 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 8•10 years ago
|
||
Change the label of the dialog from "File upload" to "Media upload"
Attachment #559650 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 9•10 years ago
|
||
Main part: XUL dialog with orientation support, and saving to a DOMFile
Attachment #559651 -
Flags: review?(mark.finkle)
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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 12•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #559648 -
Flags: review+ → review?(jonas)
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0df8c6fcea9e https://hg.mozilla.org/mozilla-central/rev/8ecc8009b371 https://hg.mozilla.org/mozilla-central/rev/78b3f8faf4e3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Updated•10 years ago
|
status-firefox9:
--- → fixed
Comment 17•10 years ago
|
||
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
Comment 18•10 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
Comment 19•9 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #531404 -
Flags: ui-review?(madhava)
You need to log in
before you can comment on or make changes to this bug.
Description
•