627470 - simple-storage improvements

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

Attachment: patch

Spun out of simple-storage discussions in bug 624607.  This patch

* removes from the doc the discussion about the ability to set
  the `storage` object to a new value.  That ability is still
  present so as not to break existing add-ons that use it.

* changes a couple of pref names to reflect the project's name
  change.  They're now consistent with similar changes made to
  context-menu prefs (bug 612020).

* makes sure not to remove any directories beyond the "jetpack"
  directory in the user's profile directory when the store is
  removed from disk.  (simple-storage stores live at profdir/
  jetpack/addon-id/simple-storage/store.json.)  Currently it
  just assumes any directories above the jetpack directory are
  not empty, but that's not necessarily the case, and it
  definitely shouldn't be deleting directories it didn't make.

* adds file.basename() to support the previous point.

I'd like to do Myk's proxy suggestion to warn when properties are set on the module itself (bug 624607 comment 19), but I don't know anything about proxies right now.

Comment 1

[Drew Willcoxon :adw]

Attachment: patch 2

I probably shouldn't hardcode Unix-like paths in the file test.  This patch uses file.join() instead.  But in order to test that basename() works on a path with no basename, I modified dirname() to return the empty string when its path has no dirname instead of throwing, which seems like the right behavior anyway.  (There doesn't seem to be a way in Gecko to initialize a file/path with the top of a volume.)

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 505501 [details] [diff] [review]
patch 2

>diff --git a/packages/addon-kit/lib/simple-storage.js b/packages/addon-kit/lib/simple-storage.js

>-const WRITE_PERIOD_PREF = "jetpack.addon-kit.simple-storage.writePeriod";
>+const WRITE_PERIOD_PREF = "extensions.addon-sdk.simple-storage.writePeriod";

This is ok, but I would actually retain "addon-kit" in preference to "addon-sdk", as the module seems more tied to the addon-kit package than the SDK itself, since it can be used via Builder, and I can imagine the project one day producing an "addon development addon" or building a core Firefox feature that integrates addon-kit and some tools for creating addons into Firefox itself.


>diff --git a/packages/api-utils/tests/test-file.js b/packages/api-utils/tests/test-file.js

>+                   "basename should work on paths with components");
...
>+                   "basename should work on paths with no components");

It might also be useful to test that basename works on a path with a filename but no directory components, i.e. something like /foo.txt.

Comment 3

[Drew Willcoxon :adw]

(In reply to comment #2)
> This is ok, but I would actually retain "addon-kit" in preference to
> "addon-sdk", as the module seems more tied to the addon-kit package than the
> SDK itself

That perspective makes sense, but I chose "addon-sdk" from the perspective that "Add-on SDK" is the name of the project and larger platform to which this code and pref are related, as "Jetpack" was.  Does that make sense?  In other words, I'd like to tie this pref name to the project and platform, not to the in-the-weeds detail of which package it's related to.  Among other things, that has the advantage of allowing users to filter on all Jetpack-related prefs in about:config.  If you still think retaining "addon-kit" would be better, I'll change it, but in that case I'd also like to change the context-menu prefs as part of this patch too.

> It might also be useful to test that basename works on a path with a filename
> but no directory components, i.e. something like /foo.txt.

Sure.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> That perspective makes sense, but I chose "addon-sdk" from the perspective that
> "Add-on SDK" is the name of the project and larger platform to which this code
> and pref are related, as "Jetpack" was.  Does that make sense?

It does make sense.

I guess it's just not clear to me that the SDK is the platform.  To my mind, the platform is the set of APIs and to some degree also the architecture (the loader, the support for CommonJS modules, etc.) rather than the particular products (Add-on SDK, Add-on Builder, and perhaps in the future some addon or core feature of Firefox) through which the platform is used.

And the name of the project itself remains "Jetpack", even though the products it produces have been renamed.

On the other hand, I can't think of another name for the platform.  It's not "addon-kit", since that's just one of at least two packages of APIs that are part of the platform (the other being api-utils).

I guess it could be called "the Jetpack platform", but we have been getting away from the use of the term "jetpack" in user-facing code and docs.

Lately I've taken to referring to it as "the (next great, next generation, new, etc.) add-on development platform".  But I don't see how to derive a name for these preferences from that.

So "addon-sdk" is fine.  After all, it's just a name.  It will smell as sweet no matter what it's called!

Comment 5

[Drew Willcoxon :adw]

Attachment: patch 3

OK.  I've been thinking of "Add-on SDK" as not only this cfx-based product but the ecosystem around it, including the Builder and everything else.  This name question comes up in other places too, again in simple-storage for example (as you've seen in this patch), where I use "jetpack" as a directory name.  Anyhow...

This patch adds a single-component-path check to the basename test.  I'll land it once the tree thaws for 1.0b3.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #5)
> OK.  I've been thinking of "Add-on SDK" as not only this cfx-based product but
> the ecosystem around it, including the Builder and everything else.  This name
> question comes up in other places too, again in simple-storage for example (as
> you've seen in this patch), where I use "jetpack" as a directory name. 

Yeah.  I think "jetpack" as a directory name is fine, since that's internal rather than user-facing (even if some users manipulate their profiles).  Perhaps preferences like this are sufficiently internal as well, or perhaps we should call the platform "jetpack".  I dunno.  Naming things is hard!


> This patch adds a single-component-path check to the basename test.  I'll land
> it once the tree thaws for 1.0b3.

Roxors!

Comment 7

[Drew Willcoxon :adw]

https://github.com/mozilla/addon-sdk/commit/e64a016e67ef99c7d44a821618ded23855138bb8