1442355 - Add a getter for custom color transforms

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P3)

Description

[Eitan Isaacson [:eeejay]]

Web extensions will need a way to know if there is a currently applied filter.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Add a getter for docshell's colormatrix. r?kats

Figured kats would be the best reviewer because he did the previous setter.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8955278 [details] [diff] [review]
Add a getter for docshell's colormatrix. r?kats

Review of attachment 8955278 [details] [diff] [review]:
-----------------------------------------------------------------

Comments below, but I'd prefer if you got review from somebody else - there might code conventions to follow with IDL implementations that I'm not totally up to speed on. e.g. looking at [1] they use a bunch of macros which looks like a good idea but I don't know if it's required in this code or not.

[1] https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/accessible/xpcom/xpcAccessibleTable.cpp#293

::: docshell/base/nsDocShell.cpp
@@ +14532,5 @@
> +  if (mColorMatrix) {
> +    *aMatrixLen = 20;
> +    *aMatrix = (float*)moz_xmalloc(*aMatrixLen * sizeof(float));
> +    if (!*aMatrix)
> +      return NS_ERROR_OUT_OF_MEMORY;

In the error case we should make sure we exit with *aMatrixLen = 0 and *aMatrix = nullptr. Right now we will exit with *aMatrixLen=20 which is not great.

Also braces on the if condition.

@@ +14533,5 @@
> +    *aMatrixLen = 20;
> +    *aMatrix = (float*)moz_xmalloc(*aMatrixLen * sizeof(float));
> +    if (!*aMatrix)
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    MOZ_ASSERT(sizeof(*aMatrix) == sizeof(mColorMatrix->components));

*aMatrix is a float* which is going to have a different size than a float[20], so this assertion will fail.

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Add a getter for docshell's colormatrix. r?smaug

All good points! New patch with smaug as reviewer.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8955303 [details] [diff] [review]
Add a getter for docshell's colormatrix. r?smaug

>+    MOZ_ASSERT(sizeof(float)*20 == sizeof(mColorMatrix->components));
space before and after *

>+    *aMatrixLen = 20;
>+    memcpy(*aMatrix, mColorMatrix->components, sizeof(float)*20);
ditto

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Add a getter for docshell's colormatrix.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84dfd63e8aaf
Add a getter for docshell's colormatrix. r=smaug

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/84dfd63e8aaf