Devtools/CC: Remove continuous repainting feature

Devtools: Remove continuous repainting feature

BUG=523040
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ea463c633e5a51e85c3a805848cbd40914b2b896
Cr-Commit-Position: refs/heads/master@{#348763}

[samli, 2015-08-18 05:21:56]

samli@chromium.org changed reviewers:
+ pfeldman@chromium.org

[samli, 2015-08-18 05:25:39]

The CQ bit was checked by samli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-18 05:25:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1296673004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1296673004/1

[commit-bot: I haz the power, 2015-08-18 06:47:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-18 06:47:37]

Dry run: This issue passed the CQ dry run.

[samli, 2015-08-18 07:39:33]

samli@chromium.org changed reviewers:
+ vollick@chromium.org

[samli, 2015-08-18 07:39:50]

+vollick for owner's

[danakj, 2015-08-18 18:07:18]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org, pdr@chromium.org

[danakj, 2015-08-18 18:07:19]

This points to a BUG entitled "Devtools UI: Add an elements toolbar". How does
that relate to removing continuous repainting?

Don't we use this in telemetry tests?

[paulirish, 2015-08-20 18:33:37]

On 2015/08/18 at 18:07:19, danakj wrote:
> This points to a BUG entitled "Devtools UI: Add an elements toolbar". How does
that relate to removing continuous repainting?

Good call.
I've updated the bug to point to
https://code.google.com/p/chromium/issues/detail?id=523040, which is exclusive
to this item.

[samli, 2015-08-21 00:21:52]

On 2015/08/18 at 18:07:19, danakj wrote:
> This points to a BUG entitled "Devtools UI: Add an elements toolbar". How does
that relate to removing continuous repainting?
> 
> Don't we use this in telemetry tests?

I'm not sure. I just found
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
which looks like its triggers paints in a different way. So, AFAIK, the code in
this patch isn't used by telemetry.

[danakj, 2015-08-21 18:35:25]

Ya, that runs raf but doesn't cause any repainting that I can see. pdr do you
know?

[danakj, 2015-08-21 18:39:47]

OK pdr says the repaint tests are driving by code in cc not by this mechanism.
Cool

[pdr., 2015-08-21 18:44:15]

On 2015/08/21 at 18:39:47, danakj wrote:
> OK pdr says the repaint tests are driving by code in cc not by this mechanism.
Cool

A bunch of benchmarks explicitly cause paints directly, like rasterize and
record. That said, I'm not sure about the raf case samli found.. I think we'll
trigger a paint in that case. For example, if there is an animated gif on the
page, each raf will cause invalidations and paints.

[danakj, 2015-08-21 18:56:10]

https://codereview.chromium.org/1296673004/diff/1/cc/layers/heads_up_display_...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/1296673004/diff/1/cc/layers/heads_up_display_...
cc/layers/heads_up_display_layer_impl.cc:272: if (debug_state.show_fps_counter)
{
you can just early out if !show_fps_counter and don't need to have these two if
statements and the MakeEmpty()?

https://codereview.chromium.org/1296673004/diff/1/cc/layers/heads_up_display_...
cc/layers/heads_up_display_layer_impl.cc:273: // Don't show the FPS display when
continuous painting is enabled, because
This comment should go

https://codereview.chromium.org/1296673004/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1296673004/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:236:
paint_time_counter_(PaintTimeCounter::Create()),
this can go away?

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

https://codereview.chromium.org/1296673004/diff/1/cc/trees/thread_proxy.cc#ne...
cc/trees/thread_proxy.cc:634: void ThreadProxy::SetDebugStateOnImplThread(
Can this method go away?

[samli, 2015-08-26 21:08:25]

PTAL.

https://codereview.chromium.org/1296673004/diff/1/cc/layers/heads_up_display_...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/1296673004/diff/1/cc/layers/heads_up_display_...
cc/layers/heads_up_display_layer_impl.cc:272: if (debug_state.show_fps_counter)
{
On 2015/08/21 at 18:56:09, danakj wrote:
> you can just early out if !show_fps_counter and don't need to have these two
if statements and the MakeEmpty()?

Done.

https://codereview.chromium.org/1296673004/diff/1/cc/layers/heads_up_display_...
cc/layers/heads_up_display_layer_impl.cc:273: // Don't show the FPS display when
continuous painting is enabled, because
On 2015/08/21 at 18:56:09, danakj wrote:
> This comment should go

Done.

https://codereview.chromium.org/1296673004/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1296673004/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:236:
paint_time_counter_(PaintTimeCounter::Create()),
On 2015/08/21 at 18:56:09, danakj wrote:
> this can go away?

Done.

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

https://codereview.chromium.org/1296673004/diff/1/cc/trees/thread_proxy.cc#ne...
cc/trees/thread_proxy.cc:634: void ThreadProxy::SetDebugStateOnImplThread(
On 2015/08/21 at 18:56:09, danakj wrote:
> Can this method go away?

I don't think so.

[danakj, 2015-08-27 18:44:45]

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

https://codereview.chromium.org/1296673004/diff/1/cc/trees/thread_proxy.cc#ne...
cc/trees/thread_proxy.cc:634: void ThreadProxy::SetDebugStateOnImplThread(
On 2015/08/26 21:08:24, samli wrote:
> On 2015/08/21 at 18:56:09, danakj wrote:
> > Can this method go away?
> 
> I don't think so.

Can you explain? The whole Proxy::SetDebugState method and this one do nothing
anymore.

[samli, 2015-08-28 01:42:44]

On 2015/08/27 at 18:44:45, danakj wrote:
> https://codereview.chromium.org/1296673004/diff/1/cc/trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/1296673004/diff/1/cc/trees/thread_proxy.cc#ne...
> cc/trees/thread_proxy.cc:634: void ThreadProxy::SetDebugStateOnImplThread(
> On 2015/08/26 21:08:24, samli wrote:
> > On 2015/08/21 at 18:56:09, danakj wrote:
> > > Can this method go away?
> > 
> > I don't think so.
> 
> Can you explain? The whole Proxy::SetDebugState method and this one do nothing
anymore.

I'm not sure. Just a quick search and I saw LayerTreeDebugState being used with
other options other than continuous painting so I assumed it was doing something
in those cases. eg. layer_tree_host_unittest_context.cc:976

[danakj, 2015-08-28 17:24:21]

On Thu, Aug 27, 2015 at 6:42 PM, <samli@chromium.org> wrote:

> On 2015/08/27 at 18:44:45, danakj wrote:
>
>> https://codereview.chromium.org/1296673004/diff/1/cc/trees/thread_proxy.cc
>> File cc/trees/thread_proxy.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1296673004/diff/1/cc/trees/thread_proxy.cc#ne...
>
>> cc/trees/thread_proxy.cc:634: void ThreadProxy::SetDebugStateOnImplThread(
>> On 2015/08/26 21:08:24, samli wrote:
>> > On 2015/08/21 at 18:56:09, danakj wrote:
>> > > Can this method go away?
>> >
>> > I don't think so.
>>
>
> Can you explain? The whole Proxy::SetDebugState method and this one do
>> nothing
>>
> anymore.
>
> I'm not sure. Just a quick search and I saw LayerTreeDebugState being used
> with
> other options other than continuous painting so I assumed it was doing
> something
> in those cases. eg. layer_tree_host_unittest_context.cc:976
>

Yes the debug state is used. I'm not proposing deleting the debug state.
But this function is not using it? It's doing nothing now. Am I miserading
it?


>
> https://codereview.chromium.org/1296673004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[samli, 2015-08-31 18:00:07]

On 2015/08/28 at 17:24:21, danakj wrote:
> On Thu, Aug 27, 2015 at 6:42 PM, <samli@chromium.org> wrote:
> 
> > On 2015/08/27 at 18:44:45, danakj wrote:
> >
> >> https://codereview.chromium.org/1296673004/diff/1/cc/trees/thread_proxy.cc
> >> File cc/trees/thread_proxy.cc (right):
> >>
> >
> >
> >
> >
https://codereview.chromium.org/1296673004/diff/1/cc/trees/thread_proxy.cc#ne...
> >
> >> cc/trees/thread_proxy.cc:634: void ThreadProxy::SetDebugStateOnImplThread(
> >> On 2015/08/26 21:08:24, samli wrote:
> >> > On 2015/08/21 at 18:56:09, danakj wrote:
> >> > > Can this method go away?
> >> >
> >> > I don't think so.
> >>
> >
> > Can you explain? The whole Proxy::SetDebugState method and this one do
> >> nothing
> >>
> > anymore.
> >
> > I'm not sure. Just a quick search and I saw LayerTreeDebugState being used
> > with
> > other options other than continuous painting so I assumed it was doing
> > something
> > in those cases. eg. layer_tree_host_unittest_context.cc:976
> >
> 
> Yes the debug state is used. I'm not proposing deleting the debug state.
> But this function is not using it? It's doing nothing now. Am I miserading
> it?

Sorry, not particularly familiar with this code. PTAL, removed
SetDebugStateOnImplThread() and SetDebugState () is empty in ThreadProxy. Is
that what you meant?
> 
> 
> >
> > https://codereview.chromium.org/1296673004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
>

[danakj, 2015-09-03 20:01:21]

https://codereview.chromium.org/1296673004/diff/40001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/1296673004/diff/40001/cc/trees/thread_proxy.h...
cc/trees/thread_proxy.h:177: void SetDebugState(const LayerTreeDebugState&
debug_state) override{};
I mean you can delete this function, it doesn't need to exist if it does
nothing? It's empty in SingleThreadProxy too.

https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/single_th...

[samli, 2015-09-09 23:44:40]

samli@chromium.org changed reviewers:
- pdr@chromium.org, pfeldman@chromium.org, vollick@chromium.org

[samli, 2015-09-09 23:55:51]

PTAL

https://codereview.chromium.org/1296673004/diff/40001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/1296673004/diff/40001/cc/trees/thread_proxy.h...
cc/trees/thread_proxy.h:177: void SetDebugState(const LayerTreeDebugState&
debug_state) override{};
On 2015/09/03 at 20:01:21, danakj wrote:
> I mean you can delete this function, it doesn't need to exist if it does
nothing? It's empty in SingleThreadProxy too.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/single_th...

Removed.

[danakj, 2015-09-10 20:18:14]

Thanks! LGTM

Can you also remove the setContinuousPaintingEnabled method over here:
https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...
?

And then follow up with a blink patch to remove it there?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Thanks!

[samli, 2015-09-10 20:29:19]

The CQ bit was checked by samli@chromium.org

[samli, 2015-09-10 20:29:19]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1296673004/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-09-10 20:30:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1296673004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1296673004/80001

[commit-bot: I haz the power, 2015-09-10 20:42:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-10 20:42:49]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[samli, 2015-09-11 00:08:45]

samli@chromium.org changed reviewers:
+ enne@chromium.org, sky@chromium.org

[samli, 2015-09-11 00:08:46]

Need OWNERs for components/ and content/renderer/

[sky, 2015-09-11 15:29:20]

html_viewer LGTM

[enne (OOO), 2015-09-11 17:16:29]

lgtm

[samli, 2015-09-11 17:45:02]

The CQ bit was checked by samli@chromium.org

[samli, 2015-09-11 17:45:02]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1296673004/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-09-11 17:45:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1296673004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1296673004/100001

[commit-bot: I haz the power, 2015-09-11 21:31:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 21:31:48]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[samli, 2015-09-14 21:44:46]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2015-09-14 21:45:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1296673004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1296673004/100001

[commit-bot: I haz the power, 2015-09-14 23:35:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-14 23:36:03]

Patchset 6 (id:??) landed as
https://crrev.com/ea463c633e5a51e85c3a805848cbd40914b2b896
Cr-Commit-Position: refs/heads/master@{#348763}

[Stephen White, 2015-09-21 21:36:38]

Note: this commit seems to have broken the FPS counter (GPU rasterization status
missing, etc). Reverting this change locally makes it come back.

I've logged crbug.com/534591 for this.

[commit-bot: I haz the power, 2015-09-23 12:38:59]

Patchset 6 (id:??) landed as
https://crrev.com/ea463c633e5a51e85c3a805848cbd40914b2b896
Cr-Commit-Position: refs/heads/master@{#348763}