Bug 67417 – [chromium] Make CCThreadProxy draw

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nat Duca]

The CCThreadProxy is mostly implemented, but needs to be updated to the new Update/Commit flow.

[Nat Duca]

Created attachment 106102 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106102 [details]
Patch

Attachment 106102 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9580560

[WebKit Review Bot]

Comment on attachment 106102 [details]
Patch

Attachment 106102 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9579655

[James Robinson]

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

[Nat Duca]

Created attachment 107441 [details]
Real 2 phase commit

[WebKit Review Bot]

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

[James Robinson]

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?

[James Robinson]

CCThreadProxy.cpp didn't compile on cr-mac EWS, although the actual failure seems to be omitted.

[Nat Duca]

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!

[Nat Duca]

Created attachment 107523 [details]
New hotness

[Nat Duca]

Created attachment 107578 [details]
Better impl of finishAllRendering

[James Robinson]

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

[Nat Duca]

Created attachment 107586 [details]
Rebase and nits

[Nat Duca]

Created attachment 107727 [details]
Merge win the tests.

[James Robinson]

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

[James Robinson]

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.

[David Levin]

(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.

[James Robinson]

(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.

[James Robinson]

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;
...

[Adrienne Walker]

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.

[Nat Duca]

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. :/

[David Levin]

(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 :).

[James Robinson]

(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?

[WebKit Review Bot]

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

[James Robinson]

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

[Nat Duca]

Oddly, this passed the mac trybots:

http://build.chromium.org/p/tryserver.chromium/waterfall?branch=&committer=[e-mail]&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...

[James Robinson]

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.

[James Robinson]

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).

[James Robinson]

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.

[David Levin]

(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 :).

[James Robinson]

Committed r95699: <http://trac.webkit.org/changeset/95699>

[James Robinson]

Reverted r95699 for reason:

Makes many chromium compositor tests crash

Committed r95702: <http://trac.webkit.org/changeset/95702>

[Nat Duca]

Created attachment 108389 [details]
More tweaks

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 108389 [details]
More tweaks

Attachment 108389 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9792496

[James Robinson]

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?

[James Robinson]

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.

[WebKit Review Bot]

Comment on attachment 108389 [details]
More tweaks

Attachment 108389 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9796529

[Nat Duca]

(In reply to comment #37)
> Alternate proposal: how about finishAllRendering() only does a blocking call to the thread to request a 

Sure.

[Nat Duca]

Created attachment 108556 [details]
Dont commit in finish.

[WebKit Review Bot]

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.

[Nat Duca]

Created attachment 108559 [details]
Set Impl thread when setting layer renderer

[WebKit Review Bot]

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.

[James Robinson]

Comment on attachment 108559 [details]
Set Impl thread when setting layer renderer

R=me. Will land by hand to babysit.

[James Robinson]

Committed r96066: <http://trac.webkit.org/changeset/96066>

[Nat Duca]

(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.