1275766 - Re-enable parallel traversal for stylo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

We turned it off a while back in our sprint to wikipedia, and various issues have arisen in the interim. I have patches to fix all the remaining issues so that we can turn it back on, and hopefully leave it as such.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stub out nsPresContext::HasAuthorSpecifiedRules. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Invoke the proper URLValue constructor from Servo. v1

Right now these are just coercing the values and invoking the main thread
destructor.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix counter style.assertion. v1

This is backwards.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Give nsStyleQuoteValues threadsafe refcounting. v1

The contents are immutable after creation and safe to destroy on any thread.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Implement Gecko_GetElementId and Gecko_GetClassOrClassList. v1

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8756609 [details] [diff] [review]
Part 5 - Implement Gecko_GetElementId and Gecko_GetClassOrClassList. v1

Review of attachment 8756609 [details] [diff] [review]:
-----------------------------------------------------------------

Flagging ms2ger for feedback on the DOM attr usage.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8756606 [details] [diff] [review]
Part 2 - Invoke the proper URLValue constructor from Servo. v1

Review of attachment 8756606 [details] [diff] [review]:
-----------------------------------------------------------------

s/destructor/constructor/ in the commit message, I think.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8756609 [details] [diff] [review]
Part 5 - Implement Gecko_GetElementId and Gecko_GetClassOrClassList. v1

Review of attachment 8756609 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoBindings.cpp
@@ +196,5 @@
> +    nsAString::const_iterator iter, end;
> +    str.BeginReading(iter);
> +    str.EndReading(end);
> +    while (iter != end) {
> +      MOZ_ASSERT(nsContentUtils::IsHTMLWhitespace(*iter++));

You could collapse this block down to a single (though a little long) assertion statement, if you want:

MOZ_ASSERT(nsContentUtils::TrimWhitespace<nsContentUtils::IsHTMLWhitespace>(attr->GetStringValue()).IsEmpty());

@@ +218,5 @@
> +  nsTArray<nsCOMPtr<nsIAtom>>* atomArray = attr->GetAtomArrayValue();
> +  nsCOMPtr<nsIAtom>* elements = atomArray->Elements();
> +  nsIAtom** rawElements = reinterpret_cast<nsIAtom**>(elements);
> +  *aClassList = rawElements;
> +  return atomArray->Length();

Maybe assert in here that the returned length is > 1?

But I don't think that's always true.  For example if you have class="x " (i.e. with some white space but still only a single class name) it'll still have an atom array.

If that's right, the calling convention will need to change.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 5 - Implement Gecko_GetElementId and Gecko_GetClassOrClassList. v2

Comment 10

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ff1aca29f01

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9de9eee6be3
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e8227f66e97
https://hg.mozilla.org/integration/mozilla-inbound/rev/faf4134b30b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c8478b70c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe87fe3e36a9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9de9eee6be3
https://hg.mozilla.org/mozilla-central/rev/9e8227f66e97
https://hg.mozilla.org/mozilla-central/rev/faf4134b30b9
https://hg.mozilla.org/mozilla-central/rev/53c8478b70c0
https://hg.mozilla.org/mozilla-central/rev/fe87fe3e36a9