danwent / Perspectives

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

Migrate stringbundle use to wrapped interface #172

Closed daveschaefer closed 8 years ago

daveschaefer commented 8 years ago

There is a lot of code right now that uses document.getElementById() to create or use string bundles:

if(Perspectives.strbundle == null) {
    Perspectives.strbundle = document.getElementById("notary_strings");
}
// ...
Perspectives.strbundle.getString("notaryLookupFor")

In several places we have extra checks to make sure the strbundle object is not null before taking any action. Yet despite these checks, in some places the object is still null because the XUL object its expecting hasn't been loaded yet.

This is the wrong way to retrieve strings from stringbundles. Instead we should use the nsIStringBundleService as described here - https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Miscellaneous#Using_string_bundles_from_JavaScript

That makes sure we can always retrieve the string and the value won't be null. In addition it means callers don't have to worry about the internal state of string bundles or how they work. The caller shouldn't have to worry about internal state. The system that deals with retrieving strings should handle its own state and wrap/hide it, so callers don't need to worry.

Included in the 4.6.4 release I have added wrapper functions so you can call e.g. Perspectives.getString() or Perspectives.getFormattedString() to retrieve the string any time and not worry about state.

After the 4.6.4 release is out we should go through the code and clean up all of these uses, to avoid having each caller track internal state.

This would also help pave the way towards isolating all browser-specfic functionality behind an interface, so it can be easily implemented and swapped (i.e. so we can port Perspectives to another browser) - i.e. part of the work for #75 .

daveschaefer commented 8 years ago

Fixed in #175