cc: Do not request redraw on commit when impl-side painting

cc: Do not request redraw on commit when impl-side painting

When impl-side painting, we draw when the pending tree
becomes active and should no longer draw on commit.

BUG=168724
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177274

[enne (OOO), 2013-01-10 03:45:31]

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc#newcode414
cc/thread_proxy.cc:414: if (!m_layerTreeHost->settings().implSidePainting)
I understand moving the conditional in the commit function, but why is this main
thread setNeedsRedraw conditional as well?

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc#newcode737
cc/thread_proxy.cc:737: // This will also kick off the a scheduler action.
Can you reconcile this with the previous comment that says "this must be last"?

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc#newcode739
cc/thread_proxy.cc:739: m_schedulerOnImplThread->setNeedsRedraw();
I feel like removing the redraw here needs to correspondingly have a
setNeedsRedraw in activatePendingTree.

[nduca, 2013-01-10 11:01:09]

https://codereview.chromium.org/11830040/diff/1/cc/scheduler_state_machine.cc
File cc/scheduler_state_machine.cc (left):

https://codereview.chromium.org/11830040/diff/1/cc/scheduler_state_machine.cc...
cc/scheduler_state_machine.cc:188: m_needsRedraw = true;
this seems really funky

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc#newcode414
cc/thread_proxy.cc:414: if (!m_layerTreeHost->settings().implSidePainting)
yeah this makes no sense to me

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc#newcode736
cc/thread_proxy.cc:736: 
I dislike this.

I'd rather this was all in the state machine somehow. It smells like what you
need to do is say "if we commit && impl side painting mode, dont set needs
redraw."

[brianderson, 2013-01-10 18:55:32]

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc#newcode414
cc/thread_proxy.cc:414: if (!m_layerTreeHost->settings().implSidePainting)
Ah, I saw this being called after a commit and assumed it was called after every
commit.  It is only called after the first commit after a tab switch.  Will get
rid of this.

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc#newcode736
cc/thread_proxy.cc:736: 
On 2013/01/10 11:01:09, nduca wrote:
> I dislike this.
> 
> I'd rather this was all in the state machine somehow. It smells like what you
> need to do is say "if we commit && impl side painting mode, dont set needs
> redraw."

I had moved it out here because all the other setNeedsRedraws are initiated from
the this file and it was difficult for me to discover that a redraw was being
initiated from within the state machine itself, two levels lower.

By calling setNeedsRedraw here, do I run the risk of drawing at the wrong time
or drawing when we don't need to?

https://codereview.chromium.org/11830040/diff/1/cc/thread_proxy.cc#newcode739
cc/thread_proxy.cc:739: m_schedulerOnImplThread->setNeedsRedraw();
On 2013/01/10 03:45:31, enne wrote:
> I feel like removing the redraw here needs to correspondingly have a
> setNeedsRedraw in activatePendingTree.

This is meant to go on top of my other patch, which does setNeedsRedraw in
onHasPendingTreeStateChanged.

[brianderson, 2013-01-11 00:06:17]

Please take a look again.

[nduca, 2013-01-11 04:07:31]

lgtm with nit

https://codereview.chromium.org/11830040/diff/7001/cc/scheduler.h
File cc/scheduler.h (right):

https://codereview.chromium.org/11830040/diff/7001/cc/scheduler.h#newcode52
cc/scheduler.h:52: static scoped_ptr<Scheduler> create(SchedulerClient* client,
scoped_ptr<FrameRateController> frameRateController, bool
implSidePaintingEnabled)
How about passing in the Settings object?

https://codereview.chromium.org/11830040/diff/7001/cc/scheduler_state_machine.cc
File cc/scheduler_state_machine.cc (right):

https://codereview.chromium.org/11830040/diff/7001/cc/scheduler_state_machine...
cc/scheduler_state_machine.cc:13: :
m_implSidePaintingEnabled(implSidePaintingEnabled)
this makes sense to stay as a bool...

[brianderson, 2013-01-12 00:38:34]

https://codereview.chromium.org/11830040/diff/7001/cc/scheduler.h
File cc/scheduler.h (right):

https://codereview.chromium.org/11830040/diff/7001/cc/scheduler.h#newcode52
cc/scheduler.h:52: static scoped_ptr<Scheduler> create(SchedulerClient* client,
scoped_ptr<FrameRateController> frameRateController, bool
implSidePaintingEnabled)
On 2013/01/11 04:07:31, nduca wrote:
> How about passing in the Settings object?

Done.

https://codereview.chromium.org/11830040/diff/7001/cc/scheduler_state_machine.cc
File cc/scheduler_state_machine.cc (right):

https://codereview.chromium.org/11830040/diff/7001/cc/scheduler_state_machine...
cc/scheduler_state_machine.cc:13: :
m_implSidePaintingEnabled(implSidePaintingEnabled)
On 2013/01/11 04:07:31, nduca wrote:
> this makes sense to stay as a bool...

I'm just going to pass it all the way down, just in case.

[nduca, 2013-01-12 00:39:04]

sgtm

[commit-bot: I haz the power, 2013-01-14 18:03:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11830040/15001

[commit-bot: I haz the power, 2013-01-14 18:29:51]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[brianderson, 2013-01-15 00:57:11]

Requesting another review pass for my changes to the tests.

[nduca, 2013-01-15 01:54:54]

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest_scroll.cc:561: impl->setNeedsRedraw();
any idea why this one happened?

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_settings.cc
File cc/layer_tree_settings.cc (left):

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_settings.cc...
cc/layer_tree_settings.cc:68: if (CommandLine::ForCurrentProcess()->HasSwitch(
i'm confused about the code movement here, whats the change that got made and
whats the cleanup?

https://codereview.chromium.org/11830040/diff/13005/cc/scheduler_state_machin...
File cc/scheduler_state_machine.cc (right):

https://codereview.chromium.org/11830040/diff/13005/cc/scheduler_state_machin...
cc/scheduler_state_machine.cc:13:
SchedulerStateMachine::SchedulerStateMachine(const LayerTreeSettings
&layerTreeSettings)
I'm not liking the machine having access to all the settings. This means that
all of a sudden, its possible for the state machine to branch on just about  a
zillion things. Is it possible to just make a simple

struct SchedulerSettings {
   bool implSidePainting
}

that you pass in? You've done the hard work --- you shold be able to edit the
raw patch for this changelist in your editor, replacing LayerTreeSettings with
SchedulerSettings. Then reset --hard origin/master, git patch -p1 <
~/edited_patch.patch and you should be good to go.

[enne (OOO), 2013-01-15 01:58:35]

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest_scroll.cc:561: impl->setNeedsRedraw();
On 2013/01/15 01:54:54, nduca wrote:
> any idea why this one happened?

Yeah, I talked with Brian about this.  Previously this test assumed there'd be a
redraw post commit that would get kicked off to try to draw the old main frame
again while there was a pending tree.

That said, I wonder if this is a race between the commit and the redraw, where
it could try to redraw and rather than getting into the else block it re-does
the first part of this if block.

I wonder if something like commitComplete should instead set the redraw in this
test.

[brianderson, 2013-01-15 02:01:56]

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest_scroll.cc:561: impl->setNeedsRedraw();
On 2013/01/15 01:58:35, enne wrote:
> On 2013/01/15 01:54:54, nduca wrote:
> > any idea why this one happened?
> 
> Yeah, I talked with Brian about this.  Previously this test assumed there'd be
a
> redraw post commit that would get kicked off to try to draw the old main frame
> again while there was a pending tree.
> 
> That said, I wonder if this is a race between the commit and the redraw, where
> it could try to redraw and rather than getting into the else block it re-does
> the first part of this if block.
> 
> I wonder if something like commitComplete should instead set the redraw in
this
> test.

Will have commit complete trigger setNeedsRedraw once.

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_settings.cc
File cc/layer_tree_settings.cc (left):

https://codereview.chromium.org/11830040/diff/13005/cc/layer_tree_settings.cc...
cc/layer_tree_settings.cc:68: if (CommandLine::ForCurrentProcess()->HasSwitch(
On 2013/01/15 01:54:54, nduca wrote:
> i'm confused about the code movement here, whats the change that got made and
> whats the cleanup?

I should undo this.  At some point, I was adding a new constructor that would
benefit from this reordering, but decided not to add it.

https://codereview.chromium.org/11830040/diff/13005/cc/scheduler_state_machin...
File cc/scheduler_state_machine.cc (right):

https://codereview.chromium.org/11830040/diff/13005/cc/scheduler_state_machin...
cc/scheduler_state_machine.cc:13:
SchedulerStateMachine::SchedulerStateMachine(const LayerTreeSettings
&layerTreeSettings)
On 2013/01/15 01:54:54, nduca wrote:
> I'm not liking the machine having access to all the settings. This means that
> all of a sudden, its possible for the state machine to branch on just about  a
> zillion things. Is it possible to just make a simple
> 
> struct SchedulerSettings {
>    bool implSidePainting
> }
> 
> that you pass in? You've done the hard work --- you shold be able to edit the
> raw patch for this changelist in your editor, replacing LayerTreeSettings with
> SchedulerSettings. Then reset --hard origin/master, git patch -p1 <
> ~/edited_patch.patch and you should be good to go.

Sounds good.

[brianderson, 2013-01-16 01:39:07]

ptal

[nduca, 2013-01-16 02:16:02]

lgtm. Go ahead and land without any more review. But no virtual ~, make a copy
in state machine and style nit

https://codereview.chromium.org/11830040/diff/29001/cc/scheduler.h
File cc/scheduler.h (right):

https://codereview.chromium.org/11830040/diff/29001/cc/scheduler.h#newcode24
cc/scheduler.h:24: virtual ~SchedulerSettings() { }
its POD so you dont need ~

https://codereview.chromium.org/11830040/diff/29001/cc/scheduler.h#newcode110
cc/scheduler.h:110: const SchedulerSettings &schedulerSettings);
style looks funky
const Foo& foo

[commit-bot: I haz the power, 2013-01-16 02:53:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11830040/37001

[brianderson, 2013-01-16 03:31:26]

Many of the trybots are failing to link in SchedulerSettings.  I've added the
new files to cc.gyp though and I cannot reproduce locally.

Any clue what I'm doing wrong.

Here is an example trybot logfile:
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/76...

[danakj, 2013-01-16 03:36:18]

https://codereview.chromium.org/11830040/diff/37001/cc/scheduler_settings.h
File cc/scheduler_settings.h (right):

https://codereview.chromium.org/11830040/diff/37001/cc/scheduler_settings.h#n...
cc/scheduler_settings.h:10: class SchedulerSettings
You want CC_EXPORT here (defined in cc/cc_export.h)

[commit-bot: I haz the power, 2013-01-16 04:06:20]

Retried try job too often on linux_chromeos for step(s) aura_unittests,
base_unittests, browser_tests, cacheinvalidation_unittests, check_deps,
chromeos_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, net_unittests, ppapi_unittests,
printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests,
unit_tests

[brianderson, 2013-01-16 19:54:16]

Thanks for the quick response Dana. Giving CC_EXPORT a try.

[commit-bot: I haz the power, 2013-01-16 19:56:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11830040/53001

[commit-bot: I haz the power, 2013-01-17 00:03:49]

Change committed as 177274