Closed Bug 549315 Opened 14 years ago Closed 14 years ago

Implement Localization service for Jetpack

Categories

(Add-on SDK Graveyard :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbuchner, Assigned: zbraniecki)

References

Details

Attachments

(3 files, 9 obsolete files)

Assignee: nobody → gandalf
working on that
Status: NEW → ASSIGNED
Target Milestone: -- → 0.3
Attached file common pool API v1 (obsolete) —
This is the first version of a common pool API.

It provides a basic structure of the library, with one exposed method (get), one property (locale) and a singleton service for entity management in a form of:

server -> storage -> memory

The API itself should remain as simple as it is right now, with a potential extension in form of a second argument to get(s, {}) in which any context information will be provided - such extension will be backward compatible and will not break the API.

I have a simple server written together with some tools for extracting entities for jetpack, but that's not a 0.3 material at all.

for reviewer: except of routine pattern check, I'd like you to confirm that this setup gives me across-the-house singleton service that will not be rebooted when new jetpack with the same code is loaded and can be updated by updating the contract ID.

I did not include any updateMemory|Storage code because getStorageConn fails on pulling ProfD :) 

File "resource://testpack-l10n-lib/l10n.js", line 57, in getStorageConn
    .get("ProfD", Ci.nsIFile);
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///Users/zbraniecki/projects/mozilla/jetpack/jetpack-sdk/packages/jetpack-core/lib/securable-module.js -> resource://testpack-l10n-lib/l10n.js :: getStorageConn :: line 57"  data: no]
Attachment #439654 - Flags: review?
this toolset allows for entity extracting from jetpacks and uplodaing to the site that later exposes them to jetpack services.
Attached file simple usecase example
this is an example jetpack code that uses common pool API
Attachment #439654 - Flags: review? → review?(myk)
Attachment #439654 - Flags: review?(myk)
Attachment #439654 - Flags: review-
Attachment #439654 - Flags: feedback?(avarma)
Comment on attachment 439654 [details]
common pool API v1

This file needs to have a license header like the one at the top of the tab-browser module <http://hg.mozilla.org/labs/jetpack-sdk/file/52d2fc691038/packages/jetpack-core/lib/tab-browser.js> (except with your name rather than Atul's, of course).

It would also be better to provide it as a patch against the latest trunk, with the module being located in packages/jetpack-core/lib/.  And it needs to include tests of the functionality, which should be located in packages/jetpack-core/tests/.


>/**
> * Sets locale to a given locale code
> *
> * @param   code       {String}
> *          locale code
> */
>exports.__defineSetter__('locale', function(code) {
>  locale = code;
>})
>exports.__defineGetter__('locale', function() {
>  return locale;
>})

It's not clear what the use case is for this part of the API.  If the goal is to enable addons to dynamically change the user locale for the entire application (including core application code as well as addons), then that seems like worthwhile functionality, but it would fit better into a high-level "app" API that provides access to application-wide state.

It's also something that it would make more sense to commit once the functionality works rather than just as a stub, as it is here.


>  this.handler = xpcom.register({name: "L10n Common Pool Service",
>                                 contractID: contractID,
>                                 create: CommonPoolService,
>                                 categories: [{service: true}]});

If I understand this correctly, the first addon that gets loaded instantiates an instance of CommonPoolService in a context, and then any additional addons that get loaded reuse the existing instance.

That seems to contradict the overall Jetpack model in which each module used by an addon is loaded only for that addon, and I wonder about its consequences.  I know the goal here is to prevent addons from all requesting the same set of resources, which is a worthy goal.  I just want to make sure we consider all the issues.

One issue that comes to mind is versioning, which it seems like we could do by updating the version in the contract ID if we needed to make breaking changes to the implementation down the road without breaking existing extensions.  Another question that comes to mind is security, and there I'm not sure what the issues are.

Requesting feedback from Atul on this.


> I did not include any updateMemory|Storage code because getStorageConn fails
> on pulling ProfD :) 

Hmm, I'm not sure what's going on there.  At first glance, the code doesn't seem incorrect.  It looks like we'll need to dig into this further.
Attached patch common pool API v2 (obsolete) — Splinter Review
updated to myk's comments.

I removed .locale API for now and I removed the Storage related API until we solve it.

Instead, I added a simple synchronous XHR call done once per service initialization that can load Pool if service url is given.

This will allow me to set up a simple server that people can use to test the API.

Rerequesting review
Attachment #439654 - Attachment is obsolete: true
Attachment #440220 - Flags: review?(myk)
Attachment #440220 - Flags: feedback?
Attachment #439654 - Flags: feedback?(avarma)
I've been doing some thinking about this, and here's a suggested series of steps to take to implement this functionality:

1. a simple common pool API implemented by a localization module that is designed to synchronously retrieve a common pool of localized strings from a remote server (which may not be available yet) and uses it to translate strings retrieved via the API; (this is the step we're at currently)

2. a unit test for the module that uses an HTTP server provided by the test harness (bug 561926) to serve a common pool to the module and then checks that the API correctly retrieves and returns localized strings from it;

3. asynchronous retrieval and local caching of the common pool, with strings translated via the local cache rather than by synchronously retrieving the local pool from the server;

4. a reference implementation of the common pool server running on a labs experimental server;

5. sharing of the local cache across installed addons.

These steps don't necessarily have to happen in exactly this order.  Step four, in particular, can be done in parallel, and Zandr is working on it over in bug 560470.  Also, it might be better to skip the synchronous stage and go directly to asynchronous retrieval and local caching of the common pool, which we'll need anyway before we can commit this API and make it available to addon developers.

Does this seem like a reasonable plan?
yup, that makes a lot of sense to me. The question is if you want to provide the API ASAP so that jetpack devs can get used to it (_() thing) in which case it may make sense to deploy step 1 and work on 2/3/5 while waiting on 4, or you want to wait for it to make sense in which case it doesn't make sense to deploy anything unless 4 is there.
Depends on: 562003
Target Milestone: 0.3 → 0.4
Cool. For the unit testing of talking-to-the-server, I would recommend using a mock/stub/fake object for the network request (XHR I assume?), since it results in faster tests and is also generally more flexible. A full-pipeline integration test or two would be nice, but it's important that the hundreds/thousands of unit tests we have run as fast as possible so we can keep developers' code-test cycles short.
I'm also working on a mock XHR object now for a web-based project, so let me know if you'd like to use it--I can make a bug for it and submit a patch.
that would be awesome to get!
Depends on: 562234
(In reply to comment #8)
> yup, that makes a lot of sense to me. The question is if you want to provide
> the API ASAP so that jetpack devs can get used to it (_() thing) in which case
> it may make sense to deploy step 1 and work on 2/3/5 while waiting on 4, or you
> want to wait for it to make sense in which case it doesn't make sense to deploy
> anything unless 4 is there.

I think we need either 1/2 or 1/4 for an initial implementation, although preferably we'll have 1/2/4.

I talked to Zandr on Friday, and he should have a server instance available soon.  Is your reference implementation reasonably complete enough for us to test an API against it?
Hmm, although probably we need 3 as well before we can check it in, as I don't think we want to be doing synchronous remote network calls.
Depends on: 560470
3 depends on bug 562003.
Comment on attachment 440220 [details] [diff] [review]
common pool API v2

I read through the patch, and this is not a review, just feedback!  But that's ok, as the patch will need to be updated anyway per the conversations we've been having.  Unsetting the review flag for now, and setting feedback+ to indicate that I've looked at this and provided feedback! (not sure if that's the right way to use the flag, though)

1. The patch uses an XPCOM service to create a singleton that handles retrieval and caching of the common pool, presumably because such a singleton could be shared across all addons installed in a user's Firefox profile, whereas a regular object used as a singleton would be specific to each addon and thus created as many times as there are addons installed that use this API.

This would be the first module that uses an XPCOM service in this fashion, and while it would conserve network, memory, and disk resources, it would also raise tricky issues, like how to dynamically unload the XPCOM service only when the last addon using the API has been uninstalled.

And there may be other ways to conserve those resources (like relying on the Mozilla web cache for network conservation), or, depending on how taxing the API proves to be, they may not need to be conserved in the first place (like disk space, given the relatively small data size and the ample disk available on most computers).

Thus I recommend we drop the constraint that multiple instances of this module share resources in the initial implementation of this API.  Instead, each instance of the module should retrieve the common pool from the server and cache it in memory and/or on disk in a way that doesn't interact with other instances of the module.

Then we can revisit the question as we identify resource consumption patterns that require conservation.


2. The patch loads the common pool when the module is first loaded.  Once the patch incorporates caching of the pool in local storage, it seems like the optimal approach would be to set a long-lived (i.e. cross-session) timer that periodically (once per day?) retrieves and caches the common pool in local storage, then load the local cache into memory when the module is first loaded (or when the first localization is requested).


3. Regarding caching of the pool in local storage, mozStorage may be the most performant solution, but it's worth investigating whether serialization of JSON to disk is performant enough to suffice for this relatively simple data storage use case, at least for the initial implementation.
Attachment #440220 - Flags: review?(myk)
Attachment #440220 - Flags: feedback?
Attachment #440220 - Flags: feedback+
Thanks for the feedback Myk!

1) Having multiple instances of Common Pool Service will raise other issues, in particular multiple race conditions. I can spend more time here if you want to explain why I believe that this solution is better.

I also spent some time with Brian and Atul to make sure that such solution fits the future schema and is unloadable. Atul confirmed that we will unload properly, Brian confirmed that we should have such singleton model layed out for the future anyway.

If you think we will not want to have this ever (ever = when we have one jetpack lib per browser) then I see a goal in coding for all those race condition cases. If you agree that we will eventually get singleton model then I'd prefer to stick to this solution in order to make code clean and simple.

2) I'm currently working on an improved patch (sorry for not removing the review flag on this one). My current plan is to:
a) be able to load JSON file from jetpack bundle on install
b) asynchronously run server-client sync once per day or so
c) experiment with how much I want to store in memory and what in storage

Item a) gives me ability to run server-client later since some basic localization experience should be available out of the box.
Item b) is open to fine tuning.

3) I will try both. For now I'm using storage just to get things running, but I'll be happy to try JSON once I have the overall superreview granted :)
Gandalf, could you restate what the race conditions were? I had thought that the main reason to have a singleton was just to conserve disk resources and network bandwidth, which is certainly a worthy goal. But when combined with the intricacies of actually managing a singleton across multiple extensions--which it should be noted no Jetpack-based extensions have yet done--it seems like we have to weigh the costs vs. benefits more. Particularly when one major implementation detail involves how the common pool is serialized to disk, which Myk pointed out to me.
sure, let me then restate them.

If we go for multiple, independently running instances of CommonPool Service we:

1) have to control which service is currently updating the sqlite DB
2) or have multiple sqlite DB's - one per jetpack

3) have to use more resources, like we cannot query for entity needed by multiple jetpacks because each service has to make its own query for its own jetpack

4) we have to watch out for multiple commonpool services in the future triggering the same behavior and concuring for it (think: switching app locale)

In fact the common pool serialization thing (which will be done via sqlite probably, or json less probably) is IMO much easier if we have just one service that manages the pool and receives calls from multiple jetpacks, instead of one-service-per-jetpack case.

I'd like to understand better why you consider this to be risky. I think from chatting with you guys I got a feeling that we will in the future have to move to single jetpack-lib per application scenario with a space for such singletones anyway.
This is mostly risky because of the weird edge-cases that can occur when e.g. the "current" common-pool service--let's say it's running off the addon "foo"--goes "down" because its host addon has been disabled/uninstalled-at-runtime. (When an addon is disabled/uninstalled, *all* its resources are freed.)

At such a point, we'd have to figure out some sort of scheme allowing for another addon's common-pool code to "take over" the role as the singleton.  We'd then also have to make sure that this singleton read the exact same kind of serialization from disk that the old one wrote, and that it had the exact same API for all the other addons to interact with.

So, it could easily lead to dependency/versioning friction with other installed Jetpacks, which is part of what the XPIs created by the SDK are trying to avoid, by having the Gecko platform be their only dependency.

The much easier way to do this would be to just follow your proposal (2) for now, while the common pool library evolves, and then eventually uplift it to the Gecko platform once it's ready for prime-time, at which point newly-created Jetpacks could use it just like they use other singleton services in the platform.

Note that another benefit to just doing (2) for now is that since there aren't currently many Jetpack-built XPIs out, it's not actually a big deal that each Jetpack has its own sqlite database with the common pool in it. It would certainly be a big deal if Jetpack-based extensions were as popular as Firefox addons in general, but for now we have the freedom to not have to worry about it while we iterate.

Perhaps there are edge cases to having multiple instantiations of this service in the application at once that I'm not aware of, though--in which case I guess we're in a pickle, since it means we're going to have to deal with prickly edge cases either way.

Another crazy idea that just came to mind: one way we could deal with this in a potentially less-risky way is by having the common pool service just be a hidden content-privileged iframe pointing at e.g. 'https://common-pool.mozilla.org', whose content stores everything to localStorage and communicates with addons via postMessage. One neat thing about this solution would be that regular web content could use it too.
Attached patch WIP v3 (obsolete) — Splinter Review
This is an updated patch that follows the discussion progress

1) Added updateStorage and updateMemory that are fired once per day and once per 12 hours (respectively)
2) Removed the singleton logic
3) Improved locale handling

what is not in the patch:
1) tests
2) any form of logic around what entities should land in memory vs storage
3) entity overlays (per jetpack, per use case)
4) any form of avoiding conflicts between multiple common pool services running
5) code to load the potentially bundled JSON into database
6) some lines in the code are over 80 chars long

This patch actually works against my POC web service and can be used to provide localization from web service, so I'd like to get a feedback on the code flow and any additional edits you would like to see.
Attachment #440220 - Attachment is obsolete: true
Attachment #445722 - Flags: feedback?
Attachment #445722 - Flags: feedback? → feedback?(myk)
Comment on attachment 445722 [details] [diff] [review]
WIP v3

This is really coming along!  The basic architecture seems to be fine.  I'm still a bit leery about using mozstorage to store data, as it's a relatively heavyweight solution for a pretty simple schema (and data size).  But it should be ok.

Besides unit tests and API documentation, the primary piece that seems to be missing here is the mechanism by which the module communicates with the server about the set of strings used by the addon that would need localization.  Or is the plan to use static analysis on the AMO catalog to derive that set?

Other things I noticed...

>diff --git a/packages/jetpack-core/lib/l10n.js b/packages/jetpack-core/lib/l10n.js

>+let locale = 'de';

Presumably this will be replaced by a reference to the actual active locale (i.e. the value of general.useragent.locale)?


>+  this.I = timer.setInterval(this.updateMemory, memory_update_interval*1000)

This should use a cross-session timer, support for which we'll need to add to the "timer" module.


>+  url: 'http://127.0.0.1:8000/pool/'+locale+'/',

Can you attach the current version of your proof of concept?  Then I can set it up on the test server, and we can point this API in its direction.


>+    var sh = storage.getConnection('l10n_cp.sqlite', 'ProfD')

In order to ensure that addons all use their own copy of this file, so there are no conflicts between multiple addons that use the localization API (at least until we work through the issues and figure out how to implement a shared cache in a future phase of development), this file should be placed into an addon-specific subdirectory of the profile directory, just as simple-storage places its storage file into such a directory, via something like:

  let file = Cc["@mozilla.org/file/directory_service;1"].
                  getService(Ci.nsIProperties).
                  get("ProfD", Ci.nsIFile);
  file.append("jetpack");
  file.append(packaging.jetpackID);
  file.append("localization");
  file.mkpath(file.path);
  file.append("l10n_cp.sqlite");

Also, it'd be better to inline the code that is in storage.js into this module for now, as that'll be simpler than figuring out what the right interface should be for a general API for accessing mozstorage (we can always factor it out later once we do figure out that general interface).


>+    try {
>+      var stmt = sh.createStatement("SELECT * FROM info WHERE id=\"last_update\" AND value>strftime('%s','now')-:time;");
>+    } catch(e) {
>+      setupStorage(sh);
>+      var stmt = sh.createStatement("SELECT * FROM info WHERE id=\"last_update\" AND value>strftime('%s','now')-:time;");
>+    }

Rather than assuming an exception means the database needs to be set up, it would be better to be more explicit about what constitutes an uninitialized database.

One option is to check whether or not the tables have been defined via mozIStorageConnection::tableExists, like nsFormHistory::OpenDatabase <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/src/nsStorageFormHistory.cpp#733>).

Another is to use mozIStorageConnection::schemaVersion to store the version of the database schema, checking it when opening the database and setting it when setting up storage for a new database, like ContentPrefService::_dbInit <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/src/nsContentPrefService.js#858>.


>+    stmt.params.time = storage_update_interval;
>+    var res = stmt.step();
>+    stmt.finalize();
>+    if (!res) {
>+      console.debug('Updating storage!');
>+      if (!cpservice)
>+        this.updateStorage(sh);
>+      else
>+        cpservice.updateStorage(sh);
>+      sh.close();
>+      return true; // for now, updating storage updates the pool object
>+    }

It would probably make sense to make sure this closes the storage connection even if an exception occurs somewhere in this function (f.e. via a try/finally statement).


>+  updateStorage: function(sh) {
>+    this.pool = getPool(this.url);

The module should retrieve the pool asynchronously, as there is little advantage to retrieving it synchronously now that this is caching the pool locally, and significant downside in terms of the responsiveness of the addon (and, until we implement E10S, the entire application).


>+    for (var i in this.pool) {
>+      var stmt = sh.createStatement("INSERT OR IGNORE INTO entities (entity) VALUES(:value)");
>+      stmt.params.value = i;
>+      stmt.execute();
>+      stmt.finalize();
>+      for (var j in this.pool[i]['value']) {
>+        var stmt = sh.createStatement("INSERT OR REPLACE INTO \"values\" (entity, value, locale) VALUES(last_insert_rowid(), :value, :locale)");  
>+        stmt.params.value = this.pool[i]['value'][j];
>+        stmt.params.locale = j;
>+        stmt.execute();
>+        stmt.finalize();
>+      }
>+    }

This looks like it won't remove old values that are no longer part of the common pool.  Shouldn't it be checking for those and removing them?
Attachment #445722 - Flags: feedback?(myk) → feedback+
Depends on: 566677
No longer depends on: 566677
(In reply to comment #21)
> (From update of attachment 445722 [details] [diff] [review])
> This is really coming along!  The basic architecture seems to be fine.  I'm
> still a bit leery about using mozstorage to store data, as it's a relatively
> heavyweight solution for a pretty simple schema (and data size).  But it should
> be ok.

For the data set we have right now it is enough.
But before we push this to the server, we need to add overloads per jetpack and per case.

If we do this via plain JSON data structure we will have to build complex queries for retrieving the right value for the right use case.
With SQLite I can just build one query.

I'd like to stick to sqlite for now and consider switching back to something simpler once we're done with the API.

> Besides unit tests and API documentation, the primary piece that seems to be
> missing here is the mechanism by which the module communicates with the server
> about the set of strings used by the addon that would need localization.  Or is
> the plan to use static analysis on the AMO catalog to derive that set?

My ultimate plan was to have common-pool send the list of jetpacks installed in the query, but since we provide one jetpack-sdk per jetpack, I can just provide the jetpacks ID with the query.

> Presumably this will be replaced by a reference to the actual active locale
> (i.e. the value of general.useragent.locale)?

yup.
 
> 
> >+  this.I = timer.setInterval(this.updateMemory, memory_update_interval*1000)
> 
> This should use a cross-session timer, support for which we'll need to add to
> the "timer" module.

Not sure what you mean here.

> 
> >+  url: 'http://127.0.0.1:8000/pool/'+locale+'/',
> 
> Can you attach the current version of your proof of concept?  Then I can set it
> up on the test server, and we can point this API in its direction.

http://hg.mozilla.org/users/zbraniecki_mozilla.com/gandalf-packages/file/77bff19e685d/packages/l10n/python-lib/l10njetpacksite
> Also, it'd be better to inline the code that is in storage.js into this module
> for now, as that'll be simpler than figuring out what the right interface
> should be for a general API for accessing mozstorage (we can always factor it
> out later once we do figure out that general interface).

right, I added it at some point hoping it'll shape up into a nice API, but for now storage is pretty empty, so I'll inline it for the next WIP.

 
> One option is to check whether or not the tables have been defined via
> mozIStorageConnection::tableExists, like nsFormHistory::OpenDatabase
> <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/src/nsStorageFormHistory.cpp#733>).

yup. thanks!
 
> Another is to use mozIStorageConnection::schemaVersion to store the version of
> the database schema, checking it when opening the database and setting it when
> setting up storage for a new database, like ContentPrefService::_dbInit
> <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/src/nsContentPrefService.js#858>.

the goal for now was to enable initial run, not schema upgrades. I consider doing pretty dummy wipe outs on version upgrades.
 
> It would probably make sense to make sure this closes the storage connection
> even if an exception occurs somewhere in this function (f.e. via a try/finally
> statement).

yup! thanks!
 
> 
> >+  updateStorage: function(sh) {
> >+    this.pool = getPool(this.url);
> 
> The module should retrieve the pool asynchronously, as there is little
> advantage to retrieving it synchronously now that this is caching the pool
> locally, and significant downside in terms of the responsiveness of the addon
> (and, until we implement E10S, the entire application).

yeah, I'll go with async in the next WIP
Target Milestone: 0.4 → 0.5
Attached patch patch v1 (obsolete) — Splinter Review
updated patch to comments, integrated storage, moved to async and cleaned up WIP.
Attachment #445722 - Attachment is obsolete: true
Attachment #449937 - Flags: review?
Attached patch patch v 1.1 (obsolete) — Splinter Review
ouch, the previous patch was passing storagehandler between asyncly launched methods. I'm just getting new handler after async response now.
Attachment #449937 - Attachment is obsolete: true
Attachment #449948 - Flags: review?
Attachment #449937 - Flags: review?
Attachment #449948 - Flags: review? → review?(myk)
Comment on attachment 449948 [details] [diff] [review]
patch v 1.1

Almost there!  Many of these review comments are nits based on the Jetpack code style guidelines <https://wiki.mozilla.org/Labs/Jetpack/Reboot/Style_Guide> (also see the contribution page <https://wiki.mozilla.org/Labs/Jetpack/Contribution> for references to various other implementation guides).


>diff -r 0623cd990857 packages/jetpack-core/lib/l10n.js

We haven't talked about this before, but it's been on my mind that "l10n" is a bit jargony for our audience of casual developers who are not familiar with localization tools and techniques, so it seems like it would make more sense to spell the name of this module out as localization.js.


>+let jetpack = require("self");

Nit: here and elsewhere, it would be better to call this "program" rather than "jetpack", as Jetpack is the name of the project, and we used to use it as the name of the things you install, but we're moving away from that name in favor of simply calling them addons or applications, depending on whether you are using Jetpack to build addons or XULRunner apps, and the common term we're using to describe both addons and applications is programs.


>+function CommonPoolService() {

Nit: does this code ever instantiate more than one service object at a time?  If not, it would be simpler to make this a plain object rather than a constructor with a prototype.


>+  if (this.pool === null)
>+    this.updateMemory();
>+  this.I = timer.setInterval(this.updateMemory, memory_update_interval*1000)

Nit: semi-colon at the end of the statement.

Also, when calling updateMemory via a timer, |this| will be the global object (not the CommonPoolService instance) inside updateMemory, so the call inside updateMemory to this.updateStorage() will fail.  To fix that, you have to call updateMemory in this clunky fashion:

  let self = this;
  this.I = timer.setInterval(function () self.updateMemory(),
                             memory_update_interval*1000);

Alternately, if the service were a plain object, then you would specify this time after declaring that object and simply do:

  cpservice.I = timer.setInterval(function () cpservice.updateMemory(),
                                  memory_update_interval*1000);


>+  url: 'http://127.0.0.1:8000/pool/'+locale+'/?jid='+jetpack.id,

127.0.0.1:8000 -> l10n.mozillalabs.com

Also, this is more a question about the server API than the client implementation, but why is the locale specified in the path but the addon ID in the query string?


>+  updateMemory: function() {
>+    var sh = getStorageHandler('l10n_cp.sqlite', 'ProfD')

Nit: semi-colon at the end of the statement.


>+    ensureStorageSchema(sh);
>+    var stmt = sh.createStatement("SELECT * FROM info WHERE id=\"last_update\" AND value>strftime('%s','now')-:time;");

These lines should be wrapped at 80 columns, breaking up the string into appended substrings if necessary, i.e.:

    var stmt = sh.createStatement(
      "SELECT * FROM info WHERE id=\"last_update\ " +
      "AND value>strftime('%s','now')-:time;"
    );


>+    stmt.params.time = storage_update_interval;
>+    var res = stmt.step();
>+    stmt.finalize();
>+    if (!res) {
>+      console.debug('Updating storage!');
>+    	try {

Nit: here and elsewhere, use spaces instead of tabs for indentation.


>+        if (!cpservice)
>+          this.updateStorage();
>+        else
>+          cpservice.updateStorage();
>+      } finally {
>+        sh.close();
>+      }
>+      return true; // for now, updating storage updates the pool object

Nit: don't specify a return value when returning here, since none of the callers pay attention to the return value, and specifying one here (but not at the end of the function) triggers a "function does not always return a value" JavaScript strict warning.


>+    console.debug('Not updating storage!');
>+    cpservice.pool = {};
>+    let stmt = sh.createStatement("SELECT t1.entity,t2.value FROM entities AS t1 LEFT JOIN \"values\" AS t2 ON t1.rowid=t2.entity WHERE locale=:locale");
>+    stmt.params.locale = locale;
>+    while (stmt.step()) {
>+      var entity = stmt.row.entity;
>+      var val = {};
>+      val[locale] = stmt.row.value;
>+      cpservice.pool[entity] = {"value": val};
>+    }
>+    stmt.finalize();
>+    cpservice.last_update = getUnixTimeStamp();
>+    sh.close();

This database access should be wrapped in a try/finally block to ensure the statement gets finalized and the storage handler gets closed in the event of an exception while retrieving the data.

Also, it would be better to wait to reset the pool until you've retrieved all data, in case data retrieval fails, i.e.:

    let pool = {};
    let stmt;
    try {
      stmt = sh.createStatement("SELECT t1.entity,t2.value FROM entities AS t1 LEFT JOIN \"values\" AS t2 ON t1.rowid=t2.entity WHERE locale=:locale");
      stmt.params.locale = locale;
      while (stmt.step()) {
        var entity = stmt.row.entity;
        var val = {};
        val[locale] = stmt.row.value;
        pool[entity] = {"value": val};
      }
      cpservice.pool = pool;
      cpservice.last_update = getUnixTimeStamp();
    }
    finally {
      if (stmt)
        stmt.finalize();
      sh.close();
    }


>+  updateStorage: function() {
>+    getPool(this.url);
>+  },

Nit: since getPool has no other callers, its code might as well be inlined in this method.


>+  _updateStorage: function() {

Nit: the underscore in this method's name suggests it is private, but it gets called by external code (the readystatechange handler in the getPool function), so it would be better to call it something that doesn't imply that it is private (like finishUpdatingStorage).  Alternately, you could inline its code into the readystatechange handler, since that is its only caller.


>+function ensureStorageSchema(conn) {
>+  if (conn.schemaVersion==1)
>+    return true;

Nit: don't specify a return value when returning here, since the caller doesn't pay attention to the return value, and specifying one here (but not at the end of the function) triggers a "function does not always return a value" JavaScript strict warning.


>+  conn.executeSimpleSQL('CREATE TABLE IF NOT EXISTS entities (entity string unique)')
>+  conn.executeSimpleSQL('CREATE TABLE IF NOT EXISTS "values" (entity int, value string, locale string, primary key (entity, locale))')
>+  conn.executeSimpleSQL('CREATE TABLE IF NOT EXISTS info (id int unique, value string)')

Nit: semi-colons at the ends of the statements.

Nit: it would be better to call the values.entity column "values.entityID" to clarify that it stores the ID of the entity from the entities table rather than the entity itself.

Also, the type declarations for the table columns should be ones that SQLite recognizes as representing a storage class.  See Datatypes in SQLite Version 3 <http://www.sqlite.org/datatype3.html> for the excruciating details, but the summary is that "string" columns should be "TEXT" and "int" columns should be "INT".


>+  let stmt = conn.createStatement("INSERT INTO \"info\" (id,value) VALUES ('last_update', '0')");  

Nit: no trailing spaces at the end of the statement.


>+function getStorageHandler(filename) {
>+  let fh = Cc["@mozilla.org/file/directory_service;1"].
>+                  getService(Ci.nsIProperties).
>+                  get("ProfD", Ci.nsIFile);

Nit: indent continuation lines for XPCOM component accesses like so:

  let fh = Cc["@mozilla.org/file/directory_service;1"].
           getService(Ci.nsIProperties).
           get("ProfD", Ci.nsIFile);

>+  fh.append("jetpack");
>+  fh.append(packaging.jetpackID);
>+  fh.append("localization");
>+  file.mkpath(fh.path);
>+  fh.append("l10n_cp.sqlite");
>+
>+  var storageService = Cc["@mozilla.org/storage/service;1"]  
>+                          .getService(Ci.mozIStorageService);  

Nit: use "var" or "let" consistently within functions (and ideally across the entire file, preferring let to var), except where circumstances warrant the inconsistency.

Nit: no trailing spaces at the end of the statements.

Nit: when breaking statements at a period operator, leave the period on the preceding line, i.e.:

  var storageService = Cc["@mozilla.org/storage/service;1"].
                       getService(Ci.mozIStorageService);


Other than these issues, the patch just needs tests and documentation!

Note: mock XHR is at risk for 0.5, but we're not going to block on it for APIs that would use it for testing, like this one and Request (bug 547091).  Instead, point tests to a test server (either your own, if you have one that is publicly available, or the one at l10n.mozillalabs.com, on which Zandr is going to stand up the reference implementation by the end of the week per bug 571122), just as Request does.
Attachment #449948 - Flags: review?(myk) → review-
thanks myk! The coding style inconsistencies were caused by me misusing my new editor of choice under Linux. Sorry for sending patch without clearing that.

I'll apply the changes and resend for review :)
Attached patch patch v2 (obsolete) — Splinter Review
I applied all your suggestions and then started working on the future compatibility with the service API and one thing led to another...

long story short, I'm attaching patch v2. I ultimately decided to ignore the concept of using sqlite for now, and I just store the JSON file (I may revisit it in a future iterations once we support shared resources).

I added support for per-app exceptions and contexts, but I did not add per-line exception cause I don't think I have a way to retrieve a file/line from which the function has been called for now. I'll investigate it for 0.6.

I wrote very basic tests that use _injectPool function for now,but if you think it's better to point to a server, let me know.

I also removed server url entirely until we have a service, and added a method do declare the server manually.

I think that the code is much cleaner and more robust now. 

Let me know how that works. :)
Attachment #449948 - Attachment is obsolete: true
Attachment #450962 - Flags: review?
Attachment #450962 - Flags: review? → review?(myk)
Comment on attachment 450962 [details] [diff] [review]
patch v2

(In reply to comment #26)
> thanks myk! The coding style inconsistencies were caused by me misusing my new
> editor of choice under Linux. Sorry for sending patch without clearing that.

No worries, the style stuff takes time to get the hang of, as JS is such a flexible language, and the style conventions of Mozilla code are so voluminous and shifting (like the sands of time)!


(In reply to comment #27)
> I applied all your suggestions and then started working on the future
> compatibility with the service API and one thing led to another...

How sure are we about this future compatibility?  I haven't heard anything about the status of the service work.  Do we have a pretty sense of the API for the service, or is that still very much subject to change?


>diff -r 7fa6ebb155c3 packages/jetpack-core/lib/localization.js

>+// check if update is over one day old
>+let storage_update_interval = 84600;
>+// update memory twice per day
>+let memory_update_interval = 42300;

Nit: 86400 and 43200 (seconds/day and seconds/day/2)?

Nit: make these const and all-caps, i.e.:

  const STORAGE_UPDATE_INTERVAL = 86400;
  const MEMORY_UPDATE_INTERVAL = 43200;


>+/**
>+ * Returns translation of a string if one is available,
>+ * otherwise returns the string itself
>+ *
>+ * @param   k
>+ *          The string to be localized
>+ *
>+ * @param   context
>+            The context of the string, NIY

Not Invented Yet?

Can you explain the use case for this new "context" feature?  Without docs in this patch or explanation in the bug, it's not clear what the reasoning is behind it.


>+exports.get = function (k, context) {

This function should validate input parameters to make sure they are the types it expects them to be.

Also, do you have a sense of how likely it is that we will add parameters to this call in the future?  I seem to recall some discussion about passing one or more additional parameters for l20n at some point.  We should consider using a parameter pattern that we are likely to be using elsewhere in Jetpack (we recently identified a use for it in the tabs API), which is to handle the simple, common use case with a single parameter that takes a primitive argument and more complicated use cases with a single parameter that takes an object argument that provides named parameters, i.e.:

  get("foo")                 <- the simple case
  get({ text: "foo", ... }); <- more complicated cases


>+  let pool = cps.pool;
>+  // testing if we have this entity
>+  // for this locale in the pool
>+  if (pool !== null &&
>+      k in cps.pool &&
>+      locale in cps.pool[k]) {

Nit: conditionals that can fit on one line are typically not wrapped in Mozilla code, i.e.:

  if (pool !== null && k in cps.pool && locale in cps.pool[k]) {


>+/**
>+ * Returns translation of a string using
>+ * context if the context is provided
>+ *
>+ * @param   context
>+ *          the context or null
>+ *
>+ * @param   arr
>+            array of a data format we use

Can you provide more information about this parameter here?  In the caller, it is called "entity", whereas here it is "arr", and it isn't clear what it represents and what structure it has.


>+function getContextOrGeneric(context, arr) {

Nit: since context is the optional parameter, it should be the second one in the getContextOrGeneric function's parameter list.


>+  if (context === undefined)

The function comment says context is either "context" or null, but this compares it to undefined.  If context is intended to be a complex data structure like an object, it would be better to just check for its "truthiness", i.e.:

  if (context)


>+  return fh.path

Nit: semi-colon at end of statement.


>+ * @returns integer with time in *miliseconds*

Nit: miliseconds -> milliseconds


>+function getUnixTimeStamp() {
>+  return parseInt(new Date().getTime().toString().substring(0,10))
>+}

Nit: semi-colon at end of statement.


>+/**
>+ * Allows for injecting a pool
>+ * Used by tests
>+ */
>+exports._injectPool = function(pool) {
>+  cps.pool = pool;
>+}

Instead of exporting test functionality, use test.makeSandboxedLoader in the test functions to load the module in a way that gives you access to the module's global object, then access its internals through the "sandbox" created for the module, i.e. something like:

  let loader = test.makeSandboxedLoader();
  let localization = loader.require("localization");
  let global = loader.findSandboxForModule("localization").globalScope;
  // global is the global object inside the loaded module,
  // through which you can access its private definitions.
  global.cps.pool = myTestPool;


>+/**
>+ * Allows for updating the server url
>+ * Triggers updating memory
>+ */
>+exports.setServer = function(url) {
>+  cps.url = url;
>+  cps.updateMemory();
>+}

This seems like a fundamental change to the experience we'd previously speced, in which addon authors don't need to know anything about the way their addon is localized.  Perhaps I'm missing something (in which case you should definitely enlighten me!), but it seems like it would make more sense for this code to point to a domain name under our control (f.e. l10n.mozillalabs.com), gracefully handle failures to communicate with a localization service at that domain name (which I think is already the case), and then start working without any further intervention on the part of addon authors once we get the service up and running at that domain.

Does that seem reasonable?


>diff -r 7fa6ebb155c3 packages/jetpack-core/tests/test-localization.js

>+exports.testGeneric = function (test) {
>+  test.assertEqual(_("Cancel"),
>+                     "Anuluj",
>+                     "Generic translation should be used");
>+};
>+
>+exports.testContext = function (test) {
>+  test.assertEqual(_("Cancel", "button"),
>+                     "Anulowanie",
>+                     "Context translation should be used");
>+};
>+
>+exports.testPerAppException = function (test) {
>+  test.assertEqual(_("Add"),
>+                     "Dod2",
>+                     "Generic per-app translation should be used");
>+};
>+
>+exports.testPerAppExceptionWithContext = function (test) {
>+  test.assertEqual(_("Add", "title"),
>+                     "Dod3",
>+                     "Context per-app translation should be used");
>+};

These can actually all be in a single test function.  There's no need to declare a separate function for every assert (just to break up many tests into major blocks of testing).
Attachment #450962 - Flags: review?(myk) → review-
(In reply to comment #28)
> How sure are we about this future compatibility?  I haven't heard anything
> about the status of the service work.  Do we have a pretty sense of the API for
> the service, or is that still very much subject to change?

I can hear you.
I backed it out and will bring it for 0.6 or 0.7 once we finalize the spec :)


> Instead of exporting test functionality, use test.makeSandboxedLoader in the
> test functions to load the module in a way that gives you access to the
> module's global object, then access its internals through the "sandbox" created
> for the module, i.e. something like:

Great! Thanks.

I did this, but since test is passed as an argument to the test function I don't know how to make it once per test pack.
(not a problem now since I merged tests into one function :))

> >+/**
> >+ * Allows for updating the server url
> >+ * Triggers updating memory
> >+ */
> >+exports.setServer = function(url) {
> >+  cps.url = url;
> >+  cps.updateMemory();
> >+}
> 
> This seems like a fundamental change to the experience we'd previously speced,
> in which addon authors don't need to know anything about the way their addon is
> localized.  Perhaps I'm missing something (in which case you should definitely
> enlighten me!), but it seems like it would make more sense for this code to
> point to a domain name under our control (f.e. l10n.mozillalabs.com),
> gracefully handle failures to communicate with a localization service at that
> domain name (which I think is already the case), and then start working without
> any further intervention on the part of addon authors once we get the service
> up and running at that domain.
> 
> Does that seem reasonable?

It does if we assume that we have something there. Unfortunately we don't.

I wanted to allow developers who want to play with localization to set up custom server and declare it that way. This way they can use separate server from our official one if they want without hacking on jetpack-core code.

Let me know if it makes sense.
Updated patch with docs coming.
Attached patch patch v2.1 (obsolete) — Splinter Review
- updated per myk's review
 - removed context for 0.5
 - added docs
Attachment #450962 - Attachment is obsolete: true
Attachment #451391 - Flags: review?
Attached patch patch 2.2 (obsolete) — Splinter Review
minor update to indentation in test.
Attachment #451391 - Attachment is obsolete: true
Attachment #451401 - Flags: review?(myk)
Attachment #451391 - Flags: review?
Attached patch patch 2.3 (obsolete) — Splinter Review
(per IRC conversation)

 - removed setServer
 - defined default url to l10n.mozillalabs.com
Attachment #451401 - Attachment is obsolete: true
Attachment #451431 - Flags: review?
Attachment #451401 - Flags: review?(myk)
Attachment #451431 - Flags: review? → review?(myk)
Comment on attachment 451431 [details] [diff] [review]
patch 2.3

diff --git a/packages/jetpack-core/docs/localization.md b/packages/jetpack-core/docs/localization.md

+The `localization` module provides localization functionality.

Nit: I'd like this to be a bit more descriptive.  How about:

---
The `localization` module provides simple localization functionality.
It makes it possible to retrieve localized versions of the strings in your code.
And it doesn't require you to solicit localizations from localizers or bundle
localizations with your code, as the module retrieves them automatically
from a web service based on the strings your code is using.
---

Here we might also talk about how the common pool works, but I think this is ok for now.

+<code>localization.**get**(*string*)</code>
+
+Returns the translation of the string in the locale of the program if
+the localization is available.
+
+If there is an exception for that *programID* for the given string the
+localization of the exception will be returned.

Nit: I know what this is trying to say, but it's a bit hard to understand.  I recommend instead:

The translation will either be a general translation of the given string or
a translation that is specific to this particular add-on, if one is available.


diff --git a/packages/jetpack-core/lib/localization.js b/packages/jetpack-core/lib/localization.js

+let programID = packaging.programID;

This needs to be:

  let programID = require('self').id;


+/**
+ * Allows for injecting a pool
+ * Used by tests
+ */
+exports._injectPool = function(pool) {
+  cps.pool = pool;
+}

This is no longer needed and should be removed.


diff --git a/packages/jetpack-core/tests/test-localization.js b/packages/jetpack-core/tests/test-localization.js

+exports.testGeneric = function (test) {

This is ok for now, but there's a lot more we should be testing, in particular the code for retrieving an updated common pool and storing it on disk (once mock XHR or another solution for testing network requests is implemented).


diff --git a/packages/jetpack-core/tests/test-localization.js b/packages/jetpack-core/tests/test-localization.js

+  let loader = test.makeSandboxedLoader();

This needs to be:

  let loader = test.makeSandboxedLoader({ globals: { packaging: packaging } });

And the test function needs to set the locale the tests expect to be running against:

  global.locale = "pl";

Finally, the test function needs to unload the loader after it is finished using it to prevent a memory leak:

  loader.unload();
Attachment #451431 - Flags: review?(myk) → review-
In the interests of getting this landed before the freeze, I fixed the nits and the review issues and committed this fixed version.
Attachment #451431 - Attachment is obsolete: true
Attachment #451512 - Flags: review+
https://hg.mozilla.org/labs/jetpack-sdk/rev/71bf1996121e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: 0.2 → unspecified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: