Updating Worker.postMessage(), DOMWindow.postMessage(), and...

First half of updating Worker.postMessage(), DOMWindow.postMessage(), and
MessagePort.postMessage() to accept multiple MessagePorts.

TEST=None (new functionality not yet exposed via bindings, so existing tests suffice)

BUG=19948

[darin (slow to review), 2009-08-21 15:12:32]

http://codereview.chromium.org/173193/diff/1/4
File webkit/api/src/PlatformMessagePortChannel.cpp (right):

http://codereview.chromium.org/173193/diff/1/4#newcode183
Line 183: std::vector<WebMessagePortChannel*> webChannels;
here, i would just use WebVector directly.  instead of calling .reserve(), you
would just initialize WebVector with a size.

http://codereview.chromium.org/173193/diff/1/4#newcode200
Line 200: std::vector<WebMessagePortChannel*> webChannels;
you need to avoid std::vector in the implementation parts of WebKit API since
this code is destined to live in svn.webkit.org.  Use WebVector or WTF::Vector
instead.

http://codereview.chromium.org/173193/diff/1/3
File webkit/glue/webworker_impl.h (right):

http://codereview.chromium.org/173193/diff/1/3#newcode86
Line 86: WTF::PassOwnPtr<WebCore::MessagePortChannelArray> channels);
maybe we should just have a WebMessagePortChannelArray type to wrap this?

[Andrew T Wilson (Slow), 2009-08-23 17:24:09]

The existing tests provide good coverage of the "sending a single port" case.
I'll add more extensive coverage of sending multiple ports to the WebKit code
once I've added JS bindings.

[darin (slow to review), 2009-08-23 18:57:35]

http://codereview.chromium.org/173193/diff/49/59
File webkit/api/public/WebVector.h (right):

http://codereview.chromium.org/173193/diff/49/59#newcode34
Line 34: #include <algorithm>
is this include necessary?

[Andrew T Wilson (Slow), 2009-08-24 03:28:48]

http://codereview.chromium.org/173193/diff/49/59
File webkit/api/public/WebVector.h (right):

http://codereview.chromium.org/173193/diff/49/59#newcode34
Line 34: #include <algorithm>
On 2009/08/23 18:57:35, darin wrote:
> is this include necessary?

Apparently <algorithm> is where std::swap() is defined (used by this class).

If there's a different include I should be using to pull this in, let me know.

[darin (slow to review), 2009-08-24 16:14:07]

On Sun, Aug 23, 2009 at 8:28 PM, <atwilson@chromium.org> wrote:

>
> http://codereview.chromium.org/173193/diff/49/59
> File webkit/api/public/WebVector.h (right):
>
> http://codereview.chromium.org/173193/diff/49/59#newcode34
> Line 34: #include <algorithm>
> On 2009/08/23 18:57:35, darin wrote:
>
>> is this include necessary?
>>
>
> Apparently <algorithm> is where std::swap() is defined (used by this
> class).
>


I thought it was defined in <utility>.  That seems to have been sufficient
thus far.  Did you find that something didn't compile without the
<algorithm> include?
-Darin



>
> If there's a different include I should be using to pull this in, let me
> know.
>
>
> http://codereview.chromium.org/173193
>

[Andrew T Wilson (Slow), 2009-08-24 16:25:27]

nativewebworker_impl.cc generated compilation errors when (indirectly)
including WebVector.h, because WebVector.h uses std::swap(). None of the
other code that uses WebVector had any compilation issues, so I'm guessing
that the set of includes for nativewebworker_impl.cc must differ from the
other uses.
It seems that <utility> isn't sufficient to define std::swap() since
WebVector.h already includes it.

Note also that this is on the mac, so the explicit include of <algorithm>
from win/precompiled.h does not apply.

-atw

On Mon, Aug 24, 2009 at 9:13 AM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Sun, Aug 23, 2009 at 8:28 PM, <atwilson@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/173193/diff/49/59
>> File webkit/api/public/WebVector.h (right):
>>
>> http://codereview.chromium.org/173193/diff/49/59#newcode34
>> Line 34: #include <algorithm>
>> On 2009/08/23 18:57:35, darin wrote:
>>
>>> is this include necessary?
>>>
>>
>> Apparently <algorithm> is where std::swap() is defined (used by this
>> class).
>>
>
>
> I thought it was defined in <utility>.  That seems to have been sufficient
> thus far.  Did you find that something didn't compile without the
> <algorithm> include?
> -Darin
>
>
>
>>
>> If there's a different include I should be using to pull this in, let me
>> know.
>>
>>
>> http://codereview.chromium.org/173193
>>
>
>

[Andrew T Wilson (Slow), 2009-08-24 16:31:50]

FWIW, poking around in xcode it seems that stl_algo.h and stl_algobase.h are
where std::swap() is defined, so I believe that <algorithm> is the correct
include.
-atw

On Mon, Aug 24, 2009 at 9:25 AM, Drew Wilson <atwilson@chromium.org> wrote:

> nativewebworker_impl.cc generated compilation errors when (indirectly)
> including WebVector.h, because WebVector.h uses std::swap(). None of the
> other code that uses WebVector had any compilation issues, so I'm guessing
> that the set of includes for nativewebworker_impl.cc must differ from the
> other uses.
> It seems that <utility> isn't sufficient to define std::swap() since
> WebVector.h already includes it.
>
> Note also that this is on the mac, so the explicit include of <algorithm>
> from win/precompiled.h does not apply.
>
> -atw
>
>
> On Mon, Aug 24, 2009 at 9:13 AM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Sun, Aug 23, 2009 at 8:28 PM, <atwilson@chromium.org> wrote:
>>
>>>
>>> http://codereview.chromium.org/173193/diff/49/59
>>> File webkit/api/public/WebVector.h (right):
>>>
>>> http://codereview.chromium.org/173193/diff/49/59#newcode34
>>> Line 34: #include <algorithm>
>>> On 2009/08/23 18:57:35, darin wrote:
>>>
>>>> is this include necessary?
>>>>
>>>
>>> Apparently <algorithm> is where std::swap() is defined (used by this
>>> class).
>>>
>>
>>
>> I thought it was defined in <utility>.  That seems to have been sufficient
>> thus far.  Did you find that something didn't compile without the
>> <algorithm> include?
>> -Darin
>>
>>
>>
>>>
>>> If there's a different include I should be using to pull this in, let me
>>> know.
>>>
>>>
>>> http://codereview.chromium.org/173193
>>>
>>
>>
>

[darin (slow to review), 2009-08-24 16:34:47]

On Mon, Aug 24, 2009 at 9:25 AM, Drew Wilson <atwilson@chromium.org> wrote:

> nativewebworker_impl.cc generated compilation errors when (indirectly)
> including WebVector.h, because WebVector.h uses std::swap(). None of the
> other code that uses WebVector had any compilation issues, so I'm guessing
> that the set of includes for nativewebworker_impl.cc must differ from the
> other uses.
> It seems that <utility> isn't sufficient to define std::swap() since
> WebVector.h already includes it.
>
> Note also that this is on the mac, so the explicit include of <algorithm>
> from win/precompiled.h does not apply.
>

We don't include precompiled headers in release builds on windows.

std::swap is definitely defined in utility on MSVC.  but maybe that is not
required by the standard.  indeed, including algorithm instead of utility
works just fine on MSVC too since algorithm ends up including utility.

-darin



>
> -atw
>
>
> On Mon, Aug 24, 2009 at 9:13 AM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Sun, Aug 23, 2009 at 8:28 PM, <atwilson@chromium.org> wrote:
>>
>>>
>>> http://codereview.chromium.org/173193/diff/49/59
>>> File webkit/api/public/WebVector.h (right):
>>>
>>> http://codereview.chromium.org/173193/diff/49/59#newcode34
>>> Line 34: #include <algorithm>
>>> On 2009/08/23 18:57:35, darin wrote:
>>>
>>>> is this include necessary?
>>>>
>>>
>>> Apparently <algorithm> is where std::swap() is defined (used by this
>>> class).
>>>
>>
>>
>> I thought it was defined in <utility>.  That seems to have been sufficient
>> thus far.  Did you find that something didn't compile without the
>> <algorithm> include?
>> -Darin
>>
>>
>>
>>>
>>> If there's a different include I should be using to pull this in, let me
>>> know.
>>>
>>>
>>> http://codereview.chromium.org/173193
>>>
>>
>>
>

[darin (slow to review), 2009-08-24 17:03:28]

Right.  We should delete the #include <utility> line and replace it with
#include <algorithm> since that appears to be the portable way to access
std::swap :-)
-Darin

On Mon, Aug 24, 2009 at 10:01 AM, Drew Wilson <atwilson@chromium.org> wrote:

> OK, so it sounds like <algorithm> is acceptable here then, given that
> <utility> doesn't pull it in on the Mac. It must usually be getting pulled
> in by some other common include file or else we'd see this error in other
> places.
> -atw
>
>
> On Mon, Aug 24, 2009 at 9:34 AM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Mon, Aug 24, 2009 at 9:25 AM, Drew Wilson <atwilson@chromium.org>wrote:
>>
>>> nativewebworker_impl.cc generated compilation errors when (indirectly)
>>> including WebVector.h, because WebVector.h uses std::swap(). None of the
>>> other code that uses WebVector had any compilation issues, so I'm guessing
>>> that the set of includes for nativewebworker_impl.cc must differ from the
>>> other uses.
>>> It seems that <utility> isn't sufficient to define std::swap() since
>>> WebVector.h already includes it.
>>>
>>> Note also that this is on the mac, so the explicit include of <algorithm>
>>> from win/precompiled.h does not apply.
>>>
>>
>> We don't include precompiled headers in release builds on windows.
>>
>> std::swap is definitely defined in utility on MSVC.  but maybe that is not
>> required by the standard.  indeed, including algorithm instead of utility
>> works just fine on MSVC too since algorithm ends up including utility.
>>
>> -darin
>>
>>
>>
>>>
>>> -atw
>>>
>>>
>>> On Mon, Aug 24, 2009 at 9:13 AM, Darin Fisher <darin@chromium.org>wrote:
>>>
>>>>
>>>>
>>>> On Sun, Aug 23, 2009 at 8:28 PM, <atwilson@chromium.org> wrote:
>>>>
>>>>>
>>>>> http://codereview.chromium.org/173193/diff/49/59
>>>>> File webkit/api/public/WebVector.h (right):
>>>>>
>>>>> http://codereview.chromium.org/173193/diff/49/59#newcode34
>>>>> Line 34: #include <algorithm>
>>>>> On 2009/08/23 18:57:35, darin wrote:
>>>>>
>>>>>> is this include necessary?
>>>>>>
>>>>>
>>>>> Apparently <algorithm> is where std::swap() is defined (used by this
>>>>> class).
>>>>>
>>>>
>>>>
>>>> I thought it was defined in <utility>.  That seems to have been
>>>> sufficient thus far.  Did you find that something didn't compile without
the
>>>> <algorithm> include?
>>>> -Darin
>>>>
>>>>
>>>>
>>>>>
>>>>> If there's a different include I should be using to pull this in, let
>>>>> me
>>>>> know.
>>>>>
>>>>>
>>>>> http://codereview.chromium.org/173193
>>>>>
>>>>
>>>>
>>>
>>
>

[Andrew T Wilson (Slow), 2009-08-24 17:24:20]

OK, so it sounds like <algorithm> is acceptable here then, given that
<utility> doesn't pull it in on the Mac. It must usually be getting pulled
in by some other common include file or else we'd see this error in other
places.
-atw

On Mon, Aug 24, 2009 at 9:34 AM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Mon, Aug 24, 2009 at 9:25 AM, Drew Wilson <atwilson@chromium.org>wrote:
>
>> nativewebworker_impl.cc generated compilation errors when (indirectly)
>> including WebVector.h, because WebVector.h uses std::swap(). None of the
>> other code that uses WebVector had any compilation issues, so I'm guessing
>> that the set of includes for nativewebworker_impl.cc must differ from the
>> other uses.
>> It seems that <utility> isn't sufficient to define std::swap() since
>> WebVector.h already includes it.
>>
>> Note also that this is on the mac, so the explicit include of <algorithm>
>> from win/precompiled.h does not apply.
>>
>
> We don't include precompiled headers in release builds on windows.
>
> std::swap is definitely defined in utility on MSVC.  but maybe that is not
> required by the standard.  indeed, including algorithm instead of utility
> works just fine on MSVC too since algorithm ends up including utility.
>
> -darin
>
>
>
>>
>> -atw
>>
>>
>> On Mon, Aug 24, 2009 at 9:13 AM, Darin Fisher <darin@chromium.org> wrote:
>>
>>>
>>>
>>> On Sun, Aug 23, 2009 at 8:28 PM, <atwilson@chromium.org> wrote:
>>>
>>>>
>>>> http://codereview.chromium.org/173193/diff/49/59
>>>> File webkit/api/public/WebVector.h (right):
>>>>
>>>> http://codereview.chromium.org/173193/diff/49/59#newcode34
>>>> Line 34: #include <algorithm>
>>>> On 2009/08/23 18:57:35, darin wrote:
>>>>
>>>>> is this include necessary?
>>>>>
>>>>
>>>> Apparently <algorithm> is where std::swap() is defined (used by this
>>>> class).
>>>>
>>>
>>>
>>> I thought it was defined in <utility>.  That seems to have been
>>> sufficient thus far.  Did you find that something didn't compile without the
>>> <algorithm> include?
>>> -Darin
>>>
>>>
>>>
>>>>
>>>> If there's a different include I should be using to pull this in, let me
>>>> know.
>>>>
>>>>
>>>> http://codereview.chromium.org/173193
>>>>
>>>
>>>
>>
>

[jam, 2009-08-24 19:44:08]

http://codereview.chromium.org/173193/diff/49/61
File chrome/browser/worker_host/message_port_dispatcher.cc (right):

http://codereview.chromium.org/173193/diff/49/61#newcode150
Line 150: for (unsigned int i = 0; i < sent_message_port_ids.size(); ++i) {
nit: chrome style is to use size_t (also in other places)

http://codereview.chromium.org/173193/diff/49/62
File chrome/browser/worker_host/message_port_dispatcher.h (right):

http://codereview.chromium.org/173193/diff/49/62#newcode51
Line 51: std::vector<int> sent_message_port_ids);
nit: this, and other functions that take in a std::vector, should be a const
reference
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...)

http://codereview.chromium.org/173193/diff/49/65
File chrome/common/webmessageportchannel_impl.cc (right):

http://codereview.chromium.org/173193/diff/49/65#newcode39
Line 39: std::vector<WebMessagePortChannelImpl*>& channelArray =
nit: chrome style is channel_array (also in other places below and resultPorts)

http://codereview.chromium.org/173193/diff/49/65#newcode88
Line 88: if (channels && channels->size()) {
nit: seems like the second condition isn't needed

http://codereview.chromium.org/173193/diff/49/65#newcode91
Line 91: static_cast<WebMessagePortChannelImpl*>((*channels)[i]);
nit: 4 spaces

http://codereview.chromium.org/173193/diff/49/65#newcode191
Line 191: if (sent_message_port_ids.size()) {
nit: chrome style (which I never really understood :) ) is to do

if (!sent_message_port_ids.empty())
rather than
if (sent_message_port_ids.size())

http://codereview.chromium.org/173193/diff/49/66
File chrome/common/webmessageportchannel_impl.h (right):

http://codereview.chromium.org/173193/diff/49/66#newcode9
Line 9: #include <utility>
Why is this needed?  If it's because of a missing include in WebVector, it
should be added there.

http://codereview.chromium.org/173193/diff/49/66#newcode54
Line 54: std::vector<int> sent_message_port_ids,
nit: this should be a const ref

http://codereview.chromium.org/173193/diff/49/68
File chrome/renderer/webworker_proxy.h (right):

http://codereview.chromium.org/173193/diff/49/68#newcode39
Line 39: const WebKit::WebMessagePortChannelArray& channelArray);
nit: channel_array

http://codereview.chromium.org/173193/diff/49/52
File webkit/api/src/PlatformMessagePortChannel.cpp (right):

http://codereview.chromium.org/173193/diff/49/52#newcode212
Line 212: delete webChannels;
can it ever happen that the passed in vector is empty?  if so it won't get
deleted.  a smart pointer here would be good.

http://codereview.chromium.org/173193/diff/49/56
File webkit/glue/webworkerclient_impl.cc (right):

http://codereview.chromium.org/173193/diff/49/56#newcode324
Line 324: WebMessagePortChannelArray webChannels(channels.get() ?
channels->size() : 0);
nit: web_channels

http://codereview.chromium.org/173193/diff/49/56#newcode351
Line 351: WebCore::MessagePort::entanglePorts(*context, channels.release());
I don't see this function in MessagePort.cpp (or anywhere) in my tree and webkit
trunk, where is it defined?

[Andrew T Wilson (Slow), 2009-08-25 02:38:21]

OK, addressed all comments so far.

http://codereview.chromium.org/173193/diff/49/66
File chrome/common/webmessageportchannel_impl.h (right):

http://codereview.chromium.org/173193/diff/49/66#newcode9
Line 9: #include <utility>
On 2009/08/24 19:44:08, John Abd-El-Malek wrote:
> Why is this needed?  If it's because of a missing include in WebVector, it
> should be added there.
Good question - this shouldn't be needed. I'll take it out.

http://codereview.chromium.org/173193/diff/49/66#newcode54
Line 54: std::vector<int> sent_message_port_ids,
On 2009/08/24 19:44:08, John Abd-El-Malek wrote:
> nit: this should be a const ref

Good catch. I'll update all the other occurrences.

http://codereview.chromium.org/173193/diff/49/56
File webkit/glue/webworkerclient_impl.cc (right):

http://codereview.chromium.org/173193/diff/49/56#newcode351
Line 351: WebCore::MessagePort::entanglePorts(*context, channels.release());
On 2009/08/24 19:44:08, John Abd-El-Malek wrote:
> I don't see this function in MessagePort.cpp (or anywhere) in my tree and
webkit
> trunk, where is it defined?

This patch is the chrome side of a double-sided patch. The webkit side lives
here:

https://bugs.webkit.org/show_bug.cgi?id=26902

[darin (slow to review), 2009-08-25 15:54:56]

Just some feedback on the WebKit API changes:

http://codereview.chromium.org/173193/diff/93/1087
File webkit/api/public/WebWorker.h (right):

http://codereview.chromium.org/173193/diff/93/1087#newcode34
Line 34: #include "WebCommon.h"
nit: you can delete the WebCommon.h include now

http://codereview.chromium.org/173193/diff/93/1080
File webkit/api/public/WebWorkerClient.h (right):

http://codereview.chromium.org/173193/diff/93/1080#newcode34
Line 34: #include "WebCommon.h"
nit: you can delete the WebCommon.h include now

http://codereview.chromium.org/173193/diff/93/1080#newcode48
Line 48: const WebKit::WebMessagePortChannelArray&) = 0;
nit: no need for the WebKit:: prefix here

http://codereview.chromium.org/173193/diff/93/1079
File webkit/api/src/PlatformMessagePortChannel.cpp (right):

http://codereview.chromium.org/173193/diff/93/1079#newcode182
Line 182: OwnPtr<WebCore::MessagePortChannelArray> channels =
message->channels();
given that "channels" releases ownership of the contained
MessagePortChannelArray, I think it should have a better name, like
releaseChannels.

http://codereview.chromium.org/173193/diff/93/1079#newcode185
Line 185: webChannels = new WebMessagePortChannelArray(channels->size());
why does webChannels need to be heap allocated?  perhaps you could just allocate
it on the stack, and then if m_webChannel::postMessage wants to take ownership
of the WebVector, it can just use the .swap function.

http://codereview.chromium.org/173193/diff/93/1079#newcode186
Line 186: for (unsigned int i = 0; i < channels->size(); ++i) {
nit: please use size_t instead of unsigned int here to match the return type of
size()

http://codereview.chromium.org/173193/diff/93/1079#newcode201
Line 201: WebMessagePortChannelArray* webChannelPtr;
can this be passed by reference instead?  the tryGetMessage function can use
.swap to populate the WebVector.

http://codereview.chromium.org/173193/diff/93/1079#newcode209
Line 209: for (unsigned int i = 0; i < webChannels->size(); ++i) {
nit: size_t

[Andrew T Wilson (Slow), 2009-08-25 17:43:04]

http://codereview.chromium.org/173193/diff/93/1079
File webkit/api/src/PlatformMessagePortChannel.cpp (right):

http://codereview.chromium.org/173193/diff/93/1079#newcode182
Line 182: OwnPtr<WebCore::MessagePortChannelArray> channels =
message->channels();
On 2009/08/25 15:54:56, darin wrote:
> given that "channels" releases ownership of the contained
> MessagePortChannelArray, I think it should have a better name, like
> releaseChannels.

Agreed. This is defined in an upstream file, but I'll note that for a future
patch.

http://codereview.chromium.org/173193/diff/93/1079#newcode185
Line 185: webChannels = new WebMessagePortChannelArray(channels->size());
On 2009/08/25 15:54:56, darin wrote:
> why does webChannels need to be heap allocated?  perhaps you could just
allocate
> it on the stack, and then if m_webChannel::postMessage wants to take ownership
> of the WebVector, it can just use the .swap function.

It needs to be on the heap because it may need to get marshalled to another
thread by WebMessagePortChannelImpl::postMessage().

It seemed easier/simpler to just allocate it on the heap rather than introduce
extra code/complexity on the WebMessagePortChannelImpl side, especially since
sending message ports is a very rare operation (tradeoff of simpler code vs
slightly more efficient heap allocation, and I chose the former since it's a
rare operation).

Let me know if you feel strongly about this and I'll make the change.

http://codereview.chromium.org/173193/diff/93/1079#newcode186
Line 186: for (unsigned int i = 0; i < channels->size(); ++i) {
On 2009/08/25 15:54:56, darin wrote:
> nit: please use size_t instead of unsigned int here to match the return type
of
> size()
OK, I thought that unsigned int was the preferred WebKit style, but I see now
that both are used liberally.

http://codereview.chromium.org/173193/diff/93/1079#newcode201
Line 201: WebMessagePortChannelArray* webChannelPtr;
On 2009/08/25 15:54:56, darin wrote:
> can this be passed by reference instead?  the tryGetMessage function can use
> .swap to populate the WebVector.
Yes. In my conversations with John I got the impression that out params should
always be passed as a pointer in chrome code, not reference, but I guess this
file should be WebKit style, not chrome style.

Agreed, it is much cleaner passing by reference.

[jam, 2009-08-25 18:00:49]

lgtm