Invalidate the frame view when changing the background image of the document or body.

Invalidate the frame view when changing the background image of the document or body.

To do this, add a method to request full paint invalidation on FrameView. This also requires moving the full invalidation code into invalidateTreeIfNeeded() instead of layout(), since otherwise the invalidation requested will not always happen if layout does not occur.

In any case, invalidation should be in invalidateTreeIfNeeded(), not layout().

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

[chrishtr, 2014-10-30 23:26:11]

chrishtr@chromium.org changed reviewers:
+ jchaffraix@chromium.org

[dsinclair, 2014-10-31 00:14:33]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org, wangxianzhu@chromium.org

[dsinclair, 2014-10-31 00:14:34]

https://codereview.chromium.org/683373002/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/683373002/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1720: // causing double-layout.
Can you file a bug to track this?

https://codereview.chromium.org/683373002/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1723: }
Don't think this should be here.

https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1287:
view()->frameView()->setNeedsFullPaintInvalidation();
What about doing view()->setShouldDoFullPaintInvalidation() which should force
the render view to fully invalidate? Then you don't need to add
setNeedsFullPaintInvalidation on frameView.

https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderView.cpp:171: fprintf(stderr, "change view
width\n");
Remove.

[Xianzhu, 2014-10-31 16:37:30]

https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:1287:
view()->frameView()->setNeedsFullPaintInvalidation();
Would view()->setNeedsFullPaintInvalidation() work?

[chrishtr, 2014-10-31 18:13:42]

On 2014/10/31 at 16:37:30, wangxianzhu wrote:
>
https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
> File Source/core/rendering/RenderBox.cpp (right):
> 
>
https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
> Source/core/rendering/RenderBox.cpp:1287:
view()->frameView()->setNeedsFullPaintInvalidation();
> Would view()->setNeedsFullPaintInvalidation() work?

No, because the bounds of the RenderView does not include the entire frame, it
just includes the visible part. RenderBox.cpp:1294 already does
setNeedsFullPaintInvalidation on the RenderView.

[chrishtr, 2014-10-31 18:22:24]

https://codereview.chromium.org/683373002/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/683373002/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1720: // causing double-layout.
On 2014/10/31 at 00:14:34, dsinclair wrote:
> Can you file a bug to track this?

Done.

https://codereview.chromium.org/683373002/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1723: }
On 2014/10/31 at 00:14:34, dsinclair wrote:
> Don't think this should be here.

Done.

https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderView.cpp:171: fprintf(stderr, "change view
width\n");
On 2014/10/31 at 00:14:34, dsinclair wrote:
> Remove.

Fixed.

[Xianzhu, 2014-10-31 19:02:13]

On 2014/10/31 18:13:42, chrishtr wrote:
> On 2014/10/31 at 16:37:30, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
> > File Source/core/rendering/RenderBox.cpp (right):
> > 
> >
>
https://codereview.chromium.org/683373002/diff/140001/Source/core/rendering/R...
> > Source/core/rendering/RenderBox.cpp:1287:
> view()->frameView()->setNeedsFullPaintInvalidation();
> > Would view()->setNeedsFullPaintInvalidation() work?
> 
> No, because the bounds of the RenderView does not include the entire frame, it
> just includes the visible part. RenderBox.cpp:1294 already does
> setNeedsFullPaintInvalidation on the RenderView.

I realized that because of the special background-attachment behavior of the
viewport, with default background attachment, viewport's background attachment
is attached on the scrolling contents if the viewport is scrollable.

This seems a similar case of crbug.com/417349 (background-attachment:local for
scrollable boxes). 

lgtm.

[chrishtr, 2014-10-31 19:27:43]

I adjusted some of the tests that changed due to the time at which the code is
now doing full invalidations, on window resize especially. All look ok to me,
because they are either not doing a redundant invalidation any more, or are
invalidating w.r.t. the new bounds, not the old, on a resize.

[chrishtr, 2014-11-01 17:43:13]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-11-01 17:43:32]

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

[commit-bot: I haz the power, 2014-11-01 18:39:52]

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

[commit-bot: I haz the power, 2014-11-01 18:39:53]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[chrishtr, 2014-11-03 17:39:22]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-11-03 17:40:28]

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

[commit-bot: I haz the power, 2014-11-03 18:40:22]

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

[commit-bot: I haz the power, 2014-11-03 18:40:26]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[chrishtr, 2014-11-04 06:19:47]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-11-04 06:20:23]

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

[commit-bot: I haz the power, 2014-11-04 08:17:52]

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

[commit-bot: I haz the power, 2014-11-04 08:17:53]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/34863)

[chrishtr, 2014-11-04 17:03:42]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-11-04 17:04:01]

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

[commit-bot: I haz the power, 2014-11-04 17:25:43]

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

[commit-bot: I haz the power, 2014-11-04 17:25:45]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
8bcf45109489c09316918f404b284948974bac86..2682ce52f18225de173cd0000c60a90f08f6eb6a
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1,3 +1,5 @@
+crbug.com/425872
fast/repaint/background-image-paint-invalidation-large-abspos-div.html [
NeedsRebaseline ]
+crbug.com/425872 fast/repaint/overflow-scroll-body-appear.html [
NeedsRebaseline ]
 
 # This directly has manual tests that don't have to run with run-webkit-tests
 crbug.com/359838 http/tests/ManualTests/ [ Skip ]

[chrishtr, 2014-11-04 17:46:08]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-11-04 17:46:35]

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

[commit-bot: I haz the power, 2014-11-04 19:32:17]

Committed patchset #14 (id:260001) as 184836