Re-land "Drop background color optimization for composited layers"

Re-land "Drop background color optimization for composited layers"

The original CL r168245 was reverted by r168317 because of crashes.
It's because r168245 removes null-check of document().documentElement()->renderer()
in updating-compositing-descendant-geometry process because the CL assumed all
elements own renderer at that time. However, it's not true on Mac.

So this CL does not remove the null-check to land.

BUG=114658, 340691
R=alokp@chromium.org, enne@chromium.org

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

[dshwang, 2014-03-04 17:11:24]

Hi, could you review again?

https://codereview.chromium.org/178013003/ caused crash on mac, because I remove
following code.

if (renderObject->node() && renderObject->node()->isDocumentNode()) {
    // Look to see if the root object has a non-simple background
    RenderObject* rootObject = renderObject->document().documentElement() ?
renderObject->document().documentElement()->renderer() : 0;
--    if (!rootObject)
--        return false;
    RenderStyle* style = rootObject->style();

My brave assumption was revealed to wrong. I'm sorry cbiesinger@ for that.

[dshwang, 2014-03-04 17:12:28]

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1575: if
(rootObject) {
now null-check resurrects

[alokp, 2014-03-04 17:44:38]

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1572: if
(renderObject->node() && renderObject->node()->isDocumentNode() &&
renderObject->document().documentElement()) {
Is there a need to check for renderObject->document().documentElement()?

Isn't it already be checked in the if(rootObject) below?

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1578: if
(hasBoxDecorationsOrBackgroundImage(rootObject->style()))
this if block can be combined with the above:
if (rootObject && hasBoxDecorationsOrBackgroundImage(rootObject->style())

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1585: if
(bodyObject) {
same here

[dshwang, 2014-03-04 18:56:12]

Thank you for review.

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1572: if
(renderObject->node() && renderObject->node()->isDocumentNode() &&
renderObject->document().documentElement()) {
On 2014/03/04 17:44:39, Alok Priyadarshi wrote:
> Is there a need to check for renderObject->document().documentElement()?
> 
> Isn't it already be checked in the if(rootObject) below?

it checks the third condition in the above "if statement".

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1578: if
(hasBoxDecorationsOrBackgroundImage(rootObject->style()))
On 2014/03/04 17:44:39, Alok Priyadarshi wrote:
> this if block can be combined with the above:
> if (rootObject && hasBoxDecorationsOrBackgroundImage(rootObject->style())

Done.

https://codereview.chromium.org/183833020/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1585: if
(bodyObject) {
On 2014/03/04 17:44:39, Alok Priyadarshi wrote:
> same here

Done.

[alokp, 2014-03-04 19:19:03]

lgtm

https://codereview.chromium.org/183833020/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/183833020/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1572: if
(renderObject->node() && renderObject->node()->isDocumentNode() &&
renderObject->document().documentElement()) {
nit: I prefer the original code.
RenderObject* rootObject = renderObject->document().documentElement() ?
renderObject->document().documentElement()->renderer() : 0;

[dshwang, 2014-03-04 19:31:11]

On 2014/03/04 19:19:03, Alok Priyadarshi wrote:
> lgtm

Thank you for lgtm.


>
https://codereview.chromium.org/183833020/diff/20001/Source/core/rendering/co...
> File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):
> 
>
https://codereview.chromium.org/183833020/diff/20001/Source/core/rendering/co...
> Source/core/rendering/compositing/CompositedLayerMapping.cpp:1572: if
> (renderObject->node() && renderObject->node()->isDocumentNode() &&
> renderObject->document().documentElement()) {
> nit: I prefer the original code.
> RenderObject* rootObject = renderObject->document().documentElement() ?
> renderObject->document().documentElement()->renderer() : 0;

yeah, I agree. I reverted.

[dshwang, 2014-03-04 19:31:33]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-03-04 19:31:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/183833020/40001

[commit-bot: I haz the power, 2014-03-04 20:01:41]

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

[commit-bot: I haz the power, 2014-03-04 20:01:41]

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

[abarth-chromium, 2014-03-04 21:05:21]

rslgtm

[abarth-chromium, 2014-03-04 21:05:32]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-04 21:06:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/183833020/40001

[abarth-chromium, 2014-03-04 23:46:55]

The CQ bit was unchecked by abarth@chromium.org

[abarth-chromium, 2014-03-04 23:51:44]

This CL also triggered a crash on
fast/images/link-body-content-imageDimensionChanged-crash.html on Android.  Let
me check for you whether the new version fixes that crash.

[abarth-chromium, 2014-03-05 00:02:09]

Success!  :)

[abarth-chromium, 2014-03-05 00:02:20]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-05 00:02:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/183833020/40001

[dshwang, 2014-03-05 06:05:44]

On 2014/03/05 00:02:09, abarth wrote:
> Success!  :)

Thank you! :)

[commit-bot: I haz the power, 2014-03-05 16:14:49]

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

[commit-bot: I haz the power, 2014-03-05 16:14:50]

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

[abarth-chromium, 2014-03-06 00:34:31]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-06 00:34:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/183833020/40001

[commit-bot: I haz the power, 2014-03-06 00:45:56]

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

[commit-bot: I haz the power, 2014-03-06 00:45:57]

Try jobs failed on following builders: win_blink_rel

[abarth-chromium, 2014-03-06 02:14:50]

On 2014/03/06 00:45:57, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders: win_blink_rel

Looks like there are a bunch of failures on Windows...  Maybe these just need
new baselines?

[dshwang, 2014-03-06 09:39:31]

On 2014/03/06 02:14:50, abarth wrote:
> On 2014/03/06 00:45:57, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders: win_blink_rel
> 
> Looks like there are a bunch of failures on Windows...  Maybe these just need
> new baselines?

yes, i'll do re-rebaseline.

[dshwang, 2014-03-06 11:16:36]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-03-06 11:16:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/183833020/60001

[commit-bot: I haz the power, 2014-03-06 11:54:57]

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

[commit-bot: I haz the power, 2014-03-06 11:54:58]

Try jobs failed on following builders: mac_blink_rel

[dshwang, 2014-03-06 12:01:30]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-03-06 12:01:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/183833020/80001

[commit-bot: I haz the power, 2014-03-06 13:12:31]

Change committed as 168634