danwent / Perspectives

Perspectives Firefox Extension
http://perspectives-project.org
66 stars 19 forks source link

Replace base64.js with atob() and btoa() #133

Closed ghost closed 10 years ago

ghost commented 10 years ago

Is there a reason for the existence of the base64.js file? base64 encoding/decoding can be done with window.atob() and window.btoa() respectively. Mozilla mentions a "Unicode problem" with these functions, but provides a minimalistic workaround:

function utf8_to_b64( str ) {
  return window.btoa(unescape(encodeURIComponent( str )));
}

function b64_to_utf8( str ) {
  return decodeURIComponent(escape(window.atob( str )));
}

Besides, there is the following note in base64.js

// NOTE: I got rid of the utf8 encoding done by the original code, as we 
// need to replicate base64 encoding/decoding of ASCII strings

Can I remove it?

daveschaefer commented 10 years ago

Tracing the code, you can see we currently call encode() when parsing results from a notary:

Perspectives.notaryAjaxCallback() Pers_xml.parse_server_node() Pers_util.add_der_signature_header() Pers_Base64.encode()

However, I'm not sure that means we necessarily need to keep it. If we can provide the same functionality by using the wrapped utf8_to_b64() and b64_to_utf8() functions, replacing our current encode/decode seems fine to me. Let's add some unit tests and make sure they work before and after the change.

One reason we might want to keep them - do other browsers like Chrome expose an equivalent of window.atob() and window.btoa() that we could use? If not we may have to put something in again when we become less Firefox-centric.

Were you wanting to remove this simply because it would mean less hand-coded functionality that could already be built-in?

Not sure if @danwent has any thoughts on the original change and if this is equivalent.

ghost commented 10 years ago

One reason we might want to keep them - do other browsers like Chrome expose an equivalent of window.atob() and window.btoa() that we could use?

Yes, look here: CanIUse atob-btoa MDN atob -> Browser compatibility

Were you wanting to remove this simply because it would mean less hand-coded functionality that could already be built-in?

Yes.

daveschaefer commented 10 years ago

caniuse.com

Great site!

Alright, if we can completely replace these I'm all for removing them. Thanks for digging this up. Let's add some unit tests and make the swap.

daveschaefer commented 10 years ago

This was fixed in version 4.6. Closing!