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

Eliminate space counted as message length #23451

Merged
merged 8 commits into from Jan 14, 2023
Merged

Conversation

guest271314
Copy link
Contributor

When separators=(',', ':') is not passed to json.dumps() parameter the length for input from JavaScript (which is serialized to a JSON-like format by the application) port.postMessage(new Array(174763)); is read in Python as

1048578

which is greater than 1024*1024. Essentially space character formatting were being encoded as part of the length of the JSON.

When the length should be

873816

when formatting space characters are excluded from the JSON.

Solved by passing separators=(',', ':') for “compact encoding” json — JSON encoder and decoder — Python 3.11.1 documentation.

Now we can write 1MB with

port.postMessage(new Array(209715));

from JavaScript and omit including and counting formatting space characters as part of encoded message length.

Description

Motivation

Additional details

Related issues and pull requests

When separators=(',', ':') is not passed to json.dumps() parameter the length for input from JavaScript (which is serialized to a JSON-like format by the application) port.postMessage(new Array(174763)); is read in Python as

1048578

which is greater than 1024*1024. Essentially space character formatting were being encoded as part of the length of the JSON.

When the length should be 

873816

when formatting space characters are excluded from the JSON.

Solved by passing separators=(',', ':') for “compact encoding” json — JSON encoder and decoder — Python 3.11.1 documentation.

Now we can write 1MB with

port.postMessage(new Array(209715));

from JavaScript and omit including and counting formatting space characters as part of encoded message length.
@guest271314 guest271314 requested a review from a team as a code owner January 7, 2023 07:47
@guest271314 guest271314 requested review from willdurand and removed request for a team January 7, 2023 07:47
@github-actions github-actions bot added Content:Other Any docs not covered by another "Content:" label Content:WebExt WebExtensions docs labels Jan 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2023

Preview URLs

(comment last updated: 2023-01-14 19:31:00)

@guest271314 guest271314 changed the title Eliminate white space counted as message length Eliminate space counted as message length Jan 7, 2023
# https://docs.python.org/3/library/json.html#basic-usage
# To get the most compact JSON representation, you should specify
# (',', ':') to eliminate whitespace.
encodedContent = json.dumps(messageContent, separators=(',', ':')).encode('utf-8')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
encodedContent = json.dumps(messageContent, separators=(',', ':')).encode('utf-8')
encoded_content = json.dumps(message_content, separators=(',', ':')).encode('utf-8')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@Josh-Cena
Copy link
Member

Josh-Cena commented Jan 7, 2023

Could you update the examples repo PR as well?

I realized they use different casing styles. That sucks a bit. We can wait for @Rob--W to review and merge both PRs.

@Josh-Cena Josh-Cena dismissed their stale review January 7, 2023 20:00

Review addressed; waiting for maintainer opinion

@Josh-Cena Josh-Cena requested a review from Rob--W January 7, 2023 20:00
@guest271314
Copy link
Contributor Author

Could you update the examples repo PR as well?

We're working that out now mdn/webextensions-examples#510. Not sure what the hesitancy is. The script in its current form is not so clearly, yet still broken. Feel free provide your feedback there too.

@Josh-Cena
Copy link
Member

The motivation is convincing and the code is also clear. I don't have enough domain knowledge to review it, but everything LGTM at a glance from an engineering perspective.

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Please address the issue below together with the feedback in mdn/webextensions-examples#510.

@guest271314
Copy link
Contributor Author

@Rob--W Kindly review the changes made.

@willdurand willdurand removed their request for review January 12, 2023 14:48
Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Some of the requested changes from my previous review haven't been made yet.

@guest271314

This comment was marked as abuse.

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. For consistency I'd also use /usr/bin/env -S python2 -u, but that's a pre-existing issue.

@guest271314
Copy link
Contributor Author

Looks good, thanks. For consistency I'd also use /usr/bin/env -S python2 -u, but that's a pre-existing issue.

Yes, that would be optimal to avoid ambiguity. Why are we even still deploying examples using python2 when Python 2.X is officially deprecated per Python?

@guest271314
Copy link
Contributor Author

Looks good, thanks. For consistency I'd also use /usr/bin/env -S python2 -u, but that's a pre-existing issue.

Updated.

@Rob--W Rob--W merged commit 60a1482 into mdn:main Jan 14, 2023
@guest271314 guest271314 deleted the patch-2 branch January 14, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Other Any docs not covered by another "Content:" label Content:WebExt WebExtensions docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants