stylo: Implement access to CSSFontFaceRule

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments, 4 obsolete attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
manishearth
: review+
Details | Review
59 bytes, text/x-review-board-request
manishearth
: review+
Details | Review
59 bytes, text/x-review-board-request
manishearth
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
mccr8
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
Comment hidden (empty)
Priority: -- → P2
(Assignee)

Updated

5 months ago
Blocks: 1324701
Priority: P2 → P1
Xidorn - Jonathan is going to be looking at the @font-face stuff in bug 1281962. IIUC he'll need the CSSOM bits in this bug as well, though they're otherwise mostly-separate tasks. Does that sound right?

If so, are the CSSOM bits something you can knock out without much trouble? Assuming Jonathan needs it to test is stuff in bug 1281962, it's probably worth doing this on the sooner side.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 2

5 months ago
I believe you mean bug 1290237.

From my previous investigation (bug 1290237 comment 3), it seems to me our internal usage of @font-face basically depends on CSSOM interfaces of CSSFontFaceRule, so I would say bug 1290237 is going to be completely blocked by this bug, and maybe I can do all of them in this bug together, since I don't think there are much remaining work after implementing the CSSOM bits.
Flags: needinfo?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #2)
> I believe you mean bug 1290237.

Sorry, clipboard fail. Too many bugs flying around.
 
> From my previous investigation (bug 1290237 comment 3), it seems to me our
> internal usage of @font-face basically depends on CSSOM interfaces of
> CSSFontFaceRule, so I would say bug 1290237 is going to be completely
> blocked by this bug, and maybe I can do all of them in this bug together,
> since I don't think there are much remaining work after implementing the
> CSSOM bits.

Ok sounds good. Let's have you implement the CSSOM bits, and then if there's any non-trivial amount of followup work we can hand that off the Jonathan.
(Assignee)

Comment 4

5 months ago
It seems to me that @font-face rules are mainly used by Gecko side, and Servo side doesn't really need that information, so I'm considering a different approach for integrating it.

I want to have this rule backed by Gecko class in Servo side. That says, I'd like to have Servo code fill the nsCSSValues of Gecko's nsCSSFontFaceRule directly, and make Servo's FontFaceRule just hold a pointer to the corresponding Gecko rule object (and own that object).

This is quite different from our previous approach that data is hold in Servo side, and Gecko class is just a thin wrapper of Servo object to C++.

Do you think there would be any problem with this approach?

The main problem to be solved is that, we need to support collecting all effective @font-face rules, and mapping them to FontFace object if existed when flushing user font set. And flushing user font set could happen quite frequently (although I suspect whether it really needs to, which might be something optimizable in Gecko side).

For supporting that, we have several choices:
(1) traverse the Gecko CSSOM tree for collecting
(2) traverse by Servo code, and we create C++ wrapper for them when collecting
(3) traverse by Servo code, and we make FontFace object able to be backed by Servo rule object
(4) hold data in C++ object, traverse by Servo code, and Servo returns C++ objects directly

Choice (3) is probably doable, but I'm a bit concerned about the performance consequence of getting descriptor value, especially font-family (which is a string) and src (which is a list of url / string). Generating string from Servo to Gecko is especially inefficient as we've seen in properties.

Choice (2) would be worse than (3) because it adds the extra cost to create wrapper object for every flushing.

Choice (1) is probably the worst that we need to duplicate the logic of stylesheet::effective_rules in C++ code, and we need to expand all group rules in CSSOM tree. Also it seems we don't have a fast way to check whether a media rule is active outside cascading at all.

So I think (4) would be the best choice, and may require the least code change in Gecko side since we can just reuse all the infrastructure based on the existing nsCSSFontFaceRule.

The only problem is that it is quite different than what we have been doing, so I want to confirm if there is any concern about this approach.
Flags: needinfo?(simon.sapin)
Flags: needinfo?(bobbyholley)
Call "parse_font_face_block() -> Result<FontFaceRule, ()>", then instead of wrapping into Arc<Locked<FontFaceRule>> convert to nsCSSFontFaceRule at parse time and keep a gecko reference-counted pointer in CssRule? Sounds good.

I suppose CSSOM implementation for CSSFontFaceRule would be almost entirely on the Gecko side. Stylo can provide function for parsing values of individual descriptors, to avoid duplicating parsing code between CSSFontFaceRule setters and stylesheet parsing.
Flags: needinfo?(simon.sapin)
Sounds fine to me!
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 7

5 months ago
(In reply to Simon Sapin (:SimonSapin) from comment #5)
> I suppose CSSOM implementation for CSSFontFaceRule would be almost entirely
> on the Gecko side. Stylo can provide function for parsing values of
> individual descriptors, to avoid duplicating parsing code between
> CSSFontFaceRule setters and stylesheet parsing.

CSSFontFaceRule doesn't really have much CSSOM things to do at the moment. It doesn't support mutating at all in Gecko, either via CSSOM or via FontFace.

This is something we should fix, but not for now, I suppose.
(Assignee)

Updated

5 months ago
Blocks: 1290237
(Assignee)

Comment 8

5 months ago
Almost... https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfe2638dee4aa20304cccf99047ef0b8e6946aa2
(Assignee)

Updated

5 months ago
Depends on: 1351204
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

5 months ago
mozreview-review
Comment on attachment 8851908 [details]
Bug 1345696 part 1 - Lots of fixup for the next patch.

https://reviewboard.mozilla.org/r/124148/#review126682
Attachment #8851908 - Flags: review?(cam) → review+

Comment 22

5 months ago
mozreview-review
Comment on attachment 8851909 [details]
Bug 1345696 part 2 - Move nsCSSFontFaceRule to a separate header.

https://reviewboard.mozilla.org/r/124150/#review126684

::: layout/style/FontFace.cpp:16
(Diff revision 1)
>  #include "mozilla/dom/Promise.h"
>  #include "mozilla/dom/TypedArray.h"
>  #include "mozilla/dom/UnionTypes.h"
>  #include "mozilla/CycleCollectedJSContext.h"
>  #include "nsCSSParser.h"
> -#include "nsCSSRules.h"
> +#include "nsCSSFontFaceRule.h"

Nit: Move this one line up?
Attachment #8851909 - Flags: review?(cam) → review+

Comment 23

5 months ago
mozreview-review
Comment on attachment 8851917 [details]
Bug 1345696 part 7 - Provide @font-face rules for stylo backend.

https://reviewboard.mozilla.org/r/124166/#review126692
Attachment #8851917 - Flags: review?(cam) → review+

Comment 24

5 months ago
mozreview-review
Comment on attachment 8851918 [details]
Bug 1345696 part 8 - Make test_redundant_font_download.html more robust.

https://reviewboard.mozilla.org/r/124168/#review126694

I'm worried this is hiding a real bug. :(  Can you investigate to find out what's happening here?

Comment 25

5 months ago
mozreview-review
Comment on attachment 8851919 [details]
Bug 1345696 part 8 - Update test expectations.

https://reviewboard.mozilla.org/r/124170/#review126696

Nice...
Attachment #8851919 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #24)
> Comment on attachment 8851918 [details]
> Bug 1345696 part 8 - Make test_redundant_font_download.html more robust.
> 
> https://reviewboard.mozilla.org/r/124168/#review126694
> 
> I'm worried this is hiding a real bug. :(  Can you investigate to find out
> what's happening here?

What I think _should_ be happening is that setting a new className on the test element triggers a reflow, which in turn will initiate the font-load. So if we need to explicitly flush layout in the test file here, it sounds like maybe we're not reflowing as expected in response to the className change?
(Assignee)

Comment 27

5 months ago
Or we do not reflow before timeout. I'll investigate this tomorrow, and either try to fix it or mark fail for stylo&e10s with a new bug filed.
(Assignee)

Comment 28

5 months ago
So, right, changing className doesn't trigger restyle on e10s no matter how long the timeout is, which seems like a real bug. If I move the pointer into the page, the restyle would be triggered, and the test would pass.

IIRC long ago we had a bug about that setting className doesn't trigger restyle? How was that going? And why is this bug happen only on e10s...?
(Assignee)

Updated

5 months ago
Depends on: 1351275
Attachment #8851915 - Flags: review?(continuation)

Comment 29

5 months ago
mozreview-review
Comment on attachment 8851915 [details]
Bug 1345696 part 6 - Fix cycle collection for font-face rule.

https://reviewboard.mozilla.org/r/124162/#review126868

Bobby asked me to take a look. This sounds reasonable to me, assuming that you know that this other Servo object will definitely always exist and definitely hold a reference to this rule. Reporting too many edges to an object will make things get unlinked early, which can cause weird crashes. Bobby should also check the assumptions in your patch, so I left his r? there.
Attachment #8851915 - Flags: review?(continuation) → review+
Comment on attachment 8851915 [details]
Bug 1345696 part 6 - Fix cycle collection for font-face rule.

Given that cameron reviewed the rest of this, I'll let him double-check the assumption in comment 29.
Attachment #8851915 - Flags: review?(bobbyholley) → review?(cam)

Comment 31

5 months ago
mozreview-review
Comment on attachment 8851910 [details]
Bug 1345696 part 3 - Add FFI for nsCSSFontFaceRule.

https://reviewboard.mozilla.org/r/124152/#review126880
Attachment #8851910 - Flags: review?(manishearth) → review+

Comment 32

5 months ago
mozreview-review
Comment on attachment 8851911 [details]
Rename font_face::FontFaceRule to FontFaceData.

https://reviewboard.mozilla.org/r/124154/#review126890
Attachment #8851911 - Flags: review?(manishearth) → review+

Comment 33

5 months ago
mozreview-review
Comment on attachment 8851912 [details]
Bug 1345696 part 4 - Add function for sugar of nsCSSValue.

https://reviewboard.mozilla.org/r/124156/#review126892
Attachment #8851912 - Flags: review?(manishearth) → review+

Comment 34

5 months ago
mozreview-review
Comment on attachment 8851913 [details]
Use Gecko nsCSSFontFaceRule for font face rule.

https://reviewboard.mozilla.org/r/124158/#review126894

::: servo/components/style/gecko/rules.rs:34
(Diff revision 1)
> +            })
> +        }}
> +    }
> +
> +    // font-style
> +    map_enum!(mStyle = (style: font_style) {

I think we can use the `gecko_conversion` mechanisms here.

::: servo/components/style/gecko/rules.rs:42
(Diff revision 1)
> +        oblique => NS_FONT_STYLE_OBLIQUE,
> +    });
> +
> +    // font-weight
> +    use computed_values::font_weight::T as FontWeight;
> +    descriptors.mWeight.set_integer(match data.weight {

Can this be consolidated as a method on FontWeight so that it can be used by gecko.mako.rs as well?

::: servo/components/style/gecko/rules.rs:55
(Diff revision 1)
> +        FontWeight::Weight800 => 800,
> +        FontWeight::Weight900 => 900,
> +    });
> +
> +    // font-stretch
> +    map_enum!(mStretch = (stretch: font_stretch) {

ditto re `gecko_conversion`

::: servo/components/style/gecko/rules.rs:82
(Diff revision 1)
> +    for src in data.sources.iter() {
> +        match *src {
> +            Source::Url(ref url) => {
> +                target_srcs.next().unwrap().set_url(&url.url);
> +                for hint in url.format_hints.iter() {
> +                    target_srcs.next().unwrap().set_font_format(&hint);

use `expect`s here?
Attachment #8851913 - Flags: review?(manishearth) → review+

Comment 35

5 months ago
mozreview-review
Comment on attachment 8851914 [details]
Bug 1345696 part 5 - Support CSSOM access to @font-face rules.

https://reviewboard.mozilla.org/r/124160/#review126898
Attachment #8851914 - Flags: review?(manishearth) → review+

Comment 36

5 months ago
mozreview-review
Comment on attachment 8851916 [details]
Record effective @font-face rules when updating stylist.

https://reviewboard.mozilla.org/r/124164/#review126902

Would like some more docs/comments on this one.
Attachment #8851916 - Flags: review?(manishearth) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8851918 - Attachment is obsolete: true
Attachment #8851918 - Flags: review?(cam)

Comment 48

5 months ago
mozreview-review
Comment on attachment 8851915 [details]
Bug 1345696 part 6 - Fix cycle collection for font-face rule.

https://reviewboard.mozilla.org/r/124162/#review127114

OK, this isn't great, but I can't think of a better solution that doesn't involve hooking up CC through Rust style sheet and rule objects.  And I think the assumptions are OK.

From code inspection, I can't see devtools setting any expandos on the rule object that gets returned from inDOMUtils::GetUsedFontFaces.  It might be that devtools doesn't really need to hold on to these objects, and instead we could make the API return copies of the descriptor values that were on the @font-face rules.  Can you file a followup a CC/needinfo the appropriate devtools person to ask about that?
Attachment #8851915 - Flags: review?(cam) → review+
(Assignee)

Comment 49

5 months ago
Probably we can do this: when nsRange::GetUsedFontFaces is invoked, we force a CSSOM tree build-up in the current document to construct all @font-face rules, and set a flag on the document that we need aggressive CSSOM tree build-up for @font-face rules.

Whenever a new stylesheet or new group rule is inserted, we check this flag, and if it is set, we trigger the CSSOM tree build-up.

This should avoid leak from cycle reference on nsCSSFontFaceRules. And we can reuse this mechanism whenever a new API is added which needs some rule owned by servo code.

Alternatively, we can eventually migrate FontFace to be backed by Servo data, so that we can get rid of this hack.

But I guess it is not a priority anyway. It doesn't seem to me devtools is going to leak at the moment. I'll file a followup bug for tracking this, and we can think about this latter...
(Assignee)

Updated

5 months ago
Blocks: 1351662
(Assignee)

Comment 50

5 months ago
Filed bug 1351662.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8851911 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8851913 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8851916 - Attachment is obsolete: true

Comment 59

5 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e84a3cc3233
part 1 - Lots of fixup for the next patch. r=heycam
https://hg.mozilla.org/integration/autoland/rev/badeba9aaf98
part 2 - Move nsCSSFontFaceRule to a separate header. r=heycam
https://hg.mozilla.org/integration/autoland/rev/0d1bd870cc10
part 3 - Add FFI for nsCSSFontFaceRule. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/54491183b64b
part 4 - Add function for sugar of nsCSSValue. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/59d93e337f9c
part 5 - Support CSSOM access to @font-face rules. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/f24a273f1f7c
part 6 - Fix cycle collection for font-face rule. r=heycam,mccr8
https://hg.mozilla.org/integration/autoland/rev/1f4531b2ba5f
part 7 - Provide @font-face rules for stylo backend. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3c163f0ba4b7
part 8 - Update test expectations. r=heycam

Comment 60

5 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f28a5dfdfc03
followup - Update more test expectation.

Comment 61

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2e84a3cc3233
https://hg.mozilla.org/mozilla-central/rev/badeba9aaf98
https://hg.mozilla.org/mozilla-central/rev/0d1bd870cc10
https://hg.mozilla.org/mozilla-central/rev/54491183b64b
https://hg.mozilla.org/mozilla-central/rev/59d93e337f9c
https://hg.mozilla.org/mozilla-central/rev/f24a273f1f7c
https://hg.mozilla.org/mozilla-central/rev/1f4531b2ba5f
https://hg.mozilla.org/mozilla-central/rev/3c163f0ba4b7
https://hg.mozilla.org/mozilla-central/rev/f28a5dfdfc03
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.