Cameron McCormack (:heycam) Reporter
	Description • 8 years ago

Currently we don't pass in correct sheet and base URL information when we parse a style="" attribute.  This causes, for example, a style="background-image: url(...)" with a relative URL not work.

	Xidorn Quan [:xidorn] UTC+11
	Comment 1 • 8 years ago

I'd like to work on this after finishing bug 1294299.

	Xidorn Quan [:xidorn] UTC+11
	Comment 2 • 8 years ago

I pretty much believe that creating a threadsafe holder (actually three threadsafe holders, for sheet uri, base uri, and principal) is unacceptable for performance for parsing property value via CSSOM. There are comments inside CSSParsingEnvironment stating that even reference counting has unacceptable performance overhead.

Also, given that in majority of cases, parsing style attribute doesn't really need them, the performance overhead on allocating those three temporary objects looks even more ridiculous.

I think we want to work harder on the ownership here to avoid directly owning URIs and principals inside style rules and style structs.

The first idea comes to me is to make style sheet and style set own the URIs and principals for its style rules and style structs correspondingly.

For style rules, those in style sheet are probably easy, because style sheet only has its own principal and URIs, and it always owns them. Style attribute is a bit harder because of xml:base (which we may want to remove, see bug 903372), XBL bindings, explicit base URI (used by SVG <use> element), and that the document's url can be changed by script. And style struct is probably even harder...

But follow that direction, I suggest that we can have a centralized URI/principal ownership manager at document level (for style attribute) and style set level, which owns those objects. Then we collect acquiring and releasing of those objects during parsing and restyling, then put the result into that manager after servo hands back the control to gecko. That way, we wouldn't need to pay anything if there is no url inside, and even if there is, we pay for a small hash table instead of extra allocation of threadsafe holders (it is not clear whether using hash table would be faster, but we don't have locality either way, and there should be fewer allocation by hash table than extra holders).

For making that idea safe, we can create a new smart pointer which asserts in its dtor if it still holds anything there, and requires a call with the ownership manager to clear the pointer it is holding.

heycam, what do you think about this idea?

	Xidorn Quan [:xidorn] UTC+11
	Comment 3 • 8 years ago

Make bug 1330503 block this since I think what added there could be useful for this, and that one is likely simpler than this.

Make bug 903372 block this because I don't think it is worth adding support for xml:base in stylo, but we don't want to drop feature with stylo. So we want to drop the support of xml:base before stylo. Also after dropping that, code would potentially be simpler here as well.

	Manish Goregaokar [:manishearth] Assignee
	Comment 5 • 7 years ago

There's actually a patch fixing this in bug 1329093.

	Bobby Holley (:bholley)
	Comment 6 • 7 years ago

Over to Manish per comment 5 and per bug 1343964 comment 4. 

We'll do the followup performance work in bug 1343964.

	Bobby Holley (:bholley)
	Comment 7 • 7 years ago

Manish, can we resolve this as a dupe of bug 1329093?