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

<amp-iframe> Proper base64 encoding of srcdoc #6101

Merged
merged 1 commit into from Nov 9, 2016

Conversation

aghassemi
Copy link
Contributor

Closes #6098

@aghassemi aghassemi merged commit 2df088e into ampproject:master Nov 9, 2016

// srcdoc is a DOMString and requires special handling for base64 encoding.
// See https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64/Base64_encoding_and_decoding
const encodedSrcdoc = btoa(encodeURIComponent(srcdoc)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it belongs in the base64 library, and used with our other encoding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoot, this function can replace utf8Encode and the stringToBytes stuff in bytes, and the base64 encode decode!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will send out a separate refactor CL.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might look prettier for encoding unicode?
@jridgewell Why can this replace base64 encode decode? base64Encode is simply btoa that encode ascii string (not unicode).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@muxin @jridgewell Here is what I am thinking to do:

1- Change the iframe to use utf8Encode from bytes.js and then pass it to base64EncodeFromBytes from base64.js. This should be the fastest way to do this specially for browsers that have native TextEncoder support.

2- Remove base64Encode and base64Decode from base64.js They are not used anywhere and are confusing (same way btoa is confusing) in terms of what input string are valid.

thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

utf8Encode is async indeed. How about we change base64Encode to support unicode encoding:

export function base64Encode(str) {
  return btoa(unescape(encodeURIComponent(str)));
}
export function base64Decode(str) {
  return decodeURIComponent(escape(atob(str)));
}

And base64Encode can be called in amp-iframe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unescape is deprecated and that inner function simply does what unescape does. Otherwise both solutions are the same.

I wonder why utf8Encode in bytes wasn't polyfilled with this approach when TextEncoder is not available. That would have kept it sync. @lannka Do you know?

Copy link
Contributor

Choose a reason for hiding this comment

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

Likely didn't know about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I reviewed that code but honestly I didn't know this better approach here. we can do a refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a few designs and decided mixing base64 encoding and string encoding does not make sense since they are completely different concepts. I will send a PR soon with the code and the reasoning behind the design.

// srcdoc is a DOMString and requires special handling for base64 encoding.
// See https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64/Base64_encoding_and_decoding
const encodedSrcdoc = btoa(encodeURIComponent(srcdoc)
.replace(/%([0-9A-F]{2})/g, function(match, p1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function can be hoisted out.

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Nov 15, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants