687579 - Remove support for globalStorage

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Honza Bambas (:mayhemer)]

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.

Comment 1

[Matheus Kerschbaum]

Attachment: part 1: Remove globalStorage implementation

Comment 2

[Matheus Kerschbaum]

Attachment: part 2: Nuke globalStorage tests

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We should probably land this for 11.

Comment 4

[Johnny Stenback (:jst)]

I'm in favor of doing this, but cc:ing some more people who might have thoughts on timing of this etc.

Comment 5

[Honza Bambas (:mayhemer)]

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.

Comment 6

[Honza Bambas (:mayhemer)]

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.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I added a warning for 9; see dep.

Comment 10

[Johnny Stenback (:jst)]

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.

Comment 11

[Honza Bambas (:mayhemer)]

Any updates here?  

Removing this stuff would simplify a lot of code and patches.  Also we should do this as an evangelism change soon.

Comment 12

[Josh Matthews [:jdm]]

The patches are reviewed, and Johnny suggested it land a couple weeks ago. What sort of updates are you looking for?

Comment 13

[Honza Bambas (:mayhemer)]

(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

[Mozilla RelEng Bot]

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/[e-mail]-0aca0bcc3f07
 Timed out after 06 hours without completing.

Comment 15

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8cdce56098cd
http://hg.mozilla.org/integration/mozilla-inbound/rev/7c62688f88fa

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Do we really want to land this so late in a cycle?

Comment 17

[Honza Bambas (:mayhemer)]

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.

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8cdce56098cd
https://hg.mozilla.org/mozilla-central/rev/7c62688f88fa

Comment 19

[Eric Shepherd [:sheppy]]

Someone already took care of this.