Remove support for globalStorage

RESOLVED FIXED in mozilla13

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mayhemer, Assigned: Matheus Kerschbaum)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete})

Trunk
mozilla13
addon-compat, dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
This is currently unspecified feature.  I plan to do optimization of our DOM storage code.  Removing globalStorage, that force us to keep compatibility suboptimal code, makes it easier.

Updated

6 years ago
Depends on: 688190

Updated

6 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

6 years ago
Assignee: nobody → matjk7
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 565866 [details] [diff] [review]
part 1: Remove globalStorage implementation
Attachment #565866 - Flags: review?(jst)
(Assignee)

Comment 2

6 years ago
Created attachment 565867 [details] [diff] [review]
part 2: Nuke globalStorage tests
Attachment #565867 - Flags: review?(jst)
We should probably land this for 11.
Keywords: addon-compat
I'm in favor of doing this, but cc:ing some more people who might have thoughts on timing of this etc.
(Reporter)

Comment 5

6 years ago
Comment on attachment 565866 [details] [diff] [review]
part 1: Remove globalStorage implementation

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

::: dom/src/storage/nsDOMStorage.cpp
@@ -2103,5 @@
> -      *aResult = newstorage->InitAsGlobalStorage(usedDomain);
> -      if (NS_FAILED(*aResult)) {
> -        mStorages.Remove(usedDomain);
> -        return nsnull;
> -      }

You cannot return a storage w/o initiation.  nsDOMStorageList class should be completely removed since it is (was) only returned by window.globalStorage attribute.
(Reporter)

Updated

6 years ago
Blocks: 495337
(Reporter)

Comment 6

6 years ago
Comment on attachment 565866 [details] [diff] [review]
part 1: Remove globalStorage implementation

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

And also "dom-storage-changed" observer topic invocation (nsDOMStorage::BroadcastChangeNotification) and observing (nsGlobalWindow::Observe) should be removed.
I do think we should do this. But I suspect that we'll need to do one release which warns about usage first. So we should either get such a warning into aurora (I think we've done that in the past), or land such a warning now and do removal after next aurora branch.

Comment 8

6 years ago
(In reply to Jonas Sicking (:sicking) from comment #7)
> I do think we should do this. But I suspect that we'll need to do one
> release which warns about usage first.
I'd say at least one release.

> So we should either get such a
> warning into aurora (I think we've done that in the past), or land such a
> warning now and do removal after next aurora branch.

Could we get the warning to aurora now, and remove the feature from FF 11 ?
I added a warning for 9; see dep.
Comment on attachment 565866 [details] [diff] [review]
part 1: Remove globalStorage implementation

Sorry for taking so long to get to this :(

This change looks all good. It might not apply cleanly any more, but fixing that up should be pretty easy. Now would be a good time to land this as we're still pretty early in a release cycle.
Attachment #565866 - Flags: review?(jst) → review+

Updated

6 years ago
Attachment #565867 - Flags: review?(jst) → review+
(Reporter)

Comment 11

6 years ago
Any updates here?  

Removing this stuff would simplify a lot of code and patches.  Also we should do this as an evangelism change soon.
The patches are reviewed, and Johnny suggested it land a couple weeks ago. What sort of updates are you looking for?
(Reporter)

Comment 13

6 years ago
(In reply to Josh Matthews [:jdm] from comment #12)
> The patches are reviewed, and Johnny suggested it land a couple weeks ago.
> What sort of updates are you looking for?

To land it then :)

Comment 14

6 years ago
Try run for 0aca0bcc3f07 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0aca0bcc3f07
Results (out of 49 total builds):
    success: 42
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-0aca0bcc3f07
 Timed out after 06 hours without completing.
http://hg.mozilla.org/integration/mozilla-inbound/rev/8cdce56098cd
http://hg.mozilla.org/integration/mozilla-inbound/rev/7c62688f88fa
Do we really want to land this so late in a cycle?
(Reporter)

Comment 17

6 years ago
While looking at the patches, there is more to be removed - global window and dom storage still have a code to handle "dom-storage-changed", or what is the name, event indicating change in globalStorage.  We can freely remove it too.

I'll have a followup and do the clean up after this bug settles down.
(Reporter)

Updated

6 years ago
Blocks: 732708
https://hg.mozilla.org/mozilla-central/rev/8cdce56098cd
https://hg.mozilla.org/mozilla-central/rev/7c62688f88fa
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Updated

6 years ago
Depends on: 736731
Depends on: 734125

Updated

6 years ago
Depends on: 739976

Updated

5 years ago
Depends on: 745516
Someone already took care of this.
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 762823
(Reporter)

Updated

5 years ago
Depends on: 764546

Updated

5 years ago
Depends on: 765494
You need to log in before you can comment on or make changes to this bug.