From bug 1048454 comment 8, Flame reports some curious values: - metering modes: default mode 'frame-average', supported modes: 'frame-average' 'center-weighted' 'spot-metering' 'center-weighted' 'spot-metering-adv' 'center-weighted-adv' No idea what the 'adv' modes are; not sure why there are two 'center-weighted' modes.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=116ed4d36320

Note that attachment 8531038 [details] [diff] [review] will automatically bump metering mode to 'center-weighted' which seems to improve picture quality on Flame.

Comment on attachment 8531038 [details] [diff] [review] Expose metering modes, v2 Review of attachment 8531038 [details] [diff] [review]: ----------------------------------------------------------------- r+ conditional on addressing that below. ::: dom/camera/GonkCameraControl.cpp @@ +227,5 @@ > } > > + nsString mode; > + mParams.Get(CAMERA_PARAM_METERINGMODE, mode); > + if (!mode.IsEmpty()) { If the mode is empty and center weighted is supported, don't we still want to reset the default? @@ +241,5 @@ > + DOM_CAMERA_LOGI(" '%s'\n", NS_ConvertUTF16toUTF8(modes[i]).get()); > + if (modes[i].EqualsASCII(kCenterWeighted)) { > + centerWeightSupport = i; > + } > + } We should break as soon as the center weight is found. Is there any reason we use a while instead of a for loop for less LOC?

Andrew, asking for a re-review since I juggled some code in GCC.cpp. bz, boilerplate addition of exposure mode settings to JS need WebIDL-blessage.

Comment on attachment 8534679 [details] [diff] [review] Expose metering modes, v2.1 Review of attachment 8534679 [details] [diff] [review]: ----------------------------------------------------------------- r+ with below fixed or commented on. ::: dom/camera/DOMCameraCapabilities.cpp @@ +677,5 @@ > + if (NS_WARN_IF(!mCameraControl)) { > + return; > + } > + > + if (mMeteringModes.Length() == 0) { nit: I think .IsEmpty() is nicer :). On a more serious note, if the WebIDL layer has caching enabled, do we really need to cache them again in our layer? We could probably save some memory by eliminating it. I'm not sure how often the JS engine will garbage collect these optimized cache bits, but I assume it would only do so if it was getting low on memory, in which case not using the memory here would be a good idea too...? ::: dom/camera/GonkCameraControl.cpp @@ +245,5 @@ > + } > + } > + > + mParams.Get(CAMERA_PARAM_METERINGMODE, mode); > + DOM_CAMERA_LOGI(" - metering mode: '%s'\n", kCenterWeighted); What's the point of the .Get? We should print mode, not the constant, or remove line 248. I favour the former :).

> I'm not sure how often the JS engine will garbage collect these optimized cache bits A Web IDL object with a [Cached] attribute will get pinned to the lifetime of its C++ reflection if that attribute is gotten, because the cached value disappearing is observable so we can't allow that to happen on GC.

(In reply to Boris Zbarsky [:bz] from comment #7) > > I'm not sure how often the JS engine will garbage collect these optimized cache bits > > A Web IDL object with a [Cached] attribute will get pinned to the lifetime > of its C++ reflection if that attribute is gotten, because the cached value > disappearing is observable so we can't allow that to happen on GC. Couldn't we switch to Pure instead of Constant, get the caching benefit, without the constraint of being unable to change/fail?

That doesn't matter. [Cached] means "cached until manually cleared", so the binding system ensures it doesn't go away until you manually clear it.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c0b245b244d8 Still needs WebIDL review.

Comment on attachment 8535040 [details] [diff] [review] Expose metering modes, v2.2 r=aosmond Andrew is right. mMeteringModes seems totally unnecessary and you can just get into retval directly. r=me with that.

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #11) > Andrew is right. mMeteringModes seems totally unnecessary and you can just > get into retval directly. Yeah, I was planning to remove those in a different patch, but I can bundle it into this one.

Removing caching of capabilities in DOMCameraCapabilities.*. The WebIDL framework will handle this for us. try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2759fc7fde0b Andrew, would you mind giving this a once-over to make sure I didn't botch anything?

Comment on attachment 8535138 [details] [diff] [review] Expose metering modes, v2.3 r=bz Review of attachment 8535138 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b1f2b4f3f74