[chromium] Compositor thread infrastructure
Created attachment 85364 [details] Patch
Early version of the compositor thread infrastructure. I'm open to any and all feedback, but would be especially interested in feedback on the ViewManager and CCThread system.
Attachment 85364 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8055022
Comment on attachment 85364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85364&action=review Cool start. Left comments and cleared the review bit so it doesn't show up in the review queue. At a design level I think it's a bit weird that the message objects themselves implement run(). The chrome model is that messages are essentially enums with parameters and the receiving code has to switch on the message type in order to figure out what to run. Tasks in chrome-land are data bags and pointers to functions that execute the task itself. I think that the message model would be a better model to follow here since it more clearly separates what one thread wants to say to the other (for example "begin paint with this timestamp parameter") from how the thread responds to that message. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:70 > +// Callback interface for CCViews > // Class that handles drawing of composited render layers using GL. > -class LayerRendererChromium : public RefCounted<LayerRendererChromium> { > +class LayerRendererChromium : public CCView { not sure what the comment means here. do you need it? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:72 > static PassRefPtr<LayerRendererChromium> create(PassRefPtr<GraphicsContext3D> graphicsContext3D); since you are editing lines close by...this parameter shouldn't be named > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:81 > + // sets viewport parameters > + void setViewport(const IntRect& visibleRect, const IntRect& contentRect, this comment doesn't really add anything - i'd say kill it > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:16 > - * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY > + * THIS SOFTWARE IS PROVIDED BY GOOGLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY > * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED > * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > - * DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS BE LIABLE FOR ANY > + * DISCLAIMED. IN NO EVENT SHALL GOOGLE INC. OR ITS CONTRIBUTORS BE LIABLE FOR ANY don't change this > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY GOOGLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY nope - this should say APPLE INC. use this header exactly: http://trac.webkit.org/browser/trunk/Source/WebCore/LICENSE-APPLE changing only the top line and not anything else > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:61 > + void* unused; > + waitForThreadCompletion(m_threadId, &unused); you can pass NULL as the second parameter this is a little scary actually since the value of 'unused' will be some random garbage from the stack and waitForThreadCompletion may try to write to the memory at that location. > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:78 > +void CCThread::postMessage(CCThreadMessage* msg) > +{ > + m_messageQueue.append(msg); is the idea that this will only be called from the main thread to post a message to the compositor thread? could use some ASSERT()s to that effect. > Source/WebCore/platform/graphics/chromium/cc/CCThread.h:13 > + * THIS SOFTWARE IS PROVIDED BY GOOGLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY nope > Source/WebCore/platform/graphics/chromium/cc/CCThread.h:56 > + // Called just before run... Allows an opportunity to convert > + // MainThread objects, e.g. LayerRenderChromium, to their > + // compositor thread counterpart. > + virtual void beforeRun() { } Not sure I totally understand why this needs to be separate - couldn't all the logic in beforeRun() also happen at the top of the subclass's run() implementation? > Source/WebCore/platform/graphics/chromium/cc/CCThread.h:84 > + void postMessage(CCThreadMessage* msg); > + void postMessage(CCMainThreadMessage* msg); nit: no need to name these params > Source/WebCore/platform/graphics/chromium/cc/CCViewImpl.cpp:48 > + m_frameNumber += 1; ++ ? > Source/WebCore/platform/graphics/chromium/cc/CCViewManager.cpp:204 > + // Right now, this condition variable is build using a simple > + // Mutex object, exploiting the fact that WTF Mutex lock calls in general you can't build a condition variable with a mutex so this comment is pretty confusing. in general i'd rather we just use the textbook synchronization techniques instead of trying to be clever unless we know that something is going to be a performance bottleneck. synchronization is hard and i want to try to spend as few brain cells on it as possible while we bootstrap everything else > Source/WebCore/platform/graphics/chromium/cc/NonThreadSafe.h:33 > +template <class T> > +class NonThreadSafe { this looks nifty, but it really probably belongs in wtf/. can you make a patch to add it there? > Source/WebKit/chromium/public/WebWidget.h:84 > + // Temporary (do not commit) hack until Darin can land his inversion code > + virtual void scheduleComposite() { } should this be guarded by the #if? > Source/WebKit/chromium/src/WebViewImpl.cpp:1060 > +#if USE(THREADED_COMPOSITING) > + return; > +#endif this seems wrong - we still need a way to implement WebViewImpl::paint() even in the threaded case so we can handle thumbnails and whatnot. is this just temporary? > Source/WebKit/chromium/src/WebViewImpl.h:338 > + void doUpdateAndComposite(); we don't normally put "do" in functions like this as it doesn't really convey any information. I think updateAndComposite() would be a good name for this > Source/WebKit/chromium/src/WebViewImpl.h:402 > + void doAnimate(double frameBeginTime); what's the difference between animate() and doAnimate()? just the provided frame time? in that case it might make more sense to have animate() take a double param with a default value of 0 meaning 'use whatever time you want'. the "do" is not super informative.
Thanks James. (In reply to comment #4) > At a design level I think it's a bit weird that the message objects themselves implement run(). The chrome model is that messages are essentially enums with parameters and the receiving code has to switch on the message type in order to figure out what to run. Tasks in chrome-land are data bags and pointers to functions that execute the task itself. I'm confused about the "Chrome model" you're talking about. Chrome's MessageLoop model is totally not as you describe... MessageLoop is built on processing Tasks, which have exactly 1 method: Run(). Posting tasks to specific methods is done via RunnableMethod, which derives from Task and does (considerable) method binding work. I would vastly prefer to have a RunnableMethod, but I'm not seeing equivalent code in WebKit that provides something similar. If you have a suggestion for doing this, I'd love ideas... Maybe you're talking about the IPC model? That is based on more of an enum design, but the thing to note there is that IPC messages are layered on top of the Tasks model. The postMessage are callable from any thread, fwiw. E.g. I think it likely that the compositor will post messages to itself in the near future. > this looks nifty, but it really probably belongs in wtf/. can you make a patch to add it there? <whining>But I dont wanna do that.</whining> ;) > We still need a way to implement WebViewImpl::paint() even in the threaded case so we can handle thumbnails and whatnot. is this just temporary? Temporary, but won't be an easy fix. This, and lost context, are both going to be tricky to implement. > > > Source/WebKit/chromium/src/WebViewImpl.h:402 > > + void doAnimate(double frameBeginTime); > > what's the difference between animate() and doAnimate()? just the provided frame time? in that case it might make more sense to have animate() take a double param with a default value of 0 meaning 'use whatever time you want'. the "do" is not super informative. I, too, have an allergic reaction to these various do-prefixed methods (see also, doComposite, or RenderWidget's doDeferredUpdate, which has been around for a while), but defaulting is just a bandaid on a larger issue: the public API of the WebWidget and the actual implementation vary wildly depending on which of the three render loops is active. Darin's "inversion" patch will help here, but more work will be necessary. I propose we leave the "do" trick as it emphasizes the ugliness, and forage ahead with the inversion & related cleanup as the proper solution here.
Created attachment 87683 [details] RunnableMethods and ViewManagerImpl
Attachment 87683 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8306259
Comment on attachment 87683 [details] RunnableMethods and ViewManagerImpl This architecture of this patch is getting closer to something I like. I'm 85% sure I'll be breaking this up into a few bugs: an thread infrastructure + unit tests, and subsequent implementations of the commit algorithm. So, this is largely a checkpoint, to get feedback on the new approach before I start breaking into the infrastructure commits and everything else. At a high level: 1. The Messaging between threads is built on, which is just a class with a run(). 2. On top of this, we add CCRunnableMethods, which are similar to Chrome's NewRunnableMethod system. These are method pointers with bound arguments, and derive from CCTask. This lets you call a method on a class without as much fuss. 3a. CCViewManager and CCViewManagerImpl manage the collection of active views for the main and impl thread respectively. These classes have callAsyncOImpl/Main functions that allow you to call methods on the other side of the fence. This is used for managing the view->viewImpl correspondences. This is built on the task system and RunnableMethods. 3b. We prevent accidental access of the impl pointer on the main thread using the CCNonThreadSafeMember<T>. 4. CCView and CCViewImpl have a callAsyncOnMain/Impl functions that call a method on the other side of the fence. These two functions become the workhorse for the commit algorithm. 5. The commit algorithm is implemented entirely in CCView and CCViewImpl and should be pretty easy to read. Compared to the previous patch: 5a. We now can trigger a redraw without a commit 5b. drawing is initiated on the impl thread, but then we perfom a blocking call to the MainThread to do the actual draw. That should be it.
Created attachment 87686 [details] Patch
Created attachment 88149 [details] Thread infrastructure only.
Comment on attachment 88149 [details] Thread infrastructure only. This patch is trimmed down to just be the CCThread, infrastructure for talking with the CCThread, and associated tests. task system portion of the
Hmm, re-stating comments in a non-truncated way: This patch is trimmed down to just be the CCThread, infrastructure for talking with the CCThread, and associated tests. Once this gets landed, the next patch will be to introduce the CCView and CCViewImpl, and associated unit tests. The final patch will be to tie the LayerRendererChromium and WebView into the CCView.
Comment on attachment 88149 [details] Thread infrastructure only. View in context: https://bugs.webkit.org/attachment.cgi?id=88149&action=review R- because the waitForThreadCompletion() param scares me, but everything else looks pretty good. The nonthreadsafe types aren't used yet so it's hard to say how generally useful they are. They probably will make more sense in the context of stuff that uses them. > Source/WebCore/platform/graphics/chromium/cc/CCTask.h:76 > +template <typename C, typename M, typename ARG0, typename ARG1> in chrome we use T for the type of object, Method instead of M, and A/B/C/D/... instead of ARG0/ARG1/ARG2/... which I find a bit more readable. > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:41 > +#define COMMIT_SLOWLY 0 seems unnecessary > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:46 > +PassRefPtr<CCThread> CCThread::getInstance() > +{ > + if (!ccThread) > + return adoptRef(new CCThread()); > + return PassRefPtr<CCThread>(ccThread); this should ASSERT() that it's called from the main thread > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:60 > + void* unused; > + waitForThreadCompletion(m_threadId, &unused); just pass 0 as the second param to waitForThreadCompletion(). this might actually attempt to write to some random location pointed to by whatever's on the stack here. > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:85 > + // If the thread is dead, then this task is stale, e.g. posted > + // during or slightly before compositor shutdown. > + if (!ccThread) > + return; This check seems a bit strange - if the compositor thread is going down it seems that we still may want to run any already-enqueued tasks on the main thread. It's a little hard to reason about this in the abstract.
(In reply to comment #13) > The nonthreadsafe types aren't used yet so it's hard to say how generally useful they are. They probably will make more sense in the context of stuff that uses them. Yeah. They get used in the CCViewManager. I tried the proxy suggestion but it doesn't work very well in practice, so I kept NonThreadSafe around. > > > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:60 > > + void* unused; > > + waitForThreadCompletion(m_threadId, &unused); > > just pass 0 as the second param to waitForThreadCompletion(). this might actually attempt to write to some random location pointed to by whatever's on the stack here. The 2nd arg for waitForThreadCompletion is an outparam --- the return value of the thread function is void* and the signature is waitForThreadCompletion(..., void** retval). Looking at the various implementations waitForThreadCompletion, I don't think it is wise to pass NULL to this. I renamed this to exitCode to clarify. > > > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:85 > > + // If the thread is dead, then this task is stale, e.g. posted > > + // during or slightly before compositor shutdown. > > + if (!ccThread) > > + return; > > This check seems a bit strange - if the compositor thread is going down it seems that we still may want to run any already-enqueued tasks on the main thread. It's a little hard to reason about this in the abstract. This is stale code from an older implementation. Deleting it...
Created attachment 88172 [details] Patch
Comment on attachment 88172 [details] Patch This appears to be re-inventing stuff that is already done in WebKit (see all of the post task infrastructure). Also CCNonThreadSafeMember is unused and hopefully won't have a need to be used -- better to split your object into two pieces that have items that can be accessed from multiple threads and shouldn't be. Also what does cc stand for? I see that it is a directory name but that doesn't help. WebKit typically avoids abbreviations so this is unusual.
Can you point me at the "post task" infrastructure? I looked for existing infrastructure before doing this myself, but the precident I saw was for individual sytems to build their own abstraction on top of MessageQueue. For example, I see fileapi/FileThread implements a FileThread::Task abstraction on top of WTF::MessageQueue. Or, similarly ScriptExecutionContext itself defines a task abstraction. The NonThreadSafeMember is used in the context of a larger patch (See the 3rd path) to guard a member on ViewManager. I am happy to pull that out to a later patch if it helps focus discussion here. The CC folder is where we are putting compositor-specific code that concerns itself with rendering and animating a generic layer tree. This is roughly equivalent to core animation, with CC being chrome compositor. James might be able to offer additional comments on this... (In reply to comment #16) > (From update of attachment 88172 [details]) > This appears to be re-inventing stuff that is already done in WebKit (see all of the post task infrastructure). > > Also CCNonThreadSafeMember is unused and hopefully won't have a need to be used -- better to split your object into two pieces that have items that can be accessed from multiple threads and shouldn't be. > > Also what does cc stand for? I see that it is a directory name but that doesn't help. WebKit typically avoids abbreviations so this is unusual.
(In reply to comment #17) > Can you point me at the "post task" infrastructure? I looked for existing infrastructure before doing this myself, but the precident I saw was for individual sytems to build their own abstraction on top of MessageQueue. For example, I see fileapi/FileThread implements a FileThread::Task abstraction on top of WTF::MessageQueue. Or, similarly ScriptExecutionContext itself defines a task abstraction. Good point :) However, I believe they are all written in a similar manner if you look closely, and this one doesn't follow that pattern, so it is missing some important safety aspects: see FileThreadTask.h for example. As well as common naming like performTask. (Instead of following WebKit practices, this one decides to use Chromium conventions.) Why do I care? It is on my agenda to pull out the WebKit stuff into a common place but if you do something totally different here it may be harder to convert this code over. (Also it is better to match the codebase you are in if it has conventions.) > > The NonThreadSafeMember is used in the context of a larger patch (See the 3rd path) to guard a member on ViewManager. I am happy to pull that out to a later patch if it helps focus discussion here. I would remove it since it isn't used here. I would still get the design reviewed by someone who has done these things in WebKit. > The CC folder is where we are putting compositor-specific code that concerns itself with rendering and animating a generic layer tree. This is roughly equivalent to core animation, with CC being chrome compositor. James might be able to offer additional comments on this... compositor would have been a better name for the directory (since it is already under the chromium direction the chromium part is implied), and still seems a better name if you must prefix things. Of course an alternative is a namespace. Note: It would be much better to have someone who has tackled similar issues in WebKit review this rather than folks who haven't. (Just like I wouldn't consider myself good at looking at patches for graphics acceleration related items.) Please take advantage of any of these folks who are willing. dimich, ap, kinuko, jianli, or myself have all had to deal with this are a bit. PS Here's a few impl details: CompositorTask should have a virtual destructor. CompositorThread is RefCounted (which isn't threadsafe) but it appears to be used on multiple threads. (fwiw, I getting near to landing a change which will detect that better.) The singleton pattern seems odd, in that it creates the object on the fly in a ref counted fashion (and may create the thread on the fly as a result of this). I think there is clearer ownership than is implied here by the api.
Thanks, David. Will give some of these approaches a try and see what happens. (In reply to comment #18) > (In reply to comment #17) > > Can you point me at the "post task" infrastructure? I looked for existing infrastructure before doing this myself, but the precident I saw was for individual sytems to build their own abstraction on top of MessageQueue. For example, I see fileapi/FileThread implements a FileThread::Task abstraction on top of WTF::MessageQueue. Or, similarly ScriptExecutionContext itself defines a task abstraction. > > Good point :) However, I believe they are all written in a similar manner if you look closely, and this one doesn't follow that pattern, so it is missing some important safety aspects: see FileThreadTask.h for example. > > As well as common naming like performTask. (Instead of following WebKit practices, this one decides to use Chromium conventions.) > > Why do I care? It is on my agenda to pull out the WebKit stuff into a common place but if you do something totally different here it may be harder to convert this code over. (Also it is better to match the codebase you are in if it has conventions.) > > > > > > > The NonThreadSafeMember is used in the context of a larger patch (See the 3rd path) to guard a member on ViewManager. I am happy to pull that out to a later patch if it helps focus discussion here. > > I would remove it since it isn't used here. I would still get the design reviewed by someone who has done these things in WebKit. > > > > The CC folder is where we are putting compositor-specific code that concerns itself with rendering and animating a generic layer tree. This is roughly equivalent to core animation, with CC being chrome compositor. James might be able to offer additional comments on this... > > compositor would have been a better name for the directory (since it is already under the chromium direction the chromium part is implied), and still seems a better name if you must prefix things. Of course an alternative is a namespace. > > > Note: It would be much better to have someone who has tackled similar issues in WebKit review this rather than folks who haven't. (Just like I wouldn't consider myself good at looking at patches for graphics acceleration related items.) Please take advantage of any of these folks who are willing. > > dimich, ap, kinuko, jianli, or myself have all had to deal with this are a bit. > > > PS Here's a few impl details: > CompositorTask should have a virtual destructor. > CompositorThread is RefCounted (which isn't threadsafe) but it appears to be used on multiple threads. (fwiw, I getting near to landing a change which will detect that better.) The singleton pattern seems odd, in that it creates the object on the fly in a ref counted fashion (and may create the thread on the fly as a result of this). I think there is clearer ownership than is implied here by the api.
Comment on attachment 87686 [details] Patch I'm not sure I understand the significance of creating and destroying the CCViewManager's m_impl on the ccthread. At the time the CCViewManager is destroyed all views it manages must have been removed, right? If so it seems equally valid to create/destroy it on the thread the CCViewManager is created on given the member variables CCViewManagerImpl has. Then m_impl could be a normal OwnPtr<>.
In general, I was trying to keep all impl code on the compositor thread because of TLS-related complications. I was thinking in particular about the gl context, which is thread specific. This seemed particularly hinky when you consider that Impl has various ownptrs, like the TextureManager, that might themselves frees up gl resources with an implicit or explicit assumption about the current context. It seems to me simpler to just make impl code always run on the thread. Even for construction & destruction. (In reply to comment #20) > (From update of attachment 87686 [details]) > I'm not sure I understand the significance of creating and destroying the CCViewManager's m_impl on the ccthread. At the time the CCViewManager is destroyed all views it manages must have been removed, right? If so it seems equally valid to create/destroy it on the thread the CCViewManager is created on given the member variables CCViewManagerImpl has. Then m_impl could be a normal OwnPtr<>.
Created attachment 88583 [details] Patch
OK, here's a new patch that uses the WebKitty approach. Thanks again for steering me to look at these in more detail... it wasn't obvious to me the first time I looked through the code that there would be a FileThreadTask.h that came along with the FileThread. Anyways, one open question in my new patch is the postTask path from CCThread back to the main thread. In the FileThread case, this flow is based on a ScriptExecutionContext. However, the compositor doesn't have access to this sort of thing. I opted to make a CCMainThread. I'm open to suggestions. With regard to cc/compositor/namespace discussion, I do understand where you're coming from. createCompositorThreadTask sounds a lot nicer than createCCThreadTask. It may be that as we get a bit more done, a change might make sense. Namespacing + folders might work. But, that's another changelist... I'm not sure of a cleaner way to deal with the singleton design pattern you're poking at. Whereas it seems there can be multiple FileThreads in a single renderprocess, we want CCThread to truly be a singleton --- if someone (a WebViewImpl in accelerated rendering mode) owns a reference to the CCThread, then the thread should be on and started. If nobody owns a reference to the singleton, it turns off in a blocking fashion. Any thoughts?
Attachment 88583 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8348474
Created attachment 88590 [details] Patch
Created attachment 88594 [details] Patch
Attachment 88590 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8347517
Comment on attachment 88594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88594&action=review Just a few things to address. Thanks for making it fit in better. (I will be starting my effort to clean up threading soon, and having this fit the patterns should make it easier to change more quickly and get rid of boilerplate code -- it may take a while to complete this clean up.) > Source/WebCore/platform/graphics/chromium/cc/CCMainThread.h:32 > +// CC doesn't have access to ScriptExecutionContxt so we have to duplicate this If you want to say this, the best place is the ChangeLog. typo: ScriptExecutionContxt > Source/WebCore/platform/graphics/chromium/cc/CCMainThread.h:33 > +// functionality. These tasks route to the webkit main loop The last sentence is incomplete. > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:56 > +CCThread::~CCThread() I feel like the ownership model for CCThread isn't as clear as it should be. For example, suppose that the ref counts when away completely on the main thread and the only ref count was on the thread, then this destructor would be called on the compositor thread, which is clearly wrong. This implies that really CCThread should be owned on the main thread and I suspect there is one place. Looking at your overall patch, I suspect that CCViewManager is the real owner of CCThread. So I would change getInstance to return a CCThread* and make CCViewManager own it. May there be multiple instances of CCViewManager? (If so, perhaps that is what you are trying to handle, but in that case it still feels like giving out PassRefPtr from getInstance feels like overkill.) > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:65 > + // Slear singleton. typo: Slear > Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:92 > + MutexLocker lock(m_threadCreationMutex); Nice. I was about to flag this issue above. > Source/WebCore/platform/graphics/chromium/cc/CCThread.h:46 > + // These functions are thread safe and can be called from anywhere. This comment appears misplaced. > Source/WebCore/platform/graphics/chromium/cc/CCThread.h:74 > +class CCCompletionEvent { Seems deserving of its own header. > Source/WebKit/chromium/tests/CCThreadTest.cpp:27 > +#include "cc/CCThread.h" This cc/ shouldn't be needed. If it is, then the build should be changed so it isn't. As far as I know, directory structure is never used in includes for WebKit header files. > Source/WebKit/chromium/tests/CCThreadTest.cpp:53 > + WTF::ThreadIdentifier hitThreadID; You shouldn't need WTF:: here.
Created attachment 88675 [details] Patch
(In reply to comment #28) Did most of what was suggested. For some reason, unit tests still want cc/ prefix on their includes. I'll try to poke at that at some later time. Wrt thread lifetime, I see your point as it was written. ViewManager is a singleton.... how does this sound: - CCThread not be reference counted whatsoever - ViewManager has OwnPtr<CCThread> (In reply to comment #29) > Created an attachment (id=88675) [details] > Patch
(In reply to comment #28) > > Source/WebKit/chromium/tests/CCThreadTest.cpp:27 > > +#include "cc/CCThread.h" > > This cc/ shouldn't be needed. If it is, then the build should be changed so it isn't. As far as I know, directory structure is never used in includes for WebKit header files. Not true. We do use directory structure for includes in some places in WebKit and I think this is another case where it makes sense to segregate includes explicitly. WTF and its subdirs (like wtf/text) are included with the path to emphasize that it's a separate library. JavaScriptCore internally includes some parts by explicit path (such as runtime/ and yarr/). The apple WebKit API levels include WebCore files via <WebCore/...> and WebKit2 includes API headers with #include <WebKit2/...>. There is plenty of precedence for using directory structure in includes when there's a reason to maintain a clear logical distinction. A central idea with the chromium compositor code is that it should be treated as an independent library with minimal dependencies from the rest of WebCore. The only files in the project that should interact at all with cc/ files are parts of WebCore/platform/graphics/chromium, unit tests, and a very minimal portion of WebKit/chromium/src. The internals of the library have a different threading model than most of webkit (in that multiple threads are used) and so by keeping the directory in the include my hope is that people will be less likely to peek into cc's guts without understanding what is going on inside. One style question is whether we should be using <> style includes instead of "" for files outside of the cc/ directory. I don't think it makes a large practical difference.
Of course for files inside WebCore/platform/graphics/chromium/cc there's no need for the "cc/..." prefix in #includes since the current directory is always on the include lookup path. My point was about files outside that directory including headers inside of it.
Comment on attachment 88675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88675&action=review Looks good to me. Just a few very minor nits to clean up. > Source/WebCore/ChangeLog:11 > + functionality in CCMainThread. This second sentence would be better at a comment at the CCMainThread.cpp level. > Source/WebCore/ChangeLog:36 > + (WebCore::CCThread::getInstance): This isn't in this patch. (I think you need to regenerate the ChangeLog. > Source/WebCore/platform/graphics/chromium/cc/CCThread.h:37 > +// thread is alive as long as someone holds on to a reference to the This comment is out of date.
(In reply to comment #30) > Wrt thread lifetime, I see your point as it was written. ViewManager is a singleton.... how does this sound: > - CCThread not be reference counted whatsoever > - ViewManager has OwnPtr<CCThread> Exactly! I don't see getInstance anymore. I guess it will come in later.
Comment on attachment 87686 [details] Patch Getting back to the ViewManager/impl relationship for a second (not relevant to the patch currently up for review) here's an idea: Instead of having the ViewManager keep a pointer to its impl, have it request a routing ID from the CCThread and then use that routing ID to post tasks to its impl and vice versa. The startup sequence would look something like: 1.) CCViewManager constructor requests routing ID from CCThread 2.) CCViewManager posts a task to the compositor thread requesting that a CCViewManagerImpl be created with the given routing id 3.) some time later on the compositor thread the CCViewManagerImpl is constructed I don't see any reason why the construction has to be blocking. From this point on if the CCViewManager wants to post a task (for example register a new view) all it only has to specify the routing ID and not an actual pointer to an object. The point of all this is to try to avoid main thread objects and compositor thread objects from having direct pointers to each other whenever possible.
Created attachment 88709 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 88709 [details]: inspector/debugger/debugger-breakpoints-not-activated-on-reload.html bug 55551 (authors: pfeldman@chromium.org and podivilov@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 88709 [details] Patch Clearing flags on attachment: 88709 Committed r83249: <http://trac.webkit.org/changeset/83249>
All reviewed patches have been landed. Closing bug.
pingPongUsingCondition test has never been passed on Windows Debug. Would you investigate on it please?
Looking. (In reply to comment #40) > pingPongUsingCondition test has never been passed on Windows Debug. Would you investigate on it please?
(In reply to comment #35) > I don't see any reason why the construction has to be blocking. I suppose construction doesn't have to be blocking, but then clean destruction is harder to deal with ("are we in the process of creating ourself?"). This smells like premature optimization: wiews don't get created often and the compositor thread latency is low. Sorry, but I really don't see a point in optimizing here... Wrt using IDs instead of pointers, that's a chrome-style way of doing things that is certainly worth considering. The thing I have to do anyway is revisit the previous implementation in light of things like FileStreamProxy, where the correspondence is containerized in a proxy class. That approach may be worth considering, too.