Refactoring RenderLayerCompositor to avoid redundant checks for Compositing reasons

Refactoring RenderLayerCompositor to avoid redundant checks for Compositing reasons.

A renderer can be of type either Video, Canvas, Plugin or Iframe.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153464

[pals, 2013-06-27 03:39:36]

PTAL, Thanks

[shawnsingh, 2013-07-01 06:23:28]

Did you want me to take a look yet?  It's helpful to say "please take a look" in
a message - sometimes reviewers don't see patches that come up on their list
unless they receive the message.

I think after you address my feedback, I will be OK with this patch, and after
that we can include enne@ for an OWNERS review.

https://codereview.chromium.org/18005002/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/18005002/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:1579: else if
(requiresCompositingForCanvas(renderer))
Let's add a brief comment at the beginning of this group of if-else statements,
saying "Only zero or one of the following conditions will be true for a given
RenderLayer."

I'm curious - is this just a cleanup refactoring you thought would be
appropriate?  Or is there a deeper reason that we need to use else-statements
here?  Can you please explain?

https://codereview.chromium.org/18005002/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:1589: if
(requiresCompositingForTransition(renderer) ||
requiresCompositingForAnimation(renderer))
This particular line, let's not combine these.  We only combined them
temporarily while we were limited to 32 reasons in a 32-bit field.  We're
migrating to 64-bit already, and we should be able to add the
CompositingReasonTransition reason again.

[pals, 2013-07-01 07:41:57]

Updated the patch. Please take a look

https://codereview.chromium.org/18005002/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/18005002/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:1579: else if
(requiresCompositingForCanvas(renderer))
On 2013/07/01 06:23:28, shawnsingh wrote:
> Let's add a brief comment at the beginning of this group of if-else
statements,
> saying "Only zero or one of the following conditions will be true for a given
> RenderLayer."

Done.

> 
> I'm curious - is this just a cleanup refactoring you thought would be
> appropriate?  Or is there a deeper reason that we need to use else-statements
> here?  Can you please explain?

This is just cleanup refactoring which I came accross while going through the
code.

https://codereview.chromium.org/18005002/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:1589: if
(requiresCompositingForTransition(renderer) ||
requiresCompositingForAnimation(renderer))
On 2013/07/01 06:23:28, shawnsingh wrote:
> This particular line, let's not combine these.  We only combined them
> temporarily while we were limited to 32 reasons in a 32-bit field.  We're
> migrating to 64-bit already, and we should be able to add the
> CompositingReasonTransition reason again.
> 

Done.

[shawnsingh, 2013-07-01 18:17:56]

LGTM.   It's a good idea to run try bots using "git cl try" even for simple
patches like this.   This time, I went ahead and manually started them.

+enne@ for OWNERS review.

[enne (OOO), 2013-07-01 18:21:10]

lgtm

[commit-bot: I haz the power, 2013-07-02 04:05:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/18005002/5001

[commit-bot: I haz the power, 2013-07-02 04:57:54]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-07-03 02:20:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/18005002/5001

[commit-bot: I haz the power, 2013-07-03 02:56:48]

Change committed as 153464