[webkit-reviews] review granted: [Bug 74351] [chromium] Add inhibitDraw to CCScheduler and drop root impl to prevent background flash on tab restore : [Attachment 118873] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Nat Duca
<nduca at chromium.org>'s request for review:
Bug 74351: [chromium] Add inhibitDraw to CCScheduler and drop root impl to
prevent background flash on tab restore
https://bugs.webkit.org/show_bug.cgi?id=74351

Attachment 118873: Patch
https://bugs.webkit.org/attachment.cgi?id=118873&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118873&action=review


R=me. have several naming nits for your consideration.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:224
>      if (CCThreadProxy::implThread()) {
> -	   TRACE_EVENT("CCLayerTreeHost::setNeedsCommit", this, 0);
>	   m_proxy->setNeedsCommit();
>      } else

nit: you can drop the { }s around the 'if' clause if you want to drop this
trace since it's a 1-liner now

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:108
> +CCSchedulerStateMachine::Action CCScheduler::getNextAction()

I think WebKit style would lean towards calling this 'nextAction()'

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:41
> +    virtual bool inhibitDraw() const = 0;

this looks like a simple getter, but might be complicated on the implementation
side in the future if we get fancy about checking for tiles that are visible.
if we want to anticipate that i think we might want to give this a bigger name
and make it non-const. up to you, though, for now since it's a bool getter this
works.

then again, we're gonna call this a whole lot so the implementation had better
be really fast

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:434
> +    return m_layerTreeHostImpl ? !m_layerTreeHostImpl->rootLayer() : true;

hmmmm, a bit yoda-like this ternary statement appears.	perhaps phrasing this
function call terms of a positive (shouldDraw(), canDraw(), something like
that) would some clarity to the method provide.