The rest of the Presentation API IDL as stubs.

The rest of the Presentation API IDL as stubs.

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

[whywhat, 2014-10-29 20:15:55]

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

[whywhat, 2014-10-29 20:15:55]

The first draft of the patch for early comments, not ready to submit.

[Peter Beverloo, 2014-10-29 20:51:20]

Can you add some tests for the new methods on the Presentation object? I
understand that PresentationSession is not yet.. reachable in the current code.

Do you have some kind of design document for your plans here?

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

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:8: #include
"bindings/core/v8/ScriptPromise.h"
You already include this in the header.

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:10: #include
"modules/EventTargetModules.h"
+#include "core/dom/DOMException.h"

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:46: return m_session.get();
visitor->trace(m_session);

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:63: resolver->reject(exception);
Just call DOMException::create() as part of this call.

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

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:9: #include
"bindings/core/v8/ScriptState.h"
You can forward declare this.

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:43:
PersistentWillBeMember<PresentationSession> m_session;
This should be a Member<>.

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

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.idl:12: [CallWith=ScriptState] Promise
startSession(DOMString senderId, DOMString? presentationId);
Fwiw, nullable and optional are not the same thing here. Do you mean to include
the "optional" keyword?

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

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationSession.idl:18: void
postMessage(DOMString message);
nit: blank lines between the "sections".

[whywhat, 2014-10-29 22:09:26]

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

[whywhat, 2014-10-29 22:09:27]

Added tests as well. Thanks for the quick review!

+Mark to verify I've got the spec right.

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

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:8: #include
"bindings/core/v8/ScriptPromise.h"
On 2014/10/29 20:51:19, Peter Beverloo wrote:
> You already include this in the header.

IWYU (include what you use)

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:10: #include
"modules/EventTargetModules.h"
On 2014/10/29 20:51:19, Peter Beverloo wrote:
> +#include "core/dom/DOMException.h"

Done.

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:46: return m_session.get();
On 2014/10/29 20:51:19, Peter Beverloo wrote:
> visitor->trace(m_session);

Done.

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.cpp:63: resolver->reject(exception);
On 2014/10/29 20:51:19, Peter Beverloo wrote:
> Just call DOMException::create() as part of this call.

Done.

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

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:9: #include
"bindings/core/v8/ScriptState.h"
On 2014/10/29 20:51:19, Peter Beverloo wrote:
> You can forward declare this.

Done.

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.h:43:
PersistentWillBeMember<PresentationSession> m_session;
On 2014/10/29 20:51:19, Peter Beverloo wrote:
> This should be a Member<>.

Done.

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

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/Presentation.idl:12: [CallWith=ScriptState] Promise
startSession(DOMString senderId, DOMString? presentationId);
On 2014/10/29 20:51:19, Peter Beverloo wrote:
> Fwiw, nullable and optional are not the same thing here. Do you mean to
include
> the "optional" keyword?

Done.

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

https://codereview.chromium.org/685263003/diff/1/Source/modules/presentation/...
Source/modules/presentation/PresentationSession.idl:18: void
postMessage(DOMString message);
On 2014/10/29 20:51:20, Peter Beverloo wrote:
> nit: blank lines between the "sections".

Done.

[whywhat, 2014-10-29 22:20:44]

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

[whywhat, 2014-10-29 22:20:44]

+Eric for OWNERS approval

[Peter Beverloo, 2014-10-29 22:21:17]

lgtm % comments.

Based on the link to the specification included in the IDL files, the
definitions look correct to me.

https://codereview.chromium.org/685263003/diff/40001/LayoutTests/fast/dom/Win...
File
LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt
(right):

https://codereview.chromium.org/685263003/diff/40001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt:54:
FAIL childWindow.navigator.presentation.session should be null. Threw exception
TypeError: Cannot read property 'session' of null
Is this something we should worry about?

https://codereview.chromium.org/685263003/diff/40001/Source/modules/presentat...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/685263003/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.idl:13: [CallWith=ScriptState] Promise
startSession(DOMString senderId, optional DOMString presentationId = "");
It looks like the more common way to do this is:

[Default=Undefined] optional DOMString presentationId

https://codereview.chromium.org/685263003/diff/40001/Source/modules/presentat...
File Source/modules/presentation/PresentationSession.h (right):

https://codereview.chromium.org/685263003/diff/40001/Source/modules/presentat...
Source/modules/presentation/PresentationSession.h:10: #include
"wtf/text/WTFString.h"
+#include "wtf/text/AtomicString.h"

.. under the caller IWYU :-).

[whywhat, 2014-10-29 22:34:06]

https://codereview.chromium.org/685263003/diff/40001/LayoutTests/fast/dom/Win...
File
LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt
(right):

https://codereview.chromium.org/685263003/diff/40001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt:54:
FAIL childWindow.navigator.presentation.session should be null. Threw exception
TypeError: Cannot read property 'session' of null
On 2014/10/29 22:21:17, Peter Beverloo wrote:
> Is this something we should worry about?

No idea :(

https://codereview.chromium.org/685263003/diff/40001/Source/modules/presentat...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/685263003/diff/40001/Source/modules/presentat...
Source/modules/presentation/Presentation.idl:13: [CallWith=ScriptState] Promise
startSession(DOMString senderId, optional DOMString presentationId = "");
On 2014/10/29 22:21:17, Peter Beverloo wrote:
> It looks like the more common way to do this is:
> 
> [Default=Undefined] optional DOMString presentationId

Done.

https://codereview.chromium.org/685263003/diff/40001/Source/modules/presentat...
File Source/modules/presentation/PresentationSession.h (right):

https://codereview.chromium.org/685263003/diff/40001/Source/modules/presentat...
Source/modules/presentation/PresentationSession.h:10: #include
"wtf/text/WTFString.h"
On 2014/10/29 22:21:17, Peter Beverloo wrote:
> +#include "wtf/text/AtomicString.h"
> 
> .. under the caller IWYU :-).

Done.

[mark a. foltz, 2014-10-29 23:13:14]

https://codereview.chromium.org/685263003/diff/60001/Source/modules/presentat...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/685263003/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.idl:13: [CallWith=ScriptState] Promise
startSession(DOMString senderId, [Default=Undefined] optional DOMString
presentationId);
Does Blink IDL support Promise<T>?  If so can this be written:

Promise<PresentationSession>

FYI:
http://www.w3.org/2001/tag/doc/promises-guide#webidl-promise-return-values

https://codereview.chromium.org/685263003/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.idl:14: [CallWith=ScriptState] Promise
joinSession(DOMString senderId, [Default=Undefined] optional DOMString
presentationId);
Same comment here.

[whywhat, 2014-10-30 00:51:01]

https://codereview.chromium.org/685263003/diff/60001/Source/modules/presentat...
File Source/modules/presentation/Presentation.idl (right):

https://codereview.chromium.org/685263003/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.idl:13: [CallWith=ScriptState] Promise
startSession(DOMString senderId, [Default=Undefined] optional DOMString
presentationId);
On 2014/10/29 23:13:14, mark a. foltz wrote:
> Does Blink IDL support Promise<T>?  If so can this be written:
> 
> Promise<PresentationSession>
> 
> FYI:
> http://www.w3.org/2001/tag/doc/promises-guide#webidl-promise-return-values

Good point. Done. Only found one occurrence in the Blink codebase though
(WebBluetooth).

https://codereview.chromium.org/685263003/diff/60001/Source/modules/presentat...
Source/modules/presentation/Presentation.idl:14: [CallWith=ScriptState] Promise
joinSession(DOMString senderId, [Default=Undefined] optional DOMString
presentationId);
On 2014/10/29 23:13:14, mark a. foltz wrote:
> Same comment here.

Done.

[mark a. foltz, 2014-10-31 06:17:32]

lgtm

[sof, 2014-10-31 13:28:46]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-10-31 13:28:47]

lgtm

https://codereview.chromium.org/685263003/diff/80001/Source/modules/presentat...
File Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/685263003/diff/80001/Source/modules/presentat...
Source/modules/presentation/Presentation.cpp:44: visitor->trace(m_session);
nit: the convention is to trace the local members first, then delegate. can be
tidied in a followup.

[whywhat, 2014-10-31 15:33:48]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2014-10-31 15:34:48]

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

[whywhat, 2014-10-31 15:35:33]

The CQ bit was unchecked by avayvod@chromium.org

[mlamouri (slow - plz ping), 2014-11-02 23:53:31]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-11-02 23:53:59]

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

[commit-bot: I haz the power, 2014-11-02 23:57:03]

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

[commit-bot: I haz the power, 2014-11-02 23:57:05]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[sof, 2014-11-02 23:57:05]

sigbjornf@opera.com changed reviewers:
+ mkwst@chromium.org
- mlamouri@chromium.org

[sof, 2014-11-02 23:57:05]

+mkwst for modules/ owner review, should he have some time.

(modules/presentation/OWNERS will spring into being before not too long..? )

[Mike West, 2014-11-03 04:58:12]

rubberstamp LGTM.

And please do follow sof@'s advice by adding an OWNERS file to the module's
directory, either in this CL or in a followup.

[whywhat, 2014-11-03 21:39:15]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2014-11-03 21:39:53]

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

[commit-bot: I haz the power, 2014-11-03 23:41:10]

Committed patchset #8 (id:140001) as 184806