Also, it occurs to me that I can avoid making the problem worse by not
putting the MessagePort entanglement in the proxy layer, which my patch
currently does (ick). Proving that proper API design does enable better code
re-use :)
-atw "taking my own advice"
2009/6/24 John Abd-El-Malek <j...@chromium.org>
>
>
> 2009/6/24 Drew Wilson <atwil...@google.com>
>
> I'm looking at the code in webworker_impl.cc and webworkerclient_impl.cpp,
>> vs the code in WebCore/workers/WorkerMessagingProxy.cpp.
>> As an example:
>>
>> Chrome version:
>> void WebWorkerClientImpl::postMessageToWorkerContext(
>> const WebCore::String& message) {
>> // Worker.terminate() could be called from JS before the context is
>> started.
>> if (asked_to_terminate_)
>> return;
>>
>> ++unconfirmed_message_count_;
>>
>> if (!WTF::isMainThread()) {
>> WebWorkerImpl::DispatchTaskToMainThread(
>> WebCore::createCallbackTask(&PostMessageToWorkerContextTask, this,
>> message));
>> return;
>> }
>> webworker_->postMessageToWorkerContext(
>> webkit_glue::StringToWebString(message));
>> }
>>
>> WebKit version:
>> void WorkerMessagingProxy::postMessageToWorkerContext(const String&
>> message)
>> {
>> if (m_askedToTerminate)
>> return;
>>
>> if (m_workerThread) {
>> ++m_unconfirmedMessageCount;
>>
>>
>> m_workerThread->runLoop().postTask(MessageWorkerContextTask::create(message));
>> } else
>>
>> m_queuedEarlyTasks.append(MessageWorkerContextTask::create(message));
>> }
>>
>>
>> The duplications become even more apparent as I evolve these APIs to
>> support MessagePorts. I'm just seeing similar logic (checking for terminated
>> tasks, tracking undelivered messages, and soon disentangling message ports
>> and dealing with errors) that has to be updated in parallel in both trees.
>>
>> Not a big deal, either way. Now that the Chrome implementation is pretty
>> stable, we might want to refactor the WorkerXXXXProxy APIs to see if we can
>> share more code across implementations.
>>
>
> It'd be great of course if unconfirmed_message_count_
> and asked_to_terminate_ weren't needed, but it's used to avoid a large
> number of cross process calls (the equivalent variables in WebKit would be
> in a different process). Almost all the rest of the file is converting back
> and forth between WebCore and Chrome data types in the glue layer, which is
> a necessary pain.
>
>
>> -atw
>>
>> 2009/6/24 John Abd-El-Malek <j...@chromium.org>
>>
>>
>>>
>>> 2009/6/24 Drew Wilson <atwil...@chromium.org>
>>>
>>>> BTW, I checked in with IanH - it sounds like WebSockets are also on the
>>>> Worker roadmap, so that's something to keep in mind while you iterate on
>>>> your design.
>>>> +1 to avoiding WebCore/loader, but also +1 to refactoring to enable as
>>>> much common code as possible cross-platform - I'm looking at the Chromium
>>>> worker code now, and there's a chunk of duplicated logic from the WebKit
>>>> worker implementation, which is a bit of a maintenance headache.
>>>>
>>>
>>> I'm curious, which parts of the code are you talking about?
>>>
>>>
>>>>
>>>> -atw
>>>>
>>>> 2009/6/24 Michael Nordman <micha...@google.com>
>>>>
>>>> Only skimmed thusfar as well... but from what i've seen, looks
>>>>> reasonable to me.
>>>>> * A version of the diagram you have in the chrome doc would be nice in
>>>>> the webkit doc too.
>>>>>
>>>>> * Does WebSocketHandle really need to be refcounted. I know
>>>>> ResourceHandle is a refcounted object and this design looks modeled off of
>>>>> that (which may be why you've spec'd it this way). Unless your design
>>>>> actually needs refcounting on this class, you may be able to simplify
>>>>> things
>>>>> without it. From the looks of it, WebSocketChannel 'owns' the
>>>>> WebSocketHandle.
>>>>>
>>>>> > should we reuse WebCore/loader instead of adding new component?
>>>>>
>>>>> The loader is somewhat notorious for its complexity. I think you've
>>>>> made a good decision to keep this out of there and to design the websocket
>>>>> system in a good clean modular fashion.
>>>>>
>>>>> > which component is responsible of web socket protocol framing? This
>>>>> design assumes WebSocketChannel serializes/deserializes message in web
>>>>> socket frame.
>>>>>
>>>>> Since WebSocketHandle is inevitably going to be platform specific, any
>>>>> code you want to be shared code shouldn't be slated for that class. To the
>>>>> extent the 'web socket protocol framing' can be done indepedent of the
>>>>> 'platform' socket handle (which it looks like it can be), it would be a
>>>>> good
>>>>> thing to put it in WebSocketChannel so its shared core common code
>>>>> goodness.
>>>>>
>>>>> > Regarding the "WebKit API", note that no WebCore data types can be
>>>>> used there. So you'll need to create wrapper classes.
>>>>>
>>>>> I see you have speced WebKit:: wrapper classes with the same name as
>>>>> the corresponding WebCore:: classes.
>>>>>
>>>>> I wonder if that same naming could be confusingt? The naming convention
>>>>> darin has been employing would be WebKit::WebWebSocketHandle, which
>>>>> certainly looks odd.
>>>>>
>>>>> * virtual void didReceiveData(const String& msg) {}
>>>>>
>>>>> Maybe rename this to channel client api to didReceiveMessage() to help
>>>>> distinguish between raw 'data' being surface by the 'handle', and complete
>>>>> 'messages' being surfaced by the 'channel'.
>>>>>
>>>>>
>>>>> On Wed, Jun 24, 2009 at 2:32 AM, Fumitoshi Ukai (鵜飼文敏) <
>>>>> u...@chromium.org> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> yuzo, tyoshino and I start working to implement HTML5 Web Socket and
>>>>>> write design docs
>>>>>>
>>>>>> WebKit part: http://docs.google.com/View?id=dfm7gfvg_0fpjg22gh
>>>>>> Chromium part: http://docs.google.com/View?id=dfm7gfvg_1dm97qxgm
>>>>>>
>>>>>> We'll send WebKit part to webkit-dev, if it looks ok.
>>>>>> We'd welcome if you could give us feedback on our design docs.
>>>>>>
>>>>>> Thanks,
>>>>>> ukai
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>> >>>>
>>>>
>>>
>>
>
--~--~---------~--~----~------------~-------~--~----~
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~----------~----~----~----~------~----~------~--~---
