Image Capture: reject setOptions()/applyConstraints() if any argument is unsupported

Image Capture: reject setOptions()/applyConstraints() if any argument is unsupported


This CL adds rejection of applyConstraints()/setOptions() if the control value
is out of ranges or unsupported, e.g. trying to set zoom to 1000 where the 
capability range is {1.0 - 10.0}.

LayoutTests for each of those methods are added.

BUG=718765
Review-Url: https://codereview.chromium.org/2877273002
Cr-Commit-Position: refs/heads/master@{#472143}
Committed: https://chromium.googlesource.com/chromium/src/+/64b5e1fad912daf7c311c7f984db673bcae7713c

[mcasas, 2017-05-13 02:38:47]

Description was changed from

==========
Image Capture: reject setOptions()/applyConstraints() if any argument is
unsupported


Wired rejecting a constraint if out of bounds, and LayoutTest

BUG=718765
==========

to

==========
Image Capture: reject setOptions()/applyConstraints() if any argument is
unsupported


Wired rejecting a constraint if out of bounds, and LayoutTest

getphotocapabilities.html => getPhotoCapabilities.html
setoptions.html => setOptions.html
takephoto-with-photosettings.html => takePhoto-with-PhotoSettings.html
takephoto.html => takePhoto.html

BUG=718765
==========

[mcasas, 2017-05-13 02:51:01]

Patchset #1 (id:1) has been deleted

[mcasas, 2017-05-13 02:51:07]

Patchset #1 (id:20001) has been deleted

[mcasas, 2017-05-13 02:52:35]

Patchset #1 (id:40001) has been deleted

[mcasas, 2017-05-13 05:50:00]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 05:50:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 05:54:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 05:54:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mcasas, 2017-05-15 16:17:47]

Description was changed from

==========
Image Capture: reject setOptions()/applyConstraints() if any argument is
unsupported


Wired rejecting a constraint if out of bounds, and LayoutTest

getphotocapabilities.html => getPhotoCapabilities.html
setoptions.html => setOptions.html
takephoto-with-photosettings.html => takePhoto-with-PhotoSettings.html
takephoto.html => takePhoto.html

BUG=718765
==========

to

==========
Image Capture: reject setOptions()/applyConstraints() if any argument is
unsupported


This CL adds rejection of applyConstraints()/setOptions() if the control value
is out of ranges or unsupported, e.g. trying to set zoom to 1000 where the 
capability range is {1.0 - 10.0}.

LayoutTests for each of those methods are added, and since I was
touching the area, I renamed a few tests: 

- getphotocapabilities.html => getPhotoCapabilities.html
- setoptions.html => setOptions.html
- takephoto-with-photosettings.html => takePhoto-with-PhotoSettings.html
- takephoto.html => takePhoto.html

which are correctly isolated by Rietveld.

BUG=718765
==========

[mcasas, 2017-05-15 16:19:00]

mcasas@chromium.org changed reviewers:
+ reillyg@chromium.org

[mcasas, 2017-05-15 16:19:01]

reillyg@ PTAL

 sizeable CL but mostly mechanical

[Reilly Grant (use Gerrit), 2017-05-15 16:50:49]

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imagecapture/setOptions-reject.html (right):

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imagecapture/setOptions-reject.html:46:
(capabilities) => { return { redEyeReduction : true }; },
Does the following not work?

capabilities => { redEyeReduction: true }

or, if the parser can't disambiguate between a block and an object literal:

capabilities => ({ redEyeReduction: true })

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:168: const auto
height = photo_settings.imageHeight();
Please don't use auto here. I don't know what the type of imageHeight() is
unless I go check the header. While normally I would assume that it is an int in
this code it could also be a range which I see from its usage below that it
probably is.

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:173:
kNotSupportedError, "|imageHeight| setting out of range"));
This error will be visible on the console, where I don't think the |variable|
style makes sense.

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:323:
MediaTrackConstraintSet temp_constraints;
Should this be initialized with |constraints_|?

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:330: const auto
white_balance_mode =
Just so you know, auto is fine here because getAsString() makes the type clear.

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.h (right):

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.h:41: bool
isRedEyeReductionControllable() const;
This is not a WebIDL function so it should be named according to the Chromium
style guide: IsRedEyeReductionControllable()

[mcasas, 2017-05-15 18:18:49]

ptal

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imagecapture/setOptions-reject.html (right):

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imagecapture/setOptions-reject.html:46:
(capabilities) => { return { redEyeReduction : true }; },
On 2017/05/15 16:50:48, Reilly Grant wrote:
> Does the following not work?
> 
> capabilities => { redEyeReduction: true }
> 
> or, if the parser can't disambiguate between a block and an object literal:
> 
> capabilities => ({ redEyeReduction: true })

The latter works.

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:168: const auto
height = photo_settings.imageHeight();
On 2017/05/15 16:50:48, Reilly Grant wrote:
> Please don't use auto here. I don't know what the type of imageHeight() is
> unless I go check the header. While normally I would assume that it is an int
in
> this code it could also be a range which I see from its usage below that it
> probably is.

Done.

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:173:
kNotSupportedError, "|imageHeight| setting out of range"));
On 2017/05/15 16:50:48, Reilly Grant wrote:
> This error will be visible on the console, where I don't think the |variable|
> style makes sense.

Done.

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:323:
MediaTrackConstraintSet temp_constraints;
On 2017/05/15 16:50:48, Reilly Grant wrote:
> Should this be initialized with |constraints_|?

Done.

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:330: const auto
white_balance_mode =
On 2017/05/15 16:50:48, Reilly Grant wrote:
> Just so you know, auto is fine here because getAsString() makes the type
clear.

Acknowledged.

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.h (right):

https://codereview.chromium.org/2877273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.h:41: bool
isRedEyeReductionControllable() const;
On 2017/05/15 16:50:49, Reilly Grant wrote:
> This is not a WebIDL function so it should be named according to the Chromium
> style guide: IsRedEyeReductionControllable()

Done.

[Reilly Grant (use Gerrit), 2017-05-15 18:27:49]

lgtm with nits

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints-reject.html
(right):

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints-reject.html:51:
capabilities => ({ whiteBalanceMode : "manual" }),
no spaces before the colon

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imagecapture/setOptions-reject.html (right):

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imagecapture/setOptions-reject.html:46:
capabilities => ({ redEyeReduction : true }),
here too

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:204: const auto
fill_light_mode = photo_settings.fillLightMode();
I was wrong about the type of imageHeight() and imageWidth(). Who knows what
this could be.

[mcasas, 2017-05-15 19:57:08]

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints-reject.html
(right):

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints-reject.html:51:
capabilities => ({ whiteBalanceMode : "manual" }),
On 2017/05/15 18:27:48, Reilly Grant wrote:
> no spaces before the colon

Done.

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imagecapture/setOptions-reject.html (right):

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imagecapture/setOptions-reject.html:46:
capabilities => ({ redEyeReduction : true }),
On 2017/05/15 18:27:49, Reilly Grant wrote:
> here too

Done.

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):

https://codereview.chromium.org/2877273002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:204: const auto
fill_light_mode = photo_settings.fillLightMode();
On 2017/05/15 18:27:49, Reilly Grant wrote:
> I was wrong about the type of imageHeight() and imageWidth(). Who knows what
> this could be.

Done.

[mcasas, 2017-05-15 21:04:38]

Patchset #4 (id:120001) has been deleted

[mcasas, 2017-05-15 21:05:22]

Description was changed from

==========
Image Capture: reject setOptions()/applyConstraints() if any argument is
unsupported


This CL adds rejection of applyConstraints()/setOptions() if the control value
is out of ranges or unsupported, e.g. trying to set zoom to 1000 where the 
capability range is {1.0 - 10.0}.

LayoutTests for each of those methods are added, and since I was
touching the area, I renamed a few tests: 

- getphotocapabilities.html => getPhotoCapabilities.html
- setoptions.html => setOptions.html
- takephoto-with-photosettings.html => takePhoto-with-PhotoSettings.html
- takephoto.html => takePhoto.html

which are correctly isolated by Rietveld.

BUG=718765
==========

to

==========
Image Capture: reject setOptions()/applyConstraints() if any argument is
unsupported


This CL adds rejection of applyConstraints()/setOptions() if the control value
is out of ranges or unsupported, e.g. trying to set zoom to 1000 where the 
capability range is {1.0 - 10.0}.

LayoutTests for each of those methods are added.

BUG=718765
==========

[mcasas, 2017-05-15 21:05:30]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 21:06:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mcasas, 2017-05-15 22:19:15]

The CQ bit was unchecked by mcasas@chromium.org

[mcasas, 2017-05-15 22:19:18]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2017-05-15 22:19:19]

The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2877273002/#ps140001
(title: "reverted renaming bc it breaks patching")

[commit-bot: I haz the power, 2017-05-15 22:19:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 02:03:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 02:03:15]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mcasas, 2017-05-16 02:53:17]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2017-05-16 02:53:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 06:46:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 06:46:58]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mcasas, 2017-05-16 14:46:21]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2017-05-16 14:46:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 17:21:15]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494945981915550,
"parent_rev": "21a0c8f770ea811a3830d5f8c6447306a686a63b", "commit_rev":
"64b5e1fad912daf7c311c7f984db673bcae7713c"}

[commit-bot: I haz the power, 2017-05-16 17:21:31]

Description was changed from

==========
Image Capture: reject setOptions()/applyConstraints() if any argument is
unsupported


This CL adds rejection of applyConstraints()/setOptions() if the control value
is out of ranges or unsupported, e.g. trying to set zoom to 1000 where the 
capability range is {1.0 - 10.0}.

LayoutTests for each of those methods are added.

BUG=718765
==========

to

==========
Image Capture: reject setOptions()/applyConstraints() if any argument is
unsupported


This CL adds rejection of applyConstraints()/setOptions() if the control value
is out of ranges or unsupported, e.g. trying to set zoom to 1000 where the 
capability range is {1.0 - 10.0}.

LayoutTests for each of those methods are added.

BUG=718765

Review-Url: https://codereview.chromium.org/2877273002
Cr-Commit-Position: refs/heads/master@{#472143}
Committed:
https://chromium.googlesource.com/chromium/src/+/64b5e1fad912daf7c311c7f984db...
==========

[commit-bot: I haz the power, 2017-05-16 17:21:32]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/64b5e1fad912daf7c311c7f984db...