Remove XUL style overlays

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bdahl, Assigned: bdahl)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Comment 1

a year ago
Thanks for the heads-up, Richard will handle it.
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Assignee: nobody → bdahl
Assignee

Comment 3

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

Comment 4

a year ago
mozreview-review
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)
Attachment #8964448 - Flags: review?(nika) → review-
Assignee

Comment 5

a year ago
mozreview-review-reply
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().
(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.
Flags: needinfo?(nika)

Updated

a year ago
Priority: -- → P5
Assignee

Updated

a year ago
Blocks: 1449791

Comment 7

a year ago
mozreview-review
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 :-)
Attachment #8964448 - Flags: review- → review+
Flags: needinfo?(nika)
Comment hidden (mozreview-request)

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0673137d307f
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.