|
|
Created:
4 years, 3 months ago by oshima Modified:
4 years, 1 month ago CC:
chromium-reviews, Ian Vollick, sievers+watch_chromium.org, jbauman+watch_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, cc-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck if the opacity set from animation is same as the target value to avoid unnecessary redraw.
Setting the same opacity was causing redrawing.
BUG=642223
Patch Set 1 #Patch Set 2 : Skip if the target value is same #
Total comments: 2
Patch Set 3 : Check if the value set from animation is same as the target value #
Total comments: 1
Patch Set 4 #Messages
Total messages: 33 (21 generated)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Skip if the target value is same BUG= ========== to ========== Skip if the target value is same BUG=642223 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Skip if the target value is same BUG=642223 ========== to ========== Check if the value specified from animatino is same as the target value BUG=642223 ==========
oshima@chromium.org changed reviewers: + vollick@chromium.org
Description was changed from ========== Check if the value specified from animatino is same as the target value BUG=642223 ========== to ========== Check if the value set from animation is same as the target value BUG=642223 ==========
ping?
friendly ping
oshima@chromium.org changed reviewers: + loyso@chromium.org - vollick@chromium.org
loyso@, can you review this?
oshima@chromium.org changed reviewers: + vollick@chromium.org
We miss vollick@. https://codereview.chromium.org/2298543002/diff/20001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/2298543002/diff/20001/ui/compositor/layer.cc#... ui/compositor/layer.cc:913: return; We have similar check inside cc::Layer::SetTransform. Could you explain a rationale for this change in the CL description? Thanks in advance.
Description was changed from ========== Check if the value set from animation is same as the target value BUG=642223 ========== to ========== Check if the value set from animation is same as the target value to avoid unnecessary redraw, commits. BUG=642223 ==========
Description was changed from ========== Check if the value set from animation is same as the target value to avoid unnecessary redraw, commits. BUG=642223 ========== to ========== Check if the opacity set from animation is same as the target value to avoid unnecessary redraw. Setting the same opacity was causing redrawing. BUG=642223 ==========
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2298543002/diff/20001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/2298543002/diff/20001/ui/compositor/layer.cc#... ui/compositor/layer.cc:913: return; On 2016/11/15 23:10:07, loyso wrote: > We have similar check inside cc::Layer::SetTransform. > Could you explain a rationale for this change in the CL description? > Thanks in advance. I see, yes we don't need this one. removed. I also removed the change in SetColorFromAnimation because it has similar check. It looks like brightness and grayscale may not it either technically, so I reduced the scope of this CL to the original issue which is opacity.
On 2016/11/16 14:27:35, oshima wrote: > It looks like brightness and grayscale may not it either technically, so I > reduced the scope of this CL to the original issue which is opacity. Can we move this check to the cc layer then? FYI, I'm not an owner for ui/compositor/layer.cc. And vollick@ doesn't work on chromium these days.
https://codereview.chromium.org/2298543002/diff/60001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/2298543002/diff/60001/ui/compositor/layer.cc#... ui/compositor/layer.cc:1024: cc_layer_->SetOpacity(opacity); Is it ok to change SetOpacity to return boolean so that Layer can skip ScheduleDraw?
cc::Layer::SetOpacity has that check and it calls SetNeedsCommit if needed. All you need is to remove unconditional ScheduleDraw(); call in ui::Layer. FYI, ui ScheduleDraw() == cc SetNeedsCommit().
On 2016/11/17 00:16:39, loyso wrote: > cc::Layer::SetOpacity has that check and it calls SetNeedsCommit if needed. All > you need is to remove unconditional ScheduleDraw(); call in ui::Layer. > FYI, ui ScheduleDraw() == cc SetNeedsCommit(). I see, in that case, all I need is to remove ScheduleDraw. PTAL
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
non-owner LGTM.
p.s. Could you add a unit test, please? |