Figure out whether we should prefer storing the stringified versions of WebIDL enums in cache

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla39
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

4 years ago
Currently we have a few cases where we store integers corresponding to the internal value of WebIDL enums in the cache database.  I realized this morning that this could be problematic, since now we're effectively relying on these numbers to never change, but adding new values in the WebIDL for example may change them, and this dependency is quite non-obvious.

I suggest going back to storing the string versions of these enum values in the database.  That's still obviously relying on the values to not be removed from the WebIDL, but such a change is probably not very likely since we don't really remove Web visible stuff once we've added them.

Ben, what do you think?
Flags: needinfo?(bkelly)
It seems like these enums should not be changing very often.  Can we just static_assert the current values with a comment that says the DB schema must be migrated if they change?
Flags: needinfo?(bkelly)
Assignee

Comment 2

4 years ago
Sure, that would prevent against someone accidentally changing the IDL and breaking everyone's caches!  I'll take this.
Assignee: nobody → ehsan
Assignee

Comment 3

4 years ago
These assertions will catch future accidental changes to these enums
which will invalidate the data that we store in the Cache database.
Attachment #8582505 - Flags: review?(bkelly)
Comment on attachment 8582505 [details] [diff] [review]
Add static_asserts that check the validity of the enum values that we write into the cache database

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

I was thinking if the enums do change, it may be easier just to write a webidl-to-schema map function instead of doing a migration.  This is good for now, though.

Thanks!
Attachment #8582505 - Flags: review?(bkelly) → review+
https://hg.mozilla.org/mozilla-central/rev/6773b2f30d90
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.