Change webcamPrivate.set/get/reset to async APIs.

Currently webcamPrivate.set/get/reset functions are all sync APIs, which doesn't work for VISCA cameras.

Also fix a bug that Calling webcamPrivate.set/get excessively with small time-interval causes browser crashes.
The reason is a temporary variable is binded to the asynchronous function ViscaWebcam::OnCommandCompleted().

BUG=519028
Committed: https://crrev.com/50ab547896bb9f0d4afa94b1676f4a5e022fae31
Cr-Commit-Position: refs/heads/master@{#346479}

[xdai1, 2015-08-10 21:33:08]

Patchset #1 (id:1) has been deleted

[xdai1, 2015-08-10 21:58:05]

xdai@chromium.org changed reviewers:
+ zork@chromium.org

[xdai1, 2015-08-10 21:58:06]

Zach, could you help to review the CL please? Thanks for your help.

[xdai1, 2015-08-12 16:12:00]

On 2015/08/10 21:58:06, xdai1 wrote:
> Zach, could you help to review the CL please? Thanks for your help.

Friendly ping?

[xdai1, 2015-08-14 00:03:20]

Patchset #2 (id:40001) has been deleted

[xdai1, 2015-08-18 16:49:50]

On 2015/08/12 16:12:00, xdai1 wrote:
> On 2015/08/10 21:58:06, xdai1 wrote:
> > Zach, could you help to review the CL please? Thanks for your help.
> 
> Friendly ping?

ping again?

[Zachary Kuznia, 2015-08-19 20:49:44]

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
File extensions/browser/api/webcam_private/v4l2_webcam.cc (right):

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/v4l2_webcam.cc:165: success &=
SetWebcamParameter(fd_.get(), V4L2_CID_PANTILT_CMD,
It's not a good idea to use bitwise AND to combine bools.  Please just set
success to false fail immediately when a call fails.

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
File extensions/browser/api/webcam_private/visca_webcam.cc (right):

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/visca_webcam.cc:176: // If there is no
pending comand in the command queue, send it now.
nit: "If this is the only command in the queue"

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/visca_webcam.cc:242: base::Bind(callback,
false));
Is there a reason this isn't just:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(callback,
success));

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/visca_webcam.cc:269: switch (type) {
Could you add a short comment describing the format of response?

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
File extensions/browser/api/webcam_private/webcam_private_api.h (left):

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/webcam_private_api.h:113: //
AsyncApiFunction:
Why did you remove these comments?

https://codereview.chromium.org/1281263003/diff/60001/extensions/common/api/w...
File extensions/common/api/webcam_private.idl (right):

https://codereview.chromium.org/1281263003/diff/60001/extensions/common/api/w...
extensions/common/api/webcam_private.idl:35: static void get(DOMString webcamId,
InquireType type,
This API shouldn't change, unless there are performance implications or side
effects to requesting all state.

[xdai1, 2015-08-21 22:57:03]

Zach, I've addressed your comments, please take another look, thanks!

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
File extensions/browser/api/webcam_private/v4l2_webcam.cc (right):

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/v4l2_webcam.cc:165: success &=
SetWebcamParameter(fd_.get(), V4L2_CID_PANTILT_CMD,
On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> It's not a good idea to use bitwise AND to combine bools.  Please just set
> success to false fail immediately when a call fails.

Done.

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
File extensions/browser/api/webcam_private/visca_webcam.cc (right):

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/visca_webcam.cc:176: // If there is no
pending comand in the command queue, send it now.
On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> nit: "If this is the only command in the queue"

Done.

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/visca_webcam.cc:242: base::Bind(callback,
false));
On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> Is there a reason this isn't just:
> BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(callback,
> success));

You're right. Done.

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/visca_webcam.cc:269: switch (type) {
On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> Could you add a short comment describing the format of response?

Actually I described the response format together with the command. See line 42
for kGetPanTiltCommand as an example. So probably I just add a comment here to
refer the place where the response format can be found.

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
File extensions/browser/api/webcam_private/webcam_private_api.h (left):

https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
extensions/browser/api/webcam_private/webcam_private_api.h:113: //
AsyncApiFunction:
On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> Why did you remove these comments?

It seems a little redundant? I noticed that several classes in this file don't
have this kind of comments while other do. Besides, if we do want to add "//
AsyncApiFunction" comment here, maybe we should move it before
"~WebcamPrivateCloseWebcamFunction()" since it's also an override function?

[xdai1, 2015-08-25 17:07:20]

On 2015/08/21 22:57:03, xdai1 wrote:
> Zach, I've addressed your comments, please take another look, thanks!
> 
>
https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
> File extensions/browser/api/webcam_private/v4l2_webcam.cc (right):
> 
>
https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
> extensions/browser/api/webcam_private/v4l2_webcam.cc:165: success &=
> SetWebcamParameter(fd_.get(), V4L2_CID_PANTILT_CMD,
> On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> > It's not a good idea to use bitwise AND to combine bools.  Please just set
> > success to false fail immediately when a call fails.
> 
> Done.
> 
>
https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
> File extensions/browser/api/webcam_private/visca_webcam.cc (right):
> 
>
https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
> extensions/browser/api/webcam_private/visca_webcam.cc:176: // If there is no
> pending comand in the command queue, send it now.
> On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> > nit: "If this is the only command in the queue"
> 
> Done.
> 
>
https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
> extensions/browser/api/webcam_private/visca_webcam.cc:242:
base::Bind(callback,
> false));
> On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> > Is there a reason this isn't just:
> > BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(callback,
> > success));
> 
> You're right. Done.
> 
>
https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
> extensions/browser/api/webcam_private/visca_webcam.cc:269: switch (type) {
> On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> > Could you add a short comment describing the format of response?
> 
> Actually I described the response format together with the command. See line
42
> for kGetPanTiltCommand as an example. So probably I just add a comment here to
> refer the place where the response format can be found.
> 
>
https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
> File extensions/browser/api/webcam_private/webcam_private_api.h (left):
> 
>
https://codereview.chromium.org/1281263003/diff/60001/extensions/browser/api/...
> extensions/browser/api/webcam_private/webcam_private_api.h:113: //
> AsyncApiFunction:
> On 2015/08/19 20:49:44, Zachary Kuznia wrote:
> > Why did you remove these comments?
> 
> It seems a little redundant? I noticed that several classes in this file don't
> have this kind of comments while other do. Besides, if we do want to add "//
> AsyncApiFunction" comment here, maybe we should move it before
> "~WebcamPrivateCloseWebcamFunction()" since it's also an override function?

Ping?

[Zachary Kuznia, 2015-08-26 23:31:13]

https://codereview.chromium.org/1281263003/diff/80001/extensions/browser/api/...
File extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc
(right):

https://codereview.chromium.org/1281263003/diff/80001/extensions/browser/api/...
extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc:345:
success_ &= success;
You shouldn't use bitwise AND for bools.

https://codereview.chromium.org/1281263003/diff/80001/extensions/browser/api/...
extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc:360:
webcam_private::WebcamConfiguration result;
You shouldn't assume that async functions are invoked in order.  Instead, track
which ones have completed, and set the result when all three have finished.

[xdai1, 2015-08-27 20:02:34]

On 2015/08/26 23:31:13, Zachary Kuznia wrote:
>
https://codereview.chromium.org/1281263003/diff/80001/extensions/browser/api/...
> File extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc
> (right):
> 
>
https://codereview.chromium.org/1281263003/diff/80001/extensions/browser/api/...
> extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc:345:
> success_ &= success;
> You shouldn't use bitwise AND for bools.
> 
>
https://codereview.chromium.org/1281263003/diff/80001/extensions/browser/api/...
> extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc:360:
> webcam_private::WebcamConfiguration result;
> You shouldn't assume that async functions are invoked in order.  Instead,
track
> which ones have completed, and set the result when all three have finished.

Zach, I've addressed your comments. Please take another look, thanks!

[xdai1, 2015-08-31 17:33:13]

On 2015/08/27 20:02:34, xdai1 wrote:
> On 2015/08/26 23:31:13, Zachary Kuznia wrote:
> >
>
https://codereview.chromium.org/1281263003/diff/80001/extensions/browser/api/...
> > File extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1281263003/diff/80001/extensions/browser/api/...
> > extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc:345:
> > success_ &= success;
> > You shouldn't use bitwise AND for bools.
> > 
> >
>
https://codereview.chromium.org/1281263003/diff/80001/extensions/browser/api/...
> > extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc:360:
> > webcam_private::WebcamConfiguration result;
> > You shouldn't assume that async functions are invoked in order.  Instead,
> track
> > which ones have completed, and set the result when all three have finished.
> 
> Zach, I've addressed your comments. Please take another look, thanks!

friendly ping?

[Zachary Kuznia, 2015-08-31 19:47:18]

lgtm

[xdai1, 2015-08-31 21:34:47]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2015-08-31 21:35:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281263003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281263003/100001

[commit-bot: I haz the power, 2015-08-31 21:54:03]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-08-31 21:54:47]

Patchset 4 (id:??) landed as
https://crrev.com/50ab547896bb9f0d4afa94b1676f4a5e022fae31
Cr-Commit-Position: refs/heads/master@{#346479}