Expose set/get camera parameter to JS: - setParameter(key, value) - getParameter(key) These APIs are needed to support the vendor-specific features. e.g. vendor's own camera solution needs some setter/getter API for the solution. But because it's not a general feature, it's not reasonable to make it a new API. In this case, the vendor's camera app can use this API.

I think it needs the below APIs. - setParameter(String key, int value) - setParameter(String key, String value) - getParameter(String key) //returns String value - getIntParameter(String key) //returns int value How about this?

It's some ambiguous between int parameter case and string parameter case I think it's better to make their name different with each other to - setParameter(String key, String value) and - setIntParameter(String key, int value)

Mike, What do you think about this issue? - Do you agree with needs of this API? - If so, how is my proposal? In case of MADAI, The APIs like this are needed.

The Patch for the below APIs - void SetParameter(DOMString aKey, DOMString aValue) - void SetParameter(DOMString aKey, long aValue) - DOMString GetParameter(DOMString aKey) - long GetIntParameter(DOMString aKey)

I added the check if the owner window is a certified app. So although it makes it exposed to a privileged app and a hosted app, they can not use these APIs and can not abuse them.

Comment on attachment 8380581 [details] [diff] [review] SetParameter-GetParameter-for-int-and-string.patch, v1 Review of attachment 8380581 [details] [diff] [review]: ----------------------------------------------------------------- General feedback: when submitting a patch, please make sure you include at least 8 lines of context. You can do this by adding the following lines to your ~/.hgrc file: [diff] git = 1 showfunc = 1 unified = 8 Overall this looks good, but see my suggested changes to CameraControl.webidl. ::: dom/camera/DOMCameraControl.h @@ +116,5 @@ > ErrorResult& aRv); > + void SetParameter(const nsAString& aKey, const nsAString& aValue, ErrorResult& aRv); > + void SetParameter(const nsAString& aKey, int32_t aValue, ErrorResult& aRv); > + void GetParameter(const nsAString& aKey, nsString& aValue, ErrorResult& aRv); > + int32_t GetIntParameter(const nsAString& aKey, ErrorResult& aRv); See my suggested changes to CameraControl.webidl. @@ +166,5 @@ > > nsresult Set(JSContext* aCx, uint32_t aKey, const JS::Value& aValue, uint32_t aLimit); > nsresult Get(JSContext* aCx, uint32_t aKey, JS::Value* aValue); > + > + bool CheckCertifiedAppPermission(); Please rename this to IsAppCertified(), but I think with my suggested change to CameraControl.webidl, it won't be needed anymore. ::: dom/camera/GonkCameraControl.cpp @@ +478,5 @@ > +nsGonkCameraControl::Set(const nsAString& aKey, const nsAString& aValue) > +{ > + nsresult rv = mParams.Set(aKey, aValue); > + if (NS_FAILED(rv)) { > + DOM_CAMERA_LOGE("Camera parameter aKey=%s failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv); Change %s to '%s' to make any erronous whitespace in the key easy to see. @@ +495,5 @@ > +nsGonkCameraControl::Set(const nsAString& aKey, int32_t aValue) > +{ > + nsresult rv = mParams.Set(aKey, aValue); > + if (NS_FAILED(rv)) { > + DOM_CAMERA_LOGE("Camera parameter aKey=%s failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv); Change %s to '%s' to make any erronous whitespace in the key easy to see. @@ +499,5 @@ > + DOM_CAMERA_LOGE("Camera parameter aKey=%s failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv); > + return rv; > + } > + return PushParameters(); > +} Please create a new version of SetAndPush() that takes a const nsAString& aKey value, and call that from your two new setters. ::: dom/camera/GonkCameraParameters.cpp @@ +687,5 @@ > +GonkCameraParameters::GetTranslated(const nsAString& aKey, nsAString& aValue) > +{ > + const char* val; > + nsresult rv = GetImpl(NS_ConvertUTF16toUTF8(aKey).get(), val); > + if (NS_FAILED(rv)) { Change to: if (NS_WARN_IF(NS_FAILED(rv))) { ::: dom/webidl/CameraControl.webidl @@ +330,5 @@ > optional CameraSetConfigurationCallback onSuccess, > optional CameraErrorCallback onError); > + > + /* sets a string parameter(aKey and aValue) to a camera device directly; > + aKey and aValue can be camera device specific. Change this to: "sends an opaque, platform-specific parameter directly to the camera without interpretation or validation; using this method to change a setting that is already exposed by its own method (e.g. focusMode, flashMode, ISO) may cause problems, and is not recommended." @@ +334,5 @@ > + aKey and aValue can be camera device specific. > + > + The following cases will throw an exception. > + - If setParameter with a string parameter failed > + - aKey and aValue are not available in a camera device I don't think it is possible to tell when setting is a particular key is supported; the same with a particular value. At the camera library level, I think setParameters() ignores unrecognized keys and values. @@ +335,5 @@ > + > + The following cases will throw an exception. > + - If setParameter with a string parameter failed > + - aKey and aValue are not available in a camera device > + - setParameter is not permitted in a caller app What is a "caller app"? @@ +338,5 @@ > + - aKey and aValue are not available in a camera device > + - setParameter is not permitted in a caller app > + */ > + [Throws] > + void setParameter(DOMString aKey, DOMString aValue); Take a look at this: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#AvailableIn I think tagging these methods as [AvailableIn=CertifiedApps] will make your implementation much simpler. @@ +349,5 @@ > + - aKey and aValue are not available in a camera device > + - setParameter is not permitted in a caller app > + */ > + [Throws] > + void setParameter(DOMString aKey, long aValue); This should be combined with the above as a single method: void setParameter(DOMString key, (DOMString or long)value); See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Union_types @@ +352,5 @@ > + [Throws] > + void setParameter(DOMString aKey, long aValue); > + > + /* returns the value of a string parameter from a camera device directly; > + aKey and the return value can be camera device specific. Change this to: "returns the value of a platform-specific parameter directly from the camera without interpretation or validation; using this method to query a setting that is already exposed by its own method (e.g. focusMode, flashMode, ISO) may cause problems and is not recommended." @@ +371,5 @@ > + - aKey is not available in a camera device > + - getParameter is not permitted in a caller app > + */ > + [Throws] > + long getIntParameter(DOMString aKey); The above two should be combined as a single method: (DOMString or long) getParameter(DOMString key); See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Union_types

I've modified according to your comments. However, while combining getParameter(key) and getIntParameter(key) to (long or DOMString) getParameter(key) using an union type, I met a problem. It's caused by the ambiguous signature between being called for long and for string. In Gonk level implementation, I was not able to decide if it should be mapped to camera HAL for long or something for string. So, if I make your suggestion real, it should try both of calls to long type and string type, and then it should pick the succeed one. I think it's very poor. What do you think about this case?

And now I have to check permission part more and add the test scripts.

(In reply to Youngwoo Jo from comment #7) > > However, while combining getParameter(key) and getIntParameter(key) to (long > or DOMString) getParameter(key) using an union type, I met a problem. > It's caused by the ambiguous signature between being called for long and for > string. In Gonk level implementation, I was not able to decide if it should > be mapped to camera HAL for long or something for string. In this case, I could call the ICameraControl string version and do strtol() on the result. If it's an integer, set the return value for that case, else set it as a string.

I understood your opinion. I became to have a different idea, which is that the interfaces of these APIs should be modified. If Android::CameraParameters.set/get for all the types are implemented based on the string type, I think that only set/get API for string is enough to provide all the functionalities to the app-side. So I think it's better to remove the APIs related to the integer type. What do you think about this? And I updated the new patch.

Comment on attachment 8381499 [details] [diff] [review] SetParameter-GetParameter-for-int-and-String.patch, v2 When adding updated versions of patch, please tag their descriptions with a suitable version number; it makes it easier to sort out interdiffs.

Comment on attachment 8381499 [details] [diff] [review] SetParameter-GetParameter-for-int-and-String.patch, v2 (I should get the version right, too.)

Comment on attachment 8381943 [details] [diff] [review] SetParameter-GetParameter-API.patch, v3 Review of attachment 8381943 [details] [diff] [review]: ----------------------------------------------------------------- This is getting close. djf, what do you think? Is it worth having getParameter() return (long or DOMString), so that the '+'-operator in JS works as expected? (See my comments on dom/webidl/CameraControl.webidl.) ::: dom/camera/DOMCameraControl.h @@ +164,5 @@ > > nsresult Set(JSContext* aCx, uint32_t aKey, const JS::Value& aValue, uint32_t aLimit); > nsresult Get(JSContext* aCx, uint32_t aKey, JS::Value* aValue); > > + bool IsAppCertified(); With Get/SetParameter() tagged as [AvailableIn=CertifiedApps], this should no longer be required and can be removed. ::: dom/camera/GonkCameraControl.cpp @@ +338,5 @@ > +nsGonkCameraControl::SetAndPush(const nsAString& aKey, const T& aValue) > +{ > + nsresult rv = mParams.Set(aKey, aValue); > + if (NS_FAILED(rv)) { > + DOM_CAMERA_LOGE("Camera parameter aKey=\'%s\' failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv); The backslashes are not required, please remove them. Also, this line is very long; please break it before the NS_Convert call and indent the next line by 2 spaces. e.g. if (NS_FAILED(rv)) { DOM_CAMERA_LOGE("Camera parameter aKey='%s' failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv); return rv; } @@ +492,5 @@ > + if (NS_FAILED(rv)) { > + DOM_CAMERA_LOGE("Camera parameter aKey=\'%s\' failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv); > + return rv; > + } > + return PushParameters(); This entire function can be reduced to a call to 'return SetAndPush(aKey, aValue);' ::: dom/webidl/CameraControl.webidl @@ +343,5 @@ > + a setting that is already exposed by its own method (e.g. focusMode, > + flashMode, ISO) may cause problems and is not recommended. > + */ > + [AvailableIn=CertifiedApps, Throws] > + DOMString getParameter(DOMString key); The reason why I want to support a union of (long or DOMString) as the return type is because of how Javascript's '+'-operator handles strings vs. numbers. If this method always returns strings, then: var x = CameraControl.getParameter("parameter-that-returns-a-numeric-value"); // e.g. 10 var y = x + 1; // because x is a string, 1 --> "1" dump(y); // prints "101"

Mike, Thanks for asking. I can't think, offhand, of a DOM method that can return an integer or a string. JS programmers are pretty used to calling parseInt() when we call getAttribute() on an HTML element, when we query a CSS style, or even when we use the new dataset parameter elt.dataset.x = 1; // integer is converted to a string var x = parseInt(elt.dataset.x); So it is fine with me if getParameter() always returns a string. In a way, it even makes the API simpler to understand. This is probably a discussion for a followup bug, but has there been any thought about having a permission that controls use of specific vendor parameters? Like the permission "cameraparams:x,y,z" would allow use of 'x', 'y',and 'z' as keys for setParameter() and getParameter(). This would increase auditability, I suppose. If we did that could we open the API up to privileged apps?

Comment on attachment 8381943 [details] [diff] [review] SetParameter-GetParameter-API.patch, v3 Forgot to change the feedback flag.

Did we actually agree to land this kind of open api? I don't see any discussion on dev-webapi about that change.

(In reply to Fabrice Desré [:fabrice] from comment #16) > > Did we actually agree to land this kind of open api? I don't see any > discussion on dev-webapi about that change. Apologies, I didn't realize any more follow-up was needed. I'll drop this into dev-webapi in the morning.

I removed IsAppCertified method and modified the patch by Mikeh's review.

We have decided not to follow this approach.