Correct PushMessageData #382
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
saschanaz
reviewed
Jul 9, 2024
saschanaz
reviewed
Jul 9, 2024
There was a problem hiding this comment.
It seems PushMessageData is only set for PushEvent if any buffer exists and otherwise data becomes null, and PushMessageData itself does not expect null. Gecko does not expect null either, so it matches the implementation. (I assume you checked WebKit, and my quick check says WebKit does not either.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Give PushMessageData a clear internal concept to hold the bytes and also make it clear that it cannot be null. None of the methods were prepared to handle it being null and nothing ever creates a PushMessageData object where it is null.
Partially addresses the concerns in #380 and fixes #381.
I installed tidy-html5 but that appears to not work. There are no instructions provided by this repository as to how to do formatting so I guess I'm fine with that happening in an automatic follow-up cleanup commit...
One thing that I'm not familiar with is ReSpec and how it does linking so that probably needs careful review here as I'm trying to introduce some new links.
Preview | Diff