1450753 - Remove XUL style overlays

Status: RESOLVED FIXED

(Core :: XUL, enhancement, P5)

Description

[Brendan Dahl [:bdahl]]

As far as I can tell these are only used in one[1] test in mozilla central.



[1] https://searchfox.org/mozilla-central/rev/b80994a43e5d92c2f79160ece176127eed85dcc9/chrome/test/unit/data/test_bug564667/chrome.manifest#16

Comment 1

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up, Richard will handle it.

Comment 2

[Brendan Dahl [:bdahl]]

Attachment: Bug 1450753 - Remove XUL style overlays.

Style overlays are no longer used outside of tests.

Review commit: https://reviewboard.mozilla.org/r/233178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233178/

Comment 3

[Brendan Dahl [:bdahl]]

Ideally I'd like to land bug 1448162 first, as some of this patch built on top of that.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8964448 [details]
Bug 1450753 - Remove XUL style overlays.

https://reviewboard.mozilla.org/r/233178/#review239008

r- only because I'm worried about the nsISerializable implementation changes.

::: dom/xul/nsXULPrototypeDocument.cpp
(Diff revision 1)
> -    uint32_t count, i;
> -    nsCOMPtr<nsIURI> styleOverlayURI;
> -
> -    rv = aStream->Read32(&count);
> -    if (NS_FAILED(rv)) {
> -      return rv;
> -    }
> -
> -    for (i = 0; i < count; ++i) {
> -        rv = aStream->ReadObject(true, getter_AddRefs(supports));
> -        if (NS_FAILED(rv)) {
> -          return rv;
> -        }
> -        styleOverlayURI = do_QueryInterface(supports);
> -        mStyleSheetReferences.AppendObject(styleOverlayURI);
> -    }
> -
> -

I'm mildly worried about this particular change, as we occasionally persist nsISerializable objects to disk, and read them from disk.

I don't know if these objects are ever written to disk, but I think I would be more comfortable if we continue reading/writing the count uint32_t, and report an error if we read a non-zero value.

I'd also like to add a comment there mentioning why the value is there for legacy purposes.

::: dom/xul/nsXULPrototypeDocument.cpp
(Diff revision 1)
> -    uint32_t count;
> -
> -    count = mStyleSheetReferences.Count();
> -    nsresult tmp = aStream->Write32(count);
> -    if (NS_FAILED(tmp)) {
> -      rv = tmp;
> -    }
> -
> -    uint32_t i;
> -    for (i = 0; i < count; ++i) {
> -        tmp = aStream->WriteCompoundObject(mStyleSheetReferences[i],
> -                                           NS_GET_IID(nsIURI), true);
> -        if (NS_FAILED(tmp)) {
> -          rv = tmp;
> -        }
> -    }

(this change too for the same reasons as above)

Comment 5

[Brendan Dahl [:bdahl]]

Comment on attachment 8964448 [details]
Bug 1450753 - Remove XUL style overlays.

https://reviewboard.mozilla.org/r/233178/#review239008

> I'm mildly worried about this particular change, as we occasionally persist nsISerializable objects to disk, and read them from disk.
> 
> I don't know if these objects are ever written to disk, but I think I would be more comfortable if we continue reading/writing the count uint32_t, and report an error if we read a non-zero value.
> 
> I'd also like to add a comment there mentioning why the value is there for legacy purposes.

I may be missing something as this is my first run in with this caching code, but it look like these are only every written to the startup cache. If I follow everything it looks like we wipe out this cache on any update/version change:


Starting over at https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/toolkit/xre/nsAppRunner.cpp#4304
 - first we check version
 - we should get either cachesOK=false or versionOK=false which then sets startupCacheValid=false
 - calls StartupCache::IgnoreDiskCache(). That either directly wipes the cache or later deletes it in StartupCache::Init().

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Brendan Dahl [:bdahl] from comment #5)
> I may be missing something as this is my first run in with this caching
> code, but it look like these are only every written to the startup cache. If
> I follow everything it looks like we wipe out this cache on any
> update/version change:
> 
> 
> Starting over at
> https://searchfox.org/mozilla-central/rev/
> a0665934fa05158a5a943d4c8b277465910c029c/toolkit/xre/nsAppRunner.cpp#4304
>  - first we check version
>  - we should get either cachesOK=false or versionOK=false which then sets
> startupCacheValid=false
>  - calls StartupCache::IgnoreDiskCache(). That either directly wipes the
> cache or later deletes it in StartupCache::Init().

Had a conversation with olli on IRC about this (https://mozilla.logbot.info/content/20180404#c14559961). I want to read through the relevant code to convince myself that this is OK before we land it, which I probably won't get around to today.

Opening a ni? on myself here so I don't forget to do it.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8964448 [details]
Bug 1450753 - Remove XUL style overlays.

https://reviewboard.mozilla.org/r/233178/#review254490

I've convinced myself this is OK. Thanks :-)

Comment 8

[Brendan Dahl [:bdahl]]

Comment on attachment 8964448 [details]
Bug 1450753 - Remove XUL style overlays.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233178/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0673137d307f
Remove XUL style overlays. r=Nika

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/0673137d307f