Closed Bug 328162 Opened 19 years ago Closed 19 years ago

Evaluate Storage Options for Session Data

Categories

(Firefox :: Tabbed Browser, enhancement)

2.0 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 2 beta1

People

(Reporter: dietrich, Assigned: dietrich)

References

Details

(Keywords: fixed1.8.1, Whiteboard: 181b1+)

Attachments

(1 file, 2 obsolete files)

Need to determine pros/cons of using the current text format vs. XML, SQLite or RDF.
Blocks: 328154
Assignee: nobody → dietrich
Flags: blocking-firefox2+
Status: NEW → ASSIGNED
The current implementation uses an ini file to store the session data. This format is not typical of our current data storage approaches, but is not unprecedented (eg: profiles.ini). It provides these benefits: - standard well-known format - easily machine- and human-readable Some downsides to using ini: - not ideal for representing hierarchal data - Yet Another File Format SQLite: It may be advantageous to move the data into SQLite for consistency and integration reasons, but AFAICT there is no overwhelming requirement to do so at this time. RDF: The project seems to be trending away from using RDF, in favor of mozStorage. The RDF APIs are cumbersome to work with, and have performance issues when working with large datasets. (Using mozStorage-backed-RDF, might solve the perf question?) XML: http://www.tbray.org/ongoing/When/200x/2006/01/08/No-New-XML-Languages (SWAG-ing at 0 days until conversation dictates otherwise)
Whiteboard: 0d
> XML: http://www.tbray.org/ongoing/When/200x/2006/01/08/No-New-XML-Languages That page discusses entirely different thing (not that I think that XML is the right choice here). I would like the toSource/eval[InSandbox] variant considered seriously (because if there are any shortcomings with it, I'd like them to be pointed out to me). It's only slightly less readable than an INI format (why do we care about its readability anyway?) and it is results in significally easier code.
>> XML: http://www.tbray.org/ongoing/When/200x/2006/01/08/No-New-XML-Languages > That page discusses entirely different thing (not that I think that XML is the > right choice here). It discusses reasons why not to create a new XML vocabulary. How is that not applicable to this discussion? > I would like the toSource/eval[InSandbox] variant considered seriously (because > if there are any shortcomings with it, I'd like them to be pointed out to me). > It's only slightly less readable than an INI format (why do we care about its > readability anyway?) and it is results in significally easier code. Yes, the code would be simpler and more compact. I'll swag it for A2. Are there instances of other parts of the project doing storage this way? WRT to readability: What I was trying to get at is that while ini doesn't provide any major benefits, there had been no overwhelming case for changing it.
Whiteboard: 0d → 1d
(In reply to comment #3) > >> XML: http://www.tbray.org/ongoing/When/200x/2006/01/08/No-New-XML-Languages > It discusses reasons why not to create a new XML vocabulary. How is that not > applicable to this discussion? That article talks about other kinds of languages. You can see this from the point author tries to make (there should be multiple interoperable implementations, years to develop the language, complex validating tools, etc.) Anyways I don't think XML is appropriate here either, so I don't know why I made that comment. -- AFAIK no other code in tree uses toSource/evalInSandbox today, which is why I'm not sure if this approach has downsides I'm unaware of.
Testing using toSource/eval for storing the session data. This allows us to remove the INI parsing/serializing code, modestly decreasing the size and complexity of the patch for 328159. Needs to be updated to use evalInSandbox.
re-targetted at beta 1
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1
Whiteboard: 1d → [swag: .5d]
Attached patch implements toSource/evalInSandbox storage (obsolete) (deleted) — Splinter Review
Attachment #218920 - Attachment is obsolete: true
Attachment #226766 - Flags: review?(mconnor)
Should you really intend change the storage format, make sure to not leave any references to the INI format (i.e. change or remove the file extension). And then you could/should also get rid of the capitalized fields (i.e. use windows instead of Window, tabs instead of Tab, etc.). Out of curiosity: has this change been discussed anywhere?
(In reply to comment #8) > Should you really intend change the storage format, make sure to not leave any > references to the INI format (i.e. change or remove the file extension). And > then you could/should also get rid of the capitalized fields (i.e. use windows > instead of Window, tabs instead of Tab, etc.). Thanks, will do. > Out of curiosity: has this change been discussed anywhere? Here on the bug, and on the wiki. Some benefits: - no extra de/serialization at load, save and API - makes it easy for extension authors to convert to other formats (and removes a step: deserializing from Ini) - removes more code from both classes (this is only a plus if the "JS component size hurts Ts" theory is true)
Attachment #226766 - Flags: review?(mconnor)
Attached patch fixes simon's comments (deleted) — Splinter Review
Attachment #226766 - Attachment is obsolete: true
Attachment #226837 - Flags: review?(mconnor)
Attachment #226837 - Flags: review?(mconnor) → review+
Checked in on trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [swag: .5d] → [swag: .5d] 181b1+
Comment on attachment 226837 [details] [diff] [review] fixes simon's comments Working fine on trunk (after fixing bug 342889, which is already approved for branch).
Attachment #226837 - Flags: approval1.8.1?
Comment on attachment 226837 [details] [diff] [review] fixes simon's comments You are cleared to land on runway 181, taxi left and contact apron control on 171.9. G'day.
Attachment #226837 - Flags: approval1.8.1? → approval1.8.1+
Keywords: fixed1.8.1
Whiteboard: [swag: .5d] 181b1+ → 181b1+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: