Once an attribute is persisted to localstore.rdf, it will be applied regardless of whether persistance is still specified for that attribute. It should check if persist is still specified for the attribute before applying (and remove it from localstore if it's not).

Note to self: this will affect documents that programmatically persist without having an accompanying persist attribute.

One thing to note for this is that if we change the xul to specify persist where currently the attributes are only persisted from js, there could be a weird case where someone depends on that behaviour (e.g. "apply" vs. "cancel"). All our current uses don't have that problem, though one could argue that the persist on close is redundant when you're also doing persist "when needed". Would it be worth adding a |restore| function to nsIDOMXULDocument? hewitt: could you see what would need to change in DOM Inspector once this bug is fixed? /extensions/inspector/resources/content/jsutil/xul/inFormManager.js, line 75 -- aDoc.persist(aEl.getAttribute("id"), "value"); Not sure what you're doing here. The line above it does something like: aEl.setAttribute("value", aEl.value);. What are you trying to do there? /extensions/inspector/resources/content/utils.js, line 71 -- document.persist(aId, attrs[i]); This is in a persistAll function that persists all attributes for a given element. Apart from the question why you're calling that instead of letting xul persist do its job (I'm sure there's a good explanation), you'd probably need to modify this to skip the id and persist attributes, and add a persist attribute enumerating all the other attributes. ----- ----- ----- ----- jag: /extensions/wallet/resources/content/walletNavigatorOverlay.xul, line 61 -- document.persist(id, 'hidden'); Add persist="hidden" to: http://lxr.mozilla.org/seamonkey/source/extensions/wallet/resources/content/walletNavigatorOverlay.xul#136 /extensions/wallet/resources/content/walletNavigatorOverlay.xul, line 62 -- document.persist(elementID, 'checked'); Add persist="checked" to: http://lxr.mozilla.org/seamonkey/source/extensions/wallet/resources/content/walletNavigatorOverlay.xul#97 /xpfe/communicator/resources/content/utilityOverlay.js, line 174 -- document.persist(id, 'hidden'); /xpfe/communicator/resources/content/utilityOverlay.js, line 175 -- document.persist(elementID, 'checked'); Ugh. Similar to goToggleFormToolbar: http://lxr.mozilla.org/seamonkey/search?string=goToggleToolbar /xpfe/components/console/resources/content/console.js, line 52 -- document.persist("ConsoleBox", "mode"); /xpfe/components/console/resources/content/console.js, line 65 -- document.persist("ConsoleBox", "sortOrder"); Add persist="mode sortOrder" to: http://lxr.mozilla.org/seamonkey/source/xpfe/components/console/resources/content/console.xul#151 /xpfe/components/console/resources/content/console.js, line 101 -- document.persist(toolbar.id, "hidden"); /xpfe/components/console/resources/content/console.js, line 102 -- document.persist(bc.id, "checked"); Same toolbar code. /xpfe/components/sidebar/resources/sidebarOverlay.js, line 1000 -- setTimeout("document.persist('sidebar-panels-splitter-box','height');",100); Add persist="height" to: http://lxr.mozilla.org/seamonkey/source/xpfe/components/sidebar/resources/sidebarOverlay.xul#61 /xpfe/global/resources/content/bindings/toolbar.xml, line 37 -- document.persist(toolbar.id, "moz-collapsed"); /xpfe/global/resources/content/bindings/toolbar.xml, line 57 -- document.persist(toolbar.id, "moz-collapsed"); This one is slightly harder. We'd either end up adding persist="moz-collapsed" to every toolbar, or add some js way of reading the value from localstore. We can't add it to the xbl binding in a declerative way, because that is (or should be) called after we already applied localstore. Adding an "ignore-persisted" attribute might look good for a while (backward compatible, less intrusive), but then you'll end up with a large-ish list of attributes you want to ignore after a few version changes. Input?

->waterson as per jag

This blocks us from hiding the total/unread columns by default for all users with existing accounts. (bug 95468) We added two new columns to the folderpane: Unread and Total. We want both to be hidden by default, so I hacked up a patch, basically adding: |hidden="true" persist="hidden"| to the outliner. I realized that it was now never persisted. hyatt told me about this bug. The result for all users with existing accounts is an unusable folderpane which looks something like this: | Name | Unr... | Total | St... 123... 2890 Ha... 433 924 Ac... 0 0 (for screenshots: attachment 45947 [details] , attachment 45948 [details] [diff] [review]) So we now get a bunch of bugreports and feedback in the newsgroups from desperated users that thinks their folders are gone, their accounts "broken"... let alone their messages. According to the feedback, many users don't even understand how to hide or resize these columns so they end up importing (thus recovering, from their point of view) their emails in Outlook and switching email app or throwing away their existing accounts with unread messages. Sorry for the long comment, but this has extremely negative impact on mailnews and I wanted you to know. I think you should consider this for 0.9.4. Thanks.

This sounds like it oughtta be nsenterprise...

Invert logic so that we read the persist attribute's value, and use that as the driver for applying persistent values. Do we really need this for mozilla-0.9.4? At one point I thought jag said all the problems had been hacked around, but...

Comment on attachment 47868 [details] [diff] [review] only apply persistent values to those attributes listed in the ``persist'' attribute sr=jst

As far as I know, the issues I've mentioned in my 2001-08-14 00:27 comment are still open issues, checking this in would break those cases. I believe security/* has hacked/worked around their issues. If this is needed for 0.9.4, I can try and get these places fixed tonight. Can you get a "yes, we want this" or "no, can wait"?

Comment on attachment 47868 [details] [diff] [review] only apply persistent values to those attributes listed in the ``persist'' attribute (Removed the - lines for clarity) >+ nsAutoString persist; >+ element->GetAttr(kNameSpaceID_None, nsXULAtoms::persist, persist); > >+ nsAString::const_iterator iter, end; >+ persist.BeginReading(iter); >+ persist.EndReading(end); In this case, if you use |nsAutoString::const_iterator| (or |nsAFlatString::const_iterator|, it's the same thing), your iterators will for now be the normal |nsStringIterator|s, but will become |const PRUnichar*| (since your string is known to be flat and doesn't need multi fragment handling logic). Generally you can safely use this pattern: nsSomeString foopy; // put something in foopy nsSomeString::const_iterator iter, end; foopy.BeginReading(iter); foopy.EndReading(end); This will end up using the most appropriate iterator for that type of string. >+ while (iter != end) { >+ while (iter != end && (nsCRT::IsAsciiSpace(*iter) || *iter == PRUnichar(','))) >+ ++iter; > >+ nsAString::const_iterator mark = iter; > >+ while (iter != end && !nsCRT::IsAsciiSpace(*iter) && *iter != PRUnichar(',')) >+ ++iter; >+ >+ ApplyPersistentAttributeTo(aResource, element, Substring(mark, iter)); > } This doesn't look right. Aren't you going to skip the first token in the persist attribute this way? You should just be able to remove the first while scan loop and get what you want.

Ok, I'm moving this to mozilla-0.9.5. I assume that if anyone really needs it, they'll scream. jag: I'll change the |nsAString::const_iterator to |nsAutoString::const_iterator|; thanks for pointing that out. Also, I think the loop is correct (at least, it ought not fail in the way you suggested). Specifically, the first nested |while| will skip over any whitespace, but if there is none, will leave |iter| positioned at the start of the string.

Waterson: Let me remind you of my scream from 2001-08-25 18:16. Does that count? ;) Mailnews is still horked for new users post-folderpane rewrite and I don't think we should ship 0.9.4 like that.

Okay, back to mozilla-0.9.4. jag, can you work up a patch for the instances you describe on 2001-08-14 00:27?

Waterson: d'oh, you're right. That parse loop is fine. Okay, I'll go convert. Can you give me that |void readPersisted(in DOMString id, in DOMString attribute);| I mentioned in e-mail? Should I implement it?

XUL changes look fine by me. hyatt and/or ben, could you sr= this whole mess?

First of all, hwaara has glommed onto the wrong bug. This bug has nothing to do with the folder pane issue (which is actually about attribute removals not causing the localstore to unassert). I disagree with this change and believe this warrants more consideration before landing. This change basically says that any time you use document.persist(), you are now screwed. Unless there's a corresponding read function on XUL document, you won't be able to bring those attributes back. IMO the way to fix this bug is to keep the behavior of the local store the same and to rev IDs if/when you decide to alter their persistence properties. I am planning on converting persistence code to overlays (hopefully before 1.0), and I am not going to get into a partial overlay application hell. You either persisted or you didn't, and if you want to change this, rev the ID of the element in question or change your entire file name (version your file names). My $0.02.

Sounds good to me!

A bug that I filed, but 115296 (Persist should work for items created by JS) is related to this bug. Mabye people who were / are working on this bug would like to know about it.

Why this bug was marked as resolved wontfix? The suggested work around of this bug is ridiculous! Not only it requires multiple changes in the code when persistence no longer needed (versus simply remove persist attribute), but it also pollutes the xulstore.json storage with no way of cleaning it!