[webkit-reviews] review granted: [Bug 20879] Implement HTML5 channel messaging : [Attachment 23654] updated patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 20879: Implement HTML5 channel messaging
https://bugs.webkit.org/show_bug.cgi?id=20879

Attachment 23654: updated patch
https://bugs.webkit.org/attachment.cgi?id=23654&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
>+/* Hash table for constructor */
>+
>+static const HashTableValue JSMessageChannelConstructorTableValues[1] =
>+{
>+    { 0, 0, 0, 0 }
>+};
>+
>+static const HashTable JSMessageChannelConstructorTable = { 0,
JSMessageChannelConstructorTableValues, 0 };
>+
>+const ClassInfo JSMessageChannelConstructor::s_info = {
"MessageChannelConstructor", 0, &JSMessageChannelConstructorTable, 0 };

I don't think you need a table at all.

>+	  Document* m_document;

This should be private.

>+void Document::dispatchMessagePortEvents()
>+{
>+    RefPtr<Document> protect(this);
>+    HashSet<MessagePort*> ports = m_messagePorts; // Make a frozen copy.

Can you make the frozen copy by copying to a Vector?

>+void Document::createdMessagePort(MessagePort* port)
>+{

This should assert that the MessagePort is not null.

>+void Document::destroyedMessagePort(MessagePort* port)
>+{

As should this.

>+	  MessagePort* port1() { return m_port1.get(); }
>+	  MessagePort* port2() { return m_port2.get(); }

These should be const.

>+class CloseMessagePortTimer : public TimerBase {
>+public:
>+    CloseMessagePortTimer(PassRefPtr<MessagePort> port)
>+	  : m_port(port)
>+    {

This should assert that the port is not null.

>+    virtual void fired()
>+    {
>+	  ASSERT(!m_port->active());
>+
>+	  // Closing may destroy the port, dispatch any remaining messages now.

>+	  if (m_port->queueIsOpen())
>+	      m_port->dispatchMessages();
>+
>+	  RefPtr<Event> evt = Event::create(EventNames::closeEvent, false,
true);
>+	  if (m_port->onCloseListener()) {
>+	      evt->setTarget(m_port.get());
>+	      evt->setCurrentTarget(m_port.get());
>+	      m_port->onCloseListener()->handleEvent(evt.get(), false);
>+	  }
>+
>+	  ExceptionCode ec = 0;
>+	  m_port->dispatchEvent(evt.release(), ec, false);
>+	  ASSERT(!ec);
>+
>+	  delete this;

I think this would be cleaner if the all this work was done in a member
function of MessagePort, which this fired method calls.

>+    m_entangledPort->m_messageQueue.append(MessageEvent::create(message, "",
"", m_createdWithDocument->domWindow(), newMessagePort.get()));

Why is the origin the empty string?

>+PassRefPtr<MessagePort> MessagePort::startConversation(Document*
scriptContext, const String& message)

Perhaps the variable should be named scriptContextDocument?

>+    m_entangledPort->m_messageQueue.append(MessageEvent::create(message, "",
"", m_createdWithDocument->domWindow(), port2.get()));

Why is the origin the empty string?

>+    MessagePort* otherPort = m_entangledPort;

otherPort is not a great name for this.

>+void MessagePort::dispatchMessages()
>+{
>+    // Messages for documents that are not fully active get dispatched too,
but JSAbstractEventListener::handleEvent() doesn't call handlers for these.
>+    ASSERT(queueIsOpen());

Is it possible for an event dispatch to cause a MessagePort to go away?  Should
it protect itself against it here?

>+	  Document* createdWithDocument() { return m_createdWithDocument; }

This sounds like it would return a boolean.  Perhaps it should be called
something like, documentCreatedWith, associatedDocument, maybe even just
document().

r=me and Anders!