Closed Bug 1340728 Opened 3 years ago Closed 3 years ago

stylo: add constructors for @font-face descriptors

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: canova, Assigned: canova, Mentored)

References

Details

Attachments

(1 file)

Currently, font-family, src, and unicode-range descriptors need constructors for servo side.
I'm working on this.
Assignee: nobody → canaltinova
Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

https://reviewboard.mozilla.org/r/113586/#review115398

::: layout/style/ServoBindings.cpp:1211
(Diff revision 1)
> +    = nsCSSValue::Array::Create(aLen);
> +  return srcVals;
> +}
> +
> +void
> +Gecko_CSSValue_SetArray(nsCSSValueBorrowedMut aCSSValue, RefPtr<nsCSSValue::Array> aVals)

With the changes I request below there is no need to manipulate nsCSSValue::Array from the Rust side, so I’d prefer this function to take a length instead of an array, and be merged with Gecko_CSSValue_CreateArray.

::: layout/style/ServoBindings.cpp:1217
(Diff revision 1)
> +{
> +  aCSSValue->SetArrayValue(aVals, eCSSUnit_Array);
> +}
> +
> +void
> +Gecko_CSSValue_SetURL(RefPtr<nsCSSValue::Array> aVals,

This function does two things:

* Create an URL nsCSSValue
* Assign it in an Array at a given position

I’d prefer the two to be separated, since we might later have URL values that are not in an array.

Gecko_CSSValue_GetArrayItem already exists and returns a mutable refernce to a value inside an array at a given position. So this function should take nsCSSValueBorrowedMut instead of array + position, similar to Gecko_CSSValue_SetString.

::: layout/style/ServoBindings.cpp:1235
(Diff revision 1)
> +  value.SetURLValue(urlVal);
> +  aVals->Item(aPosition) = value;
> +}
> +
> +void
> +Gecko_CSSValue_SetLocal(RefPtr<nsCSSValue::Array> aVals, int32_t aPosition, const nsString& aFamily)

Same as above, this should take nsCSSValueBorrowedMut instead of an array + pointer.

::: layout/style/ServoBindings.cpp:1241
(Diff revision 1)
> +{
> +  aVals->Item(aPosition).SetStringValue(aFamily, eCSSUnit_Local_Font);
> +}
> +
> +void
> +Gecko_CSSValue_SetFontRanges(RefPtr<nsCSSValue::Array> aVals, int32_t aPosition, uint32_t aRange)

Same as above, this should take nsCSSValueBorrowedMut instead of an array + pointer.

It also looks like nothing in this function is specific to font ranges, so it should be renamed Gecko_CSSValue_SetInteger.
Bobby, in bug 1335308 you added assertions like this:

 void
 Gecko_CSSValue_SetAbsoluteLength(nsCSSValueBorrowedMut aCSSValue, nscoord aLen)
 {
+  MOZ_ASSERT(aCSSValue->GetUnit() == eCSSUnit_Null || aCSSValue->IsLengthUnit());
+  // The call below could trigger refcounting if aCSSValue were a
+  // FontFamilyList, but we just asserted that it's not. So we can
+  // whitelist this for static analysis.
   aCSSValue->SetIntegerCoordValue(aLen);
 }

If I understand correctly, this is necessary because dropping a previous value with a different unit could could the value’s refcount to be decremented, but refcounting is not thread-safe and we might be calling this function from outside the main thread.

This patch adds new similar setters. Do they need similar assertions?
Flags: needinfo?(bobbyholley)
Addressed the review comments. It definitely looks better now. Thanks!
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

https://reviewboard.mozilla.org/r/113586/#review115446

Looks good!

Waiting for bholley’s answer regarding unit assertions.
Attachment #8838783 - Flags: review?(simon.sapin) → review+
(In reply to Simon Sapin (:SimonSapin) from comment #4)
> This patch adds new similar setters. Do they need similar assertions?

Assuming those invariants hold in terms of the callers on the servo side, we should add similar assertions.
Flags: needinfo?(bobbyholley)
I added assertions to all functions but I don't know if all of them are necessary or not. Could you look at these please Bobby?
Also I'm not very familiar with unit types in gecko. I hope I didn't do anything wrong here :)
Flags: needinfo?(bobbyholley)
Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

https://reviewboard.mozilla.org/r/113586/#review115732

So, the problem here is that these are complex types, some of which don't have threadsafe refcounts. This means that the asserts in this patch are not strict enough. For example, Gecko_CSSValue_SetArray is not safe to call if |aCSSValue| is already an array (which the assertions in this patch allow), since changing the value triggers a decrement of the non-threadsafe refcount of the old array.

This begs the question: in what situations do we call these functions with non-eCSSUnit_Null aCSSValue arguments? If we never do, we can just tighten the assertion. Otherwise, we'll need to think up a solution.
Attachment #8838783 - Flags: review-
Flags: needinfo?(bobbyholley)
(See the DO_RELEASE stuff in searchfox.org/mozilla-central/source/layout/style/nsCSSValue.cpp for the hazardous release described above)
Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

https://reviewboard.mozilla.org/r/113586/#review116028
Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

https://reviewboard.mozilla.org/r/113586/#review116090
Attachment #8838783 - Flags: review+
Removed the changes in servo/ directory. I'll open a PR for it.
Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

https://reviewboard.mozilla.org/r/113586/#review116526
Attachment #8838783 - Flags: review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a8775af9fb34
stylo: add constructors for @font-face descriptors r=manishearth,SimonSapin
https://hg.mozilla.org/mozilla-central/rev/a8775af9fb34
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.