1340728 - stylo: add constructors for @font-face descriptors

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Currently, font-family, src, and unicode-range descriptors need constructors for servo side.

Comment 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

I'm working on this.

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1340728 - stylo: add constructors for @font-face descriptors

Review commit: https://reviewboard.mozilla.org/r/113586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113586/

Comment 3

[Simon Sapin (:SimonSapin)]

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.

Comment 4

[Simon Sapin (:SimonSapin)]

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?

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113586/diff/1-2/

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Addressed the review comments. It definitely looks better now. Thanks!

Comment 7

[Simon Sapin (:SimonSapin)]

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.

Comment 8

[Bobby Holley (:bholley)]

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

Comment 9

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113586/diff/2-3/

Comment 10

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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 :)

Comment 11

[Bobby Holley (:bholley)]

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.

Comment 12

[Bobby Holley (:bholley)]

(See the DO_RELEASE stuff in searchfox.org/mozilla-central/source/layout/style/nsCSSValue.cpp for the hazardous release described above)

Comment 13

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113586/diff/3-4/

Comment 14

[Simon Sapin (:SimonSapin)]

Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

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

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

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

Comment 16

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113586/diff/4-5/

Comment 17

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Removed the changes in servo/ directory. I'll open a PR for it.

Comment 18

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113586/diff/5-6/

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8838783 [details]
Bug 1340728 - stylo: add constructors for @font-face descriptors

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8775af9fb34
stylo: add constructors for @font-face descriptors r=manishearth,SimonSapin

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8775af9fb34