[webkit-reviews] review granted: [Bug 224987] experiment with averaging sampling colors across the top of the page as the scroll area background : [Attachment 426928] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 224987: experiment with averaging sampling colors across the top of the
page as the scroll area background
https://bugs.webkit.org/show_bug.cgi?id=224987

Attachment 426928: Patch

https://bugs.webkit.org/attachment.cgi?id=426928&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 426928
  --> https://bugs.webkit.org/attachment.cgi?id=426928
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426928&action=review

I’m OK with this as-is, but I think there is a lot that can be done to improve
the implementation here.

> Source/WebCore/dom/Document.cpp:3911
> +    auto getPixelColorAtTopX = [&] (int x) -> Optional<SRGBA<uint8_t>> {

We don’t use "get" in the names of these kinds of functions in WebKit.

>>> Source/WebCore/dom/Document.cpp:3917
>>> +	     hitTest(hitTestRequestTypes, hitTestResult);
>> 
>> Do we really need to do hit testing? I am a bit perplexed. This seems an
indirect way to protect against reading colors across domains: What if hit
testing is not 100% consistent with painting? Normally that would be harmless.
>> 
>> If we are not trying to protect against riding colors across domains, then I
am unclear on why a hit test is helpful.
> 
> Yes the purpose of this is to avoid 3rd-party <iframe>, such as for sites
that have a banner ad across the top of the page.

I suggest you factor this into a separate function with a name like
isInFirstPartyFrame or something along those lines, to document more clearly
what it’s doing.


The hit testing won’t necessarily exactly match the painting, so it’s a
tempting, but flawed concept on how to do this.

I think there’s an alternative, more robust and fool proof approach where we
pass arguments in that alter the behavior of the painting code rather than
relying on the connection between painting code and hit testing code. The
question in my mind is what we want to do instead of painting these third party
frames into the single pixel we are trying to sample. Would we just not paint
those frames? Then we might get the value of pixels the user would never see
that would normally be covered by a third party frame. We could paint them in a
special indicator color to help us know not to use the pixels.

>>> Source/WebCore/dom/Document.cpp:3927
>>> +	     auto snapshotData =
snapshot->getImageData(AlphaPremultiplication::Unpremultiplied,
IntRect(IntPoint::zero(), size));
>> 
>> The getImageData API is designed for use from JavaScript, with quite a bit
of inefficiency just to be compatible with JavaScript idioms. Here internally
in WebKit I am sure we can come up with something simpler that doesn’t require
memory allocation.
> 
> I was hoping there was a better way than using `ImageData`.  Do you happen to
know of one of these better ways?  I'm thinking that maybe
`ImageBuffer::toBGRAData` might be an option.

What we want is similar to what Image::singlePixelSolidColor does. That
function is designed to detect an image that happens to be a single pixel solid
color. Here we are intentionally creating an image buffer (intrinsically
opaque, which is what "solid" means) that contains a single pixel and would
like to know its color.

I suppose ImageBuffer may be the only platform-independent interface we have
for making GraphicsContext with a buffer that you can draw into. And certainly
"drawing" is the way we find out the color of a pixel on the webpage. To reuse
the code we already have maybe we would use use ImageBuffer::sinkIntoImage and
then call Image:: singlePixelSolidColor on that image.

If we want to optimize this further we could write an
ImageBuffer::singlePixelColor function and have it do the
sinkIntoImage/singlePixelSolidColor operations. This would make it easier to
optimize later for specific types of ImageBuffer by making this a virtual
function if needed. We could specialize it so it can do the work without
necessarily creating an Image.

But all of this seems a bit inefficient when repeated five times. Maybe we’d
want to paint once into a buffer that was not a single pixel, and then sample
the five pixels out of it?

Anyway, I’m OK with the current code but I think these are worth considering.

> Source/WebCore/dom/Document.cpp:3971
> +	       double redPercentDifference = fabs(snapshots[i].red -
snapshots[i - 1].red) / 255.0;
> +	       double greenPercentDifference = fabs(snapshots[i].green -
snapshots[i - 1].green) / 255.0;
> +	       double bluePercentDifference = fabs(snapshots[i].blue -
snapshots[i - 1].blue) / 255.0;
> +	       double alphaPercentDifference = fabs(snapshots[i].alpha -
snapshots[i - 1].alpha) / 255.0;
> +	       percentageDifferences[i - 1] = redPercentDifference +
greenPercentDifference + bluePercentDifference + alphaPercentDifference;

This is another example of something that needs a helper function. Writing it
out like this without a named function doesn’t seem good.

We’d want a function that takes two colors and returns the percentageDifference
rather than 5 lines of code that does the math. I’m not sure why we call this
"percent" since there is no 100 involved here. The word "percent" should really
be reserved for things in the range of 0-100. Should think further on what the
correct terminology for this is. Maybe discuss with Sam who has been thinking a
lot about color terminology lately.

> Source/WebCore/dom/Document.cpp:4000
> +    uint32_t averageRed = 0;

It’s strange to name something that will contain the sum of red channels
"average". Typically "average" would refer to a mean, not to a sum. I would
call this total, or sum.

> Source/WebCore/dom/Document.cpp:4003
> +    uint32_t averageAlpha = 0;

I don’t think this will actually work. First of all, I’m not sure we have the
right kind of code to actually draw into a buffer and give us alpha values. I’m
pretty sure that the buffer starts with an opaque background and so the alpha
will always be 1. If it wasn’t, not sure that a mean is the way we’d want to
blend the alphas together.

> Source/WebCore/dom/Document.cpp:4013
> +    auto validSnapshotCount = nonMatchingColorIndex == numSnapshots + 1 ?
numSnapshots : numSnapshots - 1;

Would be clearer to make this count by incrementing each time we add in to the
sums.

But also, I suggest we write a separate function that computes the mean of a
set of colors rather than writing it in-line here in this function. We can
remove the non-matching color index from the array (and yes, move the items
down to compact), and then pass this array of colors in to that function and
let it do the work. That’s the kind of thing that makes the code readable.

> Source/WebCore/dom/Document.cpp:4014
> +    float convertToDecimal = validSnapshotCount * 255;

We don’t want to use a verb to name a variable. I know this is a number we plan
to use to divide to convert a sum into a float mean, but that doesn’t mean the
number should be named "convert". If we are going to use this 4 times it’s
better to make it something that we can use to multiply rather than divide,
since multiplication is significantly more efficient than dividing. So it would
be better to have this be:

    float multiplier = 1.0f / (validSnapshotCount * 255).

> Source/WebCore/dom/Document.cpp:4015
> +    m_sampledPageTopColor =
makeFromComponentsClamping<SRGBA<float>>(averageRed / convertToDecimal,
averageGreen / convertToDecimal, averageBlue / convertToDecimal, averageAlpha /
convertToDecimal);

We are doing the math correctly here, so no clamping is necessary. There’s no
way that these mean color channel values will somehow be outside the 0-1 range.