MediaStream Image Capture (1): idl and wireframe .h/.cpp/LayoutTest

MediaStream Image Capture (1): idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one
member and one method, grabFrame().

A LayoutTest is added exercising some Exceptions and
the said method, which returns a rejected Promise if
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).

See https://crrev.com/1846753003 for a more complete
example of how it all fits together.

BUG=518807
Committed: https://crrev.com/06765494ed8bdfc335628c409450c23aa79ad2a7
Cr-Commit-Position: refs/heads/master@{#388799}

[mcasas, 2016-04-19 18:21:56]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-04-19 18:31:39]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-04-19 18:36:04]

Description was changed from

==========
[WIP] MediaStream Image Capture idl and wireframe h/cpp/LayoutTest

BUG=604032
==========

to

==========
MediaStream Image Capture idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).


See https://crrev.com/1846753003 for a more complete
example of other functionalities.

BUG=604032
==========

[mcasas, 2016-04-19 18:36:30]

Description was changed from

==========
MediaStream Image Capture idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).


See https://crrev.com/1846753003 for a more complete
example of other functionalities.

BUG=604032
==========

to

==========
MediaStream Image Capture idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).


See https://crrev.com/1846753003 for a more complete
example of other functionalities.

BUG=518807
==========

[mcasas, 2016-04-19 18:45:25]

Description was changed from

==========
MediaStream Image Capture idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).


See https://crrev.com/1846753003 for a more complete
example of other functionalities.

BUG=518807
==========

to

==========
MediaStream Image Capture idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).

See https://crrev.com/1846753003 for a more complete
example of how it all fits together.

BUG=518807
==========

[mcasas, 2016-04-19 23:37:52]

Patchset #1 (id:40001) has been deleted

[mcasas, 2016-04-19 23:38:15]

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

[mcasas, 2016-04-19 23:38:15]

whywhat@ PTAL

[haraken, 2016-04-19 23:50:55]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-04-19 23:50:56]

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

https://codereview.chromium.org/1890313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:38: if
(!getExecutionContext() || getExecutionContext()->activeDOMObjectsAreStopped())

A better way to do this would be:

- Make ImageCapture inherit from ContextLifecycleObserver.
- Clear all event listeners in ContextLifecycleObserver::stop().
- hasPendingActivity() should just return hasEventListeners().

If you don't need to observe suspend/resume events, ImageCapture doesn't need to
be an ActiveDOMObject. ContextLifecycleObserver would be enough.

[mcasas, 2016-04-20 00:31:51]

PTAL

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

https://codereview.chromium.org/1890313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:38: if
(!getExecutionContext() || getExecutionContext()->activeDOMObjectsAreStopped())
On 2016/04/19 23:50:56, haraken wrote:
> 
> A better way to do this would be:
> 
> - Make ImageCapture inherit from ContextLifecycleObserver.
> - Clear all event listeners in ContextLifecycleObserver::stop().
> - hasPendingActivity() should just return hasEventListeners().
> 
> If you don't need to observe suspend/resume events, ImageCapture doesn't need
to
> be an ActiveDOMObject. ContextLifecycleObserver would be enough.

Done, I think.

[haraken, 2016-04-20 00:41:04]

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

https://codereview.chromium.org/1890313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:19:
exceptionState.throwDOMException(NotSupportedError, "Cannot create an
ImageCapturer from a non-video Track.");

You're throwing an exception but creating a new ImageCapture. Is it a speced
behavior?

https://codereview.chromium.org/1890313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:36: return
hasEventListeners();

Can we add a test to check that ImageCapture is kept alive while it has an event
listener?

[mcasas, 2016-04-20 00:53:42]

PTAL

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

https://codereview.chromium.org/1890313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:19:
exceptionState.throwDOMException(NotSupportedError, "Cannot create an
ImageCapturer from a non-video Track.");
On 2016/04/20 00:41:04, haraken wrote:
> 
> You're throwing an exception but creating a new ImageCapture. Is it a speced
> behavior?

Oops, no, it isn't, sorry.

https://codereview.chromium.org/1890313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:36: return
hasEventListeners();
On 2016/04/20 00:41:04, haraken wrote:
> 
> Can we add a test to check that ImageCapture is kept alive while it has an
event
> listener?

Added one in contextDestroyed() and in the dtor.

[haraken, 2016-04-20 01:01:41]

In terms of implementation, LGTM.

https://codereview.chromium.org/1890313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):

https://codereview.chromium.org/1890313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:41: bool
ImageCapture::hasPendingActivity() const

I meant, it would be better to add a layout test to check that ImageCapture's
wrapper is not collected while it has event listeners.

[mcasas, 2016-04-20 02:54:55]

Description was changed from

==========
MediaStream Image Capture idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).

See https://crrev.com/1846753003 for a more complete
example of how it all fits together.

BUG=518807
==========

to

==========
MediaStream Image Capture (1): idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).

See https://crrev.com/1846753003 for a more complete
example of how it all fits together.

BUG=518807
==========

[mcasas, 2016-04-21 00:35:22]

On 2016/04/20 01:01:41, haraken wrote:
> In terms of implementation, LGTM.
> 
>
https://codereview.chromium.org/1890313002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):
> 
>
https://codereview.chromium.org/1890313002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:41: bool
> ImageCapture::hasPendingActivity() const
> 
> I meant, it would be better to add a layout test to check that ImageCapture's
> wrapper is not collected while it has event listeners.


Glad to do that, can you point to any example?

Ping whywhat@

[whywhat, 2016-04-21 14:46:22]

On 2016/04/21 at 00:35:22, mcasas wrote:
> On 2016/04/20 01:01:41, haraken wrote:
> > In terms of implementation, LGTM.
> > 
> >
https://codereview.chromium.org/1890313002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp
(right):
> > 
> >
https://codereview.chromium.org/1890313002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:41: bool
> > ImageCapture::hasPendingActivity() const
> > 
> > I meant, it would be better to add a layout test to check that
ImageCapture's
> > wrapper is not collected while it has event listeners.
> 
> 
> Glad to do that, can you point to any example?

I think worker has a good example:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

You need to be able to trigger the event similar to postMessage in the example.

Otherwise, lgtm

> 
> Ping whywhat@

[mcasas, 2016-04-21 15:49:17]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-04-21 15:49:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1890313002/#ps120001
(title: "Added TODO for the mentioned LayoutTests")

[commit-bot: I haz the power, 2016-04-21 15:49:36]

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

[commit-bot: I haz the power, 2016-04-21 17:18:44]

Description was changed from

==========
MediaStream Image Capture (1): idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).

See https://crrev.com/1846753003 for a more complete
example of how it all fits together.

BUG=518807
==========

to

==========
MediaStream Image Capture (1): idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).

See https://crrev.com/1846753003 for a more complete
example of how it all fits together.

BUG=518807
==========

[commit-bot: I haz the power, 2016-04-21 17:18:46]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-04-22 19:35:37]

Description was changed from

==========
MediaStream Image Capture (1): idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one 
member and one method, grabFrame(). 

A LayoutTest is added exercising some Exceptions and 
the said method, which returns a rejected Promise if 
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).

See https://crrev.com/1846753003 for a more complete
example of how it all fits together.

BUG=518807
==========

to

==========
MediaStream Image Capture (1): idl and wireframe .h/.cpp/LayoutTest

This CL adds a minimalistic ImageCapture.idl, with one
member and one method, grabFrame().

A LayoutTest is added exercising some Exceptions and
the said method, which returns a rejected Promise if
the Video Track associated to the ImageCapture is in
an inappropriate state (i.e. not live, or disabled).

See https://crrev.com/1846753003 for a more complete
example of how it all fits together.

BUG=518807

Committed: https://crrev.com/06765494ed8bdfc335628c409450c23aa79ad2a7
Cr-Commit-Position: refs/heads/master@{#388799}
==========

[commit-bot: I haz the power, 2016-04-22 19:35:39]

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