1571530 - Round of CSS loader cleanup.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task, P3)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Move Loader::Sheets out of line. r=heycam

I'm going to tweak it a bit.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Cleanup initialization and usage of Loader::mSyncCallback. r=heycam

Little pet peeve o' mine.

Depends on D40687

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Remove fromCompleteSheets from the Loader caching code. r=heycam

Depends on D40688

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Move SheetLoadData out of line too.

Now that Sheets is out of line this can move to the cpp file too.

Depends on D40689

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Make the parsing mode part of the cache key.

Rather than checking after-the fact and dropping the cached result on the floor.

Depends on D40690

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Factor out the stylesheet cache bits.

Just moving code around but hopefully the new code is nicer :)

Depends on D40691

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Cleanup CreateSheet so that it returns its arguments rather than having outparams.

Also, it's infallible, so no need to have an error state.

Depends on D40692

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Move inline sheet creation somewhere else that isn't CreateSheet.

It doesn't do much for them.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Cleanup a bit PreloadedStyleSheet construction.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Don't uselessly check for OOM in Loader::PostLoadEvent.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Make Loader APIs return a Result.

On the fence on this one, but I do think it's nicer.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Remove various bool arguments in sheet loader APIs.

They're bad, specially if they do vastly different thing in overloaded
functions, like aUseSystemPrincipal and aIsPreload. :)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Don't use nsBaseHashtable in the CSS loader.

It's a refptr hashtable, really.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Trivially cleanup a condition.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Cleanup confusing lifetime of SheetLoadData.

Saving a refcount bump is not worth the churn. Use a proper RefPtr<>
everywhere instead of manual refcounting, and don't make DoSheetComplete call
NS_RELEASE unconditionally.

Also, make clear by using references where null is expected or not.

Also, properly use a RefPtr in mPendingDatas, since they are strong pointers,
in fact.

Finally, remove some unused macros from nsCSSValue of which this code was the
last consumer.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Cleanup slightly SheetComplete, and use the right boolean to notify.

This is mostly straight-forward cleanup, but there's a behavior change which
was an oversight from bug 1386840, the regular mObservers stuff didn't pass the
right thing (was passing only mWasAlternate rather than whether it was
deferred).

I think that as a result, only in XML documents (since nsXMLContentSink is the
only thing that adds itself as a global CSS loader observer that ever looks at
this boolean), we may end up breaking this assert:

https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/dom/base/nsContentSink.cpp#183

(If there are any sheets with non-matching media queries and such).

But I couldn't find a test-case that broke it (tried SVG / XHTML), and in other
documents like pure XML you cannot specify a media query...

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd79f0fbb18e
Cleanup a bit PreloadedStyleSheet construction. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3618e35bce0
Don't uselessly check for OOM in Loader::PostLoadEvent. r=heycam

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/cd79f0fbb18e
https://hg.mozilla.org/mozilla-central/rev/b3618e35bce0

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Make PrepareSheet take a reference.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Move a bit of code around in Loader::LoadInlineSheet.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1571530 - Remove sheets from the <link> cache when they have been touched by CSS OM.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fbe01c0e461
Move Loader::Sheets out of line. r=heycam

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76b29d6f9e86
Cleanup initialization and usage of Loader::mSyncCallback. r=heycam

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e0d1725d65e
Move SheetLoadData out of line too. r=heycam
https://hg.mozilla.org/integration/autoland/rev/bec4c05b5a70
Make the parsing mode part of the cache key. r=heycam

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4727bbeb8b85
Trivially cleanup a condition. r=heycam

Comment 26

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1fbe01c0e461
https://hg.mozilla.org/mozilla-central/rev/76b29d6f9e86
https://hg.mozilla.org/mozilla-central/rev/2e0d1725d65e
https://hg.mozilla.org/mozilla-central/rev/bec4c05b5a70
https://hg.mozilla.org/mozilla-central/rev/4727bbeb8b85

Comment 27

[Cameron McCormack (:heycam)]

Thanks for this cleanup!

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4918a9dc15d7
Factor out the stylesheet cache bits. r=heycam
https://hg.mozilla.org/integration/autoland/rev/eec457de4550
Cleanup CreateSheet so that it returns its arguments rather than having outparams. r=heycam
https://hg.mozilla.org/integration/autoland/rev/cf17f0433abf
Move inline sheet creation somewhere else that isn't CreateSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/db8dfad2e3b3
Make Loader APIs return a Result. r=heycam
https://hg.mozilla.org/integration/autoland/rev/589addc490dc
Remove various bool arguments in sheet loader APIs. r=heycam
https://hg.mozilla.org/integration/autoland/rev/1f4c64e73cfc
Don't use nsBaseHashtable in the CSS loader. r=heycam
https://hg.mozilla.org/integration/autoland/rev/aec66f88bfd0
Cleanup confusing lifetime of SheetLoadData. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a28efee19e9c
Cleanup slightly SheetComplete, and use the right boolean to notify. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3d9de500e9c5
Make PrepareSheet take a reference. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e10941ec60df
Move a bit of code around in Loader::LoadInlineSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/852c5aaa34f7
Remove sheets from the <link> cache when they have been touched by CSS OM. r=heycam

Comment 29

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4918a9dc15d7
https://hg.mozilla.org/mozilla-central/rev/eec457de4550
https://hg.mozilla.org/mozilla-central/rev/cf17f0433abf
https://hg.mozilla.org/mozilla-central/rev/db8dfad2e3b3
https://hg.mozilla.org/mozilla-central/rev/589addc490dc
https://hg.mozilla.org/mozilla-central/rev/1f4c64e73cfc
https://hg.mozilla.org/mozilla-central/rev/aec66f88bfd0
https://hg.mozilla.org/mozilla-central/rev/a28efee19e9c
https://hg.mozilla.org/mozilla-central/rev/3d9de500e9c5
https://hg.mozilla.org/mozilla-central/rev/e10941ec60df
https://hg.mozilla.org/mozilla-central/rev/852c5aaa34f7