Closed Bug 732708 Opened 8 years ago Closed 7 years ago

Remove globalStorage artifacts

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: mayhemer, Assigned: Ms2ger)

References

Details

Attachments

(6 files)

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.
Duplicate of this bug: 739976
(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/
Attachment #610156 - Flags: feedback+
Comment on attachment 610156 [details] [diff] [review]
patch to remove nsIDOMStorageList

Try run looks good:

https://tbpl.mozilla.org/?tree=Try&rev=2777fd15934d
Attachment #610156 - Flags: review?(honzab.moz)
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.
Attachment #610156 - Flags: review?(honzab.moz) → review+
(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 ;)
Assignee: honzab.moz → Ms2ger
(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
Whiteboard: [leave open]
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.
Attachment #611771 - Flags: review?(honzab.moz) → review+
Comment on attachment 611772 [details] [diff] [review]
Part c: Remove dom-storage-changed observer topic

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

r=honzab
Attachment #611772 - Flags: review?(honzab.moz) → review+
Attachment #617252 - Flags: review?(honzab.moz)
Comment on attachment 617250 [details] [diff] [review]
Part d: StorageEventObsolete, NS_NewDOMStorage

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

r=honzab
Attachment #617250 - Flags: review?(honzab.moz) → review+
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.
Attachment #617251 - Flags: review?(honzab.moz) → review+
Comment on attachment 617252 [details] [diff] [review]
Part f: nsStorageSH

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

r=honzab
Attachment #617252 - Flags: review?(honzab.moz) → review+
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.
If there's still stuff left, we can file new bugs.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.