Last Comment Bug 687579 - Remove support for globalStorage
: Remove support for globalStorage
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla13
Assigned To: Matheus Kerschbaum
:
Mentors:
Depends on: 688190 734125 736731 739976 745516 764546 765494
Blocks: 762823 495337 687566 732708
  Show dependency treegraph
 
Reported: 2011-09-19 12:08 PDT by Honza Bambas (:mayhemer)
Modified: 2012-06-16 09:07 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: Remove globalStorage implementation (21.71 KB, patch)
2011-10-09 22:14 PDT, Matheus Kerschbaum
jst: review+
Details | Diff | Splinter Review
part 2: Nuke globalStorage tests (37.36 KB, patch)
2011-10-09 22:15 PDT, Matheus Kerschbaum
jst: review+
Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2011-09-19 12:08:34 PDT
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 2011-10-09 22:14:42 PDT
Created attachment 565866 [details] [diff] [review]
part 1: Remove globalStorage implementation
Comment 2 Matheus Kerschbaum 2011-10-09 22:15:28 PDT
Created attachment 565867 [details] [diff] [review]
part 2: Nuke globalStorage tests
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-10-10 04:55:32 PDT
We should probably land this for 11.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2011-10-10 11:54:23 PDT
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) 2011-10-10 12:17:40 PDT
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) 2011-10-10 12:23:03 PDT
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 2011-10-10 12:45:27 PDT
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] (vacation Aug 25-28) 2011-10-10 12:51:40 PDT
(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 (⌚ UTC+1/+2) 2011-10-10 13:13:25 PDT
I added a warning for 9; see dep.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-07 12:24:08 PST
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) 2012-02-22 03:40:20 PST
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] 2012-02-22 06:20:52 PST
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) 2012-02-22 06:35:57 PST
(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 2012-03-03 00:46:50 PST
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.
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 05:46:09 PST
Do we really want to land this so late in a cycle?
Comment 17 Honza Bambas (:mayhemer) 2012-03-03 08:13:48 PST
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 19 Eric Shepherd [:sheppy] 2012-04-29 10:35:37 PDT
Someone already took care of this.

Note You need to log in before you can comment on or make changes to this bug.