Bug 72426 – [chromium] Fuse MainThread and CCThread

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Fuse MainThread and CCThread

[Nat Duca]

Created attachment 115253 [details]
Patch

[Nat Duca]

Created attachment 115254 [details]
Patch

[David Levin]

Comment on attachment 115254 [details]
Patch

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

Just a drive by comment.

> Source/WebCore/platform/graphics/chromium/cc/CCScopedThreadProxy.h:35
> +// any point from the main thread after which no more tasks posted to the proxy will run. In other words, all

Are these references (line 35 and 39) to main thread still true?

[James Robinson]

Comment on attachment 115253 [details]
Patch

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

R=me but please fix comments before landing

> Source/WebCore/platform/graphics/chromium/cc/CCScopedThreadProxy.h:35
> +// any point from the main thread after which no more tasks posted to the proxy will run. In other words, all

this comment and others still refer to the 'main thread' even though this could apply to any thread pair.  Could you update the rest of this comment block?

> Source/WebCore/platform/graphics/chromium/cc/CCScopedThreadProxy.h:52
> +    // Can be called from any thread. Posts a task to the main thread that runs unless

references main thread, but should be perhaps 'target' thread?

> Source/WebCore/platform/graphics/chromium/cc/CCScopedThreadProxy.h:82
> +    bool m_shutdown; // Only accessed on the main thread

update comment

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:66
> +    m_mainThreadProxy = CCScopedThreadProxy::create(CCProxy::mainThread());

is there a reason this was moved from the initialization list to here?

> Source/WebKit/chromium/src/CCThreadImpl.cpp:108
> +      m_threadID = currentThread();
> +      return;

4-space intend please, this is webkit

> Source/WebKit/chromium/src/WebKit.cpp:35
> +#include "cc/CCProxy.h"

i don't think this sorting is right - we normally do case-sensitive sorting, right?

> Source/WebKit/chromium/src/WebKit.cpp:53
> +

extra newline here - please remove

> Source/WebKit/chromium/src/WebKit.cpp:109
> +    delete WebCore::CCProxy::mainThread();

For the impl thread we just leak the CCThreadImpl, since there's only one per address space and it just holds a couple pointers.

[James Robinson]

Stupid bugzilla...

[James Robinson]

Comment on attachment 115254 [details]
Patch

Same comments as last time.

[Nat Duca]

> > Source/WebKit/chromium/src/WebKit.cpp:35
> > +#include "cc/CCProxy.h"
> 
> i don't think this sorting is right - we normally do case-sensitive sorting, right?

check-webkit-style cries unless this is here. I'm stumped...

[WebKit Review Bot]

Comment on attachment 115254 [details]
Patch

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

[Nat Duca]

Committed r100370: <http://trac.webkit.org/changeset/100370>

[David Levin]

(In reply to comment #7)
> > > Source/WebKit/chromium/src/WebKit.cpp:35
> > > +#include "cc/CCProxy.h"
> > 
> > i don't think this sorting is right - we normally do case-sensitive sorting, right?
> 
> check-webkit-style cries unless this is here. I'm stumped...

What do you mean by unless this is here?

If you put it before CCThreadImpl.h, it should be upset, but this looks incorrect as well. (Note by default check-webkit-style is going to run on the diff which may omit some errors that you introduce like an incorrect sorting of #include "Logging.h".  You can run check-webkit-style filename to get all the errors for that file.)