Add support for the "repaint" layout tests (LayoutTests/fast/repaint/*), whic...

Add support for the "repaint" layout tests (LayoutTests/fast/repaint/*), which draw a gray mask over regions that have not been updated.
This still doesn't match the mac expected results, because clip area is wrong.

BUG=8630
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=11396

[eroman, 2009-02-09 23:18:30]

-------------
Reviewers
-------------

[pamg]
please review all.

[tony]
please review webwidgethost_gtk.cc.
In particular is the change I made to |grect|
in PaintCanvasToView() going to cause problems?

[awalker]
please review webwidgethost_mac.mm.
My mac force is weak so please check carefully.
I verified that it does at least compile :-)

[brettw]
please review bitmap_platform_device_win.{cc,h}.
I exposed this on the SkBitmap for use by TestShell::DumpImage (test_shell.cc).

-------------
Background: what are the repaint layout tests?
-------------

The repaint Layout tests are mainly found in LayoutTests/fast/repaint/*.

They work by calling layoutTestController.display() to compose a gray mask onto
the output.
As pixels get repainted, they overwrite the gray. So the repainted regions show
up in the pixel dump as non-puke-gray.

-------------
What about the chromium baselines?
-------------

Currently all of these tests "pass", because they were rebaselined to.
But in actuality they are not testing repainting.

I have not added the new baselines to this CL yet -- still working on them.
There are at least 100 tests impacted by this.

Notably, our baselines are going to look different than upstream,
because of a difference in how we track damaged regions.

Whereas webkit does accurate book-keeping (list of rects) we track a single rect
(smallest rect enclosing all the sub-rects).
To see how this plays out, compare the following:

** webkit baseline **

http://www/~ericroman/tmp/repaint/backgroundSizeRepaint-webkit.png

** current chromium baseline (wrong) **

http://www/~ericroman/tmp/repaint/backgroundSizeRepaint-curBaseline.png

** new chromium baseline: **

http://www/~ericroman/tmp/repaint/backgroundSizeRepaint-newBaseline.png

###
The other difference (which I haven't explained yet), is that all of our new
repaint baselines show repaint invalidations to span the full width of the
window.
###

[tony, 2009-02-09 23:23:00]

punting gtk review to evan, as he already wrote this for test_shell_gtk but
didn't submit because he had the same problem you did.

On 2009/02/09 23:18:30, eroman wrote:
> -------------
> Reviewers
> -------------
> 
> [pamg]
> please review all.
> 
> [tony]
> please review webwidgethost_gtk.cc.
> In particular is the change I made to |grect|
> in PaintCanvasToView() going to cause problems?
> 
> [awalker]
> please review webwidgethost_mac.mm.
> My mac force is weak so please check carefully.
> I verified that it does at least compile :-)
> 
> [brettw]
> please review bitmap_platform_device_win.{cc,h}.
> I exposed this on the SkBitmap for use by TestShell::DumpImage
(test_shell.cc).
> 
> -------------
> Background: what are the repaint layout tests?
> -------------
> 
> The repaint Layout tests are mainly found in LayoutTests/fast/repaint/*.
> 
> They work by calling layoutTestController.display() to compose a gray mask
onto
> the output.
> As pixels get repainted, they overwrite the gray. So the repainted regions
show
> up in the pixel dump as non-puke-gray.
> 
> -------------
> What about the chromium baselines?
> -------------
> 
> Currently all of these tests "pass", because they were rebaselined to.
> But in actuality they are not testing repainting.
> 
> I have not added the new baselines to this CL yet -- still working on them.
> There are at least 100 tests impacted by this.
> 
> Notably, our baselines are going to look different than upstream,
> because of a difference in how we track damaged regions.
> 
> Whereas webkit does accurate book-keeping (list of rects) we track a single
rect
> (smallest rect enclosing all the sub-rects).
> To see how this plays out, compare the following:
> 
> ** webkit baseline **
> 
> http://www/~ericroman/tmp/repaint/backgroundSizeRepaint-webkit.png
> 
> ** current chromium baseline (wrong) **
> 
> http://www/~ericroman/tmp/repaint/backgroundSizeRepaint-curBaseline.png
> 
> ** new chromium baseline: **
> 
> http://www/~ericroman/tmp/repaint/backgroundSizeRepaint-newBaseline.png
> 
> ###
> The other difference (which I haven't explained yet), is that all of our new
> repaint baselines show repaint invalidations to span the full width of the
> window.
> ###

[Evan Martin, 2009-02-09 23:28:22]

On 2009/02/09 23:23:00, tony wrote:
> punting gtk review to evan, as he already wrote this for test_shell_gtk but
> didn't submit because he had the same problem you did.

I am a bit sad this effort was duplicated -- I had claimed "repaint" on the
spreadsheet since I had a patch that implemented this.

I could never figure out why the boxes were too wide, so I never checked it in. 
:(

Anyway, I would suggest checking in your code that makes the tests closer to
correct, but then not rebaselining until we have fixed repainting to not be too
wide.

[Evan Martin, 2009-02-09 23:30:14]

for reference:
 http://codereview.chromium.org/16224/show

I approached getting at the rendered pixels differently, which meant I could
write the gray-overdrawing code only once using Skia instead of how you did it,
with platform-specific changes.

[Evan Martin, 2009-02-09 23:31:10]

On 2009/02/09 23:30:14, Evan Martin wrote:
> I approached getting at the rendered pixels differently, which meant I could
> write the gray-overdrawing code only once using Skia instead of how you did
it,
> with platform-specific changes.

Er, disregard that last bit -- I approached it differently but I still needed
platform specific changes.  :)

[Amanda Walker, 2009-02-09 23:31:51]

http://codereview.chromium.org/21192/diff/22/1008
File webkit/tools/test_shell/mac/webwidget_host.mm (right):

http://codereview.chromium.org/21192/diff/22/1008#newcode231
Line 231: flipped:NO]];
You probably don't need to mess with the NSGraphicsContext at all--it's only
needed for Cocoa drawing operations.  You should be able to use CoreGraphics
operations to paint the gray rect directly into |bitmap_context|.

[Evan Martin, 2009-02-09 23:32:20]

Oh yeah, one more comment!

In this bit:
http://codereview.chromium.org/16224/diff/210/409
I found that if I set a clip rect in Skia before I passed it on to WebKit (see
the clipRect call at the bottom) it would do the gray parts properly.  But I
don't think it should be necessary, and that sort of black magic is Brett's
domain more htan mine.

[eroman, 2009-02-10 01:57:05]

> I found that if I set a clip rect in Skia before I passed it on to
> WebKit (see the clipRect call at the bottom) it would do the gray parts
> properly.  But I don't think it should be necessary, and that sort of
> black magic is Brett's domain more htan mine.

Ah, sounds like you already explained this then.

I confirmed that WebKit does in fact clip by the dirty rect before painting the
frameview, whereas we do not.

So the overzealous painting on the sides would not be reflected in the
upstream's webview implementaiton.

-----------------
Here is the upstream win/WebView.cpp:
-----------------
if (frameView && frameView->frame() && frameView->frame()->contentRenderer()) {
    gc.clip(dirtyRect);
    frameView->paint(&gc, dirtyRect);
}

-----------------
And here is ours (webframe_impl.cc):
-----------------
if (frame_->document() && frameview()) {
  frameview()->paint(&gc, dirty_rect);
} else { ...


So this is easy to fix, should just be a matter of patching
WebFrameImpl::Paint().

Will update once I have confirmed.

> I am a bit sad this effort was duplicated -- I had claimed "repaint" on
> the spreadsheet since I had a patch that implemented this.

Yeah. Although not too bad given still needed the multiplatform work. Now the
crap-work to verify all the baselines :-)

http://codereview.chromium.org/21192/diff/22/1008
File webkit/tools/test_shell/mac/webwidget_host.mm (right):

http://codereview.chromium.org/21192/diff/22/1008#newcode231
Line 231: flipped:NO]];
On 2009/02/09 23:31:52, Amanda Walker wrote:
> You probably don't need to mess with the NSGraphicsContext at all--it's only
> needed for Cocoa drawing operations.  You should be able to use CoreGraphics
> operations to paint the gray rect directly into |bitmap_context|.

Done.

Thanks for explaining that distinction.
Replaced with a simpler call to CGContextFillRect.

[Pam (message me for reviews), 2009-02-10 22:25:59]

I'm not an expert on most of this, but it LGTM, with the comments/nits below.

- Pam

http://codereview.chromium.org/21192/diff/2002/3009
File skia/ext/bitmap_platform_device_win.h (right):

http://codereview.chromium.org/21192/diff/2002/3009#newcode70
Line 70: // Replace occurrences of kMagicTransparencyColor with black. See
Nit: Use declarative rather than imperative ("Replaces" instead of "Replace"),
to fit the style guide and the other comments in the file. (I admit, this is a
picky one.)

http://codereview.chromium.org/21192/diff/2002/3009#newcode71
Line 71: // fixupAlphaBeforeCompositing()'s documentation for an explanation of
why
Can you add a pointer to where fixupAlphaBeforeCompositing is documented, since
it's not in this file?

[eroman, 2009-02-11 00:06:41]

http://codereview.chromium.org/21192/diff/2002/3009
File skia/ext/bitmap_platform_device_win.h (right):

http://codereview.chromium.org/21192/diff/2002/3009#newcode70
Line 70: // Replace occurrences of kMagicTransparencyColor with black. See
On 2009/02/10 22:26:00, Pam wrote:
> Nit: Use declarative rather than imperative ("Replaces" instead of "Replace"),
> to fit the style guide and the other comments in the file. (I admit, this is a
> picky one.)

Done.

http://codereview.chromium.org/21192/diff/2002/3009#newcode71
Line 71: // fixupAlphaBeforeCompositing()'s documentation for an explanation of
why
On 2009/02/10 22:26:00, Pam wrote:
> Can you add a pointer to where fixupAlphaBeforeCompositing is documented,
since
> it's not in this file?

Done.

[brettw, 2009-02-11 01:34:34]

http://codereview.chromium.org/21192/diff/50/1039
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/21192/diff/50/1039#newcode251
Line 251: skia::BitmapPlatformDeviceWin::fixupAlpha(src_bmp);
Are the changes to BitmapPlatformDeviceWin just to avoid a const_cast here? If
so, I would just do the const cast and not change the device code. With your
code, you're actually changing the source bitmap anyway, so you're voilating the
constness. We do const_cast in other places.

[eroman, 2009-02-11 01:50:59]

http://codereview.chromium.org/21192/diff/50/1039
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/21192/diff/50/1039#newcode251
Line 251: skia::BitmapPlatformDeviceWin::fixupAlpha(src_bmp);
On 2009/02/11 01:34:34, brettw wrote:
> Are the changes to BitmapPlatformDeviceWin just to avoid a const_cast here? If
> so, I would just do the const cast and not change the device code. With your
> code, you're actually changing the source bitmap anyway, so you're voilating
the
> constness. We do const_cast in other places.

The intent was to avoid calling fixupAlphaBeforeCompositing() directly on the
WebWidgetHost's canvas. And instead copy into an SkBitmap and do the fixup
there.

Is this not necessary? From a comment I read, it seemed that doing the fixup
finalizes the layer, so it would screw up subsequent paints by the webwidget
host.

[bitmap_platform_device_win.cc]

// However, once this has been done, no more can be drawn to
// the layer because fixing the alphas *again* will result in incorrect
// values.

[eroman, 2009-02-11 02:12:28]

>When you use the copy constructor, it actually references the other
> bitmap's pixels, sharing the same backing store. Even when the other
> bitmap is const. This is kind of confusing. But anyway, you're already
> changing the original, I think, so if it works, it works.

Oh I see (I had assumed it was COW).

Changed to using SkBitmap::copyTo(), and used a const_cast so the parameter to
fixupAlpha() is not const.

[eroman, 2009-02-14 01:22:27]

ping.

[brettw, 2009-02-14 20:02:38]

LGTM. Sorry for the delay!

http://codereview.chromium.org/21192/diff/76/1064
File webkit/tools/test_shell/webwidget_host.h (right):

http://codereview.chromium.org/21192/diff/76/1064#newcode57
Line 57: void PaintToCanvas();
These two declarations should go below the constructor/destructor.

[Evan Martin, 2009-02-19 19:53:48]

did you ever commit this?