UI impl side painting default on

UI impl side painting default on

Turn on Impl Side Painting on Aura browser compositor by default
and add a disable compositor switch.

(Last attempt to re-land is at https://codereview.chromium.org/1160093005/, the try bot failure seem to be fixed with https://codereview.chromium.org/1166983007/ which is committed)

R=danakj
BUG=314185
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/3be4ab2d48dad92483a369cdaf44ee164efd1558
Cr-Commit-Position: refs/heads/master@{#333656}

[weiliangc, 2015-02-26 23:12:37]

Quick and dirty way to turn on ui compositor impl-side painting.

[piman, 2015-02-26 23:24:25]

On 2015/02/26 23:12:37, weiliangc wrote:
> Quick and dirty way to turn on ui compositor impl-side painting.

drive-by, should we keep a disable flag in the short term to help debug issues
if any?
This has been tremendously useful for use-surfaces.

[reveman, 2015-02-26 23:24:50]

reveman@chromium.org changed reviewers:
+ reveman@chromium.org

[reveman, 2015-02-26 23:24:51]

https://codereview.chromium.org/962833003/diff/20001/ui/compositor/compositor...
File ui/compositor/compositor_switches.cc (left):

https://codereview.chromium.org/962833003/diff/20001/ui/compositor/compositor...
ui/compositor/compositor_switches.cc:20: const char kUIEnableImplSidePainting[]
= "ui-enable-impl-side-painting";
Can we turn this into ui-disable-impl-side-painting until we know for sure that
it's going to stick?

[weiliangc, 2015-02-26 23:26:17]

Patchset #2 (id:20001) has been deleted

[weiliangc, 2015-02-26 23:26:29]

Patchset #2 (id:40001) has been deleted

[weiliangc, 2015-02-26 23:29:18]

Updated to have a disable command line flag.

[piman, 2015-02-26 23:36:27]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2015-02-26 23:36:28]

LGTM! ship it.

[danakj, 2015-02-26 23:37:38]

LGTM

[danakj, 2015-02-26 23:37:46]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-02-26 23:40:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962833003/60001

[weiliangc, 2015-02-27 00:24:36]

The CQ bit was unchecked by weiliangc@chromium.org

[weiliangc, 2015-02-27 00:24:50]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-02-27 00:24:50]

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

[weiliangc, 2015-02-27 00:25:07]

The CQ bit was unchecked by weiliangc@chromium.org

[weiliangc, 2015-02-27 00:25:49]

Patchset #3 (id:80001) has been deleted

[weiliangc, 2015-02-27 00:27:25]

Patchset #3 (id:100001) has been deleted

[weiliangc, 2015-02-27 00:28:12]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-02-27 00:28:12]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/962833003/#ps100002
(title: "correction of missed one reference to switch")

[commit-bot: I haz the power, 2015-02-27 00:29:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962833003/100002

[commit-bot: I haz the power, 2015-02-27 04:26:28]

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

[commit-bot: I haz the power, 2015-02-27 04:26:29]

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

[weiliangc, 2015-02-27 05:17:22]

There are 3 failures:

- compositor unittests:
  - because impl side painting use picture layer instead of content layer, the
scale and paint_size is different in layer unittests

- content browser test
  - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current number of
threads is 1 not 0
  - My guess is GetNumWorkerThreads() is called before this, which set number of
threads to default, 1
  - Either from PicturePile's create or TaskGraphRunner()

- view unittests
  - cull_set size doesn't change. Need to update Paint function for View?

[reveman, 2015-02-27 15:39:47]

On 2015/02/27 at 05:17:22, weiliangc wrote:
> There are 3 failures:
> 
> - compositor unittests:
>   - because impl side painting use picture layer instead of content layer, the
scale and paint_size is different in layer unittests
> 
> - content browser test
>   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current number
of threads is 1 not 0

Where is this SetNumWorkerThreads() call done from? It needs to happen before we
create any compositor instances..

>   - My guess is GetNumWorkerThreads() is called before this, which set number
of threads to default, 1
>   - Either from PicturePile's create or TaskGraphRunner()
> 
> - view unittests
>   - cull_set size doesn't change. Need to update Paint function for View?

[weiliangc, 2015-02-27 15:45:10]

On 2015/02/27 15:39:47, reveman wrote:
> On 2015/02/27 at 05:17:22, weiliangc wrote:
> > There are 3 failures:
> > 
> > - compositor unittests:
> >   - because impl side painting use picture layer instead of content layer,
the
> scale and paint_size is different in layer unittests
> > 
> > - content browser test
> >   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current number
> of threads is 1 not 0
> 
> Where is this SetNumWorkerThreads() call done from? It needs to happen before
we
> create any compositor instances..

(with codesearch) Only callsite I can find is at RenderThreadImpl
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

> 
> >   - My guess is GetNumWorkerThreads() is called before this, which set
number
> of threads to default, 1
> >   - Either from PicturePile's create or TaskGraphRunner()
> > 
> > - view unittests
> >   - cull_set size doesn't change. Need to update Paint function for View?

[reveman, 2015-02-27 15:54:25]

On 2015/02/27 at 15:45:10, weiliangc wrote:
> On 2015/02/27 15:39:47, reveman wrote:
> > On 2015/02/27 at 05:17:22, weiliangc wrote:
> > > There are 3 failures:
> > > 
> > > - compositor unittests:
> > >   - because impl side painting use picture layer instead of content layer,
the
> > scale and paint_size is different in layer unittests
> > > 
> > > - content browser test
> > >   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current
number
> > of threads is 1 not 0
> > 
> > Where is this SetNumWorkerThreads() call done from? It needs to happen
before we
> > create any compositor instances..
> 
> (with codesearch) Only callsite I can find is at RenderThreadImpl
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...


Ok, that's not supposed to affect the browser compositor. Let me guess, this is
only happening in single process mode? I think we need to make that
SetNumWorkerThreads call conditional on single-process mode and call it
somewhere else in that case. 

> > 
> > >   - My guess is GetNumWorkerThreads() is called before this, which set
number
> > of threads to default, 1
> > >   - Either from PicturePile's create or TaskGraphRunner()
> > > 
> > > - view unittests
> > >   - cull_set size doesn't change. Need to update Paint function for View?

[weiliangc, 2015-02-27 16:52:21]

On 2015/02/27 15:54:25, reveman wrote:
> On 2015/02/27 at 15:45:10, weiliangc wrote:
> > On 2015/02/27 15:39:47, reveman wrote:
> > > On 2015/02/27 at 05:17:22, weiliangc wrote:
> > > > There are 3 failures:
> > > > 
> > > > - compositor unittests:
> > > >   - because impl side painting use picture layer instead of content
layer,
> the
> > > scale and paint_size is different in layer unittests
> > > > 
> > > > - content browser test
> > > >   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current
> number
> > > of threads is 1 not 0
> > > 
> > > Where is this SetNumWorkerThreads() call done from? It needs to happen
> before we
> > > create any compositor instances..
> > 
> > (with codesearch) Only callsite I can find is at RenderThreadImpl
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> 
> 
> Ok, that's not supposed to affect the browser compositor. Let me guess, this
is
> only happening in single process mode? I think we need to make that
> SetNumWorkerThreads call conditional on single-process mode and call it
> somewhere else in that case. 

Updated CL to not call SetNumWorkerThreads in SingleProcess mode.

Should I open a bug for the "call it somewhere else in that case?"

> 
> > > 
> > > >   - My guess is GetNumWorkerThreads() is called before this, which set
> number
> > > of threads to default, 1
> > > >   - Either from PicturePile's create or TaskGraphRunner()
> > > > 
> > > > - view unittests
> > > >   - cull_set size doesn't change. Need to update Paint function for
View?

[danakj, 2015-02-27 16:57:02]

The test needs to pass a command line in probably so the
compositordependencies are right. There are some render widget tests I had
to do this for also if you look thru git log in content/renderer. Or I can
find it later.
On Feb 27, 2015 7:54 AM, <reveman@chromium.org> wrote:

> On 2015/02/27 at 15:45:10, weiliangc wrote:
>
>> On 2015/02/27 15:39:47, reveman wrote:
>> > On 2015/02/27 at 05:17:22, weiliangc wrote:
>> > > There are 3 failures:
>> > >
>> > > - compositor unittests:
>> > >   - because impl side painting use picture layer instead of content
>> layer,
>>
> the
>
>> > scale and paint_size is different in layer unittests
>> > >
>> > > - content browser test
>> > >   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current
>>
> number
>
>> > of threads is 1 not 0
>> >
>> > Where is this SetNumWorkerThreads() call done from? It needs to happen
>>
> before we
>
>> > create any compositor instances..
>>
>
>  (with codesearch) Only callsite I can find is at RenderThreadImpl
>>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/content/renderer/render_thread_impl.cc&q=setnumworkerThreads&sq=
> package:chromium&type=cs&l=672
>
>
> Ok, that's not supposed to affect the browser compositor. Let me guess,
> this is
> only happening in single process mode? I think we need to make that
> SetNumWorkerThreads call conditional on single-process mode and call it
> somewhere else in that case.
>
>  >
>> > >   - My guess is GetNumWorkerThreads() is called before this, which set
>>
> number
>
>> > of threads to default, 1
>> > >   - Either from PicturePile's create or TaskGraphRunner()
>> > >
>> > > - view unittests
>> > >   - cull_set size doesn't change. Need to update Paint function for
>> View?
>>
>
>
> https://codereview.chromium.org/962833003/
>

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

[weiliangc, 2015-02-27 17:11:50]

this? https://codereview.chromium.org/801973002

You mean pass in whether browser compositor is impl-side-painting? or pass in
single-thread?

On 2015/02/27 16:57:02, danakj wrote:
> The test needs to pass a command line in probably so the
> compositordependencies are right. There are some render widget tests I had
> to do this for also if you look thru git log in content/renderer. Or I can
> find it later.
> On Feb 27, 2015 7:54 AM, <mailto:reveman@chromium.org> wrote:
> 
> > On 2015/02/27 at 15:45:10, weiliangc wrote:
> >
> >> On 2015/02/27 15:39:47, reveman wrote:
> >> > On 2015/02/27 at 05:17:22, weiliangc wrote:
> >> > > There are 3 failures:
> >> > >
> >> > > - compositor unittests:
> >> > >   - because impl side painting use picture layer instead of content
> >> layer,
> >>
> > the
> >
> >> > scale and paint_size is different in layer unittests
> >> > >
> >> > > - content browser test
> >> > >   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current
> >>
> > number
> >
> >> > of threads is 1 not 0
> >> >
> >> > Where is this SetNumWorkerThreads() call done from? It needs to happen
> >>
> > before we
> >
> >> > create any compositor instances..
> >>
> >
> >  (with codesearch) Only callsite I can find is at RenderThreadImpl
> >>
> > https://code.google.com/p/chromium/codesearch#chromium/
> > src/content/renderer/render_thread_impl.cc&q=setnumworkerThreads&sq=
> > package:chromium&type=cs&l=672
> >
> >
> > Ok, that's not supposed to affect the browser compositor. Let me guess,
> > this is
> > only happening in single process mode? I think we need to make that
> > SetNumWorkerThreads call conditional on single-process mode and call it
> > somewhere else in that case.
> >
> >  >
> >> > >   - My guess is GetNumWorkerThreads() is called before this, which set
> >>
> > number
> >
> >> > of threads to default, 1
> >> > >   - Either from PicturePile's create or TaskGraphRunner()
> >> > >
> >> > > - view unittests
> >> > >   - cull_set size doesn't change. Need to update Paint function for
> >> View?
> >>
> >
> >
> > https://codereview.chromium.org/962833003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[weiliangc, 2015-02-27 17:12:16]

On 2015/02/27 17:11:50, weiliangc wrote:
> this? https://codereview.chromium.org/801973002
> 
> You mean pass in whether browser compositor is impl-side-painting? or pass in
> single-thread?
^ I mean single-process?
> 
> On 2015/02/27 16:57:02, danakj wrote:
> > The test needs to pass a command line in probably so the
> > compositordependencies are right. There are some render widget tests I had
> > to do this for also if you look thru git log in content/renderer. Or I can
> > find it later.
> > On Feb 27, 2015 7:54 AM, <mailto:reveman@chromium.org> wrote:
> > 
> > > On 2015/02/27 at 15:45:10, weiliangc wrote:
> > >
> > >> On 2015/02/27 15:39:47, reveman wrote:
> > >> > On 2015/02/27 at 05:17:22, weiliangc wrote:
> > >> > > There are 3 failures:
> > >> > >
> > >> > > - compositor unittests:
> > >> > >   - because impl side painting use picture layer instead of content
> > >> layer,
> > >>
> > > the
> > >
> > >> > scale and paint_size is different in layer unittests
> > >> > >
> > >> > > - content browser test
> > >> > >   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current
> > >>
> > > number
> > >
> > >> > of threads is 1 not 0
> > >> >
> > >> > Where is this SetNumWorkerThreads() call done from? It needs to happen
> > >>
> > > before we
> > >
> > >> > create any compositor instances..
> > >>
> > >
> > >  (with codesearch) Only callsite I can find is at RenderThreadImpl
> > >>
> > > https://code.google.com/p/chromium/codesearch#chromium/
> > > src/content/renderer/render_thread_impl.cc&q=setnumworkerThreads&sq=
> > > package:chromium&type=cs&l=672
> > >
> > >
> > > Ok, that's not supposed to affect the browser compositor. Let me guess,
> > > this is
> > > only happening in single process mode? I think we need to make that
> > > SetNumWorkerThreads call conditional on single-process mode and call it
> > > somewhere else in that case.
> > >
> > >  >
> > >> > >   - My guess is GetNumWorkerThreads() is called before this, which
set
> > >>
> > > number
> > >
> > >> > of threads to default, 1
> > >> > >   - Either from PicturePile's create or TaskGraphRunner()
> > >> > >
> > >> > > - view unittests
> > >> > >   - cull_set size doesn't change. Need to update Paint function for
> > >> View?
> > >>
> > >
> > >
> > > https://codereview.chromium.org/962833003/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[reveman, 2015-02-27 17:34:15]

https://codereview.chromium.org/962833003/diff/130001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/962833003/diff/130001/content/renderer/render...
content/renderer/render_thread_impl.cc:583: use_image_texture_target_ = target;
Btw, we need to make sure LayerTreeSettings::use_image_texture_target is set
correctly for the browser compositor. The logic that determines what the value
of that field needs to be can be found here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

I guess we need to move some of that code to compositor_util.cc..

https://codereview.chromium.org/962833003/diff/130001/content/renderer/render...
content/renderer/render_thread_impl.cc:673: if
(!command_line.HasSwitch(switches::kSingleProcess))
We need to call SetNumWorkerThreads in the browser process prior to creating the
browser compositor. And I think it would be nice with a comment making it clear
that this is not needed in single process mode because we've already set it for
the browser compositor.

[danakj, 2015-02-27 17:43:24]

On Fri, Feb 27, 2015 at 9:11 AM, <weiliangc@chromium.org> wrote:

> this? https://codereview.chromium.org/801973002
>
> You mean pass in whether browser compositor is impl-side-painting? or pass
> in
> single-thread?


Oh I think I'm mistaken, we're just missing the call in the browser as
david says.


>
>
> On 2015/02/27 16:57:02, danakj wrote:
>
>> The test needs to pass a command line in probably so the
>> compositordependencies are right. There are some render widget tests I had
>> to do this for also if you look thru git log in content/renderer. Or I can
>> find it later.
>> On Feb 27, 2015 7:54 AM, <mailto:reveman@chromium.org> wrote:
>>
>
>  > On 2015/02/27 at 15:45:10, weiliangc wrote:
>> >
>> >> On 2015/02/27 15:39:47, reveman wrote:
>> >> > On 2015/02/27 at 05:17:22, weiliangc wrote:
>> >> > > There are 3 failures:
>> >> > >
>> >> > > - compositor unittests:
>> >> > >   - because impl side painting use picture layer instead of content
>> >> layer,
>> >>
>> > the
>> >
>> >> > scale and paint_size is different in layer unittests
>> >> > >
>> >> > > - content browser test
>> >> > >   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because
>> current
>> >>
>> > number
>> >
>> >> > of threads is 1 not 0
>> >> >
>> >> > Where is this SetNumWorkerThreads() call done from? It needs to
>> happen
>> >>
>> > before we
>> >
>> >> > create any compositor instances..
>> >>
>> >
>> >  (with codesearch) Only callsite I can find is at RenderThreadImpl
>> >>
>> > https://code.google.com/p/chromium/codesearch#chromium/
>> > src/content/renderer/render_thread_impl.cc&q=setnumworkerThreads&sq=
>> > package:chromium&type=cs&l=672
>> >
>> >
>> > Ok, that's not supposed to affect the browser compositor. Let me guess,
>> > this is
>> > only happening in single process mode? I think we need to make that
>> > SetNumWorkerThreads call conditional on single-process mode and call it
>> > somewhere else in that case.
>> >
>> >  >
>> >> > >   - My guess is GetNumWorkerThreads() is called before this, which
>> set
>> >>
>> > number
>> >
>> >> > of threads to default, 1
>> >> > >   - Either from PicturePile's create or TaskGraphRunner()
>> >> > >
>> >> > > - view unittests
>> >> > >   - cull_set size doesn't change. Need to update Paint function for
>> >> View?
>> >>
>> >
>> >
>> > https://codereview.chromium.org/962833003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/962833003/
>

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

[danakj, 2015-02-27 17:47:30]

On Thu, Feb 26, 2015 at 9:17 PM, <weiliangc@chromium.org> wrote:

> There are 3 failures:
>
> - compositor unittests:
>   - because impl side painting use picture layer instead of content layer,
> the
> scale and paint_size is different in layer unittests
>

if we want to test scaling, before: test content bounds/scale on main
"thread". after: test the high res tiling on the impl "thread".


>
> - content browser test
>   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current
> number of
> threads is 1 not 0
>   - My guess is GetNumWorkerThreads() is called before this, which set
> number of
> threads to default, 1
>   - Either from PicturePile's create or TaskGraphRunner()
>
> - view unittests
>   - cull_set size doesn't change. Need to update Paint function for View?
>
>
>
> https://codereview.chromium.org/962833003/
>

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

[weiliangc, 2015-02-27 17:51:32]

On 2015/02/27 17:47:30, danakj wrote:
> On Thu, Feb 26, 2015 at 9:17 PM, <mailto:weiliangc@chromium.org> wrote:
> 
> > There are 3 failures:
> >
> > - compositor unittests:
> >   - because impl side painting use picture layer instead of content layer,
> > the
> > scale and paint_size is different in layer unittests
> >
> 
> if we want to test scaling, before: test content bounds/scale on main
> "thread". after: test the high res tiling on the impl "thread".
> 
If ui::Layer is PictureLayer, shouldn't scale/content bounds already tested?
> 
> >
> > - content browser test
> >   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current
> > number of
> > threads is 1 not 0
> >   - My guess is GetNumWorkerThreads() is called before this, which set
> > number of
> > threads to default, 1
> >   - Either from PicturePile's create or TaskGraphRunner()
> >
> > - view unittests
> >   - cull_set size doesn't change. Need to update Paint function for View?
> >
> >
> >
> > https://codereview.chromium.org/962833003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-02-27 17:52:52]

On Fri, Feb 27, 2015 at 9:51 AM, <weiliangc@chromium.org> wrote:

> On 2015/02/27 17:47:30, danakj wrote:
>
>> On Thu, Feb 26, 2015 at 9:17 PM, <mailto:weiliangc@chromium.org> wrote:
>>
>
>  > There are 3 failures:
>> >
>> > - compositor unittests:
>> >   - because impl side painting use picture layer instead of content
>> layer,
>> > the
>> > scale and paint_size is different in layer unittests
>> >
>>
>
>  if we want to test scaling, before: test content bounds/scale on main
>> "thread". after: test the high res tiling on the impl "thread".
>>
>
> If ui::Layer is PictureLayer, shouldn't scale/content bounds already
> tested?


Compositor tests are testing the interaction between ui::Layer and
cc::PictureLayer. Making sure ui::Layer sets the right things etc.


>
>
>  >
>> > - content browser test
>> >   - TileTaskWorkerPool::SetNumWorkerThreads() DCHECKs because current
>> > number of
>> > threads is 1 not 0
>> >   - My guess is GetNumWorkerThreads() is called before this, which set
>> > number of
>> > threads to default, 1
>> >   - Either from PicturePile's create or TaskGraphRunner()
>> >
>> > - view unittests
>> >   - cull_set size doesn't change. Need to update Paint function for
>> View?
>> >
>> >
>> >
>> > https://codereview.chromium.org/962833003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/962833003/
>

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

[enne (OOO), 2015-03-02 21:59:26]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2015-03-02 21:59:27]

https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_del...
File ui/compositor/layer_delegate.h (right):

https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_del...
ui/compositor/layer_delegate.h:22: // clipped to the Layer's invalid rect. In
Impl-Side Painting, canvas's rect
I think this comment change leaks implementation details of cc which may no
longer be true if ui switches to display lists.  I think you could just leave
it.

https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (left):

https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(),
l1->bounds().size());
You could land these test fixes now if you wanted, as they're testing
assumptions that are not true in general about cc.

[enne (OOO), 2015-03-02 22:01:27]

https://codereview.chromium.org/962833003/diff/190001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/962833003/diff/190001/content/renderer/render...
content/renderer/render_thread_impl.cc:676: if
(!command_line.HasSwitch(switches::kSingleProcess))
danakj also suggests via proxy enne that this could be landed separately ahead
of time too.

That keeps the "turn it on" patch really simple.

[weiliangc, 2015-03-02 22:37:22]

https://codereview.chromium.org/962833003/diff/190001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/962833003/diff/190001/content/renderer/render...
content/renderer/render_thread_impl.cc:676: if
(!command_line.HasSwitch(switches::kSingleProcess))
On 2015/03/02 at 22:01:27, enne wrote:
> danakj also suggests via proxy enne that this could be landed separately ahead
of time too.
> 
> That keeps the "turn it on" patch really simple.

https://codereview.chromium.org/971943002/

https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_del...
File ui/compositor/layer_delegate.h (right):

https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_del...
ui/compositor/layer_delegate.h:22: // clipped to the Layer's invalid rect. In
Impl-Side Painting, canvas's rect
On 2015/03/02 at 21:59:27, enne wrote:
> I think this comment change leaks implementation details of cc which may no
longer be true if ui switches to display lists.  I think you could just leave
it.

Done.

https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (left):

https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(),
l1->bounds().size());
On 2015/03/02 at 21:59:27, enne wrote:
> You could land these test fixes now if you wanted, as they're testing
assumptions that are not true in general about cc.

https://codereview.chromium.org/974603002/

[weiliangc, 2015-04-02 21:56:03]

On 2015/03/02 at 22:37:22, weiliangc wrote:
>
https://codereview.chromium.org/962833003/diff/190001/content/renderer/render...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/962833003/diff/190001/content/renderer/render...
> content/renderer/render_thread_impl.cc:676: if
(!command_line.HasSwitch(switches::kSingleProcess))
> On 2015/03/02 at 22:01:27, enne wrote:
> > danakj also suggests via proxy enne that this could be landed separately
ahead of time too.
> > 
> > That keeps the "turn it on" patch really simple.
> 
> https://codereview.chromium.org/971943002/
> 
>
https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_del...
> File ui/compositor/layer_delegate.h (right):
> 
>
https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_del...
> ui/compositor/layer_delegate.h:22: // clipped to the Layer's invalid rect. In
Impl-Side Painting, canvas's rect
> On 2015/03/02 at 21:59:27, enne wrote:
> > I think this comment change leaks implementation details of cc which may no
longer be true if ui switches to display lists.  I think you could just leave
it.
> 
> Done.
> 
>
https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_uni...
> File ui/compositor/layer_unittest.cc (left):
> 
>
https://codereview.chromium.org/962833003/diff/190001/ui/compositor/layer_uni...
> ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(),
l1->bounds().size());
> On 2015/03/02 at 21:59:27, enne wrote:
> > You could land these test fixes now if you wanted, as they're testing
assumptions that are not true in general about cc.
> 
> https://codereview.chromium.org/974603002/

Rebased.

Now failes on CompositorTest.LocksTimeOut
[27457:27458:0402/175011:1842394163757:FATAL:test_simple_task_runner.cc(21)]
Check failed: thread_checker_.CalledOnValidThread().
#0 0x7f9e2d28cd2e base::debug::StackTrace::StackTrace()
#1 0x7f9e2d2defbf logging::LogMessage::~LogMessage()
#2 0x000000482105 base::TestSimpleTaskRunner::PostDelayedTask()
#3 0x7f9e2d3b29d8 base::TaskRunner::PostTask()
#4 0x7f9e2e406e83 cc::(anonymous
namespace)::TaskSetFinishedTaskImpl::TaskSetFinished()
#5 0x7f9e2e406d43 cc::(anonymous
namespace)::TaskSetFinishedTaskImpl::RunOnWorkerThread()
#6 0x7f9e2e3c3394 cc::TaskGraphRunner::RunTaskWithLockAcquired()
#7 0x7f9e2e3c2e72 cc::TaskGraphRunner::Run()
#8 0x000000698395 cc::TestTaskGraphRunner::Run()
#9 0x0000006983bc cc::TestTaskGraphRunner::Run()
#10 0x7f9e2d3cceb3 base::DelegateSimpleThread::Run()
#11 0x7f9e2d3ccca9 base::SimpleThread::ThreadMain()
#12 0x7f9e2d3bde96 base::(anonymous namespace)::ThreadFunc()
#13 0x7f9e2b4cc182 start_thread
#14 0x7f9e2b1f947d clone

Looks happening in TileTaskRunner.Shutdown()

[danakj, 2015-04-02 21:58:13]

Who's thread_checker_ is that? What thread is is expecting? What's it
getting?

On Thu, Apr 2, 2015 at 2:56 PM, <weiliangc@chromium.org> wrote:

> On 2015/03/02 at 22:37:22, weiliangc wrote:
>
> https://codereview.chromium.org/962833003/diff/190001/
> content/renderer/render_thread_impl.cc
>
>> File content/renderer/render_thread_impl.cc (right):
>>
>
>
> https://codereview.chromium.org/962833003/diff/190001/
> content/renderer/render_thread_impl.cc#newcode676
>
>> content/renderer/render_thread_impl.cc:676: if
>>
> (!command_line.HasSwitch(switches::kSingleProcess))
>
>> On 2015/03/02 at 22:01:27, enne wrote:
>> > danakj also suggests via proxy enne that this could be landed separately
>>
> ahead of time too.
>
>> >
>> > That keeps the "turn it on" patch really simple.
>>
>
>  https://codereview.chromium.org/971943002/
>>
>
>
> https://codereview.chromium.org/962833003/diff/190001/ui/
> compositor/layer_delegate.h
>
>> File ui/compositor/layer_delegate.h (right):
>>
>
>
> https://codereview.chromium.org/962833003/diff/190001/ui/
> compositor/layer_delegate.h#newcode22
>
>> ui/compositor/layer_delegate.h:22: // clipped to the Layer's invalid
>> rect. In
>>
> Impl-Side Painting, canvas's rect
>
>> On 2015/03/02 at 21:59:27, enne wrote:
>> > I think this comment change leaks implementation details of cc which
>> may no
>>
> longer be true if ui switches to display lists.  I think you could just
> leave
> it.
>
>  Done.
>>
>
>
> https://codereview.chromium.org/962833003/diff/190001/ui/
> compositor/layer_unittest.cc
>
>> File ui/compositor/layer_unittest.cc (left):
>>
>
>
> https://codereview.chromium.org/962833003/diff/190001/ui/
> compositor/layer_unittest.cc#oldcode527
>
>> ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(),
>>
> l1->bounds().size());
>
>> On 2015/03/02 at 21:59:27, enne wrote:
>> > You could land these test fixes now if you wanted, as they're testing
>>
> assumptions that are not true in general about cc.
>
>  https://codereview.chromium.org/974603002/
>>
>
> Rebased.
>
> Now failes on CompositorTest.LocksTimeOut
> [27457:27458:0402/175011:1842394163757:FATAL:test_
> simple_task_runner.cc(21)]
> Check failed: thread_checker_.CalledOnValidThread().
> #0 0x7f9e2d28cd2e base::debug::StackTrace::StackTrace()
> #1 0x7f9e2d2defbf logging::LogMessage::~LogMessage()
> #2 0x000000482105 base::TestSimpleTaskRunner::PostDelayedTask()
> #3 0x7f9e2d3b29d8 base::TaskRunner::PostTask()
> #4 0x7f9e2e406e83 cc::(anonymous
> namespace)::TaskSetFinishedTaskImpl::TaskSetFinished()
> #5 0x7f9e2e406d43 cc::(anonymous
> namespace)::TaskSetFinishedTaskImpl::RunOnWorkerThread()
> #6 0x7f9e2e3c3394 cc::TaskGraphRunner::RunTaskWithLockAcquired()
> #7 0x7f9e2e3c2e72 cc::TaskGraphRunner::Run()
> #8 0x000000698395 cc::TestTaskGraphRunner::Run()
> #9 0x0000006983bc cc::TestTaskGraphRunner::Run()
> #10 0x7f9e2d3cceb3 base::DelegateSimpleThread::Run()
> #11 0x7f9e2d3ccca9 base::SimpleThread::ThreadMain()
> #12 0x7f9e2d3bde96 base::(anonymous namespace)::ThreadFunc()
> #13 0x7f9e2b4cc182 start_thread
> #14 0x7f9e2b1f947d clone
>
> Looks happening in TileTaskRunner.Shutdown()
>
> https://codereview.chromium.org/962833003/
>

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

[danakj, 2015-04-20 23:46:19]

LGTM let's land this and revert by friday just to see what breaks (if anything).
We can land and stick this once perf regressions are resolved.

Can you mention this plan in the description as well?

Thanks!

[enne (OOO), 2015-04-20 23:49:45]

lgtm as well, although it's a bit odd to change layer_unittest in this patch.

[weiliangc, 2015-04-21 03:23:59]

On 2015/04/20 23:49:45, enne wrote:
> lgtm as well, although it's a bit odd to change layer_unittest in this patch.

layer_unittest is probably added on when I did merge for rebase. Removed.

[weiliangc, 2015-04-21 03:26:08]

Currently there are perf regression for ui impl side painting. That is an known
issue.

The plan is to temporarily turn this on, flush out any bugs, and revert on
Friday.

Will only permanently turn on after perf regression is addressed.
(crbug.com/466426)

[weiliangc, 2015-04-21 03:26:15]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-04-21 03:26:15]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/962833003/#ps290001
(title: "rm layer unittest change")

[commit-bot: I haz the power, 2015-04-21 03:26:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962833003/290001

[commit-bot: I haz the power, 2015-04-21 05:38:25]

Committed patchset #12 (id:290001)

[commit-bot: I haz the power, 2015-04-21 05:39:39]

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

[Alexander Potapenko, 2015-04-21 13:59:27]

A revert of this CL (patchset #12 id:290001) has been created in
https://codereview.chromium.org/1092933005/ by glider@chromium.org.

The reason for reverting is: I suspect this CL broke the MSan bots, see
https://crbug.com/479133
Will send a tryjob to check that..

[weiliangc, 2015-06-05 04:36:28]

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

[commit-bot: I haz the power, 2015-06-05 04:37:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962833003/290001

[commit-bot: I haz the power, 2015-06-05 04:40:35]

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

[commit-bot: I haz the power, 2015-06-05 04:40:37]

Dry run: 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_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[weiliangc, 2015-06-05 15:45:52]

Re-landing.

(Last attempt to re-land is at https://codereview.chromium.org/1160093005/, the
try bot failure seem to be fixed with
https://codereview.chromium.org/1166983007/ which is committed)

[weiliangc, 2015-06-05 15:45:58]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-06-05 15:45:59]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/962833003/#ps310001
(title: "rebase will try reland")

[commit-bot: I haz the power, 2015-06-05 15:47:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962833003/310001

[commit-bot: I haz the power, 2015-06-05 16:47:13]

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

[commit-bot: I haz the power, 2015-06-05 16:47:15]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[weiliangc, 2015-06-09 23:59:14]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-06-09 23:59:14]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/962833003/#ps430001
(title: "this is for checking in")

[commit-bot: I haz the power, 2015-06-10 00:01:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962833003/430001

[danakj, 2015-06-10 01:04:23]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2015-06-10 01:04:25]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-06-10 01:09:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962833003/430001

[commit-bot: I haz the power, 2015-06-10 02:18:19]

Committed patchset #19 (id:430001)

[commit-bot: I haz the power, 2015-06-10 02:19:13]

Patchset 19 (id:??) landed as
https://crrev.com/3be4ab2d48dad92483a369cdaf44ee164efd1558
Cr-Commit-Position: refs/heads/master@{#333656}