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

Source/modules/presentation/PresentationSession.cpp

Index: Source/modules/presentation/PresentationSession.cpp
diff --git a/Source/modules/presentation/PresentationSession.cpp b/Source/modules/presentation/PresentationSession.cpp
index 4727bebdb9c0397281ff4af261da66203d9597eb..d4c8d123ea13c2d03f521f2b1f68777551d8d3c8 100644
--- a/Source/modules/presentation/PresentationSession.cpp
+++ b/Source/modules/presentation/PresentationSession.cpp
@@ -6,21 +6,35 @@
 #include "modules/presentation/PresentationSession.h"
 
 #include "core/dom/Document.h"
+#include "core/events/Event.h"
 #include "core/frame/LocalFrame.h"
 #include "modules/EventTargetModules.h"
 #include "modules/presentation/Presentation.h"
 #include "modules/presentation/PresentationController.h"
-#include "public/platform/WebString.h"
-#include "public/platform/modules/presentation/WebPresentationSessionClient.h"
 #include "wtf/OwnPtr.h"
 
 namespace blink {
 
+namespace {
+
+const AtomicString SessionStateToString(WebPresentationSessionClient::SessionState state)

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

Why do you return an AtomicString here and not just a String?

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

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

[whywhat, 2015/03/23 21:12:16]

Done.

[whywhat, 2015/03/23 21:12:16]

To avoid extra conversion (from String to AtomicString)

+{
+    switch (state) {
+    case WebPresentationSessionClient::SessionStateConnected:
+        return "connected";
+    case WebPresentationSessionClient::SessionStateDisconnected:
+    default:
+        return "disconnected";
+    }
+}
+
+} // namespace
+
 PresentationSession::PresentationSession(LocalFrame* frame, const WebString& id, const WebString& url)
     : DOMWindowProperty(frame)
     , m_id(id)
     , m_url(url)
-    , m_state("disconnected")
+    , m_state(WebPresentationSessionClient::SessionStateDisconnected)
 {
 }
 
@@ -63,19 +77,35 @@ DEFINE_TRACE(PresentationSession)
     DOMWindowProperty::trace(visitor);
 }
 
+const AtomicString PresentationSession::state() const

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

Why is this returning a string instead of the enum value?

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

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.

[whywhat, 2015/03/23 21:12:16]

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

[whywhat, 2015/03/23 21:12:16]

Acknowledged.

+{
+    return SessionStateToString(m_state);
+}
+
 void PresentationSession::postMessage(const String& message)
 {
 }
 
 void PresentationSession::close()
 {
-    if (m_state != "connected")
+    if (m_state != WebPresentationSessionClient::SessionStateConnected)
         return;
     PresentationController* controller = presentationController();
     if (controller)
         controller->closeSession(m_url, m_id);
 }
 
+bool PresentationSession::matches(WebPresentationSessionClient* client) const
+{
+    return client && client->getUrl() == m_url && client->getId() == m_id;

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

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?

[whywhat, 2015/03/23 21:12:16]

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.

+}
+
+void PresentationSession::didChangeState(WebPresentationSessionClient::SessionState state)
+{
+    m_state = state;

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

What if m_state == state?  We don't need to dispatch an event then.

[whywhat, 2015/03/23 21:12:16]

Done.

+    dispatchEvent(Event::create(EventTypeNames::statechange));
+}
+
 PresentationController* PresentationSession::presentationController()
 {
     if (!frame())