Bug 26905 - Don't use PlatformMessagePortChannel in any files
Summary: Don't use PlatformMessagePortChannel in any files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-01 14:26 PDT by John Abd-El-Malek
Modified: 2009-07-01 23:45 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (4.60 KB, patch)
2009-07-01 15:14 PDT, John Abd-El-Malek
fishd: review-
Details | Formatted Diff | Diff
updated changelog (4.58 KB, patch)
2009-07-01 17:56 PDT, John Abd-El-Malek
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Abd-El-Malek 2009-07-01 14:26:35 PDT
This will allow Chromium port's version to be implemented outside the WebKit repository.  The reason to do this is so that our version can derive from classes that are in the Chromium repository.
Comment 1 John Abd-El-Malek 2009-07-01 15:14:27 PDT
Created attachment 32146 [details]
Proposed patch
Comment 2 Eric Seidel (no email) 2009-07-01 16:30:52 PDT
Seems kind strange.  That will make it so that it's never possible to build a version of WebKit from the WebKit repository.  That seems directly in contrast with historical goals...
Comment 3 Darin Fisher (:fishd, Google) 2009-07-01 16:53:16 PDT
Comment on attachment 32146 [details]
Proposed patch

> Index: WebCore/ChangeLog
...
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Small refactoring of MessagePortChannel so that a port's PlatformMessagePortChannel doesn't
> +        have to reside in the WebKit repository.

I think it would be more accurate to say "so that PlatformMessagePortChannel may
be defined at the WebKit layer"

Otherwise, R=me
Comment 4 Sam Weinig 2009-07-01 17:15:31 PDT
Wouldn't that be a layering violation?  Or are you considering adding a client interface for that?
Comment 5 John Abd-El-Malek 2009-07-01 17:56:15 PDT
Created attachment 32166 [details]
updated changelog
Comment 6 John Abd-El-Malek 2009-07-01 17:59:21 PDT
Eric: I'm not sure I understand your concern.  If you're referring to someone just going to webkit.org and fetching/building, that'll work as before.  I just moved some functions around.

What I did is equivalent to how ResourceHandleInternal is declared in ResourceHandle.h but it's up to the port to define the class.

Sam: not sure what you mean?
Comment 7 Sam Weinig 2009-07-01 20:52:00 PDT
> Sam: not sure what you mean?

WebCore classes should not depend on WebKit with the exception of client interfaces. If that does happen, that is considered a layering violation.
Comment 8 Darin Fisher (:fishd, Google) 2009-07-01 21:43:13 PDT
> WebCore classes should not depend on WebKit with the exception of client
> interfaces. If that does happen, that is considered a layering violation.

The Chromium port works differently to help avoid an extraneous number of interfaces (vtables) between WebKit and WebCore.  Instead, what we do is specify a set of 'undefined' functions (see ChromiumBridge) and sometimes 'undefined' classes (ResourceHandle is a good example) that are left to the WebKit layer to define.  In the WebKit layer, those undefined WebCore symbols and classes are implemented in terms of WebKit API interfaces (vtables).  Note: This WebKit API I speak of is the Chromium WebKit API, which is still brewing in the Chromium repository (soon to be upstreamed to svn.webkit.org; see my post to webkit-dev if you are interested).  Since only the Chromium port has need to route such things up through WebKit, this helps keep WebCore simple, while keeping the set of dependencies reasonably well scoped.

If you think about it, this is actually very similar to the WebKitSystemInterface approach that PLATFORM(MAC) uses.  That encapsulates the set of additional dependencies required by the PLATFORM(MAC) port of WebCore.  In Chromium's case, we preferred to bridge up to the embedder instead of having the embedder also provide support below WebCore.
Comment 9 Darin Fisher (:fishd, Google) 2009-07-01 23:27:32 PDT
Landed as:  http://trac.webkit.org/changeset/45467
Comment 10 Sam Weinig 2009-07-01 23:45:07 PDT
(In reply to comment #8)
> > WebCore classes should not depend on WebKit with the exception of client
> > interfaces. If that does happen, that is considered a layering violation.
> 
> The Chromium port works differently to help avoid an extraneous number of
> interfaces (vtables) between WebKit and WebCore.  Instead, what we do is
> specify a set of 'undefined' functions (see ChromiumBridge) and sometimes
> 'undefined' classes (ResourceHandle is a good example) that are left to the
> WebKit layer to define.  In the WebKit layer, those undefined WebCore symbols
> and classes are implemented in terms of WebKit API interfaces (vtables).  Note:
> This WebKit API I speak of is the Chromium WebKit API, which is still brewing
> in the Chromium repository (soon to be upstreamed to svn.webkit.org; see my
> post to webkit-dev if you are interested).  Since only the Chromium port has
> need to route such things up through WebKit, this helps keep WebCore simple,
> while keeping the set of dependencies reasonably well scoped.
> 
> If you think about it, this is actually very similar to the
> WebKitSystemInterface approach that PLATFORM(MAC) uses.  That encapsulates the
> set of additional dependencies required by the PLATFORM(MAC) port of WebCore. 
> In Chromium's case, we preferred to bridge up to the embedder instead of having
> the embedder also provide support below WebCore.
> 

While this bug is probably the wrong venue to have this discussion, I do want to state that I don't think ports making different architectural decisions is good idea for the health of the project.  It is something I seriously object to.  In previous discussions with members of the Chromium team it has been indicated that the bridge was a temporary measure and the existing forms of abstraction would be adopted and extended where necessary (glue on the bottom in WebCore/platform/ and glue on top in the form of client interfaces).