Closed Bug 1275766 Opened 4 years ago Closed 4 years ago

Re-enable parallel traversal for stylo

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files, 1 obsolete file)

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.
Right now these are just coercing the values and invoking the main thread
destructor.
Attachment #8756606 - Flags: review?(cam)
This is backwards.
Attachment #8756607 - Flags: review?(cam)
The contents are immutable after creation and safe to destroy on any thread.
Attachment #8756608 - Flags: review?(cam)
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.
Attachment #8756609 - Flags: review?(cam)
Attachment #8756609 - Flags: feedback?(Ms2ger)
Attachment #8756605 - Flags: review?(cam) → review+
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.
Attachment #8756606 - Flags: review?(cam) → review+
Attachment #8756607 - Flags: review?(cam) → review+
Attachment #8756608 - Flags: review?(cam) → review+
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.
Attachment #8756609 - Flags: review?(cam)
Attachment #8756609 - Attachment is obsolete: true
Attachment #8756609 - Flags: feedback?(Ms2ger)
Attachment #8756655 - Flags: review?(cam) → review+
Attachment #8756655 - Flags: feedback?(Ms2ger)
You need to log in before you can comment on or make changes to this bug.