Bug 224987

Summary:

experiment with averaging sampling colors across the top of the page as the scroll area background

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

New Bugs

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, cdumez, changseok, darin, esprehn+autocc, ews-watchlist, fred.wang, glenn, hi, kangil.han, kondapallykalyan, pdr, sam, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

225167, 225942

Attachments:

Description	Flags
Patch	none
Patch	darin: review+
Patch	none
Patch	none

Devin Rousso

Reported 2021-04-23 10:53:11 PDT

instead of relying on the CSS `background-color` of the `<html>`/`<body>` we maybe might want to sample colors from the top of the page, such as for if the page has an image/gradient background

Attachments
Patch (50.11 KB, patch) 2021-04-23 11:03 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (50.11 KB, patch) 2021-04-23 11:06 PDT, Devin Rousso	darin: review+	Details Formatted Diff Diff
Patch (50.87 KB, patch) 2021-04-28 15:55 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (52.34 KB, patch) 2021-04-28 16:09 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2021-04-23 10:53:31 PDT

<rdar://problem/76251889>

Devin Rousso

Comment 2 2021-04-23 11:03:10 PDT

Created attachment 426926 [details] Patch

Devin Rousso

Comment 3 2021-04-23 11:06:39 PDT

Created attachment 426928 [details] Patch fix style issue

Darin Adler

Comment 4 2021-04-23 13:16:44 PDT

Comment on attachment 426928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426928&action=review > Source/WebCore/dom/Document.cpp:3913 > + IntSize size(1, 1); Not sure that 1x1 is the best way to do this. We want to use actual pixels, which are not necessarily 1x1. On a 2x display this rectangle covers 4 device pixels. Is this what we want? > Source/WebCore/dom/Document.cpp:3917 > + constexpr OptionSet<HitTestRequest::RequestType> hitTestRequestTypes { HitTestRequest::ReadOnly, HitTestRequest::DisallowUserAgentShadowContent }; > + HitTestResult hitTestResult(location); > + 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. > 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.

Devin Rousso

Comment 5 2021-04-23 20:24:42 PDT

Comment on attachment 426928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426928&action=review >> Source/WebCore/dom/Document.cpp:3913 >> + IntSize size(1, 1); > > Not sure that 1x1 is the best way to do this. We want to use actual pixels, which are not necessarily 1x1. On a 2x display this rectangle covers 4 device pixels. Is this what we want? Good point. I would like to just keep it as single physical pixels, as I'm not looking at more than one pixel. I'll look into if I can make `snapshotFrameRect` take a `FloatRect` instead :) >> 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. >> 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.

Darin Adler

Comment 6 2021-04-25 08:48:05 PDT

Comment on attachment 426928 [details] 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.

Darin Adler

Comment 7 2021-04-25 08:49:15 PDT

Wait, where’s the testing for this new function? Please don’t land all this code without any tests!

Darin Adler

Comment 8 2021-04-25 09:05:58 PDT

Comment on attachment 426928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426928&action=review >>>> 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. Looking into third-party frame painting, I found mechanism that already exists (actually more than one, but this seemed the best) that could be used to do this with a little bit of refactoring and rearranging. There’s already something called SecurityOriginPaintPolicy that is passed in to FrameView::paintContents, but FrameView::paintContentsForSnapshot always passes SecurityOriginPaintPolicy::AnyOrigin. We could probably get what we want, omitting the third party frames, if we can get the policy to be to be SecurityOriginPaintPolicy::AccessibleOriginOnly. Maybe do this by adding a SnapshotOptions flag, and also adding an argument to paintContentsForSnapshot. Unfortunately, because we are using SnapshotOptionsInViewCoordinates, which sends us down a different code path, it might be a little bit harder to support that. But maybe that flag is not quite right. I think we want the top of the document, not the top of the view, and we don’t want to include things like scrollbars.

Devin Rousso

Comment 9 2021-04-26 16:30:29 PDT

Comment on attachment 426928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426928&action=review >>>>> 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. > > Looking into third-party frame painting, I found mechanism that already exists (actually more than one, but this seemed the best) that could be used to do this with a little bit of refactoring and rearranging. There’s already something called SecurityOriginPaintPolicy that is passed in to FrameView::paintContents, but FrameView::paintContentsForSnapshot always passes SecurityOriginPaintPolicy::AnyOrigin. We could probably get what we want, omitting the third party frames, if we can get the policy to be to be SecurityOriginPaintPolicy::AccessibleOriginOnly. Maybe do this by adding a SnapshotOptions flag, and also adding an argument to paintContentsForSnapshot. > > Unfortunately, because we are using SnapshotOptionsInViewCoordinates, which sends us down a different code path, it might be a little bit harder to support that. But maybe that flag is not quite right. I think we want the top of the document, not the top of the view, and we don’t want to include things like scrollbars. The idea behind this is to completely bail if the snapshot were to happen inside a 3rd party `<iframe>` as in that situation we don't have a solid (or close to it) color across the top of the page (it is possible for the 3rd party `<iframe>` to have the same color as the rest of the page, but IMO if we allowed for that scenario that would blur the line of "what is page content and what isn't"). We don't want to get the color underneath because, as you said, that's not something the user would ever see. I totally forgot about `SnapshotOptionsInViewCoordinates`! I'd added that in there when first trying to get this to work as a "I wonder if this would make a difference". I'll remove it. >>>> 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. Yeah it'd be nice if there was a simpler way to say "get the color of the pixel at this location". I did some testing and `ImageBuffer::toBGRAData` appears to work as an alternative. I'm currently trying to change this to use `WebCore::Color` instead with something like ``` auto image = ImageBuffer::sinkIntoImage(WTFMove(snapshot)); return image->singlePixelSolidColor(); ``` to see whether that code would be easier to read and to maybe/hopefully leverage more of the existing code around `Color`. >> Source/WebCore/dom/Document.cpp:3971 >> + 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. Yeah I agree that "percent" isn't the best word here. IMO it sounded better than "Ratio" (e.g. `SampledPageTopColorAdjacentRatioMaxDifference`) or "Fractional" (e.g. `SampledPageTopColorMaxAdjacentFractionalDifference`). My only "defense" of using "percent" is that it fits when discussing the logic verbally (e.g. "the colors are 2% different" even though it's expressed as a decimal). I'll reach out to Sam and see if he has any ideas. In the meantime, do you have any suggestions? >> 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. Oh wow yeah good points. I'll remove the logic around alpha. >> 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. I was trying to avoid having to do a move/copy, but if it makes the code easier to read I could just use a `Vector`. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SampledPageTopColor.mm:125 > +TEST(SampledPageTopColor, ZeroMaxPercentDifference) (In reply to Darin Adler from comment #7) > Wait, where’s the testing for this new function? > > Please don’t land all this code without any tests! These are the API tests I added for this new code.

Darin Adler

Comment 10 2021-04-26 16:48:35 PDT

Comment on attachment 426928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426928&action=review >>> 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. > > I was trying to avoid having to do a move/copy, but if it makes the code easier to read I could just use a `Vector`. Sorry, please don’t use Vector. I was thinking more the classes for dealing with color components. For a fixed size thing it would be std::array, but I wasn’t suggesting that. I don’t think "avoiding copying" is important enough to make the code this confusing. Instead I think you should do the remove operation in the array. My idea is that the mean function would be like this: Color meanColor(const Color colors[], unsigned numColors) But details and types might be different. Not even sure that "mean" is the right way to average together a set of pixels.

Darin Adler

Comment 11 2021-04-26 16:50:40 PDT

Comment on attachment 426928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426928&action=review >>>>>> 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. >> >> Looking into third-party frame painting, I found mechanism that already exists (actually more than one, but this seemed the best) that could be used to do this with a little bit of refactoring and rearranging. There’s already something called SecurityOriginPaintPolicy that is passed in to FrameView::paintContents, but FrameView::paintContentsForSnapshot always passes SecurityOriginPaintPolicy::AnyOrigin. We could probably get what we want, omitting the third party frames, if we can get the policy to be to be SecurityOriginPaintPolicy::AccessibleOriginOnly. Maybe do this by adding a SnapshotOptions flag, and also adding an argument to paintContentsForSnapshot. >> >> Unfortunately, because we are using SnapshotOptionsInViewCoordinates, which sends us down a different code path, it might be a little bit harder to support that. But maybe that flag is not quite right. I think we want the top of the document, not the top of the view, and we don’t want to include things like scrollbars. > > The idea behind this is to completely bail if the snapshot were to happen inside a 3rd party `<iframe>` as in that situation we don't have a solid (or close to it) color across the top of the page (it is possible for the 3rd party `<iframe>` to have the same color as the rest of the page, but IMO if we allowed for that scenario that would blur the line of "what is page content and what isn't"). We don't want to get the color underneath because, as you said, that's not something the user would ever see. > > I totally forgot about `SnapshotOptionsInViewCoordinates`! I'd added that in there when first trying to get this to work as a "I wonder if this would make a difference". I'll remove it. Doing 5 different hit tests just smells like the wrong solution to me, for this problem. I think it’s worth doing more thinking about how to achieve the goal (don't use the color of an embedded website) to avoid the hit testing. I’m not even sure that the check for HTMLIFrameElement is the correct rule.

Sam Weinig

Comment 12 2021-04-26 17:37:23 PDT

Comment on attachment 426928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426928&action=review > Source/WebCore/dom/Document.cpp:3896 > + auto maxPercentDifference = page->settings().sampledPageTopColorMaxPercentDifference(); This should just be settings()->sampledPageTopColorMaxPercentDifference() > Source/WebCore/dom/Document.cpp:3955 > + auto snapshot = getPixelColorAtTopX(frameWidth * i / (numSnapshots - 1)); This seems pretty inefficient since snapshots will cause a FrameView::paintContentsForSnapshot(), paintContents(), etc. I think you are better off doing a single snapshot if can? generally, it seems a bit worrisome to be doing this many paints here. >>> Source/WebCore/dom/Document.cpp:3971 >>> + 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. > > Yeah I agree that "percent" isn't the best word here. IMO it sounded better than "Ratio" (e.g. `SampledPageTopColorAdjacentRatioMaxDifference`) or "Fractional" (e.g. `SampledPageTopColorMaxAdjacentFractionalDifference`). My only "defense" of using "percent" is that it fits when discussing the logic verbally (e.g. "the colors are 2% different" even though it's expressed as a decimal). > > I'll reach out to Sam and see if he has any ideas. In the meantime, do you have any suggestions? If you want a sRGB pixel in 0-1, I recommend converting. auto ithSnapshot = convertColor<SRGBA<float>>(snapshots[i]); auto ithMinusOneSnapshot = convertColor<SRGBA<float>>(snapshots[i - 1]); // Or you could add an operator- to ColorComponents. auto difference = mapColorComponents([] (auto a, auto b) { return a - b }, asColorComponents(ithSnapshot), asColorComponents(ithMinusOneSnapshot)); That said, I would recommend using a color space like Lab to do differences, as getting the difference between colors is a known problem (look up "Delta E" for more), and sRGB is not a great color space for it. In general, it would be good to avoid using sRGB directly anywhere in WebCore. I am working to remove existing usages, as they all represent code that doesn't work correctly with larger gamuts (e.g. P3, etc). >> 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. You can just do m_sampledPageTopColor = SRGBA<uint8_t> { averageRed, averageGreen, ... }. There doesn't seem to be a compelling reason to convert to float manually here.

Sam Weinig

Comment 13 2021-04-26 17:39:55 PDT

> 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. Seems like the render tree walk done in the paint should handle this rather than doing both a hit test and paint. Perhaps a new PaintBehavior bit if we don't have one already.

Darin Adler

Comment 14 2021-04-26 17:41:44 PDT

(In reply to Sam Weinig from comment #12) > That said, I would recommend using a color space like Lab to do differences, > as getting the difference between colors is a known problem (look up "Delta > E" for more), and sRGB is not a great color space for it. Hooray, that is such a great suggestion. Devin may need tips on how to do that correctly. > You can just do m_sampledPageTopColor = SRGBA<uint8_t> { averageRed, > averageGreen, ... }. There doesn't seem to be a compelling reason to convert > to float manually here. There seems a small benefit to using float: probably gives us rounding instead of truncating? But we can also do the integer average computation in a way that does rounding.

Devin Rousso

Comment 15 2021-04-28 15:55:34 PDT

Created attachment 427301 [details] Patch Thanks for all the feedback! I've switched to using `Lab<float>`, but I wasn't able to get end-to-end usage of it as AFAICT it doesn't seem like `snapshotFrameRect` supports outputting to a non-sRGB `ImageBuffer`. Since this is an off-by-default feature that we're currently experimenting with, I'm going to land it with FIXMEs for <https://webkit.org/b/225167> (Sampled Page Top Color: hook into painting logic instead of taking snapshots) and tackle the issues around hit testing not being consistent with painting and supporting non-sRGB colors there (assuming that we do in fact want to do this, i.e. un-experimentialize).

Devin Rousso

Comment 16 2021-04-28 16:09:58 PDT

Created attachment 427302 [details] Patch oops forgot to update ChangeLog

EWS

Comment 17 2021-04-28 17:51:57 PDT

Committed r276744 (237144@main): <https://commits.webkit.org/237144@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427302 [details].

Radar WebKit Bug Importer

Comment 18 2021-04-28 17:52:16 PDT

<rdar://problem/77294992>

Darin Adler

Comment 19 2021-04-29 08:16:24 PDT

Comment on attachment 427302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427302&action=review I have some comments that should not block landing this patch, but I think this still has significant room for improvement. > Source/WebCore/dom/Document.cpp:3282 > + determineSampledPageTopColor(); It does not seem even remotely OK to add this side effect to a function named enqueuePaintTimingEntryIfNeeded! We should figure out how and when the loading process should trigger this sampling. It’s really rubbing me the wrong way to build this into the Document class, by the way. Not right for the separation of responsibilities. > Source/WebCore/dom/Document.cpp:3909 > +void Document::determineSampledPageTopColor() We should see what we can do to move this out of document. This includes all sorts of high level stuff like snapshotting that doesn’t belong as a member function of Document itself. The one benefit this gets from being a member of Document is storing the color inside the document. But I’m not sure that benefit is good enough to justify putting all the code here. I think we should look for a better place to put it. > Source/WebCore/dom/Document.cpp:3933 > + IntPoint location(x, 0); > + IntSize size(1, 1); I suggest we just use an IntRect for this instead of two separate locals. What happened to the research we wanted to do about how scale factor affects this? > Source/WebCore/dom/Document.cpp:3956 > + // Bail if the non-matching color is not the first or last snapshot, or there already is an non-matching color. What’s the rationale for allowing only 1 non-matching color of 5, and requiring that it be the first or last of the 5? I don’t see anything here that explains how we chose that. I hope we’re not designing our algorithm around not wanting to delete an item from the middle of an array of 5! > Source/WebCore/dom/Document.cpp:8749 > +bool Document::isHitTestLocationThirdPartyFrame(const HitTestLocation& location) This helper should just be a function that takes a document, not a member function of Document. It should be grouped with other hit testing functions, not in the Document class itself. Doing this is following a path that just makes Document more and more into a "god class" if we add general purpose functions like this at the document level. > Source/WebCore/dom/Document.cpp:8757 > + if (!hitTestNode) > + return false; This isn’t needed. The is<> function already checks for null. > Source/WebCore/dom/Document.cpp:8762 > + return areRegistrableDomainsEqual(downcast<HTMLIFrameElement>(*hitTestNode).location(), m_url); I would have expected this to use a SecurityOrigin function instead of calling areRegistrableDomainsEqual directly.

Note You need to log in before you can comment on or make changes to this bug.