Fixing crash in PictureLayerImpl::MarkVisibleResourcesAsRequired when low-res tiles are disabled

Fixing crash in PictureLayerImpl::MarkVisibleResourcesAsRequired when low-res tiles are disabled and adjusting unit tests for it.

Disabling low resolution tiling was added here:
https://codereview.chromium.org/196473007/
However, if we just add switch --disable-low-res-tiling,
right now it will cause crash (null pointer) in
PictureLayerImpl::MarkVisibleResourcesAsRequired. Also
adding some unittests for disabled low res tiles, including
the ones, that would fail without fix in
PictureLayerImpl::MarkVisibleResourcesAsRequired:
NoLowResPictureLayerImplTest.NothingRequiredIfAllHighResTilesShared
and NoLowResPictureLayerImplTest.NothingRequiredIfActiveMissingTiles.

BUG=none

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

[Sergey, 2014-05-02 01:29:25]

Hello. Tried to disable low resolution tiles, but right now it makes
cc_unittests fail a lot. So, please review this as a fix.

I guess I will have to change a lot of things in this patch to make it suitable,
as it is my first contribution here, so please, hang in there and be patient :)

I am not sure, that all the formatting and codding approaches are correct, so
your comments are appreciated.

Thank you.

[enne (OOO), 2014-05-02 17:03:16]

https://codereview.chromium.org/260963008/diff/1/cc/layers/picture_layer_impl...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/260963008/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:707:
host_impl_.settings().create_low_res_tiling ? 2u : 1u,
Just make the test set the settings appropriately rather than trying to handle
every case.

[Anton, 2014-05-06 14:48:26]

On 2014/05/02 01:29:25, Sergey wrote:
> Hello. Tried to disable low resolution tiles, but right now it makes
> cc_unittests fail a lot. So, please review this as a fix.
> 
> I guess I will have to change a lot of things in this patch to make it
suitable,
> as it is my first contribution here, so please, hang in there and be patient
:)
> 
> I am not sure, that all the formatting and codding approaches are correct, so
> your comments are appreciated.
> 
> Thank you.

Some people have mentioned that low res tiles were still helping on Nexus 10.
I had done my experiments on Nexus 4 and Nexus 7 where it doesn't seem to help.
Have you tried on Nexus 10 to see what difference it makes?

[Sergey, 2014-05-06 23:36:08]

On 2014/05/02 17:03:16, enne wrote:
>
https://codereview.chromium.org/260963008/diff/1/cc/layers/picture_layer_impl...
> File cc/layers/picture_layer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/260963008/diff/1/cc/layers/picture_layer_impl...
> cc/layers/picture_layer_impl_unittest.cc:707:
> host_impl_.settings().create_low_res_tiling ? 2u : 1u,
> Just make the test set the settings appropriately rather than trying to handle
> every case.

Hi Enne,

I thought of doing just that, but in that case, I guess, we will have to double
the amount of test cases: make the same tests with different
create_low_res_tiling setting. And if we use this approach it will make
exponential growth of test cases from every added new setting. Is it fine,
should it be like that? Or am I wrong, assuming that almost all the test cases
should be doubled?

As for the adjusting test cases for the setting, that is not yet set: this
particular one could end-up being different for Android and PC. Thus, it would
be nice, if unit tests would not fail, no matter what default setting is.

Best regards,
 Sergey.

[enne (OOO), 2014-05-06 23:42:36]

Most of those tests are not testing whether or not you are creating low res
tilings, they are testing other things.  I would recommend that these tests
should probably continue testing low res tilings, as some of them depend on it. 
You could write a few new tests that make sure that low res tilings are and are
not created depending on the setting, rather than doubling all tests.  As you
say, these unit tests should behave the same regardless of platform.

[Sergey, 2014-05-06 23:47:05]

Ok, thanks, I will adjust the patch.

[Sergey, 2014-05-07 00:03:39]

> Some people have mentioned that low res tiles were still helping on Nexus 10.
> I had done my experiments on Nexus 4 and Nexus 7 where it doesn't seem to
help.
> Have you tried on Nexus 10 to see what difference it makes?

Ops.. The change in layer_tree_settings.cc was not intentional :(
Included that one by mistake right now -- there should be more investigation
before actual disabling, of course. Will fix in the next patch.

[Sergey, 2014-05-07 02:34:25]

Ok, adjusted to comments. Please, re-review :)

https://codereview.chromium.org/260963008/diff/1/cc/layers/picture_layer_impl...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/260963008/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:707:
host_impl_.settings().create_low_res_tiling ? 2u : 1u,
On 2014/05/02 17:03:16, enne wrote:
> Just make the test set the settings appropriately rather than trying to handle
> every case.

Done.

[Sergey, 2014-05-12 01:10:33]

Dear all, this is a kind reminder.

Thank you.

On 2014/05/07 02:34:25, Sergey wrote:
> Ok, adjusted to comments. Please, re-review :)

[reveman, 2014-05-12 02:30:52]

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:793: if (ShouldHaveLowResTiling()) {
why this change? is this broken before this change?

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
File cc/test/fake_layer_tree_settings.h (right):

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
cc/test/fake_layer_tree_settings.h:12: class FakeLayerTreeSettings : public
LayerTreeSettings {
why not change ImplSidePaintingSettings instead of adding a new class?

[Sergey, 2014-05-12 08:07:21]

Thank you for the comments, made a reply.

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:793: if (ShouldHaveLowResTiling()) {
On 2014/05/12 02:30:52, reveman wrote:
> why this change? is this broken before this change?

If you set create_low_res_tiling=false, you will get
twin_layer_->tilings_->num_tilings() = 1. Thus, the original condition will turn
to false and there would not be a try to get high resolution tiling from twin
layer. Thus, made it two cases.

Logically this change seemed correct... Is there some misunderstanding from my
side?

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
File cc/test/fake_layer_tree_settings.h (right):

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
cc/test/fake_layer_tree_settings.h:12: class FakeLayerTreeSettings : public
LayerTreeSettings {
On 2014/05/12 02:30:52, reveman wrote:
> why not change ImplSidePaintingSettings instead of adding a new class?

Judging from ImplSidePaintingSettings class name, I thought that it would not be
good to use this class to actually change some other settings in constructor. So
I thought more general purpose class name would be better, as in the future
there might be a need to add some more settings changes into the settings class
for unit testing.

If we will use ImplSidePaintingSettings we might end up writing
explanations/excuses like "the name goes from previous versions..", about a
class, that has a lot of setting changes.

[reveman, 2014-05-12 15:44:03]

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:793: if (ShouldHaveLowResTiling()) {
On 2014/05/12 08:07:21, Sergey wrote:
> On 2014/05/12 02:30:52, reveman wrote:
> > why this change? is this broken before this change?
> 
> If you set create_low_res_tiling=false, you will get
> twin_layer_->tilings_->num_tilings() = 1. Thus, the original condition will
turn
> to false and there would not be a try to get high resolution tiling from twin
> layer. Thus, made it two cases.
> 
> Logically this change seemed correct... Is there some misunderstanding from my
> side?

Probably not but if you're fixing a bug in the implementation and not just some
unit tests you might want to do that separately or at least make it clear in the
description of the patch that this is currently broken. Does the patch currently
include a test that will fail without this change?

Is there any way you can make this look less like something that was tacked on
as side effect of making low res tiling optional and more like the initial
design?

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:832: ShouldHaveLowResTiling()) {
is this required?

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
File cc/test/fake_layer_tree_settings.h (right):

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
cc/test/fake_layer_tree_settings.h:12: class FakeLayerTreeSettings : public
LayerTreeSettings {
On 2014/05/12 08:07:21, Sergey wrote:
> On 2014/05/12 02:30:52, reveman wrote:
> > why not change ImplSidePaintingSettings instead of adding a new class?
> 
> Judging from ImplSidePaintingSettings class name, I thought that it would not
be
> good to use this class to actually change some other settings in constructor.
So
> I thought more general purpose class name would be better, as in the future
> there might be a need to add some more settings changes into the settings
class
> for unit testing.
> 
> If we will use ImplSidePaintingSettings we might end up writing
> explanations/excuses like "the name goes from previous versions..", about a
> class, that has a lot of setting changes.

You should probably remove ImplSidePaintingSettings in that case. Not clear to
me why we need a separate class for this at all though. Can't we just use
LayerTreeSettings directly? I assume ImplSidePaintingSettings was just added as
a convenience until impl-side painting has become default. That makes some sense
as it's so commonly used but adding derived classes for adjusting other setting
seems overkill imo.

[Sergey, 2014-05-12 23:37:46]

Dear reveman, thanks for your comments.

It seems this patch ends up including 3 logically different changes, that
depends on each another:
1. ImplSidePaintingSettings removal - refactoring.
2. Fixing small bug in PictureLayerImpl for new create_low_res_tiling setting.
3. Adjusting PictureLayerImpl unit tests for new create_low_res_tiling setting.

Could you, please, advise, if I should split it into several patches or just
change a description of this patch?

If it is better to split, then what is the best way to do it in your opinion?

Thank you for your kind support!

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:793: if (ShouldHaveLowResTiling()) {
On 2014/05/12 15:44:04, reveman wrote:
> Probably not but if you're fixing a bug in the implementation and not just
some
> unit tests you might want to do that separately or at least make it clear in
the
> description of the patch that this is currently broken. Does the patch
currently
> include a test that will fail without this change?

There was one, let me make sure we have it... Actually, I am not sure, that we
will have a failing test, as according to the MarkVisibleTilesAsRequired
function finding twin tiling is optional (we are searching here for a pointer,
that will be passed into the MarkVisibleTilesAsRequired as optional_twin_tiling
parameter). But I guess we should try our best to actually find it.

> Is there any way you can make this look less like something that was tacked on
> as side effect of making low res tiling optional and more like the initial
> design?

Agree, let me try.

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:832: ShouldHaveLowResTiling()) {
On 2014/05/12 15:44:04, reveman wrote:
> is this required?

Actually, this one is more required, then the above :)
Without it we get into MarkVisibleTilesAsRequired with low_res = NULL and crash.

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
File cc/test/fake_layer_tree_settings.h (right):

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
cc/test/fake_layer_tree_settings.h:12: class FakeLayerTreeSettings : public
LayerTreeSettings {
On 2014/05/12 15:44:04, reveman wrote:
> On 2014/05/12 08:07:21, Sergey wrote:
> > On 2014/05/12 02:30:52, reveman wrote:
> > > why not change ImplSidePaintingSettings instead of adding a new class?
> > 
> > Judging from ImplSidePaintingSettings class name, I thought that it would
not
> be
> > good to use this class to actually change some other settings in
constructor.
> So
> > I thought more general purpose class name would be better, as in the future
> > there might be a need to add some more settings changes into the settings
> class
> > for unit testing.
> > 
> > If we will use ImplSidePaintingSettings we might end up writing
> > explanations/excuses like "the name goes from previous versions..", about a
> > class, that has a lot of setting changes.
> 
> You should probably remove ImplSidePaintingSettings in that case. Not clear to
> me why we need a separate class for this at all though. Can't we just use
> LayerTreeSettings directly? I assume ImplSidePaintingSettings was just added
as
> a convenience until impl-side painting has become default. That makes some
sense
> as it's so commonly used but adding derived classes for adjusting other
setting
> seems overkill imo.

Agree on that, let's make some refactoring :)

[Sergey, 2014-05-13 05:37:44]

I've adjusted the source code, please, look through. The only thing undecided
yet is if I should break it into several patches.

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:793: if (ShouldHaveLowResTiling()) {
On 2014/05/12 15:44:04, reveman wrote:
> Probably not but if you're fixing a bug in the implementation and not just
some
> unit tests you might want to do that separately or at least make it clear in
the
> description of the patch that this is currently broken. Does the patch
currently
> include a test that will fail without this change?
Found that test cases once again: NothingRequiredIfAllHighResTilesShared and
NothingRequiredIfActiveMissingTiles. Added them for low res disabled case.

> Is there any way you can make this look less like something that was tacked on
> as side effect of making low res tiling optional and more like the initial
> design?

Done. Please, check, if now it looks better.

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
File cc/test/fake_layer_tree_settings.h (right):

https://codereview.chromium.org/260963008/diff/40001/cc/test/fake_layer_tree_...
cc/test/fake_layer_tree_settings.h:12: class FakeLayerTreeSettings : public
LayerTreeSettings {
On 2014/05/12 15:44:04, reveman wrote:
> On 2014/05/12 08:07:21, Sergey wrote:
> > On 2014/05/12 02:30:52, reveman wrote:
> > > why not change ImplSidePaintingSettings instead of adding a new class?
> > 
> > Judging from ImplSidePaintingSettings class name, I thought that it would
not
> be
> > good to use this class to actually change some other settings in
constructor.
> So
> > I thought more general purpose class name would be better, as in the future
> > there might be a need to add some more settings changes into the settings
> class
> > for unit testing.
> > 
> > If we will use ImplSidePaintingSettings we might end up writing
> > explanations/excuses like "the name goes from previous versions..", about a
> > class, that has a lot of setting changes.
> 
> You should probably remove ImplSidePaintingSettings in that case. Not clear to
> me why we need a separate class for this at all though. Can't we just use
> LayerTreeSettings directly? I assume ImplSidePaintingSettings was just added
as
> a convenience until impl-side painting has become default. That makes some
sense
> as it's so commonly used but adding derived classes for adjusting other
setting
> seems overkill imo.

Done.

[reveman, 2014-05-13 15:38:05]

Doing all this as part of the same patch is fine with me. Just make sure you
update the description so it's clear that this is actually fixing a bug.

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:798: tilings_->num_tilings() <= 2) {
what if |should_have_low_res| is true but tilings_->num_tilings() returns 1?
that would previously cause us to not enter this clause but now it does. Is that
a potential problem?

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:801: if (twin_low_res || !should_have_low_res)
Do we need this check or could we just make it unconditional and move it above
line 799?

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:820: should_have_low_res) {
Maybe move the "should_have_low_res" check into it's own if statement below to
make it more clear that skipping the call to MarkVisibleTilesAsRequired when
should_have_low_res is false is not OK.

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:2284: :
PictureLayerImplTest(LayerTreeSettings(true, false))
You could just add a NoLowResTilingsSettings class to this file instead as this
is the only place you need to set create_low_res_tiling to false.

https://codereview.chromium.org/260963008/diff/60001/cc/test/impl_side_painti...
File cc/test/impl_side_painting_settings.h (left):

https://codereview.chromium.org/260963008/diff/60001/cc/test/impl_side_painti...
cc/test/impl_side_painting_settings.h:12: class ImplSidePaintingSettings :
public LayerTreeSettings {
I think it's fine to keep this for now as it's used by multiple unit tests and
just remove it when the impl_side_painting setting is gone.

https://codereview.chromium.org/260963008/diff/60001/cc/trees/layer_tree_sett...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/260963008/diff/60001/cc/trees/layer_tree_sett...
cc/trees/layer_tree_settings.h:21: );
Not sure we should do this. Why should these settings be singled out and
adjustable using the ctor? If we add everything, then that would result in
cryptic calls to this ctor. I think it's better to keep the default ctor not
take any arguments and instead set them for each test case. Possible create some
derived class such as ImplSidePaintingSettings where convenient but keep those
in an anonymous namespace when possible.

[Sergey, 2014-05-14 05:53:35]

I've tried to adjust everything to the "commit candidate" level. Please, review.

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:798: tilings_->num_tilings() <= 2) {
On 2014/05/13 15:38:05, reveman wrote:
> what if |should_have_low_res| is true but tilings_->num_tilings() returns 1?
> that would previously cause us to not enter this clause but now it does. Is
that
> a potential problem?

If |should_have_low_res| is true and tilings_->num_tilings() = 1, then we have
low_res = NULL and condition (low_res || !should_have_low_res) = false. Thus, we
would not enter this block, as before.

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:801: if (twin_low_res || !should_have_low_res)
On 2014/05/13 15:38:05, reveman wrote:
> Do we need this check or could we just make it unconditional and move it above
> line 799?

For some reason before we only executed this if we have found twin tiling for
low res tiles. I would like to leave the logic intact, when we should have low
res. And when we are not supposed to have low res still try to find twin tiles
for high res. Thus this condition.

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:820: should_have_low_res) {
On 2014/05/13 15:38:05, reveman wrote:
> Maybe move the "should_have_low_res" check into it's own if statement below to
> make it more clear that skipping the call to MarkVisibleTilesAsRequired when
> should_have_low_res is false is not OK.

Done.

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:2284: :
PictureLayerImplTest(LayerTreeSettings(true, false))
On 2014/05/13 15:38:05, reveman wrote:
> You could just add a NoLowResTilingsSettings class to this file instead as
this
> is the only place you need to set create_low_res_tiling to false. 

Done.

https://codereview.chromium.org/260963008/diff/60001/cc/test/impl_side_painti...
File cc/test/impl_side_painting_settings.h (left):

https://codereview.chromium.org/260963008/diff/60001/cc/test/impl_side_painti...
cc/test/impl_side_painting_settings.h:12: class ImplSidePaintingSettings :
public LayerTreeSettings {
On 2014/05/13 15:38:05, reveman wrote:
> I think it's fine to keep this for now as it's used by multiple unit tests and
> just remove it when the impl_side_painting setting is gone.

Done.

https://codereview.chromium.org/260963008/diff/60001/cc/trees/layer_tree_sett...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/260963008/diff/60001/cc/trees/layer_tree_sett...
cc/trees/layer_tree_settings.h:21: );
On 2014/05/13 15:38:05, reveman wrote:
> Not sure we should do this. Why should these settings be singled out and
> adjustable using the ctor? If we add everything, then that would result in
> cryptic calls to this ctor. I think it's better to keep the default ctor not
> take any arguments and instead set them for each test case. Possible create
some
> derived class such as ImplSidePaintingSettings where convenient but keep those
> in an anonymous namespace when possible.

Done.

[reveman, 2014-05-14 15:13:49]

Please add a single line to the top of the description that describes the change
as much as a single line can do and reformat the rest of the description to wrap
at ~74 columns. Just look at other changes in cc/ to see what the current
convention is.

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:798: tilings_->num_tilings() <= 2) {
On 2014/05/14 05:53:35, Sergey wrote:
> On 2014/05/13 15:38:05, reveman wrote:
> > what if |should_have_low_res| is true but tilings_->num_tilings() returns 1?
> > that would previously cause us to not enter this clause but now it does. Is
> that
> > a potential problem?
> 
> If |should_have_low_res| is true and tilings_->num_tilings() = 1, then we have
> low_res = NULL and condition (low_res || !should_have_low_res) = false. Thus,
we
> would not enter this block, as before.

Could be low_res != NULL if high_res = NULL but we would still not enter the
block so I guess this looks good.

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:801: if (twin_low_res || !should_have_low_res)
On 2014/05/14 05:53:35, Sergey wrote:
> On 2014/05/13 15:38:05, reveman wrote:
> > Do we need this check or could we just make it unconditional and move it
above
> > line 799?
> 
> For some reason before we only executed this if we have found twin tiling for
> low res tiles. I would like to leave the logic intact, when we should have low
> res. And when we are not supposed to have low res still try to find twin tiles
> for high res. Thus this condition.

Fine to keep existing logic if you prefer but cleaning it up by removing this
conditional if not necessary would be nice as a follow up.

https://codereview.chromium.org/260963008/diff/70001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/260963008/diff/70001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:2283: {
hm, think '{' should be on previous line. did you run this through cl format?

[Sergey, 2014-05-15 01:08:04]

Fixed according to comments, please review.

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:801: if (twin_low_res || !should_have_low_res)
On 2014/05/14 15:13:49, reveman wrote:
> Fine to keep existing logic if you prefer but cleaning it up by removing this
> conditional if not necessary would be nice as a follow up.

Ok, let it be my next patch :) And I would like to do some research on it too.

https://codereview.chromium.org/260963008/diff/70001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/260963008/diff/70001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:2283: {
On 2014/05/14 15:13:49, reveman wrote:
> hm, think '{' should be on previous line. did you run this through cl format?

Ops, that's true. Some things after cl format start look not like in all the
other file, but I guess that's how it should be... Please, check new patch.

[reveman, 2014-05-15 04:43:52]

lgtm % enne's review

[enne (OOO), 2014-05-15 17:40:05]

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:796: if (high_res && (low_res ||
!should_have_low_res) && twin_layer_ &&
This conditional and the one in the following block are very confusing to read
now.  Can you break this into more readable subexpressions?

[Sergey, 2014-05-16 02:26:12]

Dear enne, please re-review.

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:808: if (!twin_high_res || (!twin_low_res &&
should_have_low_res) ||
@enne, do you mean this one? Unfortunately, the only idea I have how to break is
the on in patch set 1. But I think this one is actually better and is
readable... No?

It is essentially the same, as before the patch, except changing the part "or we
didn't get twin low res" to "or we didn't get twin low res, while we should".

https://codereview.chromium.org/260963008/diff/110001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/110001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:798: high_res && !should_have_low_res &&
num_tilings == 1) &&
How about this one, did it get any better?... Or please see what it was in patch
set 1.

[reveman, 2014-05-16 19:17:56]

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:796: if (high_res && (low_res ||
!should_have_low_res) && twin_layer_ &&
On 2014/05/15 17:40:06, enne wrote:
> This conditional and the one in the following block are very confusing to read
> now.  Can you break this into more readable subexpressions?

Are all these checks here necessary or just an optimization? What if we just
made this something like:

if (twin_layer_ &&
    tilings_->num_tilings() <= 2 &&
    tilings_->num_tilings() == twin_layer_->tilings_->num_tilings()) {
  twin_low_res = low_res ? GetTwinTiling(low_res) : NULL;
  twin_high_res = high_res ? GetTwinTiling(high_res) : NULL;
}

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:808: if (!twin_high_res || (!twin_low_res &&
should_have_low_res) ||
On 2014/05/16 02:26:12, Sergey wrote:
> @enne, do you mean this one? Unfortunately, the only idea I have how to break
is
> the on in patch set 1. But I think this one is actually better and is
> readable... No?
> 
> It is essentially the same, as before the patch, except changing the part "or
we
> didn't get twin low res" to "or we didn't get twin low res, while we should".

Maybe something like this would help:

bool missing_twin_low_res = ShouldHaveLowResTiling() && !twin_low_res;
bool missing_twin_high_res = twin_high_res;

Btw, why are we checking "!twin_high_res || !twin_low_res" here at all? Do they
both have to be set to NULL if one of them is NULL? Or is this just an
optimization to avoid comparing transforms?

[enne (OOO), 2014-05-16 20:46:06]

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:808: if (!twin_high_res || (!twin_low_res &&
should_have_low_res) ||
On 2014/05/16 19:17:57, reveman wrote:
> On 2014/05/16 02:26:12, Sergey wrote:
> > @enne, do you mean this one? Unfortunately, the only idea I have how to
break
> is
> > the on in patch set 1. But I think this one is actually better and is
> > readable... No?
> > 
> > It is essentially the same, as before the patch, except changing the part
"or
> we
> > didn't get twin low res" to "or we didn't get twin low res, while we
should".
> 
> Maybe something like this would help:
> 
> bool missing_twin_low_res = ShouldHaveLowResTiling() && !twin_low_res;
> bool missing_twin_high_res = twin_high_res;
> 
> Btw, why are we checking "!twin_high_res || !twin_low_res" here at all? Do
they
> both have to be set to NULL if one of them is NULL? Or is this just an
> optimization to avoid comparing transforms?

Assuming that you only do this in the case that a layer has a twin and they each
have either a high res or both a high and low res, then you have four cases for
this optimization.

(1) Both layers only have high res: ok
(2) Both layers have high res and low res: ok
(3) Pending layer has both, active layer has only high res: ok (but no need for
a second pass, previously not handled as a simplification)
(4) Pending layer has only high res, active layer has both: not ok (this is not
handled in this patch correctly)

#4 is not ok because the active layer could be displaying low res tiles, and if
you are not able to mark low res tiles as required, you could flash from low res
to missing.

I think this logic would make more sense if the ShouldHaveLowResTiling() was not
consulted and you just allowed the first 3 cases above.

[reveman, 2014-05-16 21:24:30]

On 2014/05/16 20:46:06, enne wrote:
>
https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
> File cc/layers/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
> cc/layers/picture_layer_impl.cc:808: if (!twin_high_res || (!twin_low_res &&
> should_have_low_res) ||
> On 2014/05/16 19:17:57, reveman wrote:
> > On 2014/05/16 02:26:12, Sergey wrote:
> > > @enne, do you mean this one? Unfortunately, the only idea I have how to
> break
> > is
> > > the on in patch set 1. But I think this one is actually better and is
> > > readable... No?
> > > 
> > > It is essentially the same, as before the patch, except changing the part
> "or
> > we
> > > didn't get twin low res" to "or we didn't get twin low res, while we
> should".
> > 
> > Maybe something like this would help:
> > 
> > bool missing_twin_low_res = ShouldHaveLowResTiling() && !twin_low_res;
> > bool missing_twin_high_res = twin_high_res;
> > 
> > Btw, why are we checking "!twin_high_res || !twin_low_res" here at all? Do
> they
> > both have to be set to NULL if one of them is NULL? Or is this just an
> > optimization to avoid comparing transforms?
> 
> Assuming that you only do this in the case that a layer has a twin and they
each
> have either a high res or both a high and low res, then you have four cases
for
> this optimization.
> 
> (1) Both layers only have high res: ok
> (2) Both layers have high res and low res: ok
> (3) Pending layer has both, active layer has only high res: ok (but no need
for
> a second pass, previously not handled as a simplification)
> (4) Pending layer has only high res, active layer has both: not ok (this is
not
> handled in this patch correctly)
> 
> #4 is not ok because the active layer could be displaying low res tiles, and
if
> you are not able to mark low res tiles as required, you could flash from low
res
> to missing.
> 
> I think this logic would make more sense if the ShouldHaveLowResTiling() was
not
> consulted and you just allowed the first 3 cases above.

Thanks for explaining. This makes sense. Would be great if the code could be
structured similar to the 4 statements above.

[Sergey, 2014-05-19 08:18:27]

Please, recheck. All cc_unittests are passed. Tested a bit in android chrome
shell -- could not see any degradation right away.

Please, let me know what other tests I can run to check, if everything is fine.

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:796: if (high_res && (low_res ||
!should_have_low_res) && twin_layer_ &&
On 2014/05/16 19:17:57, reveman wrote:
> On 2014/05/15 17:40:06, enne wrote:
> > This conditional and the one in the following block are very confusing to
read
> > now.  Can you break this into more readable subexpressions?
> 
> Are all these checks here necessary or just an optimization? What if we just
> made this something like:
> 
> if (twin_layer_ &&
>     tilings_->num_tilings() <= 2 &&
>     tilings_->num_tilings() == twin_layer_->tilings_->num_tilings()) {
>   twin_low_res = low_res ? GetTwinTiling(low_res) : NULL;
>   twin_high_res = high_res ? GetTwinTiling(high_res) : NULL;
> }

Done.

https://codereview.chromium.org/260963008/diff/90001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:808: if (!twin_high_res || (!twin_low_res &&
should_have_low_res) ||
On 2014/05/16 20:46:07, enne wrote:
> On 2014/05/16 19:17:57, reveman wrote:
> > On 2014/05/16 02:26:12, Sergey wrote:
> > > @enne, do you mean this one? Unfortunately, the only idea I have how to
> break
> > is
> > > the on in patch set 1. But I think this one is actually better and is
> > > readable... No?
> > > 
> > > It is essentially the same, as before the patch, except changing the part
> "or
> > we
> > > didn't get twin low res" to "or we didn't get twin low res, while we
> should".
> > 
> > Maybe something like this would help:
> > 
> > bool missing_twin_low_res = ShouldHaveLowResTiling() && !twin_low_res;
> > bool missing_twin_high_res = twin_high_res;
> > 
> > Btw, why are we checking "!twin_high_res || !twin_low_res" here at all? Do
> they
> > both have to be set to NULL if one of them is NULL? Or is this just an
> > optimization to avoid comparing transforms?
> 
> Assuming that you only do this in the case that a layer has a twin and they
each
> have either a high res or both a high and low res, then you have four cases
for
> this optimization.
> 
> (1) Both layers only have high res: ok
> (2) Both layers have high res and low res: ok
> (3) Pending layer has both, active layer has only high res: ok (but no need
for
> a second pass, previously not handled as a simplification)
> (4) Pending layer has only high res, active layer has both: not ok (this is
not
> handled in this patch correctly)
> 
> #4 is not ok because the active layer could be displaying low res tiles, and
if
> you are not able to mark low res tiles as required, you could flash from low
res
> to missing.
> 
> I think this logic would make more sense if the ShouldHaveLowResTiling() was
not
> consulted and you just allowed the first 3 cases above.

Done.

https://codereview.chromium.org/260963008/diff/130001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/260963008/diff/130001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:796: twin_layer_->tilings_->num_tilings() <=
tilings_->num_tilings()) {
This condition forbids a case 4 from previous comments: when pending layer has
only high res and active has high res and low res.

[enne (OOO), 2014-05-19 18:33:04]

lgtm

[Sergey, 2014-05-22 01:09:02]

On 2014/05/19 18:33:04, enne wrote:
> lgtm

Sorry, what should I do now to land it?..

[reveman, 2014-05-22 03:48:06]

On 2014/05/22 01:09:02, Sergey wrote:
> On 2014/05/19 18:33:04, enne wrote:
> > lgtm
> 
> Sorry, what should I do now to land it?..

Check the commit checkbox on the latest patch and it will land after all tests
have passed.

[Sergey, 2014-05-22 04:00:24]

The CQ bit was checked by p.sergey@samsung.com

[commit-bot: I haz the power, 2014-05-22 04:03:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/p.sergey@samsung.com/260963008/130001

[commit-bot: I haz the power, 2014-05-22 10:06:55]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/7933)

[Sergey, 2014-05-22 10:09:56]

The CQ bit was unchecked by p.sergey@samsung.com

[Sergey, 2014-05-22 10:11:08]

On 2014/05/22 10:09:56, Sergey wrote:
> The CQ bit was unchecked by mailto:p.sergey@samsung.com

There are some unittest fails after merge, will re-upload after resolving.

[commit-bot: I haz the power, 2014-05-22 10:12:10]

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

[commit-bot: I haz the power, 2014-05-22 10:12:11]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[Sergey, 2014-05-27 10:37:45]

The CQ bit was checked by p.sergey@samsung.com

[commit-bot: I haz the power, 2014-05-27 10:38:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/p.sergey@samsung.com/260963008/150001

[commit-bot: I haz the power, 2014-05-27 12:27:04]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-27 12:32:46]

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

[commit-bot: I haz the power, 2014-05-27 12:32:47]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[Sergey, 2014-05-28 08:57:02]

The CQ bit was checked by p.sergey@samsung.com

[commit-bot: I haz the power, 2014-05-28 08:57:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/p.sergey@samsung.com/260963008/170001

[commit-bot: I haz the power, 2014-05-28 11:46:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-28 11:48:43]

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

[commit-bot: I haz the power, 2014-05-28 11:48:44]

Try jobs failed on following builders:
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)

[Sergey, 2014-05-28 23:30:15]

On 2014/05/28 11:48:44, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_clang_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)

@enne, @reveman, could you please help me with the buildbot errors?... I can't
understand what it is now :( Is it unable to update for some reason?

[enne (OOO), 2014-05-28 23:37:21]

On 2014/05/28 23:30:15, Sergey wrote:
> On 2014/05/28 11:48:44, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_clang_dbg on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)

"Failed to apply patch for cc/layers/picture_layer_impl_unittest.cc"

Looks like you to need to pull and rebase your patch against changes on trunk,
reupload, and re-CQ.

[Sergey, 2014-05-29 00:50:07]

On 2014/05/28 23:37:21, enne wrote:
> On 2014/05/28 23:30:15, Sergey wrote:
> > On 2014/05/28 11:48:44, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   linux_chromium_clang_dbg on tryserver.chromium
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
> 
> "Failed to apply patch for cc/layers/picture_layer_impl_unittest.cc"
> 
> Looks like you to need to pull and rebase your patch against changes on trunk,
> reupload, and re-CQ.

Sorry, why rebase? Isn't it just git pull to branch, resolve conflicts and
re-upload?

[enne (OOO), 2014-05-29 00:51:42]

On 2014/05/29 00:50:07, Sergey wrote:
> On 2014/05/28 23:37:21, enne wrote:
> > On 2014/05/28 23:30:15, Sergey wrote:
> > > On 2014/05/28 11:48:44, I haz the power (commit-bot) wrote:
> > > > Try jobs failed on following builders:
> > > >   linux_chromium_clang_dbg on tryserver.chromium
> > > >
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
> > 
> > "Failed to apply patch for cc/layers/picture_layer_impl_unittest.cc"
> > 
> > Looks like you to need to pull and rebase your patch against changes on
trunk,
> > reupload, and re-CQ.
> 
> Sorry, why rebase? Isn't it just git pull to branch, resolve conflicts and
> re-upload?

Sure.  Rebase or merge, it doesn't matter.

[Sergey, 2014-05-29 00:54:44]

The CQ bit was checked by p.sergey@samsung.com

[Sergey, 2014-05-29 00:55:21]

The CQ bit was unchecked by p.sergey@samsung.com

[Sergey, 2014-05-29 00:57:11]

The CQ bit was checked by p.sergey@samsung.com

[commit-bot: I haz the power, 2014-05-29 00:58:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/p.sergey@samsung.com/260963008/190001

[commit-bot: I haz the power, 2014-05-29 05:04:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[commit-bot: I haz the power, 2014-05-29 05:36:36]

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

[commit-bot: I haz the power, 2014-05-29 05:36:37]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[Sergey, 2014-05-30 01:18:10]

On 2014/05/29 05:36:37, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_aosp on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

@enne, @reveman, it seems to be some checkout problem... Am I correct? Could you
help me fixing it?

[reveman, 2014-05-30 01:23:33]

On 2014/05/30 01:18:10, Sergey wrote:
> On 2014/05/29 05:36:37, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   android_aosp on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
> 
> @enne, @reveman, it seems to be some checkout problem... Am I correct? Could
you
> help me fixing it?

Looks unrelated to your change. Let's just give it another try.

[reveman, 2014-05-30 01:23:46]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-05-30 01:25:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/p.sergey@samsung.com/260963008/190001

[commit-bot: I haz the power, 2014-05-30 06:19:00]

Change committed as 273736