Bug 67417 - [chromium] Make CCThreadProxy draw
Summary: [chromium] Make CCThreadProxy draw
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nat Duca
URL:
Keywords:
Depends on: 66807 67440
Blocks: 67499
  Show dependency treegraph
 
Reported: 2011-09-01 10:17 PDT by Nat Duca
Modified: 2011-09-27 06:42 PDT (History)
7 users (show)

See Also:


Attachments
Patch (18.14 KB, patch)
2011-09-02 00:36 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Real 2 phase commit (21.30 KB, patch)
2011-09-14 19:01 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
New hotness (24.99 KB, patch)
2011-09-15 11:51 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Better impl of finishAllRendering (27.96 KB, patch)
2011-09-15 18:22 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Rebase and nits (29.86 KB, patch)
2011-09-15 19:27 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Merge win the tests. (36.40 KB, patch)
2011-09-16 15:03 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
More tweaks (39.80 KB, patch)
2011-09-22 13:35 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Dont commit in finish. (42.08 KB, patch)
2011-09-23 16:36 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Set Impl thread when setting layer renderer (42.22 KB, patch)
2011-09-23 16:45 PDT, Nat Duca
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-09-01 10:17:29 PDT
The CCThreadProxy is mostly implemented, but needs to be updated to the new Update/Commit flow.
Comment 1 Nat Duca 2011-09-02 00:36:44 PDT
Created attachment 106102 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-02 00:43:06 PDT
Comment on attachment 106102 [details]
Patch

Attachment 106102 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9580560
Comment 3 WebKit Review Bot 2011-09-02 01:53:16 PDT
Comment on attachment 106102 [details]
Patch

Attachment 106102 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9579655
Comment 4 James Robinson 2011-09-02 13:29:40 PDT
Comment on attachment 106102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106102&action=review

Looks like this patch isn't quite complete, it's either missing some files or has too many.

> Source/WebCore/ChangeLog:3
> +        [chromium] Finish implementaiton of CCThreadProxy

this is a bit ambitious, I doubt we're completely done with CCThreadProxy :).  can you say what this patch does?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:309
> +    // TODO: use the CCScheduler ot decide when to manage the conversion of this

TODO->FIXME
ot -> to

> Source/WebKit/chromium/tests/CCSchedulerTest.cpp:28
> +#include "CCScheduler.h"

did you mean to add this file to the patch? i don't see a CCScheduler.h/cpp
Comment 5 Nat Duca 2011-09-14 19:01:26 PDT
Created attachment 107441 [details]
Real 2 phase commit
Comment 6 WebKit Review Bot 2011-09-14 19:40:11 PDT
Comment on attachment 107441 [details]
Real 2 phase commit

Attachment 107441 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9659838
Comment 7 James Robinson 2011-09-14 20:38:40 PDT
Comment on attachment 107441 [details]
Real 2 phase commit

View in context: https://bugs.webkit.org/attachment.cgi?id=107441&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:155
> +    deprecatedTurnOffVerifier();

Do we need to do this?  The comment says there's another way to disable the verifier (extending ThreadRestrictionVerifier?), but do we even need to do that?  What's it complaining about?

We should ASSERT() that we're on the expected thread here (main thread, i believe)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:121
> +// code that is logically a main thread operation, e.g. deletion of a Layer,

"deletion of a Layer" is a bit ambiguous since we have lots of layers - I think you mean LayerChromium here, right?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:122
> +// shoudl be delayed until the CCLayerTreeHost::commitComplete, which will run

shoudl -> should

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:152
> +    m_updateList.clear();

You are in need of an update here, there's a new function CCLayerTreeHost called clearPendingUpdate to use instead.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:82
> +// Allow IntRects across the thread boundary
> +template<> struct CrossThreadCopierBase<false, false, IntRect> : public CrossThreadCopierPassThrough<IntRect> {
> +};

General question: does this belong here, or in IntRect.h or something like that?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:90
> +    // FIXME: implement readback:
> +    // 0. finishAllRendering --- will make sure any commit pending has been done
> +    // 1. send a blocking message to other side to do another drawLayers and a readback

think we still should have an ASSERT_NOT_REACHED() here. A thread ASSERT() wouldn't hurt either

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:102
> +    if (m_commitRequested)  {

can we get a main-thread ASSERT() here?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:104
> +        // FIXME: get the beginFrameAndCommit parameters with a blocking call to the impl thread
> +        // run a beginFrameAndCommit

question: could we send a normal needsCommit message over here, and then rely on the blocking nature of the finishAllRendering completion to have the thread call back to for the bFAC before doing the finish()?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:113
>  bool CCThreadProxy::isStarted() const
>  {
> -    return m_layerTreeHostImpl;
> +    return m_started;

It's just a simple getter but could we get a thread ASSERT() here too? it's very easy to call these innocent-looking getters from the wrong thread and very hard to debug

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:-160
> -    ASSERT(isMainThread());

why delete the ASSERT()?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:179
> +    TRACE_EVENT("CCThreadProxy::setNeedsRedraw", this, 0);
> +    ccThread->postTask(createCCThreadTask(this, &CCThreadProxy::updateSchedulerStateOnCCThread, false, true));

I'm confused - why doesn't this set m_redrawRequested ?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:209
> +void CCThreadProxy::finishAllRenderingOnCCThread(CCCompletionEvent* completion)
> +{

ASSERT(isImplThread()) ?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:242
> +    // Clear the redraw and commit flag after animateAndLayout but before in case a

but before what? i think you accidentally a word

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:251
> -    // Blocking call to CCThreadProxy::performCommit
>      CCCompletionEvent completion;
>      ccThread->postTask(createCCThreadTask(this, &CCThreadProxy::commitOnCCThread, AllowCrossThreadAccess(&completion)));
>      completion.wait();

You deleted the comment saying this call is blocking, but it still looks blocking to me.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:268
> +    m_layerTreeHostImpl->commitComplete();

why does the impl have a commitComplete()?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:309
> +    if (!m_beginFrameAndCommitPendingOnCCThread) {
> +        beginFrameAndCommitOnCCThread();
> +        m_beginFrameAndCommitPendingOnCCThread = true;

nit: seems to make more sense to have beginFrameAndCommitOnCCThread() to set m_beingFrameAndCommitPendingOnCCThread to true, rather than having the caller have to do it

> Source/WebKit/chromium/ChangeLog:3
> +        [chromium] Make yCCThreadProxy draw

yCCThreadProxy -> CCThreadProxy

> Source/WebKit/chromium/ChangeLog:7
> +        Disable CCLayerTreeHostTest completely as it does not
> +        compile when the threaded switch is enabled.

:(. will husky's patch fix this?
Comment 8 James Robinson 2011-09-14 20:39:57 PDT
CCThreadProxy.cpp didn't compile on cr-mac EWS, although the actual failure seems to be omitted.
Comment 9 Nat Duca 2011-09-15 09:49:59 PDT
Thanks James!

(In reply to comment #7)
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:155
> > +    deprecatedTurnOffVerifier();

Working on it. Got some advice from @levin, we'll see what I can do.

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:90
> > +    // 0. finishAllRendering --- will make sure any commit pending has > think we still should have an ASSERT_NOT_REACHED() here. A thread ASSERT() wouldn't hurt either

You can't actually use the browser if you make this a notreached... this way, it at least works. Since this is behind a flag, I think we're safe.

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:104
> > +        // FIXME: get the beginFrameAndCommit parameters with a blocking call to the impl thread
> > +        // run a beginFrameAndCommit
> 
> question: could we send a normal needsCommit message over here, and then rely on the blocking nature of the finishAllRendering completion to have the thread call back to for the bFAC before doing the finish()?

The latest patch shows how I envisioned doing this.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:179
> > +    TRACE_EVENT("CCThreadProxy::setNeedsRedraw", this, 0);
> > +    ccThread->postTask(createCCThreadTask(this, &CCThreadProxy::updateSchedulerStateOnCCThread, false, true));
> 
> I'm confused - why doesn't this set m_redrawRequested ?
The impl thread can draw without telling the main thread. And, I'm reluctant to have the impl thread tell the main thread every time it draws since the only time this function runs is on on WM_PAINT/expose/etc. Since its low-frequency, its safe to just send it over to the other side where it can be ignored or handled as needed. I added comments to this effect on the implementation.


> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:268
> > +    m_layerTreeHostImpl->commitComplete();
> 
> why does the impl have a commitComplete()?
Tests. I'm vaguely amused cause we have this conversation about this function every time you see this code. :)

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:309
> > +    if (!m_beginFrameAndCommitPendingOnCCThread) {
> > +        beginFrameAndCommitOnCCThread();
> > +        m_beginFrameAndCommitPendingOnCCThread = true;
> 
> nit: seems to make more sense to have beginFrameAndCommitOnCCThread() to set m_beingFrameAndCommitPendingOnCCThread to true, rather than having the caller have to do it

The upcoming patch should make it clear why I didn't do it that way.

> > Source/WebKit/chromium/ChangeLog:7
> > +        Disable CCLayerTreeHostTest completely as it does not
> > +        compile when the threaded switch is enabled.
> 
> :(. will husky's patch fix this?

Yes!
Comment 10 Nat Duca 2011-09-15 11:51:26 PDT
Created attachment 107523 [details]
New hotness
Comment 11 Nat Duca 2011-09-15 18:22:52 PDT
Created attachment 107578 [details]
Better impl of finishAllRendering
Comment 12 James Robinson 2011-09-15 19:13:13 PDT
Comment on attachment 107578 [details]
Better impl of finishAllRendering

View in context: https://bugs.webkit.org/attachment.cgi?id=107578&action=review

left some nits, I will go home and try to grok the rest of this tonight on a full tummy.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:87
> +    ASSERT(m_layerTreeHost);

broken record time: can you put a threading ASSERT() here too?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:90
> +    bool success;

nit: could you initialize this bool here rather than just leave it some random value, just in case for some reason the task forgets to set it?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:228
> +
> +    m_started = true;

can this get a main thread ASSERT()?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:236
> +    if (!m_started)
> +        return;

is it valid to call stop() without calling start()? (or before start() completes, at least)?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:359
>      ASSERT(isImplThread());

nit: mind putting the ASSERT() ahead of the early out? if we somehow get this wrong i want the ASSERT() to hit regardless of the value of that flag
Comment 13 Nat Duca 2011-09-15 19:27:54 PDT
Created attachment 107586 [details]
Rebase and nits
Comment 14 Nat Duca 2011-09-16 15:03:15 PDT
Created attachment 107727 [details]
Merge win the tests.
Comment 15 James Robinson 2011-09-16 15:46:05 PDT
Comment on attachment 107586 [details]
Rebase and nits

View in context: https://bugs.webkit.org/attachment.cgi?id=107586&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:279
> +    // beginFrameAndCommitOnCCThread is enqueued. Since it CCMainThread doesn't

"Since it CCMainThread" does not parse

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:404
> +    m_redrawRequestedOnCCThread |= redrawRequested;
> +    if (!m_beginFrameAndCommitPendingOnCCThread)
> +        CCMainThread::postTask(createBeginFrameAndCommitTaskOnCCThread());
> +
> +    // If no commit is pending, but a redraw is requested, then post a redraw right away
> +    if (!m_beginFrameAndCommitPendingOnCCThread && m_redrawRequestedOnCCThread)
> +        scheduleDrawTaskOnCCThread();

I had trouble following the conditional logic here.  The first branch sets m_beginFrameAndCommitPendingOnCCThread to true, so if the first branch is hit the second can't.

I think this would be clearer with an explicit early return on the first branch instead of checking !m_beginFrameAndCommitPendingOnCCThread twice.  I think the intended flow is something like:

(1) update the state flags
(2) if we need a commit and we haven't sent a task, send the BFAC task to the main thread and do nothing else
(3) if we need a draw and we don't have one pending, post a task to ourself to draw

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:66
> +    PassOwnPtr<CCMainThread::Task> createBeginFrameAndCommitTaskOnCCThread();
> +    void createBeginFrameAndCommitTaskOnCCThread(CCCompletionEvent*, CCMainThread::Task**);

it's hard to follow the logic with these two functions sharing a name. IMO the create..() prefix should normally be used for a function that returns something, and the void version should have some other name.  It's doing something to the second parameter - maybe it's initializing the task instead of creating it?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:28
> +// CCLayerTreeHost disabled. See b67418.
> +#if 0

I think you should just leave the #if USE(THREADED_COMPOSITING) guard for this file
Comment 16 James Robinson 2011-09-16 15:54:16 PDT
Comment on attachment 107727 [details]
Merge win the tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=107727&action=review

R=me.  I left some nits on the previous patch and this one which apply, but none that are too serious.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:307
> +INSTANTIATE_TEST_CASE_P(

Very nifty - so we'll run the tests in threaded and non-threaded mode? Me likey

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:311
> +        CCSettings(false, false, false, false, false),
> +        CCSettings(false, false, true, false, false)));

now I kind of wish CCSettings some sort of c'tor where we could see what on earth these flags are at callsites.  I think I'm wishing for a different language...

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:558
> +    CCSettings setings;

setings->settings ? What actually uses this, or do you just need to have something on the stack that a magical macro picks up?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:564
> +#endif

looks like we have ourselves a comment fight going on!  I'm personally a fan of the // USE(THREADED_COMPOSITING) comment on this #endif

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:136
> +    ScopedSetImplThread impl;

I'm actually slightly surprised this works.  LayerChromium creation and destruction should always be a main thread thing, but I guess we don't have proper ASSERT()s to verify that so just doing the whole thing on the impl thread works just fine.

I think that the synchronizeTrees() function should have a ScopedSetImplThread doohickey, since normally we call that from commitTo(), but the LayerChromium setup should be main thread.  If this works for now, though, leave it this way and we can clean up when we add more ASSERT()s for LayerChromium.
Comment 17 David Levin 2011-09-16 15:58:45 PDT
(In reply to comment #16)
> (From update of attachment 107727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107727&action=review
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:311
> > +        CCSettings(false, false, false, false, false),
> > +        CCSettings(false, false, true, false, false)));
> 
> now I kind of wish CCSettings some sort of c'tor where we could see what on earth these flags are at callsites.  I think I'm wishing for a different language...

I thought we encouraged enums in these cases to help with this issue.
Comment 18 James Robinson 2011-09-16 16:01:38 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 107727 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=107727&action=review
> > > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:311
> > > +        CCSettings(false, false, false, false, false),
> > > +        CCSettings(false, false, true, false, false)));
> > 
> > now I kind of wish CCSettings some sort of c'tor where we could see what on earth these flags are at callsites.  I think I'm wishing for a different language...
> 
> I thought we encouraged enums in these cases to help with this issue.

We do when the callers normally pass in literals.  In this case, all of the callers who instantiate real CCSettings initialize with named variables, not literals:

http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp&q=CCSettings&ct=rc&exact_package=chromium&cd=2&sq=&l=2646

so this caller is actually the exception to the rule.
Comment 19 James Robinson 2011-09-16 16:02:25 PDT
Also it looks like this macro doesn't less us expand out the CCSettings initialization in the normal way of doing:
CCSettings settings;
settings.foo = true;
settings.bar = false;
...
Comment 20 Adrienne Walker 2011-09-16 16:03:39 PDT
Comment on attachment 107727 [details]
Merge win the tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=107727&action=review

>>>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:311
>>>> +        CCSettings(false, false, true, false, false)));
>>> 
>>> now I kind of wish CCSettings some sort of c'tor where we could see what on earth these flags are at callsites.  I think I'm wishing for a different language...
>> 
>> I thought we encouraged enums in these cases to help with this issue.
> 
> We do when the callers normally pass in literals.  In this case, all of the callers who instantiate real CCSettings initialize with named variables, not literals:
> 
> http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp&q=CCSettings&ct=rc&exact_package=chromium&cd=2&sq=&l=2646
> 
> so this caller is actually the exception to the rule.

That'd be a lot of enums.  Another option is to remove the args from the constructor and force callers to set named member variables explicitly.
Comment 21 Nat Duca 2011-09-16 16:04:51 PDT
Omg folks, rapid rate of replying here!

Depressingly, our subclassing is fighting with the the value-parameterized tests. Scowl. I need to go do a bit more battle with GTest to make this work. :/
Comment 22 David Levin 2011-09-16 16:05:43 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (From update of attachment 107727 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=107727&action=review
> > > > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:311
> > > > +        CCSettings(false, false, false, false, false),
> > > > +        CCSettings(false, false, true, false, false)));
> > > 
> > > now I kind of wish CCSettings some sort of c'tor where we could see what on earth these flags are at callsites.  I think I'm wishing for a different language...
> > 
> > I thought we encouraged enums in these cases to help with this issue.
> 
> We do when the callers normally pass in literals.  In this case, all of the callers who instantiate real CCSettings initialize with named variables, not literals:
> 
> http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp&q=CCSettings&ct=rc&exact_package=chromium&cd=2&sq=&l=2646
> 
> so this caller is actually the exception to the rule.

So basically the test code is the only ugly place.

btw, I'm fine with the r+ -- just to be absolutely clear :).
Comment 23 James Robinson 2011-09-16 16:06:19 PDT
(In reply to comment #20)
> (From update of attachment 107727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107727&action=review
> 
> >>>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:311
> >>>> +        CCSettings(false, false, true, false, false)));
> >>> 
> >>> now I kind of wish CCSettings some sort of c'tor where we could see what on earth these flags are at callsites.  I think I'm wishing for a different language...
> >> 
> >> I thought we encouraged enums in these cases to help with this issue.
> > 
> > We do when the callers normally pass in literals.  In this case, all of the callers who instantiate real CCSettings initialize with named variables, not literals:
> > 
> > http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp&q=CCSettings&ct=rc&exact_package=chromium&cd=2&sq=&l=2646
> > 
> > so this caller is actually the exception to the rule.
> 
> That'd be a lot of enums.  Another option is to remove the args from the constructor and force callers to set named member variables explicitly.

Yeah but then we can't use this really cool paramaterized test harness thingy from gtest.  Maybe we could have a few global CCSettings objects initialized via some part of the test harness that runs before INSTANTIATE_... evaluates?  Is that even possible?
Comment 24 WebKit Review Bot 2011-09-17 20:53:28 PDT
Comment on attachment 107727 [details]
Merge win the tests.

Attachment 107727 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9723806
Comment 25 James Robinson 2011-09-19 11:06:54 PDT
This doesn't compile on mac because of the CrossThreadCopier.h #include in IntRect.h.  You'll need to add that to the forwarding headers for the mac build :/

Maybe it's not so bad to have that in CCThreadProxy.cpp
Comment 26 Nat Duca 2011-09-20 18:26:00 PDT
Oddly, this passed the mac trybots:

http://build.chromium.org/p/tryserver.chromium/waterfall?branch=&committer=nduca@chromium.org&reload=60

James, what did you see that made you think this was a forwarding headers issue? The CrossThreadCopier is a WebCore/platform, not a WTF...
Comment 27 James Robinson 2011-09-20 18:34:11 PDT
It's broken on the Apple Mac port, not Chromium mac.  The reason I suspect forwarding headers is because of this output:

http://queues.webkit.org/results/9723806

In file included from /mnt/git/webkit-mac-ews/WebKitBuild/Release/WebCore.framework/PrivateHeaders/FrameSelection.h:30,
                 from /mnt/git/webkit-mac-ews/Source/WebKit/mac/WebView/WebFrameInternal.h:35,
                 from /mnt/git/webkit-mac-ews/Source/WebKit/mac/History/WebBackForwardList.mm:32:
/mnt/git/webkit-mac-ews/WebKitBuild/Release/WebCore.framework/PrivateHeaders/IntRect.h:29:31: error: CrossThreadCopier.h: No such file or directory
In file included from /mnt/git/webkit-mac-ews/WebKitBuild/Release/WebCore.framework/PrivateHeaders/FrameSelection.h:30,
                 from /mnt/git/webkit-mac-ews/Source/WebKit/mac/WebView/WebFrameInternal.h:35,
                 from /mnt/git/webkit-mac-ews/Source/WebKit/mac/History/WebBackForwardList.mm:32:
/mnt/git/webkit-mac-ews/WebKitBuild/Release/WebCore.framework/PrivateHeaders/IntRect.h:262: error: 'CrossThreadCopierBase' is not a template
/mnt/git/webkit-mac-ews/WebKitBuild/Release/WebCore.framework/PrivateHeaders/IntRect.h:262: error: expected template-name before '<' token
/mnt/git/webkit-mac-ews/WebKitBuild/Release/WebCore.framework/PrivateHeaders/IntRect.h:262: error: expected `{' before '<' token
/mnt/git/webkit-mac-ews/WebKitBuild/Release/WebCore.framework/PrivateHeaders/IntRect.h:262: error: expected unqualified-id before '<' token

It looks like it's trying to compile files in the WebKit/ directory, but since CrossThreadCopierBase is not forwarded from WebCore/ to WebKit/ the build fails. I might be misinterpreting the tea leaves - I'll try patching this in locally on my mac pro and see what happens.
Comment 28 James Robinson 2011-09-21 11:47:02 PDT
Moving the CrossThreadCopierBase<> declaration back to CCThreadProxy.cpp like you did initially fixes the Mac compile.  Now that I think about it more, I'm not sure if the CrossThreadCopier stuff is supposed to be public interface on IntRect or not.

Adding CrossThreadCopier.h to the the appropriate forwading headers headers from WebCore -> WebKit for the mac build would probably also fix the mac compile (I haven't tried this yet since I don't know how to do that).
Comment 29 James Robinson 2011-09-21 12:07:58 PDT
After chatting with Dave Levin in IRC we decided that the best thing to do is to add the CrossThreadCopierBase<> incantation to CrossThreadCopier.h using a forward declaration of IntRect.  This seems to blend just fine.
Comment 30 David Levin 2011-09-21 12:10:33 PDT
(In reply to comment #29)
> After chatting with Dave Levin in IRC we decided that the best thing to do is to add the CrossThreadCopierBase<> incantation to CrossThreadCopier.h using a forward declaration of IntRect.  This seems to blend just fine.

And while you're at it, feel free to add a comment in there about this being the way to do things :).
Comment 31 James Robinson 2011-09-22 00:21:21 PDT
Committed r95699: <http://trac.webkit.org/changeset/95699>
Comment 32 James Robinson 2011-09-22 00:52:39 PDT
Reverted r95699 for reason:

Makes many chromium compositor tests crash

Committed r95702: <http://trac.webkit.org/changeset/95702>
Comment 33 Nat Duca 2011-09-22 13:35:30 PDT
Created attachment 108389 [details]
More tweaks
Comment 34 WebKit Review Bot 2011-09-22 13:38:19 PDT
Attachment 108389 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/CrossThreadCopier.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 WebKit Review Bot 2011-09-22 14:19:49 PDT
Comment on attachment 108389 [details]
More tweaks

Attachment 108389 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9792496
Comment 36 James Robinson 2011-09-22 19:11:56 PDT
Comment on attachment 108389 [details]
More tweaks

View in context: https://bugs.webkit.org/attachment.cgi?id=108389&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp:44
> +#ifndef NDEBUG

moving this #ifndef down breaks the compile in release builds, at least on linux, because of unused variables

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:117
> +    commitIfNeeded();

why would we attempt to do a commit here?  This seems to be the cause of a lot of crashes

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:120
> +    // If a commit is pending, perform the commit first.

I can't remember why we want to do this instead of just doing a blocking call to the thread to glFinish() whatever it's working on and then return control to the caller.  Today the only caller of this path is in shutdown where I don't think it matters which frame gets finished, so long as we finish() on the context at the end of some frame.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:257
> +    TRACE_EVENT("CCThreadProxy::finishAllRenderingOnCCThread", this, 0);
> +    ASSERT(isImplThread());
> +    ASSERT(!m_beginFrameAndCommitPendingOnCCThread);
> +    if (m_redrawRequestedOnCCThread) {
> +        drawLayersOnCCThread();
> +        m_layerTreeHostImpl->present();
> +        m_redrawRequestedOnCCThread = false;
> +    }

why do we draw here and not just glFinish()? couldn't the caller call compositeAndReadback() if they were interested in getting a new frame put up?
Comment 37 James Robinson 2011-09-22 19:33:30 PDT
Ah, I forgot that we use finishAllRendering() in the implementation of compositeAndReadback().

Alternate proposal: how about finishAllRendering() only does a blocking call to the thread to request a glFinish(), and the whole request BFAC task stuff is moved to be used only for compositeAndReadback() where it's really needed?  I think that's much closer to the semantics we want and it keeps all of the complexity limited to compositeAndReadback() which we know is slow and maybe (extreme optimism alert) something we can get rid of someday.
Comment 38 WebKit Review Bot 2011-09-22 20:46:47 PDT
Comment on attachment 108389 [details]
More tweaks

Attachment 108389 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9796529
Comment 39 Nat Duca 2011-09-23 16:18:44 PDT
(In reply to comment #37)
> Alternate proposal: how about finishAllRendering() only does a blocking call to the thread to request a 

Sure.
Comment 40 Nat Duca 2011-09-23 16:36:22 PDT
Created attachment 108556 [details]
Dont commit in finish.
Comment 41 WebKit Review Bot 2011-09-23 16:39:26 PDT
Attachment 108556 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/CrossThreadCopier.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Nat Duca 2011-09-23 16:45:15 PDT
Created attachment 108559 [details]
Set Impl thread when setting layer renderer
Comment 43 WebKit Review Bot 2011-09-23 16:49:29 PDT
Attachment 108559 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/CrossThreadCopier.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 44 James Robinson 2011-09-26 19:03:21 PDT
Comment on attachment 108559 [details]
Set Impl thread when setting layer renderer

R=me. Will land by hand to babysit.
Comment 45 James Robinson 2011-09-26 19:17:33 PDT
Committed r96066: <http://trac.webkit.org/changeset/96066>
Comment 46 Nat Duca 2011-09-27 06:42:33 PDT
(In reply to comment #45)
> Committed r96066: <http://trac.webkit.org/changeset/96066>

Thanks James. Sorry to not have landed this weekend --- wasn't sure on the final review.