Closed Bug 1345696 Opened 7 years ago Closed 7 years ago

stylo: Implement access to CSSFontFaceRule

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 4 obsolete files)

Bug 1345696 part 1 - Lots of fixup for the next patch.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1345696 part 2 - Move nsCSSFontFaceRule to a separate header.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1345696 part 3 - Add FFI for nsCSSFontFaceRule.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1345696 part 4 - Add function for sugar of nsCSSValue.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1345696 part 5 - Support CSSOM access to @font-face rules.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1345696 part 6 - Fix cycle collection for font-face rule.
59 bytes, text/x-review-board-request
heycam
: review+
mccr8
: review+
Details
Bug 1345696 part 7 - Provide @font-face rules for stylo backend.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1345696 part 8 - Update test expectations.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
      No description provided.
Priority: -- → P2
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)
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.
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)
(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.
Blocks: 1290237
Depends on: 1351204
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 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 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 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 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?
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.
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...?
Depends on: 1351275
Attachment #8851915 - Flags: review?(continuation)
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 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 on attachment 8851911 [details]
Rename font_face::FontFaceRule to FontFaceData.

https://reviewboard.mozilla.org/r/124154/#review126890
Attachment #8851911 - Flags: review?(manishearth) → 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 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 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 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+
Attachment #8851918 - Attachment is obsolete: true
Attachment #8851918 - Flags: review?(cam)
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+
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...
Blocks: 1351662
Filed bug 1351662.
Attachment #8851911 - Attachment is obsolete: true
Attachment #8851913 - Attachment is obsolete: true
Attachment #8851916 - Attachment is obsolete: true
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
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f28a5dfdfc03
followup - Update more test expectation.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: