[PresentationAPI] Plumbing |onstatechange| event for the PresentationSession from Blink to platform/

[PresentationAPI] Plumbing |onstatechange| event for the PresentationSession from Blink to platform/

The session is referenced by its url and id.
Added an enum to represent the session state as well.

Blocks a Chromium change: https://codereview.chromium.org/1024903003

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

[whywhat, 2015-03-20 18:50:34]

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

[whywhat, 2015-03-20 18:50:35]

PTaL!

[Peter Beverloo, 2015-03-20 19:07:40]

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/Presentation.cpp:162: for (auto it =
m_openSessions.begin(); it != m_openSessions.end(); ++it) {
for (const auto& session : m_openSessions)

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.cpp:92: PresentationSession*
session = m_presentation->findSession(sessionClient);
Do you expect more events following this paradigm? Otherwise you could avoid
exposing something like findSession() in favor of following the rest of your
plumbing, e.g. isAvailableChangeWatched().


if (m_presentation)
    m_presentation->didChangeSessionState(sessionClient, state);

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:20: const AtomicString
SessionStateToString(WebPresentationSessionClient::SessionState state)
Why do you return an AtomicString here and not just a String?

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:45: // Returns true if and
only if the WebPresentationSessionClient represents this session.
"matches" sounds like something that could be a pattern. Why not call this
isEqual?

nit: could use "iff" instead of "if and only if".

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

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationController.h:29: virtual
void didChangeSessionState(WebPresentationSessionClient*,
WebPresentationSessionClient::SessionState) = 0;
This means that the Chromium patch has to land first, otherwise you'll have
unresolved pure virtual methods in your class. The patch' descriptions don't
agree.

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

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationSessionClient.h:18:
SessionStateUnknown,
When would we use "Unknown"?

[Peter Beverloo, 2015-03-20 19:17:00]

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

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationController.h:29: virtual
void didChangeSessionState(WebPresentationSessionClient*,
WebPresentationSessionClient::SessionState) = 0;
On 2015/03/20 19:07:40, Peter Beverloo wrote:
> This means that the Chromium patch has to land first, otherwise you'll have
> unresolved pure virtual methods in your class. The patch' descriptions don't
> agree.

Scrap that :-).

[mark a. foltz, 2015-03-20 20:40:21]

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

[mark a. foltz, 2015-03-20 20:40:22]

Drive-by

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:80: const AtomicString
PresentationSession::state() const
Why is this returning a string instead of the enum value?

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:100: return client &&
client->getUrl() == m_url && client->getId() == m_id;
The match between a PresentationSession and |client| shouldn't change once the
url and id are set.

Do we need to do this lookup every time?

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:105: m_state = state;
What if m_state == state?  We don't need to dispatch an event then.

[mlamouri (slow - plz ping), 2015-03-23 17:54:15]

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

[mlamouri (slow - plz ping), 2015-03-23 17:54:16]

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:20: const AtomicString
SessionStateToString(WebPresentationSessionClient::SessionState state)
On 2015/03/20 at 19:07:40, Peter Beverloo wrote:
> Why do you return an AtomicString here and not just a String?

I think using an AtomicString is fine. However, could you use
DEFINE_STATIC_LOCAL() for your strings?
Example:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Note that there is a bug open to make this pattern easier: bug 469608

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:80: const AtomicString
PresentationSession::state() const
On 2015/03/20 at 20:40:22, mark a. foltz wrote:
> Why is this returning a string instead of the enum value?

I would guess that ::state() is being used by the bindings code. IOW, if you
call session.state in Javascript, this methad will be called.

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:11: #include
"public/platform/modules/presentation/WebPresentationSessionClient.h"
With enum class for the enum, you can forward declare it with |enum class
SessionState;|

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:45: // Returns true if and
only if the WebPresentationSessionClient represents this session.
On 2015/03/20 at 19:07:40, Peter Beverloo wrote:
> "matches" sounds like something that could be a pattern. Why not call this
isEqual?
> 
> nit: could use "iff" instead of "if and only if".

A PresentationSession being equal to a PresentationSessionClient sounds odd to
me. Matches is fine as far as I am concerned.

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

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationController.h:8: #include
"public/platform/modules/presentation/WebPresentationSessionClient.h"
You should make the enum an enum class so you can forward declare it.

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

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationSessionClient.h:15: enum
SessionState {
Could you make this an enum class?

It will require you to rename SessionStafeValue as Value but it will allow
forward declaration.

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationSessionClient.h:19:
SessionStateLast = SessionStateUnknown
Do you use SessionStateLast? It is commonly used for IPC but I assume you will
be using Mojo, right?

[whywhat, 2015-03-23 21:12:17]

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/Presentation.cpp:162: for (auto it =
m_openSessions.begin(); it != m_openSessions.end(); ++it) {
On 2015/03/20 19:07:40, Peter Beverloo wrote:
> for (const auto& session : m_openSessions)

Done. I can't use a const iterator since I need to return a pointer to the
mutable object.

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationController.cpp:92: PresentationSession*
session = m_presentation->findSession(sessionClient);
On 2015/03/20 19:07:40, Peter Beverloo wrote:
> Do you expect more events following this paradigm? Otherwise you could avoid
> exposing something like findSession() in favor of following the rest of your
> plumbing, e.g. isAvailableChangeWatched().
> 
> 
> if (m_presentation)
>     m_presentation->didChangeSessionState(sessionClient, state);

Yes, there will be at least an event about receiving a message. I can move
findSession to the private section of Presentation class though and follow the
pattern of delegating to Presentation.

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:20: const AtomicString
SessionStateToString(WebPresentationSessionClient::SessionState state)
On 2015/03/20 19:07:40, Peter Beverloo wrote:
> Why do you return an AtomicString here and not just a String?

To avoid extra conversion (from String to AtomicString)

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:20: const AtomicString
SessionStateToString(WebPresentationSessionClient::SessionState state)
On 2015/03/23 17:54:16, Mounir Lamouri wrote:
> On 2015/03/20 at 19:07:40, Peter Beverloo wrote:
> > Why do you return an AtomicString here and not just a String?
> 
> I think using an AtomicString is fine. However, could you use
> DEFINE_STATIC_LOCAL() for your strings?
> Example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Note that there is a bug open to make this pattern easier: bug 469608

Done.

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:80: const AtomicString
PresentationSession::state() const
On 2015/03/23 17:54:16, Mounir Lamouri wrote:
> On 2015/03/20 at 20:40:22, mark a. foltz wrote:
> > Why is this returning a string instead of the enum value?
> 
> I would guess that ::state() is being used by the bindings code. IOW, if you
> call session.state in Javascript, this methad will be called.

Acknowledged.

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:80: const AtomicString
PresentationSession::state() const
On 2015/03/20 20:40:22, mark a. foltz wrote:
> Why is this returning a string instead of the enum value?

As I understand it, that's how WebIDL enum values are represented in Blink C++
code.

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:100: return client &&
client->getUrl() == m_url && client->getId() == m_id;
On 2015/03/20 20:40:22, mark a. foltz wrote:
> The match between a PresentationSession and |client| shouldn't change once the
> url and id are set.
> 
> Do we need to do this lookup every time?

I expected the list to be fairy small. We could use a hash map (from url/id to
the session) but I think it's a premature optimization?

Client is a short-lived object (like a message between layers) that we dispose
of as soon as it serves it purpose.

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:105: m_state = state;
On 2015/03/20 20:40:22, mark a. foltz wrote:
> What if m_state == state?  We don't need to dispatch an event then.

Done.

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

https://codereview.chromium.org/1020303004/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:11: #include
"public/platform/modules/presentation/WebPresentationSessionClient.h"
On 2015/03/23 17:54:16, Mounir Lamouri wrote:
> With enum class for the enum, you can forward declare it with |enum class
> SessionState;|

I'll try that but if I have to pass the enum by reference I probably won't
accept the suggestion.

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

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationController.h:8: #include
"public/platform/modules/presentation/WebPresentationSessionClient.h"
On 2015/03/23 17:54:16, Mounir Lamouri wrote:
> You should make the enum an enum class so you can forward declare it.

Done.

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationController.h:29: virtual
void didChangeSessionState(WebPresentationSessionClient*,
WebPresentationSessionClient::SessionState) = 0;
On 2015/03/20 19:16:59, Peter Beverloo wrote:
> On 2015/03/20 19:07:40, Peter Beverloo wrote:
> > This means that the Chromium patch has to land first, otherwise you'll have
> > unresolved pure virtual methods in your class. The patch' descriptions don't
> > agree.
> 
> Scrap that :-).

Acknowledged.

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationController.h:29: virtual
void didChangeSessionState(WebPresentationSessionClient*,
WebPresentationSessionClient::SessionState) = 0;
On 2015/03/20 19:07:40, Peter Beverloo wrote:
> This means that the Chromium patch has to land first, otherwise you'll have
> unresolved pure virtual methods in your class. The patch' descriptions don't
> agree.

Acknowledged.

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

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationSessionClient.h:15: enum
SessionState {
On 2015/03/23 17:54:16, Mounir Lamouri wrote:
> Could you make this an enum class?
> 
> It will require you to rename SessionStafeValue as Value but it will allow
> forward declaration.

Done.

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationSessionClient.h:18:
SessionStateUnknown,
On 2015/03/20 19:07:40, Peter Beverloo wrote:
> When would we use "Unknown"?

When WFT happens?

https://codereview.chromium.org/1020303004/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationSessionClient.h:19:
SessionStateLast = SessionStateUnknown
On 2015/03/23 17:54:16, Mounir Lamouri wrote:
> Do you use SessionStateLast? It is commonly used for IPC but I assume you will
> be using Mojo, right?

No, I have it for consistency only.

[mlamouri (slow - plz ping), 2015-03-23 21:25:49]

https://codereview.chromium.org/1020303004/diff/20001/Source/modules/presenta...
File Source/modules/presentation/PresentationSession.h (right):

https://codereview.chromium.org/1020303004/diff/20001/Source/modules/presenta...
Source/modules/presentation/PresentationSession.h:63: WebString m_url;
I think you should keep String here. WebString are meant as a layer between
Blink and the embedder.

You have operator WTF::String() const; that can help you here.

https://codereview.chromium.org/1020303004/diff/20001/public/platform/modules...
File public/platform/modules/presentation/WebPresentationSessionClient.h
(right):

https://codereview.chromium.org/1020303004/diff/20001/public/platform/modules...
public/platform/modules/presentation/WebPresentationSessionClient.h:15: };
Lovely <3

[mlamouri (slow - plz ping), 2015-03-23 21:37:23]

lgtm

[whywhat, 2015-03-23 21:37:46]

PTAL

https://codereview.chromium.org/1020303004/diff/20001/Source/modules/presenta...
File Source/modules/presentation/PresentationSession.h (right):

https://codereview.chromium.org/1020303004/diff/20001/Source/modules/presenta...
Source/modules/presentation/PresentationSession.h:63: WebString m_url;
On 2015/03/23 21:25:48, Mounir Lamouri wrote:
> I think you should keep String here. WebString are meant as a layer between
> Blink and the embedder.
> 
> You have operator WTF::String() const; that can help you here.

Done.

https://codereview.chromium.org/1020303004/diff/20001/public/platform/modules...
File public/platform/modules/presentation/WebPresentationSessionClient.h
(right):

https://codereview.chromium.org/1020303004/diff/20001/public/platform/modules...
public/platform/modules/presentation/WebPresentationSessionClient.h:15: };
On 2015/03/23 21:25:48, Mounir Lamouri wrote:
> Lovely <3

Acknowledged.

https://codereview.chromium.org/1020303004/diff/20001/public/platform/modules...
public/platform/modules/presentation/WebPresentationSessionClient.h:15: };
On 2015/03/23 21:25:48, Mounir Lamouri wrote:
> Lovely <3

Acknowledged.

[whywhat, 2015-03-24 17:17:16]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-03-24 17:17:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1020303004/#ps60001
(title: "Added a return statement for the conversion function")

[commit-bot: I haz the power, 2015-03-24 17:17:28]

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

[commit-bot: I haz the power, 2015-03-24 19:44:14]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192460