Closed
Bug 455070
Opened 16 years ago
Closed 16 years ago
Make sessionStorage object conform the WHATWG spec
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9.1
People
(Reporter: mayhemer, Assigned: mayhemer)
References
()
Details
(Keywords: fixed1.9.1, html5)
Attachments
(2 files, 8 obsolete files)
71.80 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
61.50 KB,
patch
|
Details | Diff | Splinter Review |
that is:
- change the getItem method to return DOMString (use nsIDOMStorage2 interface for the storage)
- change mapping of session storage by an origin and not by a domain as it is now
- compare principals instead of just string origins on some places
This bug will probably invalidate bug 394351, if it were not already invalid.
Assignee | ||
Comment 1•16 years ago
|
||
Origin is extracted using script security manager and its method GetOriginForCodebase and subsequent call of GetOrigin on the resulting principal that returns 'file://' string from file: URIs. It is probably not a proper solution for bug 357323, I guess.
This patch is dependent on localStorage implementation (bug 422526).
Attachment #338670 -
Flags: review?(dcamp)
Assignee | ||
Comment 2•16 years ago
|
||
err, GetOriginForCodebase --> GetCodebasePrincipal.
Assignee | ||
Updated•16 years ago
|
Summary: Make sessionStorage object conform the WHATWG spec → Make sessionStorage and localStrorage conform the WHATWG spec
Summary: Make sessionStorage and localStrorage conform the WHATWG spec → Make sessionStorage and localStorage conform the WHATWG spec
Assignee | ||
Comment 3•16 years ago
|
||
- using new WHATWG-compliant interface nsIDOMStorage2
- mapping session storage by an origin
- turning 'todo's in localStorage tests to regular tests + other cosmetic fixes
- added new test for localStorage mapping distinguished by schema
- dom_quickstubs.qsconf updated to work JUST with the new nsIDOMStorage2 interface, needed because js engine/xpconnect are unable to figure out which interface to delegate to when there are methods with identical name in a single DOM class even JS wrapped native is correctly QIed to interface I want; there is not any quick stubs optimization for old nsIDOMStorage interface (globalStorage)
Attachment #338670 -
Attachment is obsolete: true
Attachment #339782 -
Flags: review?(dcamp)
Attachment #338670 -
Flags: review?(dcamp)
Assignee | ||
Comment 4•16 years ago
|
||
Comment on attachment 339782 [details] [diff] [review]
fix v2
Forget to mention: I change nsIDocShell interface because:
1. I need to pass whole URI when adding new session storage to extract the origin
2. I believe these two methods, I change, have to be in an "internal" or a "private" non-scriptable interface; when some extension developer play with sessionStorage objects using these two methods he IMHO does something wrong or malignant.
Comment 5•16 years ago
|
||
nsIDocShell is anything but private and nonscriptable. If you don't want these methods scriptable, please mark them so.
Are these various domstorage specs really stable enough that we want to put the interfaces in the SDK (which implies frozen interface)?
Assignee | ||
Updated•16 years ago
|
Attachment #339782 -
Attachment is obsolete: true
Attachment #339782 -
Flags: review?(dcamp)
Assignee | ||
Updated•16 years ago
|
Summary: Make sessionStorage and localStorage conform the WHATWG spec → Make sessionStorage object conform the WHATWG spec
Assignee | ||
Comment 6•16 years ago
|
||
Up-to-date patch, just session storage patch.
Attachment #339854 -
Flags: review?(dcamp)
Assignee | ||
Updated•16 years ago
|
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Comment 8•16 years ago
|
||
Do we need this for 1.9.1?
Comment 9•16 years ago
|
||
Minusing until we know whether this is a must have for 1.9.1.
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Assignee | ||
Comment 10•16 years ago
|
||
> Do we need this for 1.9.1?
No, this is dependent on (and actually complementary to) localStorage
implementation which unfortunately missed the milestone.
Assignee | ||
Comment 11•16 years ago
|
||
Merged against the new localStorage patch. Prepared for 1.9.2. All tests has been run and passed.
Attachment #339854 -
Attachment is obsolete: true
Attachment #351223 -
Flags: review?(dcamp)
Attachment #339854 -
Flags: review?(dcamp)
Assignee | ||
Comment 12•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review dcamp]
Assignee | ||
Comment 13•16 years ago
|
||
Deps on the new localStorage patch. I remove the nsIDocShell_1_9_1 interface because this patch will not land on 1.9.1 but on 1.9.2 (hopefully). I changed sessionStorage management to work with principals beside only URIs.
Attachment #351223 -
Attachment is obsolete: true
Attachment #351224 -
Attachment is obsolete: true
Attachment #357719 -
Flags: review?(dcamp)
Attachment #351223 -
Flags: review?(dcamp)
Would be awesome to make this available to workers too
Comment 15•16 years ago
|
||
That would make workers shared-something, tho, wouldn't it? What is the result of simultaneous changes in a worker and in a webpage? Doesn't this break the run-to-completion assumption that the only observable changes (excluding Date changes) are those caused by the executing script itself? (I assume the navigator object added in a worker is wholly distinct from the navigator object in any other web page or worker, but that might be wrong.)
Hmm.. interesting point that it's shared-something. Implementation wise this should be ok, but it does mean that we're relying on scripts not to have races. I'll bring this up with W3C/WHATWG.
Assignee | ||
Comment 17•16 years ago
|
||
What exactly means "shared-something"? Workers already see the localStorage through the WorkerUtils interface (actually through the WorkerGlobalScope). The object (localStorage and same would be sessionStorage) is bound to the origin of the script that invoked a method of the worker (the one that posted a message to that worker, as I understand). It is doable to make it available, imo.
Assignee | ||
Comment 18•16 years ago
|
||
Requesting again b1.9.1 because we should preserve Storage as sessionStorage's constructor. See bug 492219.
Flags: blocking1.9.1- → blocking1.9.1?
Assignee | ||
Updated•16 years ago
|
Attachment #357719 -
Flags: review?(dcamp) → review?(jst)
Comment 19•16 years ago
|
||
Our implementation of workers don't expose any storage objects to worker scripts. I don't understand why we would need to block 1.9.1 on this bug.
Assignee | ||
Comment 20•16 years ago
|
||
Patch merged against recent trunk and built upon patches for bug 492219 and bug 487695.
Attachment #357719 -
Attachment is obsolete: true
Attachment #377849 -
Flags: review?(jst)
Attachment #357719 -
Flags: review?(jst)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review dcamp] → [needs review jst]
Comment 21•16 years ago
|
||
I agree that we should block on this now that I understand more about it. We never shipped anything with sessionStorage support, so the API change isn't as big of a deal, and shipping this now would mean we'd have a *really* hard time changing our API to be conformant with the spec later on. Blocking.
Honza, the last attachment is good to go, right? For the branch, we'll need some new interfaces as we can't change interfaces there any more, but other than that it looks good. I wish it was a smaller patch, but I guess there's not much we can do about that as most of the changes look necessary. Let me know if there's anything we could avoid doing for 1.9.1, just to keep the size down given how close to release we are.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Assignee | ||
Comment 22•16 years ago
|
||
This is patch as it should be landed on the trunk. It's identical to "v2 (merged)" just updated iid changes from bug 492219.
I will create patch for 1.9.1 with new _1_9_1 interfaces where I make idl changes.
Attachment #377849 -
Attachment is obsolete: true
Attachment #378343 -
Flags: review?(jst)
Attachment #377849 -
Flags: review?(jst)
Comment 23•16 years ago
|
||
Comment on attachment 378343 [details] [diff] [review]
v2 (merged iid changes) [Checkin comment 34]
Looks good. r+sr=jst
Attachment #378343 -
Flags: superreview+
Attachment #378343 -
Flags: review?(jst)
Attachment #378343 -
Flags: review+
Updated•16 years ago
|
Whiteboard: [needs review jst] → [can land]
Comment 24•16 years ago
|
||
Honza: ETA on the branchsafe patch?
Whiteboard: [can land] → [needs 1.9.1 branchsafe patch]
Assignee | ||
Comment 25•16 years ago
|
||
(In reply to comment #24)
> Honza: ETA on the branchsafe patch?
Should be today.
Assignee | ||
Comment 26•16 years ago
|
||
Comment on attachment 378343 [details] [diff] [review]
v2 (merged iid changes) [Checkin comment 34]
http://hg.mozilla.org/mozilla-central/rev/ef25b4175b78
Attachment #378343 -
Attachment description: v2 (merged iid changes) → v2 (merged iid changes) [Checkin comment 24]
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•16 years ago
|
||
Build failure, reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 28•16 years ago
|
||
From the build log this looks like some bad dependencies with quickstubs. Perhaps you could touch base with jorendorff to see if this is something we can fix without having to clobber the world?
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242808071.1242808274.29083.gz
Assignee | ||
Updated•16 years ago
|
Attachment #378343 -
Attachment description: v2 (merged iid changes) [Checkin comment 24] → v2 (merged iid changes) [Backed out]
Assignee | ||
Comment 29•16 years ago
|
||
I'm adding new _1_9_1 interfaces for docshell that works as a sessionStorage manager and for dom storage window (implemented by nsGlobalWindow) to change sessionStorage attribute.
This file modifies dom_quickstubs.qsconf so there should be no problem after landing.
Attachment #378577 -
Flags: superreview?(bzbarsky)
Comment 30•16 years ago
|
||
I landed a fix for the dependency problem: http://hg.mozilla.org/mozilla-central/rev/ac136abf6fd3
Comment 31•16 years ago
|
||
Comment on attachment 378577 [details] [diff] [review]
v2 for 1.9.1 [Checkin comment 38]
jst's probably the right reviewer here, since he's familiar with the patch and all.
Attachment #378577 -
Flags: superreview?(bzbarsky) → superreview?(jst)
Comment 32•16 years ago
|
||
Comment on attachment 378577 [details] [diff] [review]
v2 for 1.9.1 [Checkin comment 38]
- In nsDocShell::GetSessionStorageForPrincipal():
+ nsDocShell* topDocShell = static_cast<nsDocShell*>(topItem.get());
Should this not QI to nsIDocShell_MOZILLA_1_9_1_SessionStorage rather than casting to nsDocShell?
Looks good with that fixed. r+sr=jst
Attachment #378577 -
Flags: superreview?(jst)
Attachment #378577 -
Flags: superreview+
Attachment #378577 -
Flags: review+
Assignee | ||
Comment 33•16 years ago
|
||
(In reply to comment #32)
> Should this not QI to nsIDocShell_MOZILLA_1_9_1_SessionStorage rather than
> casting to nsDocShell?
It would be better, but C++ function overloading handles this well. I'll fix it before land.
Thanks for review.
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•16 years ago
|
Attachment #378343 -
Attachment description: v2 (merged iid changes) [Backed out] → v2 (merged iid changes) [Checkin comment 34]
Assignee | ||
Comment 34•16 years ago
|
||
Comment on attachment 378343 [details] [diff] [review]
v2 (merged iid changes) [Checkin comment 34]
http://hg.mozilla.org/mozilla-central/rev/b2e48c0aa965
Relanded.
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 branchsafe patch] → [needs 1.9.1 landing]
Assignee | ||
Comment 35•16 years ago
|
||
Backed out again, tests fail on windows.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 36•16 years ago
|
||
I didn't clear the sessionStorage object at the start of the test. There were some data left from some other test. Fixing my test.
Attachment #378343 -
Attachment is obsolete: true
Assignee | ||
Comment 37•16 years ago
|
||
Comment on attachment 378696 [details] [diff] [review]
v2.1 [Checkin comment 37]
http://hg.mozilla.org/mozilla-central/rev/f1c65acaccf3
Attachment #378696 -
Attachment description: v2.1 → v2.1 [Checkin comment 37]
Assignee | ||
Updated•16 years ago
|
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 38•16 years ago
|
||
Comment on attachment 378577 [details] [diff] [review]
v2 for 1.9.1 [Checkin comment 38]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4b8e9d27a9ad
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/564aec224cee
Attachment #378577 -
Attachment description: v2 for 1.9.1 → v2 for 1.9.1 [Checkin comment 38]
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Comment 39•16 years ago
|
||
Had to land:
http://hg.mozilla.org/mozilla-central/rev/865b5be2ce4d
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/027583c55155
to fix orange on linux, it was due to a simple character case problem in a few paths in a test.
Assignee | ||
Comment 40•16 years ago
|
||
(In reply to comment #39)
> to fix orange on linux, it was due to a simple character case problem in a few
> paths in a test.
Thanks! I didn't have a linux machine to test on...
Comment 41•16 years ago
|
||
This caused bug 494453.
Depends on: 494453
Comment 42•16 years ago
|
||
Could this have also caused Bug 494543? One regression range implicates it, still confirming on 1.9.1 -- we're getting an NS_ERROR_DOM_SECURITY_ERR that's breaking the dell.com cart.
Sites that were written to the last beta could see changed behaviour here, right? What about sites that were written to the old localStorage API? Not having beta coverage for this makes me sad.
Comment 43•16 years ago
|
||
Note that a partial backout of this bug was done in bug 495112, so you'll want to follow there. Honza should file a follow up for whatever next-set-of-steps are to get this into shape.
Comment 44•16 years ago
|
||
Followup bug 495337 filed for the parts of this that was backed out.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•