Closed Bug 559505 Opened 14 years ago Closed 10 years ago

localstore.rdf kills ponies

Categories

(Core Graveyard :: RDF, defect)

Product:
Core Graveyard
Component:
RDF
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: Dolske, Assigned: rvitillo)

References

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: [ponies])

Attachments

(3 files, 12 obsolete files)

fred.txt
3.21 KB, text/plain
Details
Bug 559505 - localstore.rdf kills ponies, v11
104.77 KB, patch
Details | Diff | Splinter Review
Talos: Allow IO on xulstore.json
1.97 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
Attached patch Patch (WIP) (obsolete) — — Splinter Review 
Localstore.rdf kills ponies. Dead. We've grumbled for a while about wanting to kill this off (or at least kill off the common uses of it), I sat down and took a rough shot at it.

Checkpointing a hacky, doesn't-quite-compile WIP, since it seems far enough along that I think I see what remains to be done (famous last words).
Attached patch Patch v.1 (WIP) (obsolete) — — Splinter Review 
Looks like this is working, although I'm not saving values and just return dummy data.
Attachment #439182 - Attachment is obsolete: true
Attached patch Patch v.2 (WIP) (obsolete) — — Splinter Review 
Functional replacement for localstore.rdf as used by XUL documents, still a few other places in the tree to switch over (now marked with FIXME).
Attachment #439662 - Attachment is obsolete: true
Attachment #439740 - Attachment description: Patch v.1 (WIP) → Patch v.2 (WIP)
Comment on attachment 439740 [details] [diff] [review]
Patch v.2 (WIP)

Before I go any further with completing this patch, I'd like to get some general feedback from The Neils, regarding the general suitability of what I'm doing here. Not sure if Seamonkey / Thunderbird / other apps are doing anything of special concern here... My Thunderbird localstore.rdf doesn't have anything surprising in it, and a brief skim of the comm-central tree seems to indicate that moving SM/TB over to this scheme shouldn't be very hard.

Main TODOs as of Patch v.2:
* write the code to migrate existing localstore.rdf data to the new format.
* TB/SM reqs & patches
* tests
Attachment #439740 - Flags: feedback?(enndeakin)
Attachment #439740 - Flags: feedback?(neil)
Comment on attachment 439740 [details] [diff] [review]
Patch v.2 (WIP)

>+    nsresult rv = GetElementById(aID, getter_AddRefs(domelement));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    NS_ENSURE_TRUE(domelement, NS_OK);
This is a public API. Do we really want to be whining like this?

>+DIRS = public src
PARALLEL_DIRS?

>+interface nsISimpleEnumerator;
nsIStringEnumerator would be easier to use.

>+        const FILE_PERMS    = 0644;
This is almost always wrong. Either use 0600 (private) or 0666 (ask umask)

>+            // XXX why does this work even if I don't call safeStream.finish() in the callback?
Because NS_AsyncCopy already knows about safe output streams.

>+        if (!(id in obj))
>+            obj[id] = {};
Hmm, this is going to fail for an id of, say, "watch".
Attachment #439740 - Flags: feedback?(neil) → feedback+
(In reply to comment #3)
> * write the code to migrate existing localstore.rdf data to the new format.
Careful here, because we have two sorts of data in the existing rdf; attribute persist data, which is grouped by document URI, and RDF tree twisty persist data, which always has the value NC:open="true" but can have any URI.
(In reply to comment #5)
> (In reply to comment #3)
> > * write the code to migrate existing localstore.rdf data to the new format.
> Careful here, because we have two sorts of data in the existing rdf; attribute
> persist data, which is grouped by document URI, and RDF tree twisty persist
> data, which always has the value NC:open="true" but can have any URI.

This had looked easy to store with the current API; I was thinking of doing persister.persistValue("containers", containerName, "open", "true").

But on closer inspection, nsIXULTreeBuilder and nsIXULTemplateBuilder both expose the RDF goop through the API. So if, for example, I switch nsXULTreeBuilder.cpp over to this new code, what should it do when someone calls getResourceAtIndex()?

Maybe it's possible to create dummy RDF objects such that getResourceAtIndex() / getIndexOfResource() roundtrips correctly, but nothing else RDF-y works? I'm not really sure how this stuff gets used.
Attached patch Patch v.3 (obsolete) — — Splinter Review 
Checkpointing; compiles but is completely untested.
Attachment #439740 - Attachment is obsolete: true
Attachment #439740 - Flags: feedback?(enndeakin)
Ftr,
http://mxr.mozilla.org/comm-central/find?text=&string=localstore.rdf
{
/mozilla/xulrunner/app/profile/localstore.rdf
/mozilla/browser/locales/generic/profile/localstore.rdf
/mail/app/profile/localstore.rdf
/mail/test/mozmill/migration-from-tb2/localstore.rdf
/suite/locales/generic/profile/localstore.rdf
}
Blocks: FF2SM
Version: unspecified → Trunk
Being able to kill off RDF would really help with bug 709193.
Blocks: 742529
Blocks: 833098
I'm exceedingly unlikely to have the time to finish this. :(

IIRC the main hangup was the XUL tree stuff noted in comment 6. RDF is baked into those APIs, so either we need some kind of compatibility shim to not break the API, or actually change the API.
Assignee: dolske → nobody
Attached patch Reformed Patch, work in progress (obsolete) — — Splinter Review 

        Attachment #441329 -
        Attachment is obsolete: true

    
  

        Attachment #823525 -
        Attachment description: Reformed Patch → Reformed Patch, work in progress

    
    

    
  


  
Adds tests for tree builders, makes get/remove methods ignore missing keys.

    
  

        Attachment #823525 -
        Attachment is obsolete: true

    
    

    
  
+++ b/toolkit/components/xulstore/XULStore.js
@@ -0,0 +1,399 @@
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

+++ b/toolkit/components/xulstore/mozIXULStore.idl
@@ -0,0 +1,53 @@
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Please use MPLv2. Thanks muchly.

    
    

    
  
Some performance tests, where the time is for 1000 iterations:

Loading a fairly small file, which would normally be done at startup:
  Existing code using synchronous RDF - 2100ms
  Using nsIFile and json.decodeFromStream - 1400ms
  Using nsIFile and JSON.parse - 1600ms
  Using OSFile, with no json parsing - 5300ms
  Parsing only using JSON.parse - 600ms

Setting an attribute that needs to be persisted:
  Old code storing in rdf localstore - 1300ms
  New code storing using xulstore.jsm - 1960ms

The conclusion is that the new patch is 33% faster at reading a small file. It is also faster at reading larger files, although I didn't write down the numbers. However, the new patch makes setAttribute calls that require persistence 50% slower.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        localstore (obsolete)
        — 
          — Splinter Review
      

    


  
This patch:
 - uses osfile for writing
 - fixes bookmarks
 - code cleanup and documentation
 - work in progress attempt at backward compatibilty

Still to do:
 - test toolbar migration (from nsBrowserGlue.js)
 - fix browser_toolbar_migration.js test
 - write file on exit not working yet

    
    

    
  
> Setting an attribute that needs to be persisted:
>   Old code storing in rdf localstore - 1300ms
>   New code storing using xulstore.jsm - 1960ms

A very basic C++ implementation is twice as fast as xulstore.jsm at setting attributes.

    
    

    
  
(In reply to Neil Deakin from comment #14)
> Some performance tests, where the time is for 1000 iterations:
> 
> Loading a fairly small file, which would normally be done at startup:
>   Existing code using synchronous RDF - 2100ms
>   Using nsIFile and json.decodeFromStream - 1400ms
>   Using nsIFile and JSON.parse - 1600ms
>   Using OSFile, with no json parsing - 5300ms
>   Parsing only using JSON.parse - 600ms

Do you have a testcase for the OSFile numbers?  That's really high, and we should try to improve the performance of OSFile if possible.

    
    

    
  
The following was used on a 1830 byte file:

var olddate;
var count = 0;
 
function readFile()
{
  let filename = "xulstore.json";
  let file = Services.dirsvc.get("ProfD", Components.interfaces.nsIFile);
  file.append(filename);

  let decoder = new TextDecoder();
  let promise = OS.File.read(file.path);
  promise = promise.then(
    function onSuccess(array) {
      var txt = decoder.decode(array);
      let jsobj = JSON.parse(txt);
      count++;
      if (count > 1000) {
        var newdate = new Date();
        dump(newdate - olddate);
      }
      else {
        readFile();
      }
    }
  );
}

function readXULStore()
{
  olddate = new Date();
  readFile();
}

Note that for this bug we can't use an asynchronous api so the osfile number isn't as important.

    
    

    
  
It's not really fair to directly compare async OS.File read performance with synchronous I/O equivalents, but it may be worth a followup to look into some of these measurements in more detail. It's also possible that the 1000-iteration workload incurs overhead with OS.File that wouldn't otherwise occur in more realistic scenarios.

    
    

    
  
If the benchmark was taken during an actual startup, I imagine that the main difference is that OS.File has to wait until the main thread is available to post its results. Since startup is mostly sequential, that can make a huge difference.

    
    

    
  
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #20)
> If the benchmark was taken during an actual startup

The test above was not. The test I did ran the code in comment 18 ran when pressing a button.

    
  
Blocks: 987728
Assignee: nobody → rvitillo

    
    

    
  


  
Getting there. I fixed all remaining failing tests and added some more (https://tbpl.mozilla.org/?tree=Try&rev=38beff07dd77).

I still need to address properly the background compatibility for add-ons that require localstore.rdf; I would be inclined to leave the current implementation baked in and issue a warning, would that make sense?

        Attachment #824809 -
        Attachment is obsolete: true

        Attachment #829378 -
        Attachment is obsolete: true

        Attachment #8443610 -
        Flags: review?(enndeakin)
Flags: needinfo?(enndeakin)

    
    

    
  


  
Restored localstore.rdf support for backward compatibility.

        Attachment #8443610 -
        Attachment is obsolete: true

        Attachment #8443610 -
        Flags: review?(enndeakin)

        Attachment #8446082 -
        Flags: review?(enndeakin)
Flags: needinfo?(enndeakin)
Flags: needinfo?(l10n)

    
    

    
  
I'm pretty sure you don't request l10n feedback. For RDF, please use axel@pike.org.

Also, you didn't really ask anything?
Flags: needinfo?(l10n)

    
    

    
  
Axel, please have a look at comment 22.
Flags: needinfo?(axel)

    
    

    
  
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #22)
> I still need to address properly the background compatibility for add-ons
> that require localstore.rdf; I would be inclined to leave the current
> implementation baked in and issue a warning, would that make sense?

I wonder if Jorge can help to drill in to that. Do you have stats to figure out how many add-ons use localstore, and what prominent use cases are out there, if still?
Flags: needinfo?(axel) → needinfo?(jorge)

    
    

    
  
Add-ons shouldn't access localstore.rdf directly. Looking around the Add-ons MXR, I saw a couple of old add-ons that maybe would break if localstore.rdf is removed.

Are there other changes I should be looking into? Any API changes?
Flags: needinfo?(jorge)

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #28)
> I think all of these:
> 
> 
> https://mxr.mozilla.org/addons/search?string=rdf%3Alocal-
> store&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons
> 
> would break.

Thanks. I see that most of them haven't been updated in years, so they're probably already incompatible. I also see the Brand Thunder themes, so I'm CCing mkaply so he can look into this.

I think the important question here for compatibility is whether there is a suitable API that can be used to replace what these add-ons are doing. If there is, then it's okay to break compatibility. We can notify the affected developers so they update their code.

    
    

    
  
That's not a public MXR so I can't see the results.

    
    

    
  
The query is for rdf:local-store

It matches /chrome/chromeFiles/content/btUtilities.js, line 542 in BT add-ons like https://addons.mozilla.org/addon/bt-fantasyfishing/

    
    

    
  
(In reply to Jorge Villalobos [:jorgev] from comment #31)
> The query is for rdf:local-store
> 
> It matches /chrome/chromeFiles/content/btUtilities.js, line 542 in BT
> add-ons like https://addons.mozilla.org/addon/bt-fantasyfishing/

Yeah. Those are mega old and don't work on current Firefox and even if they did, they are in a try/catch. So we're good with this.

    
    

    
  
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #22)
> I would be inclined to leave the current
> implementation baked in and issue a warning, would that make sense?

If this is not too complicated to do then I assume it is always a good idea to have 1+ "transition" release(s).
(Maybe with some related "telemetry" reports, if applicable.)

At least, it can be good to have that in aurora (+ beta) cycle(s), to give (Thunderbird/SeaMonkey/...) a bit of time to adapt.

    
    

    
  
Comment on attachment 8446082 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v8

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>-    this._rdf = Cc["@mozilla.org/rdf/rdf-service;1"].getService(Ci.nsIRDFService);
>-    this._dataSource = this._rdf.GetDataSource("rdf:local-store");
>-    this._dirty = false;
>+    xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.mozIXULStore);
> 

let xulStore = ...


>     if (currentUIVersion < 5) {
>       // This code uncollapses PersonalToolbar if its collapsed status is not
>       // persisted, and user customized it or changed default bookmarks.
>-      let toolbarResource = this._rdf.GetResource(BROWSER_DOCURL + "PersonalToolbar");
>-      let collapsedResource = this._rdf.GetResource("collapsed");
>-      let collapsed = this._getPersist(toolbarResource, collapsedResource);
>+      let collapsed = xulStore.getValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed");
>       // If the user does not have a persisted value for the toolbar's
>       // "collapsed" attribute, try to determine whether it's customized.
>-      if (collapsed === null) {
>+      if (collapsed === "") {

Can we use hasValue here? Similarly in a few other places in this file?


>diff --git a/browser/components/places/tests/browser/browser_toolbar_migration.js b/browser/components/places/tests/browser/
>@@ -93,43 +56,52 @@ function test_many_bookmarks_toolbar()
>   ids.push(
>     PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
>                                           PlacesUtils.bookmarks.DEFAULT_INDEX)
>   );
>   ids.push(
>     PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
>                                           PlacesUtils.bookmarks.DEFAULT_INDEX)
>   );
>+  ids.push(
>+    PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
>+                                          PlacesUtils.bookmarks.DEFAULT_INDEX)
>+  );

Why do you add this?


>-
>-        for (int32_t i = int32_t(cnt) - 1; i >= 0; --i) {
>-            nsCOMPtr<nsIContent> element = aElements.SafeObjectAt(i);
...
>+        for (uint32_t i = aElements.Count(); i > 0; i--) {
>+            nsCOMPtr<nsIContent> element = aElements.SafeObjectAt(i - 1);

I think the loop was better before.


> {
>-  gOriginalMigrationVersion = Services.prefs.getIntPref("browser.migration.version");
>+  try {
>+    gOriginalMigrationVersion = Services.prefs.getIntPref("browser.migration.version");
>+  } catch(e) {
>+    gOriginalMigrationVersion = 0;
>+  }

You should probably leave this as is.


>diff --git a/content/xul/templates/src/nsXULContentUtils.h b/content/xul/templates/src/nsXULContentUtils.h
>     static nsresult
>-    GetElementResource(nsIContent* aElement, nsIRDFResource** aResult);
>-
>-    static nsresult

You should also be able to remove the MakeElementURI, MakeElementResource and MakeElementID methods which are now unused.


>
>-    if (aIsTrusted) {
>-        // If we're a privileged (e.g., chrome) document, then add the
>-        // local store as the first data source in the db. Note that
>-        // we _might_ not be able to get a local store if we haven't
>-        // got a profile to read from yet.
>-        nsCOMPtr<nsIRDFDataSource> localstore;
>-        rv = gRDFService->GetDataSource("rdf:local-store",
>-                                        getter_AddRefs(localstore));
>-        NS_ENSURE_SUCCESS(rv, rv);
>-
>-        rv = compDB->AddDataSource(localstore);
>-        NS_ASSERTION(NS_SUCCEEDED(rv), "unable to add local store to db");
>-        NS_ENSURE_SUCCESS(rv, rv);
>-    }
>-

There may be a compatiblity issue here, as I recall long ago that some people would assume that the first datasource was the local store. You could leave this code in for now.


>diff --git a/content/xul/templates/src/nsXULTreeBuilder.cpp b/content/xul/templates/src/nsXULTreeBuilder.cpp
> NS_IMPL_CYCLE_COLLECTION_INHERITED(nsXULTreeBuilder, nsXULTemplateBuilder,
>                                    mBoxObject,
>                                    mSelection,
>                                    mPersistStateStore,
>+				   mLocalStore,
>                                    mObservers)

Fix indenting


>+      mLocalStore = do_GetService("@mozilla.org/xul/xulstore;1");
>+      if(NS_WARN_IF(!mLocalStore)){
>+	return NS_ERROR_NOT_INITIALIZED;
>+      }

and here.


>         if (isOpen) {
>-            if (hasProperty) {
>-                mPersistStateStore->Unassert(container,
>-                                             nsXULContentUtils::NC_open,
>-                                             nsXULContentUtils::true_);
>-            }
>-
>+	    mLocalStore->RemoveValue(docURI, nodeid, NS_LITERAL_STRING("open"));
>             CloseContainer(aIndex);
>-        }
>-        else {
>-            if (! hasProperty) {
>-                mPersistStateStore->Assert(container,
>-                                           nsXULContentUtils::NC_open,
>-                                           nsXULContentUtils::true_,
>-                                           true);
>-            }
>+        } else {
>+	    mLocalStore->SetValue(docURI, nodeid, NS_LITERAL_STRING("open"),
>+				  NS_LITERAL_STRING("true"));
> 
>             OpenContainer(aIndex, result);
>         }
>     }

and both mLocalStore-> lines above.


>     return NS_OK;
> }
> 
>@@ -1215,20 +1186,19 @@ nsXULTreeBuilder::ReplaceMatch(nsIXULTem
> 
>             if (result != mRootResult) {
>                 // don't open containers if child processing isn't allowed
>                 bool mayProcessChildren;
>                 nsresult rv = result->GetMayProcessChildren(&mayProcessChildren);
>                 if (NS_FAILED(rv) || ! mayProcessChildren) return NS_OK;
>             }
> 
>-            bool open;
>-            IsContainerOpen(result, &open);
>-            if (open)
>+            if (IsContainerOpen(result)) {
>                 OpenContainer(iter.GetRowIndex(), result);
>+	    }
>         }
>     }

and here.

>diff --git a/rdf/base/src/nsRDFService.cpp b/rdf/base/src/nsRDFService.cpp
>-        // Strip params to get ``base'' contractID for data source.
>-        int32_t p = contractID.FindChar(char16_t('&'));
>-        if (p >= 0)
>-            contractID.Truncate(p);
>+	    ds = mLocalStore;
>+	}

Fix indenting here.


>+        else {
>+	    // It's a built-in data source. Convert it to a contract ID.
>+	    nsAutoCString contractID(
>+		NS_LITERAL_CSTRING(NS_RDF_DATASOURCE_CONTRACTID_PREFIX) +
>+		Substring(spec, 4, spec.Length() - 4));
> 
>-        ds = do_GetService(contractID.get(), &rv);
>-        if (NS_FAILED(rv)) return rv;
>+            // Strip params to get ``base'' contractID for data source.
>+            int32_t p = contractID.FindChar(PRUnichar('&'));
>+            if (p >= 0)
>+                contractID.Truncate(p);
> 
>-        nsCOMPtr<nsIRDFRemoteDataSource> remote = do_QueryInterface(ds);
>-        if (remote) {
>-            rv = remote->Init(spec.get());
>+	    ds = do_GetService(contractID.get(), &rv);
>             if (NS_FAILED(rv)) return rv;

and here.

I recall when I worked on this that mLocalStore or something would leak when I added the compatibility code here. Does that no longer happen?
I don't remember the details but I believe the window mediator would be holding a reference or something.

>diff --git a/rdf/datasource/public/moz.build b/rdf/datasource/public/moz.build
>-EXPORTS += [
>-    'nsILocalStore.h',
>-]
>+EXPORTS += []

You should be able to remove the entire line, no?


>diff --git a/toolkit/components/xulstore/XULStore.js b/toolkit/components/xulstore/XULStore.js
>+XULStore.prototype = {
>+  classID: XULSTORE_CID,
>+  classInfo: XPCOMUtils.generateCI({classID: XULSTORE_CID,
>+                                    contractID: XULSTORE_CONTRACTID,
>+                                    classDescription: "XULStore",
>+                                    interfaces: [Ci.mozIXULStore]}),
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.mozIXULStore]),


I recall from testing that I needed to implement nsISupportsWeakReference in order to get the observer to work.


>+  observe: function(subject, topic, data) {
>+    if (topic == "profile-before-change") {
>+      writeFile();

this.writeFile()


>+      let data = JSON.stringify(this._data);
>+      let encoder = new TextEncoder();
>+
>+      data = encoder.encode(data);
>+      OS.File.writeAtomic(this._storeFile.path, data, { tmpPath: this._storeFile.path + ".tmp" });

writeAtomic no longer needs the tmpPath.


>+    } catch (e) {
>+      this.log("Failed to write xulstore.json: " + e);
>+      throw e;
>+    }
>+  },
>+
>+  markAsChanged : function() {
>+    this._needsSaving = true;
>+    if (this._writeTimer)
>+      return;
>+
>+    let callback = () => {
>+      this._writeTimer = null;
>+      this.writeFile();
>+    };
>+
>+    // Don't write the file more than once every 30 seconds.
>+    this._writeTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+    this._writeTimer.initWithCallback(callback, WRITE_DELAY, Ci.nsITimer.TYPE_ONE_SHOT);
>+  },
>+
>+  /* ---------- interface implementation ---------- */
>+
>+  setValue : function (docURI, id, attr, value) {
>+    if (docURI instanceof Ci.nsIURI) {
>+      docURI = docURI.spec;
>+    }
>+
>+    // bug 319846 -- don't save really long attributes or values.
>+    if (id.length > 1024 || attr.length > 1024 || value.length > 1024)
>+      throw Components.Exception("id, attribute, or value too long", Cr.NS_ERROR_ILLEGAL_VALUE);
>+
>+    this.log("Saving " + attr + "=" + value + " for id=" + id + ", doc=" + docURI);
>+
>+    let obj = this._data;
>+    if (!(docURI in obj)) {
>+      obj[docURI] = {};
>+    }
>+    obj = obj[docURI];
>+    if (!(id in obj)) {
>+      obj[id] = {};
>+    }
>+    obj = obj[id];
>+
>+    // Don't set the value if it already set to avoid saving the file.

if it *is* already set


>+    if (attr in obj && obj[attr] == value)
>+      return;
>+
>+    obj[attr] = value; //IE, this._data[docURI][id][attr] = value;
>+
>+    this.markAsChanged();
>+  },
>+
>+  getIDsEnumerator : function (docURI) {
>+    let key = docURI.spec;

Change 'key' to docURI or something to be clearer.


>+    if (!(key in this._data))
>+      return new nsStringEnumerator([]);
>+
>+    this.log("Getting ID enumerator for doc=" + key);
>+
>+    let ids = [];
>+    let docs = this._data[key];
>+    if (docs) {
>+      for (let id in this._data[key]) {
>+        ids.push(id);
>+      }
>+    }

'docs' should instead be called 'ids' (and the array called something else) since the data is a list of ids not documents.

>+
>+    return new nsStringEnumerator(ids);
>+  },
>+
>+  getAttrsEnumerator : function (docURI, id) {
>+    let key = docURI.spec;

Same here as above as well as the remaining functions.


>+  getValue : function (docURI, id, attr) {
>+    let key = docURI.spec;
>+    this.log("get store value for id=" + id + ", attr=" + attr + ", doc=" + key);
>+
>+    let docs = this._data[key];
>+    if (docs) {
>+      let ids = docs[id];
>+      if (ids) {
>+        return ids[attr] || "";
>+      }
>+    }

Again, 'docs' should be called 'ids', and 'ids' should be called 'attrs'


>+
>+    return "";
>+  },
>+
>+  hasValue : function (docURI, id, attr) {
>+    let key = docURI.spec;
>+    this.log("has store value for id=" + id + ", attr=" + attr + ", doc=" + key);
>+
>+    let docs = this._data[key];
>+    if (docs) {
>+      let ids = docs[id];
>+      if (ids) {
>+        return attr in ids;
>+      }
>+    }

Same here.


>+
>+    return false;
>+  },
>+
>+  removeValue : function (docURI, id, attr) {
>+    let key = docURI.spec;
>+    this.log("remove store value for id=" + id + ", attr=" + attr + ", doc=" + key);
>+
>+    let docs = this._data[key];
>+    if (docs) {
>+      let ids = docs[id];
>+      if (ids && attr in ids) {
>+        delete ids[attr];

And here.

>+
>+        if (Object.getOwnPropertyNames(ids).length == 0) {
>+          delete docs[id];
>+        }

This should also delete from this._data if that is empty.

Can you reorder the functions to match that in the interface file?


>diff --git a/toolkit/components/xulstore/tests/chrome/window_persistence.xul b/toolkit/components/xulstore/tests/chrome/window_persistence.xul
...
>+function runTest(treeView)
>+{
>+  var firstRun = window.arguments[0];
>+  if (firstRun) {
>+    document.getElementById("button1").setAttribute("value", "Pressed");
>+    document.getElementById("button2").removeAttribute("value");
>+
>+    document.getElementById("button2").setAttribute("foo", "bar");
>+    document.persist("button2", "foo");
>+    document.getElementById("button2").removeAttribute("foo");
>+    document.persist("button2", "foo");

You should verify that the data is saved into the store in-between the two persist calls, and that it is removed afterwards.


>diff --git a/toolkit/components/xulstore/tests/xpcshell/test_XULStore.js b/toolkit/components/xulstore/tests/xpcshell/test_XULStore.js
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cu = Components.utils;
>+const Cr = Components.results;
>+
>+Components.utils.import("resource://gre/modules/Services.jsm");
>+Components.utils.import("resource://gre/modules/osfile.jsm")

You don't use Cu or Cr. But you do use Components.utils again.


>+add_task(function* testImport(){
>+  let src = "localstore.rdf";
>+  let dst = OS.Path.join(OS.Constants.Path.profileDir, src);
>+
>+  yield OS.File.copy(src, dst);
>+
>+  XULStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.mozIXULStore);
>+  checkOldStore();
>+});

You might want to add a comment here that this importing test relies on the xul store not yet being loaded/initialized before this point.


>diff --git a/xulrunner/app/profile/Makefile.in b/xulrunner/app/profile/Makefile.in
...
> FILES := \
>-	localstore.rdf \
> 	$(NULL)
> 
> libs:: $(FILES)
> 	$(INSTALL) $^ $(DIST)/bin/defaults/profile
> 	$(INSTALL) $^ $(DIST)/bin/defaults/profile/US

You should be able to remove the entire file. Also, I don't see the removed localstore.rdf in this patch.

    
    

    
  

      
      
      
      

        Attached file
          
        fred.txt
        — 
      

    


  
This was a test that I had wrote a while ago which could be combined with your test_XULStore.js as it tests a few more things. testEnumerator is similar to your checkArrays.

I know that some of the comments above apply to code that I wrote. Once this is more polished we probably want another reviewer to take a quick look as well.

    
    

    
  
I tested the performance with the latest patch, similar to comment 14 above:

Setting an attribute that needs to be persisted:
  Existing code - 1400ms
  With this patch - 3000ms

Not sure what happened, but this version is significantly slower.

I also put together a quick webidl implementation and it was slightly faster at 2800ms.

    
    

    
  
Comment on attachment 8446082 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v8

Two other comments:

1. We should change the interface to be nsIXULStore rather than mozIXULStore.
2. It would be nice to have the interface take a document uri string rather than an nsIURI, since the implementation uses a string anyway.

        Attachment #8446082 -
        Flags: review?(enndeakin) → review-

    
    

    
  
(In reply to Neil Deakin from comment #36)
> I tested the performance with the latest patch, similar to comment 14 above:
> 
> Setting an attribute that needs to be persisted:
>   Existing code - 1400ms
>   With this patch - 3000ms
> 
> Not sure what happened, but this version is significantly slower.
> 
> I also put together a quick webidl implementation and it was slightly faster
> at 2800ms.

I wonder though how much of a performance concern this really is; how often do we set attributes?
Flags: needinfo?(enndeakin)

    
    

    
  
Quite a lot, cf.

http://mxr.mozilla.org/mozilla-central/search?string=%20persist%3D

ie whenever those are changed / the window is closed, and then:

http://mxr.mozilla.org/mozilla-central/search?string=document.persist

whenever that code is called.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        C++ implementation (obsolete)
        — 
          — Splinter Review
      

    


  
It isn't going to be a noticeable performance issue to users, but I don't think we should be regressing performance if it isn't needed.

I found the unfinished C++ implementation I has started and have attached it, for future work.
Flags: needinfo?(enndeakin)

    
    

    
  


  
(In reply to Neil Deakin from comment #35)

Please note that in the updated patch I left the current implementation of locastore.rdf as it is for background compatibility. A warning message is issued when a client tries to access it though.

> >diff --git a/browser/components/places/tests/browser/browser_toolbar_migration.js b/browser/components/places/tests/browser/
> >@@ -93,43 +56,52 @@ function test_many_bookmarks_toolbar()
> >   ids.push(
> >     PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
> >                                           PlacesUtils.bookmarks.DEFAULT_INDEX)
> >   );
> >   ids.push(
> >     PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
> >                                           PlacesUtils.bookmarks.DEFAULT_INDEX)
> >   );
> >+  ids.push(
> >+    PlacesUtils.bookmarks.insertSeparator(PlacesUtils.toolbarFolderId,
> >+                                          PlacesUtils.bookmarks.DEFAULT_INDEX)
> >+  );
> 
> Why do you add this?

To comply with the condition in nsBrowserGlue.js, i.e.:

>        if (toolbarIsCustomized || getToolbarFolderCount() > 3) {
>          xulStore.setValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed", "false");
>        }

Note that the test fails on my machine even without the patch.

(In reply to Neil Deakin from comment #35)
> Created attachment 8453947 [details]
> fred.txt
> 
> This was a test that I had wrote a while ago which could be combined with
> your test_XULStore.js as it tests a few more things. testEnumerator is
> similar to your checkArrays.

I have added more tests to the test suite.

        Attachment #8446082 -
        Attachment is obsolete: true

        Attachment #8459529 -
        Flags: review?(enndeakin)

    
    

    
  
FYI, bug 1040774 moved to rdf/base/src/nsRDFService.cpp to rdf/base/nsRDFService.cpp.

    
    

    
  
(In reply to Neil Deakin from comment #36)
> I tested the performance with the latest patch, similar to comment 14 above:
> 
> Setting an attribute that needs to be persisted:
>   Existing code - 1400ms
>   With this patch - 3000ms

None of the 2 implementations will do any IO in this time, so looks like it's all overhead.  Did you profile where this time is spent with the js implementation? Before moving to cpp we should know why js is so much slower in this simple case.

Out of curiosity I tried calling Assert and (a locally built version of) setValue in a 1000 iterations loop but I'm very far from 3000ms, I'm around 7ms for both :/ If I increase iterations the js version wins (though I'm skipping xpcom overhead here but I suspect js to cpp is more expensive than js to js). May you share your perf testing code?

    
    

    
  
Comment on attachment 8459529 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v9

>-        // Get the datasource we intend to use to remember open state.
>-        nsAutoString datasourceStr;
>-        mRoot->GetAttr(kNameSpaceID_None, nsGkAtoms::statedatasource, datasourceStr);
>-
>-        // since we are trusted, use the user specified datasource
>-        // if non specified, use localstore, which gives us
>-        // persistence across sessions
>-        if (! datasourceStr.IsEmpty()) {
>-            gRDFService->GetDataSource(NS_ConvertUTF16toUTF8(datasourceStr).get(),
>-                                       getter_AddRefs(mPersistStateStore));
>-        }
I don't think this removal will break anything, but you should document it.

    
    

    
  
> Out of curiosity I tried calling Assert and (a locally built version of)
> setValue in a 1000 iterations loop but I'm very far from 3000ms, I'm around
> 7ms for both :/ If I increase iterations the js version wins (though I'm
> skipping xpcom overhead here but I suspect js to cpp is more expensive than
> js to js). May you share your perf testing code?

I am calling setAttribute 10000 times. This is C++ calling into the JS component.

    
    

    
  
>-nsresult
>-nsXULTreeBuilder::IsContainerOpen(nsIRDFResource* aResource, bool* aOpen)
>-{
>-    if (mPersistStateStore)
>-        mPersistStateStore->HasAssertion(aResource,
>-                                         nsXULContentUtils::NC_open,
>-                                         nsXULContentUtils::true_,
>-                                         true,
>-                                         aOpen);
>-    else
>-        *aOpen = false;
>+  nsIURI* docURI = doc->GetDocumentURI();
> 
>-    return NS_OK;
>+  nsAutoString nodeid;
>+  nsresult rv = aResult->GetId(nodeid);
>+  if (NS_FAILED(rv)) {
>+    return false;
>+  }
>+
>+
>+  nsAutoCString utf8uri;

Remove the extra blank line.


>diff --git a/rdf/base/src/nsRDFService.cpp b/rdf/base/src/nsRDFService.cpp
>     // Nope. So go to the repository to try to create it.
>     nsCOMPtr<nsIRDFDataSource> ds;
>     if (StringBeginsWith(spec, NS_LITERAL_CSTRING("rdf:"))) {
>+        if (spec.Equals("rdf:local-store")) {
>+            nsCOMPtr<nsIConsoleService> consoleService =
>+                do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>+
>+            if (consoleService) {
>+                consoleService->LogStringMessage(NS_LITERAL_STRING(
>+                    "Warning: rdf:local-store is deprecated, please use XULStore.").get());
>+            }
>+        }
>+

Since you didn't remove the automatic local-store usages from content/xul/templates, this error will occur without a means to change it.
So I don't think you should add this warning.


>+  getIDsEnumerator : function (docURI) {
>+    if (!(docURI in this._data))
>+      return new nsStringEnumerator([]);
>+
>+    this.log("Getting ID enumerator for doc=" + docURI);

Let's be consistent and move the log line before the condition. Similarly for getAttrsEnumerator.


>+interface nsIXULStore: nsISupports
>+{
>+  /**
>+   * Sets a value in the store.
>+   *
>+   * @param docURI - document URI
>+   * @param id - identifier of the node
>+   * @param attr - attribute to store
>+   * @param value - value of the attribute
>+   */
>+  void setValue(in AString doc, in AString id, in AString attr, in AString value);

You use 'docURI' in the comment text but the argument is 'doc'. Same with the other functions.


>+  /**
>+   * Returns true if the store contains a value for attr.
>+   *
>+   * @param docURI - nsIURI of the document

This isn't an nsIURI any more.


>+add_task(function* testGetValue() {
>+  // Get non-existing property
>+  checkValue(browserURI, "side-window", "height", "");
>+
>+  // Get existing property
>+  checkValue(browserURI, "main-window", "width", "994");
>+});

hasValue already has these checks. Perhaps remove them there.


>+add_task(function* testSetValue() {
...
>+  // Add another attribute
>+  checkValue(browserURI, "side-bar", "heigth", "");

You've spelled 'height' wrong.


>+  XULStore.setValue(browserURI, "side-bar", "heigth", "1000");
>+  checkValue(browserURI, "side-bar", "heigth", "1000");
>+  checkArrays(["addon-bar", "main-window", "side-bar", "sidebar-title"], getIDs(browserURI));
>+  checkArrays(["width", "heigth"], getAttributes(browserURI, 'side-bar'));
>+});
>+
>+add_task(function* testRemoveValue() {
>+  // Remove first attribute
>+  checkValue(browserURI, "side-bar", "width", "1024");
>+  XULStore.removeValue(browserURI, "side-bar", "width")
>+  checkValue(browserURI, "side-bar", "width", "");

Add a hasValue check here as well. All of your removeValue lines should end with semicolons.


>+  // Removing from an id that doesn't exists shouldn't fail
>+  XULStore.removeValue(browserURI, "foo", "bar")

exists -> exist


>+
>+  // Removing from a document that doens' exists shouldn't fail
>+  let nonDocURI = "chrome://example/content/other.xul";
>+  XULStore.removeValue(nonDocURI, "foo", "bar")

doesn't exist


Add a test for getAttributes (getAttrsEnumerator) for non-existing documents and ids.

While thinking about this, getAttrsEnumerator should be renamed and spelled out as getAttributeEnumerator.

You might consider changing the interface to take string rather than AString to avoid having to convert UTF16 to UTF8 yourself.

    
  

        Attachment #8459529 -
        Flags: review?(enndeakin) → review+

    
    

    
  
(In reply to Neil Deakin from comment #46)
> I am calling setAttribute 10000 times. This is C++ calling into the JS
> component.

I see, doesn't look like cpp to js is the more common case we have in the tree though, is it?

    
    

    
  
Comment on attachment 8459529 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v9

Review of attachment 8459529 [details] [diff] [review]:
-----------------------------------------------------------------

Just some drive_by comments, I looked at certain parts better than others, so don't count on this as an official review :)

::: browser/components/places/content/treeView.js
@@ +17,5 @@
>    this._rows = [];
>    this._flatList = aFlatList;
>    this._openContainerCallback = aOnOpenFlatContainer;
>    this._controller = aController;
> +  this._xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);

please make this a lazy getter.

@@ +1119,4 @@
>    {
> +    let ioService = Cc["@mozilla.org/network/io-service;1"].
> +                    getService(Ci.nsIIOService);
> +    return ioService.newURI(document.URL, null, null);

I guess you might make documentURI a getter and memoize it? I don't expect it to change

::: content/xul/document/src/XULDocument.cpp
@@ +1335,5 @@
>      aElement->GetAttr(kNameSpaceID_None, aAttribute, valuestr);
>  
> +    nsresult rv;
> +    nsAutoCString utf8uri;
> +    rv = mDocumentURI->GetSpec(utf8uri);

nit: declare rv on first use

@@ +2095,5 @@
>      nsCOMArray<nsIContent> elements;
>  
> +    nsAutoCString utf8uri;
> +    nsresult rv;
> +    rv = mDocumentURI->GetSpec(utf8uri);

nit: declare at first use

@@ +2136,3 @@
>      }
>  
> +    return rv;

return NS_OK; errors are handled inline

@@ +2145,5 @@
>  {
>      nsresult rv;
>  
> +    nsAutoCString utf8uri;
> +    rv = mDocumentURI->GetSpec(utf8uri);

nit: ditto :)

@@ +2183,3 @@
>          uint32_t cnt = aElements.Count();
>  
> +        for (int32_t i = int32_t(cnt) - 1 ; i >= 0; --i) {

nit: remove whitespace before ;

@@ +2187,5 @@
> +            if (!element) {
> +                 continue;
> +            }
> +
> +            rv = element->SetAttr(kNameSpaceID_None, attr, value, PR_TRUE);

you are ignoring this error and overwriting it, it should be handled instead.

@@ +2192,4 @@
>          }
>      }
>  
> +    return rv;

NS_OK

::: content/xul/document/src/XULDocument.h
@@ +319,1 @@
>      bool                       mApplyingPersistedAttrs;

fix indentation per code module convention

::: toolkit/components/xulstore/XULStore.js
@@ +5,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> +
> +const debugMode = false;

please add comment about what this does

@@ +9,5 @@
> +const debugMode = false;
> +
> +// Delay when a change is made to when the file is saved.
> +// 30 milliseconds normally, or 3 milliseconds for testing
> +const WRITE_DELAY = (debugMode ? 3 : 30) * 1000;

As far as I can tell, this is 3 seconds or 30 seconds, not milliseconds (please fix the comment)
would also be nice to specify the unit in the const name, like WRITE_DELAY_MS

@@ +18,5 @@
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/NetUtil.jsm");
> +Components.utils.import("resource://gre/modules/osfile.jsm")

please lazyModuleGetter NetUtil and osfile

@@ +31,5 @@
> +                                    contractID: XULSTORE_CONTRACTID,
> +                                    classDescription: "XULStore",
> +                                    interfaces: [Ci.nsIXULStore]}),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsIXULStore,
> +                                         Ci.nsISupportsWeakReference]),

if it's expected to behave like a service you'd better add
_xpcom_factory: XPCOMUtils.generateSingletonFactory(XULStore),

unless you expect it might be useful to have multiple instances.

@@ +47,5 @@
> +   *      "chrome://foopy/b.xul" :  { ... },
> +   *      ...
> +   *  }
> +   */
> +  _data     : {},

multiple instances would also share this object since it's defined in the prototype (wanted or mistake? It's usually better to define local cache objects in the constructor)

@@ +52,5 @@
> +  _storeFile : null,
> +  _needsSaving : false,
> +
> +  init : function () {
> +    Services.obs.addObserver(this, "profile-before-change", false);

why did you add nsISupportsWeakReference but then add a strong ref? weak ref should be fine here (indeed you then forgot to removeObserver)

@@ +54,5 @@
> +
> +  init : function () {
> +    Services.obs.addObserver(this, "profile-before-change", false);
> +
> +    let filename = STOREDB;

what's the point? just name the const better if it's for readability, like STOREDB_FILENAME?

@@ +65,5 @@
> +      this.readFile();
> +    }
> +  },
> +
> +  observe: function(subject, topic, data) {

nit: you are mixing up styles, like "init : function ()" and "observe: function()", please be coherent (I think the latter style is more common)

@@ +67,5 @@
> +  },
> +
> +  observe: function(subject, topic, data) {
> +    if (topic == "profile-before-change") {
> +      this.writeFile();

would be nice to also stop accepting requests and warn quite loudly if something tries to add data after this point (also, avoid setting up timers from now on)

@@ +103,5 @@
> +      let resource = resources.getNext().QueryInterface(Ci.nsIRDFResource);
> +      let uri;
> +
> +      try {
> +        uri = Services.io.newURI(resource.ValueUTF8, "", null);

NetUtil.newURI(resource.ValueUTF8);

@@ +164,5 @@
> +      let data = JSON.stringify(this._data);
> +      let encoder = new TextEncoder();
> +
> +      data = encoder.encode(data);
> +      OS.File.writeAtomic(this._storeFile.path, data);

honestly, I'd pass tmpPath option here, it would make the likelyhoo of storing a corrupt store less likely, for a small price.
see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm#1164

you should also handle error here, not just the exception but an eventual promise rejection should be handled and reportError'ed

@@ +182,5 @@
> +      this.writeFile();
> +    };
> +
> +    // Don't write the file more than once every 30 seconds.
> +    this._writeTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

you don't need to create a new timer instance every time, you can just create one and reuse it

    
    

    
  
Thanks for all your comments.

    
    

    
  
Just to be sure, Reminderfox for Firefox/Thunderbird is using
> .getService(Components.interfaces.nsIRDFService);
to resolve the Thunderbird msgfolder from uri.

Would that be affected by this change?

    
    

    
  
(In reply to Günter from comment #52)
> Just to be sure, Reminderfox for Firefox/Thunderbird is using
> > .getService(Components.interfaces.nsIRDFService);
> to resolve the Thunderbird msgfolder from uri.
> 
> Would that be affected by this change?

It wouldn't.

    
    

    
  
Comment on attachment 8463572 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v10

Neil, you mentioned we should let this be reviewed by someone else too. Whom do you suggest? What about Marco, he already made some good remarks.

        Attachment #8463572 -
        Flags: feedback?(enndeakin)

    
    

    
  
Marco would be ok . Or from a superreviewer (I would suggest bsmedberg or gavin).

        Attachment #8463572 -
        Flags: feedback?(enndeakin) → superreview?(benjamin)

    
    

    
  
Jorge, Benjamin suggested to add a rule in the AMO automated checking/validation to warn addons that use localstore. Could you give me some pointers on the best way to do that? Thanks!
Flags: needinfo?(jorge)

    
    

    
  
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #56)
> Jorge, Benjamin suggested to add a rule in the AMO automated
> checking/validation to warn addons that use localstore. Could you give me
> some pointers on the best way to do that? Thanks!

We got this. Just make sure the milestone is properly set when the bug lands.
Flags: needinfo?(jorge)
Keywords: addon-compat

    
    

    
  
Comment on attachment 8463572 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v10

I'm a little sad about the nsIStringEnumerator stuff here. I wonder if you can do better by declaring this in webidl instead of xpidl and returning arrays of strings?

This currently imports from localstore using the C++ RDF stuff. Can you file a followup bug to import using a .jsm instead, so we can think about removing the C++ RDF bits?

Are you set on the name "xulstore"? This is basically just a key/key/value datastore, which if it's relatively persistent is likely to be useful for things other than XUL.

I didn't do a detailed code review here, but I did read over the interface/tests usage and I don't see problems with this.

        Attachment #8463572 -
        Flags: superreview?(benjamin) → superreview+

    
    

    
  
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #58)
> Are you set on the name "xulstore"? This is basically just a key/key/value
> datastore, which if it's relatively persistent is likely to be useful for
> things other than XUL.

Do you have any particular name in mind?

    
    

    
  
kvstore?

    
    

    
  
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #58)
> This is basically just a key/key/value
> datastore, which if it's relatively persistent is likely to be useful for
> things other than XUL.

I honestly hope nobody is going to use this synchronous store on main-thread apart from localstore.  And it can't be used off the main thread cause it's a plain js component.  Naming it too generically like keyValueStore or such, would make it look like a one-size-fits-all solution to store any kind of thing. We know it's not the kind of kv store we want add-ons to use. I'd personally prefer a dedicated name like xulPersistenceStore than a generic one.

    
    

    
  
Implementing a service using webidl sounds rather odd. What would be the parent object for such thing?
We could hack somehow that the service would get junk scope as parent, but I doubt
bholley would accept that.

    
    

    
  
Benjamin, please have a look at comment 61 and 62. I am suggesting to leave the name as is and not using a webidl interface, unless there is a clean way to do it.
Flags: needinfo?(benjamin)

    
    

    
  
ok, if this is really not intended to be a generic kvstore, then the current name is fine.

As for webidl, bholley explicitly said that he thought we could use .webidl for JS components and it improves the array junk significantly. But the current version is fine especially if this is meant to be internal-only.
Flags: needinfo?(benjamin)

        Attachment #8458681 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8475301 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v11

>+  // Retrieves an nsIURI for the document
It just occurs to me that maybe document.documentURIObject is what you want?

    
    

    
  
Comment on attachment 8476012 [details] [diff] [review]
Talos: Allow IO on xulstore.json

Review of attachment 8476012 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, please land this on the talos repo.

        Attachment #8476012 -
        Flags: review?(jmaher) → review+

    
    

    
  
Ryan, could you please land Attachment #8476012 [details] [diff] on the talos repo?
Flags: needinfo?(ryanvm)

    
    

    
  
I think Joel's taking care of pushing the talos patch.
Flags: needinfo?(ryanvm)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/25c918c5f3e1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

    
    

    
  
(In reply to neil@parkwaycc.co.uk from comment #67)
> Comment on attachment 8475301 [details] [diff] [review]
> Bug 559505 - localstore.rdf kills ponies, v11
> 
> >+  // Retrieves an nsIURI for the document
> It just occurs to me that maybe document.documentURIObject is what you want?
Flags: needinfo?(rvitillo)

    
    

    
  
I have seen that, I am going to create a follow up bug to clean this up.
Flags: needinfo?(rvitillo)
No longer blocks: FF2SM
Blocks: 1056948

    
  
Depends on: 1057137

    
    

    
  
Looks like this breaks things in Thunderbird which must be addressed asap.

-------- Forwarded Message --------
Subject: 	Re: Breakage between 08-19 and 08-21
Date: 	Fri, 22 Aug 2014 09:09:11 +0300
From: 	Magnus Melin
To: 	tb-planning

Bug 559505 should account for at least part of the breakage I think.

 -Magnus

On 22.8.2014 08:13, Jonathan Protzenko wrote:
> Hi,
>
> I noticed several things breaking all at once over the past few days.
> - In 08-19, everything is fine as far as I can tell.
> - In 08-20, the call I am making (in Conversations) to
> nsIMsgCompose::initEditor fails with an error "Failure: arg 0" in the error
> console. Do you if something changed in this area?
> http://hg.mozilla.org/comm-central/pushloghtml gives little clues, so I
> suspect it must be in mozilla-central.
> - In 08-21, several things are broken at once: the thread summary no longer
> works, tabs are not saved, the state of the folder pane (unified vs. all vs.
> ...) is not saved either. Do you have any hints as to what bugs could be
> causing this?
>
> In the meantime, I would advise _not_ to use 08-21 because it seems badly
> impacted. I can file bugs if that helps.
>
> Cheers,
>
> ~ jonathan

    
    

    
  
Looks like this broke Firebug, which relies on PlacesUIUtils.localStore. This change should at least be documented at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Places_utilities_for_JavaScript.

Sebastian
Keywords: dev-doc-needed
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
(In reply to Sebastian Zartner from comment #78)
> Looks like this broke Firebug, which relies on PlacesUIUtils.localStore.
> This change should at least be documented at
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/
> Places_utilities_for_JavaScript.
> 
> Sebastian

Done, thanks. How does Firebug use localstore? Do you use it to access properties set by other components or do you just use it to store Firebug-local data?
Flags: needinfo?(sebastianzartner)
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED

    
    

    
  
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #79)
> (In reply to Sebastian Zartner from comment #78)
> > Looks like this broke Firebug, which relies on PlacesUIUtils.localStore.
> > This change should at least be documented at
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/
> > Places_utilities_for_JavaScript.
> > 
> > Sebastian
> 
> Done, thanks.

Thanks. Of course nsIXULStore still needs to be described at MDN. Referring people to 'xulstore' without giving any hint of how it works is a bit bad.

> How does Firebug use localstore? Do you use it to access
> properties set by other components or do you just use it to store
> Firebug-local data?

Just to store Firebug-local data[1], specifically the width and height of the Firebug UI[2].
For reference, I created an issue for that in the Firebug issue tracker[3]. A quick hint on how to change the current code would be nice.

Sebastian

[1] https://github.com/firebug/firebug/blob/0f024e7fee4c208a641f73db645084b8e9a38c95/extension/content/firebug/firefox/browserOverlayLib.js#L273
[2] https://github.com/firebug/firebug/blob/0f024e7fee4c208a641f73db645084b8e9a38c95/extension/content/firebug/firefox/browserOverlay.js#L167
[3] https://code.google.com/p/fbug/issues/detail?id=7640
Flags: needinfo?(sebastianzartner)

    
    

    
  
(In reply to Sebastian Zartner from comment #80)
> Thanks. Of course nsIXULStore still needs to be described at MDN. Referring
> people to 'xulstore' without giving any hint of how it works is a bit bad.

As xulstore is meant as a strict replacement for localstore.rdf and not a generic kv-store, I wouldn't want to advertise it too much. Please have a look at the interface documentation at http://dxr.mozilla.org/mozilla-central/source/toolkit/components/xulstore/nsIXULStore.idl.

> Just to store Firebug-local data[1], specifically the width and height of
> the Firebug UI[2].
> For reference, I created an issue for that in the Firebug issue tracker[3].
> A quick hint on how to change the current code would be nice.

I will comment on the issue tracker asap.

    
    

    
  
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #81)
> As xulstore is meant as a strict replacement for localstore.rdf and not a
> generic kv-store, I wouldn't want to advertise it too much.

Note though that localstore.rdf is meant to be a generic datastore, so there may some addons that use it that way.

The links pointed to in comment 80 though suggest they are just updating persist data using the rdf api rather than setting attributes and so forth, so it should be a simple conversion to use nsIXULStore instead.

But I would suggest creating a short migration guide.

    
    

    
  
Comment on attachment 8475301 [details] [diff] [review]
Bug 559505 - localstore.rdf kills ponies, v11

>+          let docURI = this._getDocumentURI();
>+          let val = this._xulStore.getValue(docURI, uri, "open");
...
>+  // Retrieves an nsIURI for the document
>+  _documentURI: null,
>+  _getDocumentURI: function()
>   {
>-    let uri = aNode.uri;
>-    NS_ASSERT(uri, "if there is no uri, we can't persist the open state");
>-    return uri ? PlacesUIUtils.RDF.GetResource(uri) : null;
>+    if (!this._documentURI) {
>+      let ioService = Cc["@mozilla.org/network/io-service;1"].
>+                      getService(Ci.nsIIOService);
>+      this._documentURI = ioService.newURI(document.URL, null, null);
>+    }
>+    return this._documentURI;
>   },
...
>+  /**
>+   * Retrieves a value in the store, or an empty string if it does not exist.
>+   *
>+   * @param doc - document URI
>+   * @param id - identifier of the node
>+   * @param attr - attribute to retrieve
>+   *
>+   * @returns the value of the attribute
>+   */
>+  AString getValue(in AString doc, in AString id, in AString attr);

I just noticed comment 47 - you don't need the document URI as an nsIURI, so just document.URL suffices, i.e.
let val = this._xulStore.getValue(document.URL, uri, "open");

    
    

    
  
I see that the old Places persist data doesn't get "upgraded" into xulstore.json (I'm not going to lose sleep over it but I didn't notice it mentioned before.)

The old Places code also tried to share the persist of the twisty open states between the sidebar and the library. The new code seems to achieve this by virtue of the bug mentioned in my previous comment.

    
    

    
  
Ah, maybe that doesn't apply to the Library, only the old Bookmarks and History windows, so it makes no difference there.

    
  
Depends on: 1057481

    
  
No longer depends on: 1057481

    
  
Depends on: 1058436

    
  
Blocks: 1069110
Blocks: 1077959
Depends on: 1368567

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: