Presentation API: implement .getAvalability() (Blink side).

Presentation API: implement .getAvalability() (Blink side).

This is part of a suite of three CLs:
1. <this>
2. https://codereview.chromium.org/1214083013
3. https://codereview.chromium.org/1220813009

BUG=498435
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198323

[mlamouri (slow - plz ping), 2015-07-02 16:36:21]

mlamouri@chromium.org changed reviewers:
+ avayvod@chromium.org, mkwst@chromium.org

[mlamouri (slow - plz ping), 2015-07-02 16:36:22]

avayvod@, PTAL at:
- Source/modules/presentation/
- public/platform/modules/presentation/

mkwst@, PTAL at:
- Source/modules/EventTargetModulesFactory.in

Thanks :)

[whywhat, 2015-07-02 22:32:38]

lgtm % nits and feedback on exposing client from the controller.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/Presentation.cpp:105:
resolver->reject(DOMException::create(InvalidStateError, "The object is no
longer attached to the frame."));
nit: If we have the same check in other methods, should we eliminate the
redundancy?

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/Presentation.cpp:108:
controller->getAvailability(presentationUrl, new CallbackPromiseAdapter<bool,
void>(resolver));
so bool means this implements Promise<bool> rather than
Promise<PresentationAvailability>, no?
I guess that bool is then converted to PresentationAvailability via its ctor,
right?

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationAvailability.cpp (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.cpp:41:
PresentationAvailability* PresentationAvailability::create(ExecutionContext*
executionContext, bool value)
What's different about this create() method? Why suspend and why not start
listening?

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.cpp:89: void
PresentationAvailability::suspend()
We thought about suspending the monitoring when the page/frame is not visible.
Is this something that could be implemented here?

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.cpp:117: m_listening =
false;
nit: perhaps swap this line with the ASSERT above?

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationAvailability.h (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.h:17: // Expose whether a
device is available.
nit: Expand the comment? Also, we call devices/displays/screens "presentation
displays" in the spec so probably should stick to the term in Blink.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.h:20: , public
ActiveDOMObject
Does it have to be ActiveDOMObject? It's not part of the DOM, is it?

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationAvailability.idl (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.idl:1: // Copyright 2014
The Chromium Authors. All rights reserved.
nit: s/2014/2015

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.idl:5: //
https://w3c.github.io/presentation-api/
nit: replace with https://w3c.github.io/presentation-api/#interface-availability

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.cpp:64: void
PresentationController::didChangeAvailability(bool available)
TODO(mlamouri) to remove these methods?

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationController.h (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.h:42: WebPresentationClient*
client();
I think this is inconsistent with the other methods and breaks the encapsulation
of the controller somewhat although makes the plumbing a bit easier.
To remain consistent there should be start/stopListening() methods in the
controller to pass the observer to the client.

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
File public/platform/modules/presentation/WebPresentationAvailabilityObserver.h
(right):

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationAvailabilityObserver.h:10:
class WebPresentationAvailabilityObserver {
nit: document the new interface and its method, please

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
File public/platform/modules/presentation/WebPresentationClient.h (right):

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:61: // Called when
the frame wants to know the availability of a device to present.
nit: s/device to present/presentation display/

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:71: // Start
listening to presentation device availabilities. The observer will
nit: s/presentation device availabilities/changes in presentation displays
availability/

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:74: virtual void
startListening(WebPresentationAvailabilityObserver*) {}
don't you need to pass a url here too?

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:76: // Stop
listening to presentation device availabilities. The observer will
ditto

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:78: virtual void
stopListening(WebPresentationAvailabilityObserver*) {}
ditto

[whywhat, 2015-07-02 22:44:02]

Also, you need to 'rebase' various failing layout tests for
navigator.presentation

[mlamouri (slow - plz ping), 2015-07-03 13:58:27]

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/Presentation.cpp:105:
resolver->reject(DOMException::create(InvalidStateError, "The object is no
longer attached to the frame."));
On 2015/07/02 at 22:32:37, whywhat wrote:
> nit: If we have the same check in other methods, should we eliminate the
redundancy?

I will look into refactoring in a follow-up. This CL is already big enough as it
is.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/Presentation.cpp:108:
controller->getAvailability(presentationUrl, new CallbackPromiseAdapter<bool,
void>(resolver));
On 2015/07/02 at 22:32:37, whywhat wrote:
> so bool means this implements Promise<bool> rather than
Promise<PresentationAvailability>, no?
> I guess that bool is then converted to PresentationAvailability via its ctor,
right?

Oouups. That was meant to be temporary :) Fixed. PTAL.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationAvailability.cpp (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.cpp:89: void
PresentationAvailability::suspend()
On 2015/07/02 at 22:32:37, whywhat wrote:
> We thought about suspending the monitoring when the page/frame is not visible.
Is this something that could be implemented here?

I do not think it will include page visibility but I would gladly do that in a
follow-up.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.cpp:117: m_listening =
false;
On 2015/07/02 at 22:32:37, whywhat wrote:
> nit: perhaps swap this line with the ASSERT above?

I prefer to have ASSERT() as early as possible.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationAvailability.h (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.h:17: // Expose whether a
device is available.
On 2015/07/02 at 22:32:37, whywhat wrote:
> nit: Expand the comment? Also, we call devices/displays/screens "presentation
displays" in the spec so probably should stick to the term in Blink.

Done.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.h:20: , public
ActiveDOMObject
On 2015/07/02 at 22:32:37, whywhat wrote:
> Does it have to be ActiveDOMObject? It's not part of the DOM, is it?

ActiveDOMObject are actually objects associated to an ExecutionContext. The name
is confusing. It's a way to make sure the objects are de-activated when the
ExecutionContext is paused/stopped. For example, when a Document is detached.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationAvailability.idl (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.idl:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2015/07/02 at 22:32:37, whywhat wrote:
> nit: s/2014/2015

Done.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.idl:5: //
https://w3c.github.io/presentation-api/
On 2015/07/02 at 22:32:37, whywhat wrote:
> nit: replace with
https://w3c.github.io/presentation-api/#interface-availability

Done.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationController.cpp (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.cpp:64: void
PresentationController::didChangeAvailability(bool available)
On 2015/07/02 at 22:32:37, whywhat wrote:
> TODO(mlamouri) to remove these methods?

In order to not have to rebase the CL that actually removes these methods, I
will not add the TODO.

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationController.h (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.h:42: WebPresentationClient*
client();
On 2015/07/02 at 22:32:38, whywhat wrote:
> I think this is inconsistent with the other methods and breaks the
encapsulation of the controller somewhat although makes the plumbing a bit
easier.
> To remain consistent there should be start/stopListening() methods in the
controller to pass the observer to the client.

The only reason why these controllers exist is to hold the FooClient that can't
be accessed from inside a module otherwise. I think these controllers should
limit themselves to this purpose: make sure m_client is never stall. This is
what the PermissionController is doing for example. It's a simple class with a
simple purpose. I think the boilerplate added by having the Controller being
used to make calls is unecessary.

In the long term, the Client should be available by other means than using a
Controller. I sent a Design Doc a while ago to blink-dev with an idea.
Unfortunately, I didn't found the time to go forward with the idea but I think
reducing Controller role is the way forward.

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
File public/platform/modules/presentation/WebPresentationAvailabilityObserver.h
(right):

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationAvailabilityObserver.h:10:
class WebPresentationAvailabilityObserver {
On 2015/07/02 at 22:32:38, whywhat wrote:
> nit: document the new interface and its method, please

Done.

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
File public/platform/modules/presentation/WebPresentationClient.h (right):

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:61: // Called when
the frame wants to know the availability of a device to present.
On 2015/07/02 at 22:32:38, whywhat wrote:
> nit: s/device to present/presentation display/

Done.

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:71: // Start
listening to presentation device availabilities. The observer will
On 2015/07/02 at 22:32:38, whywhat wrote:
> nit: s/presentation device availabilities/changes in presentation displays
availability/

Done.

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:74: virtual void
startListening(WebPresentationAvailabilityObserver*) {}
On 2015/07/02 at 22:32:38, whywhat wrote:
> don't you need to pass a url here too?

I guess we could but we wouldn't use it anyway. Given the future changes we are
going to do, I think we should keep this to a minimum.

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:76: // Stop
listening to presentation device availabilities. The observer will
On 2015/07/02 at 22:32:38, whywhat wrote:
> ditto

Done.

https://codereview.chromium.org/1222623003/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:78: virtual void
stopListening(WebPresentationAvailabilityObserver*) {}
On 2015/07/02 at 22:32:38, whywhat wrote:
> ditto

See above.

[mlamouri (slow - plz ping), 2015-07-03 14:13:10]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org

[mlamouri (slow - plz ping), 2015-07-03 14:13:10]

+jochen@ given that mkwst@ is OOO today.

jochen@, can you review:
- Source/modules/EventTargetModulesFactory.in

[whywhat, 2015-07-03 15:40:47]

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationAvailability.cpp (right):

https://codereview.chromium.org/1222623003/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailability.cpp:117: m_listening =
false;
On 2015/07/03 at 13:58:26, Mounir Lamouri wrote:
> On 2015/07/02 at 22:32:37, whywhat wrote:
> > nit: perhaps swap this line with the ASSERT above?
> 
> I prefer to have ASSERT() as early as possible.

I think logically m_listening belongs to the if above and ASSERT to the use of
executionContext() below.
There's also a nuance that even if the client is null and stopListening is not
called, m_listening becomes false anyway so consequent calls to this method will
be no-op.

[jochen (gone - plz use gerrit), 2015-07-06 10:34:01]

lgtm

[mlamouri (slow - plz ping), 2015-07-06 11:22:43]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-07-06 11:22:43]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1222623003/#ps20001
(title: "review comments")

[commit-bot: I haz the power, 2015-07-06 11:22:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1222623003/20001

[commit-bot: I haz the power, 2015-07-06 12:26:28]

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

[commit-bot: I haz the power, 2015-07-06 12:26:29]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/61742)

(exceeded global retry quota)

[mlamouri (slow - plz ping), 2015-07-06 14:55:54]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-07-06 14:55:54]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1222623003/#ps40001
(title: "fix tests")

[commit-bot: I haz the power, 2015-07-06 14:56:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1222623003/40001

[commit-bot: I haz the power, 2015-07-06 16:11:48]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198323

[mark a. foltz, 2015-07-07 23:47:14]

On 2015/07/06 at 16:11:48, commit-bot wrote:
> Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198323

This change broke compatibility with existing code that uses the availablechange
event.  I'm creating a revert so we can try again with a deprecation path for
this event.

[mlamouri (slow - plz ping), 2015-07-07 23:56:04]

On 2015/07/07 at 23:47:14, mfoltz wrote:
> On 2015/07/06 at 16:11:48, commit-bot wrote:
> > Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198323
> 
> This change broke compatibility with existing code that uses the
availablechange event.  I'm creating a revert so we can try again with a
deprecation path for this event.

What exactly broke?

Note that this Blink feature is behind an experimental flag. We usually do not
keep any kind of backward compatibility for experimental feature. The cost of
doing so would be just too high.