Evaluate Storage Options for Session Data

RESOLVED FIXED in Firefox 2 beta1

Status

()

Firefox
Tabbed Browser
--
enhancement
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: dietrich, Assigned: dietrich)

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 2 beta1
fixed1.8.1
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: 181b1+)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

12 years ago
Need to determine pros/cons of using the current text format vs. XML, SQLite or RDF.
(Assignee)

Updated

12 years ago
Blocks: 328154

Updated

12 years ago
Assignee: nobody → dietrich

Updated

12 years ago
Flags: blocking-firefox2+

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

12 years ago
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

Comment 2

12 years ago
> 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.
(Assignee)

Comment 3

12 years ago
>> 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

Comment 4

12 years ago
(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.
(Assignee)

Comment 5

12 years ago
Created attachment 218920 [details] [diff] [review]
using toSource/eval for storing the data; patches the patch in 328159

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
(Assignee)

Updated

12 years ago
Whiteboard: 1d → [swag: .5d]
(Assignee)

Comment 7

12 years ago
Created attachment 226766 [details] [diff] [review]
implements toSource/evalInSandbox storage
Attachment #218920 - Attachment is obsolete: true
Attachment #226766 - Flags: review?(mconnor)

Comment 8

12 years ago
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?
(Assignee)

Comment 9

12 years ago
(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)
(Assignee)

Updated

12 years ago
Attachment #226766 - Flags: review?(mconnor)
(Assignee)

Comment 10

12 years ago
Created attachment 226837 [details] [diff] [review]
fixes simon's comments
Attachment #226766 - Attachment is obsolete: true
Attachment #226837 - Flags: review?(mconnor)

Updated

12 years ago
Attachment #226837 - Flags: review?(mconnor) → review+
(Assignee)

Comment 11

12 years ago
Checked in on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Whiteboard: [swag: .5d] → [swag: .5d] 181b1+
(Assignee)

Comment 12

12 years ago
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+
(Assignee)

Updated

12 years ago
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.