Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure replacement color is positive #505

Merged
merged 1 commit into from
Apr 20, 2021

Conversation

andreban
Copy link
Member

No description provided.

@andreban andreban requested a review from nohe427 April 20, 2021 14:52
@@ -47,7 +47,7 @@ export class ImageHelper {
// See https://github.com/GoogleChromeLabs/bubblewrap/issues/488#issuecomment-806560923.
if (backgroundColor && image.hasAlpha()) {
// The replacement colour is the same as the background colour, but fully transparent.
const replacementColor = (backgroundColor.rgbNumber() << 8) & 0xFFFFFF00;
const replacementColor = ((backgroundColor.rgbNumber() << 8) & 0xFFFFFF00) >>> 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads like magic to me (combined with there being no commit message). I'm assuming it's something to do with promoting the integer type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR - bitten by the lack of unsigned integers :D.

  • 0xFFFFFF00 is 4294967040, right?
  • However, if you do 0xFFFFFF << 8, you get -256.
  • So, we force it to be interpreted as a positive int with (0xFFFFFF << 8) >>> 0

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the data type before and after the >>> 0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

number...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding more detailed comments on the next PR

@andreban andreban merged commit aa41b64 into GoogleChromeLabs:main Apr 20, 2021
@andreban andreban added the bug Something isn't working label May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants