328162 - Evaluate Storage Options for Session Data

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[Dietrich Ayala (:dietrich)]

Need to determine pros/cons of using the current text format vs. XML, SQLite or RDF.

Comment 1

[Dietrich Ayala (:dietrich)]

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)

Comment 2

[Nickolay_Ponomarev]

> 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.

Comment 3

[Dietrich Ayala (:dietrich)]

>> 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.

Comment 4

[Nickolay_Ponomarev]

(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.

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: 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.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

re-targetted at beta 1

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: implements toSource/evalInSandbox storage

Comment 8

[Simon Bünzli]

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?

Comment 9

[Dietrich Ayala (:dietrich)]

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

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: fixes simon's comments

Comment 11

[Dietrich Ayala (:dietrich)]

Checked in on trunk.

Comment 12

[Dietrich Ayala (:dietrich)]

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).

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

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.