Bug 687579 removes support for global storage but there are still some artifacts: - nsDOMStorage contains code to post storage event for globalstorage - nsGlobalWindow contains code to handle it - nsDOMStorage has code to switch security checkers for globalStorage - nsDOMStorage class is actually no longer needed as well as nsIDOMStorageObsolete interface I think we can make the code a half a size and complexity now!

Josh, just let you know there is tun of code we may remove. If you have time, feel free to take this instead of me.

(Also NS_NewDOMStorageList / nsDOMStorageList / nsStorageListSH / nsIDOMStorageItem)

Here's a patch for the nsIDOMStorageList stuff. Compiles, at least.

Comment on attachment 610156 [details] [diff] [review] patch to remove nsIDOMStorageList \o/

Comment on attachment 610156 [details] [diff] [review] patch to remove nsIDOMStorageList Try run looks good: https://tbpl.mozilla.org/?tree=Try&rev=2777fd15934d

Comment on attachment 610156 [details] [diff] [review] patch to remove nsIDOMStorageList Review of attachment 610156 [details] [diff] [review]: ----------------------------------------------------------------- Looks like it's all. Thanks. r=honzab.

(In reply to Honza Bambas (:mayhemer) from comment #7) > Looks like it's all. All to remove the list, not to fix this bug! Add [leave open] to whiteboard after push to inbound please.

Comment on attachment 610156 [details] [diff] [review] patch to remove nsIDOMStorageList Actually, I don't see the patch would be removing nsIDOMStorageList interface it self, as claimed by the patch description. Up to you to remove it or not, no problem to leave it in for now, IMO. I have to remove the remnants anyway and there will be much more to do.

Next time please say loud you work on something that is assigned to someone else ;)

(In reply to Honza Bambas (:mayhemer) from comment #9) > Actually, I don't see the patch would be removing nsIDOMStorageList > interface it self, as claimed by the patch description. Whoops! Fixed in the actual push, thanks. https://hg.mozilla.org/integration/mozilla-inbound/rev/17696f172c01

Comment on attachment 611771 [details] [diff] [review] Part b: Devirtualize BroadcastChangeNotification and remove a dead implementation Review of attachment 611771 [details] [diff] [review]: ----------------------------------------------------------------- Another incremental patch... I want to merge nsDOMStorage and nsDOMStorage2 classes to only a one. Then no broadcaster will be needed. But whatever.. r=honzab, thanks.

Comment on attachment 611772 [details] [diff] [review] Part c: Remove dom-storage-changed observer topic Review of attachment 611772 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab

https://hg.mozilla.org/mozilla-central/rev/a188746249cd https://hg.mozilla.org/mozilla-central/rev/0d871550085e

Comment on attachment 617250 [details] [diff] [review] Part d: StorageEventObsolete, NS_NewDOMStorage Review of attachment 617250 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab

Comment on attachment 617251 [details] [diff] [review] Part e: Pass nsDOMStorage* to InitAsSessionStorageFork Review of attachment 617251 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with the comment addressed ::: dom/src/storage/nsDOMStorage.cpp @@ +1795,5 @@ > already_AddRefed<nsIDOMStorage> > nsDOMStorage2::Fork(const nsSubstring &aDocumentURI) > { > nsRefPtr<nsDOMStorage2> storage = new nsDOMStorage2(); > + storage->InitAsSessionStorageFork(mPrincipal, aDocumentURI, mStorage); Leave the check for result and return of null on it please.

Comment on attachment 617252 [details] [diff] [review] Part f: nsStorageSH Review of attachment 617252 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab

Thanks! There is still more to do, though. I'd like to merge nsDOMStorage2 and nsDOMStorage together if possible (only forking might be tricky). Then I'd like to reorganize the code by separating classes to own files, it's hard to track what is where. This all however has to wait for bugs known to me doing larger changes in this area of code: Bug 536509, Bug 744466, Bug 722857.

Comment on attachment 617251 [details] [diff] [review] Part e: Pass nsDOMStorage* to InitAsSessionStorageFork Review of attachment 617251 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/storage/nsDOMStorage.cpp @@ +1795,5 @@ > already_AddRefed<nsIDOMStorage> > nsDOMStorage2::Fork(const nsSubstring &aDocumentURI) > { > nsRefPtr<nsDOMStorage2> storage = new nsDOMStorage2(); > + storage->InitAsSessionStorageFork(mPrincipal, aDocumentURI, mStorage); Why? new never returns null.

(In reply to Ms2ger from comment #25) > Comment on attachment 617251 [details] [diff] [review] > Part e: Pass nsDOMStorage* to InitAsSessionStorageFork > > Review of attachment 617251 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/src/storage/nsDOMStorage.cpp > @@ +1795,5 @@ > > already_AddRefed<nsIDOMStorage> > > nsDOMStorage2::Fork(const nsSubstring &aDocumentURI) > > { > > nsRefPtr<nsDOMStorage2> storage = new nsDOMStorage2(); > > + storage->InitAsSessionStorageFork(mPrincipal, aDocumentURI, mStorage); > > Why? new never returns null. I mean: rv = storage->InitAs...(); if (NS_FAILED(rv)) return nsnull;

(In reply to Honza Bambas (:mayhemer) from comment #26) > (In reply to Ms2ger from comment #25) > > Comment on attachment 617251 [details] [diff] [review] > > Part e: Pass nsDOMStorage* to InitAsSessionStorageFork > > > > Review of attachment 617251 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/src/storage/nsDOMStorage.cpp > > @@ +1795,5 @@ > > > already_AddRefed<nsIDOMStorage> > > > nsDOMStorage2::Fork(const nsSubstring &aDocumentURI) > > > { > > > nsRefPtr<nsDOMStorage2> storage = new nsDOMStorage2(); > > > + storage->InitAsSessionStorageFork(mPrincipal, aDocumentURI, mStorage); > > > > Why? new never returns null. > > I mean: > > rv = storage->InitAs...(); > if (NS_FAILED(rv)) return nsnull; Well, I did make that function return void in that patch.

(In reply to Ms2ger from comment #27) > Well, I did make that function return void in that patch. Ah, yes. Sorry, that is OK. I mistaken it for other method that needs to be checked for rv. The patch is then good to go.

https://hg.mozilla.org/mozilla-central/rev/5d5454d0fe2e https://hg.mozilla.org/mozilla-central/rev/a2e3e92acba7 https://hg.mozilla.org/mozilla-central/rev/1f825a03864a

If there's still stuff left, we can file new bugs.