Closed Bug 1571530 Opened 1 year ago Closed 4 months ago

Round of CSS loader cleanup.

Categories

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

task

Tracking

()

RESOLVED FIXED

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(18 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
No description provided.

I'm going to tweak it a bit.

Little pet peeve o' mine.

Depends on D40687

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

Depends on D40689

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

Depends on D40690

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

Depends on D40691

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

Depends on D40692

Type: defect → task

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

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

It's a refptr hashtable, really.

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.

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...

Keywords: leave-open
Priority: -- → P3
Pushed by emilio@crisal.io:
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
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fbe01c0e461
Move Loader::Sheets out of line. r=heycam
Attachment #9083104 - Attachment is obsolete: true
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76b29d6f9e86
Cleanup initialization and usage of Loader::mSyncCallback. r=heycam
Attachment #9083105 - Attachment description: Bug 1571530 - Move SheetLoadData out of line too. r=heycam → Bug 1571530 - Move SheetLoadData out of line too.
Attachment #9083106 - Attachment description: Bug 1571530 - Make the parsing mode part of the cache key. r=heycam → Bug 1571530 - Make the parsing mode part of the cache key.
Pushed by ealvarez@mozilla.com:
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
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4727bbeb8b85
Trivially cleanup a condition. r=heycam
Attachment #9083107 - Attachment description: Bug 1571530 - Factor out the stylesheet cache bits. r=heycam → Bug 1571530 - Factor out the stylesheet cache bits.
Attachment #9083108 - Attachment description: Bug 1571530 - Cleanup CreateSheet so that it returns its arguments rather than having outparams. r=heycam → Bug 1571530 - Cleanup CreateSheet so that it returns its arguments rather than having outparams.
Regressions: 1573639

Thanks for this cleanup!

Pushed by ealvarez@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.