Closed Bug 1442355 Opened 2 years ago Closed 2 years ago

Add a getter for custom color transforms

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Web extensions will need a way to know if there is a currently applied filter.
Figured kats would be the best reviewer because he did the previous setter.
Attachment #8955278 - Flags: review?(bugmail)
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.
Attachment #8955278 - Flags: review?(bugmail) → review-
All good points! New patch with smaug as reviewer.
Attachment #8955303 - Flags: review?(bugs)
Attachment #8955278 - Attachment is obsolete: true
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
Attachment #8955303 - Flags: review?(bugs) → review+
Attachment #8955303 - Attachment is obsolete: true
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84dfd63e8aaf
Add a getter for docshell's colormatrix. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/84dfd63e8aaf
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.