Closed Bug 1028497 Opened 6 years ago Closed 5 years ago

implement the CSS Font Loading API

Categories

(Core :: DOM: CSS Object Model, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: heycam, Assigned: heycam)

References

(Depends on 2 open bugs, Blocks 8 open bugs, )

Details

(Keywords: dev-doc-needed, feature)

Attachments

(31 files, 69 obsolete files)

1.45 KB, patch
Details | Diff | Splinter Review
10.46 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
13.50 KB, patch
Details | Diff | Splinter Review
5.81 KB, patch
jtd
: review+
Details | Diff | Splinter Review
59.60 KB, patch
jtd
: review+
Details | Diff | Splinter Review
36.79 KB, patch
jtd
: review+
Details | Diff | Splinter Review
770 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.71 KB, patch
jtd
: review+
Details | Diff | Splinter Review
8.27 KB, patch
jtd
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
9.76 KB, patch
jtd
: review+
Details | Diff | Splinter Review
4.71 KB, patch
jtd
: review+
Details | Diff | Splinter Review
4.23 KB, patch
jtd
: review+
Details | Diff | Splinter Review
3.20 KB, patch
jtd
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
8.95 KB, patch
jtd
: review+
Details | Diff | Splinter Review
14.81 KB, patch
jtd
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
1.42 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
1.32 KB, patch
jtd
: review+
Details | Diff | Splinter Review
1.17 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
2.91 KB, patch
jtd
: review+
Details | Diff | Splinter Review
15.71 KB, patch
jtd
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
8.56 KB, patch
jtd
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
17.17 KB, patch
jtd
: review+
Details | Diff | Splinter Review
13.00 KB, patch
jtd
: review+
Details | Diff | Splinter Review
22.57 KB, patch
jtd
: review+
Details | Diff | Splinter Review
4.10 KB, patch
jtd
: review+
Details | Diff | Splinter Review
29.86 KB, patch
jtd
: review+
Details | Diff | Splinter Review
6.25 KB, patch
jtd
: review+
Details | Diff | Splinter Review
46.06 KB, patch
jtd
: review+
Details | Diff | Splinter Review
33.44 KB, patch
jtd
: review+
Details | Diff | Splinter Review
29.67 KB, patch
bzbarsky
: review+
jtd
: review+
Details | Diff | Splinter Review
119.77 KB, patch
jtd
: review+
Details | Diff | Splinter Review
No description provided.
Depends on: 1026080
Depends on: 754215
Keywords: feature, relnote
Depends on: 1031153
Depends on: 1031186
Depends on: 1031187
Depends on: 1031199
Depends on: 1031202
Depends on: 1031205
Depends on: 1031206
Current patch queue at: http://mcc.id.au/temp/bug-1028497/
Depends on: 1031967
Things I'm going to defer to followup bugs:

  - Implementing the load() and check() methods on FontFaceSet.
  - Handling modification of descriptor values on FontFace objects.
  - Supporting FontFaceSet in workers (not much point until we have
    canvas in workers anyway, bug 801176).

Things I'm not implementing now because of ongoing discussion on www-style:

  - The FontFaceSet constructor.
  - The full Set-like API on FontFaceSet.
Attachment #8448626 - Flags: review?(jdaggett)
Attachment #8448626 - Attachment description: 0011-Bug-1028497-Part-1-Add-pref-for-CSS-Font-Loading-API.patch → Part 1: Add pref for CSS Font Loading API.
Not requesting review on this until we work out what to do with the Set-like API.
Attachment #8448631 - Attachment description: 0014-Bug-1028497-Part-4-Implement-CSSFontFaceLoadEvent-co.patch → Part 4: Implement CSSFontFaceLoadEvent constructor and fontfaces attribute.
This introduces a new class FontFaceDataSource, which can wrap either a
(nsCSSFontFaceRule object, nsStyleSet::sheetType) pair or a FontFace object.
nsUserFont set is changed to deal with an array of FontFaceDataSources rather
than an array of nsFontFaceRuleContainer objects.  For now, we don't
handle FontFace objects; this will be done in part 14.
Attachment #8448633 - Flags: review?(jdaggett)
This pointer to the nsCSSFontFaceRule will allow us to forward the
descriptor IDL attributes (family, style, etc.) to the rule, once we
start supporting CSS-connected FontFace objects.
Attachment #8448635 - Flags: review?(jdaggett)
This adds storage for the descriptor values on a FontFace and implements
the getters for them.
Attachment #8448636 - Flags: review?(jdaggett)
Attachment #8448639 - Flags: review?(jdaggett)
Attachment #8448640 - Attachment description: 0020-Bug-1028497-Part-10-Add-public-nsCSSParser-function-.patch → Part 10: Add public nsCSSParser function for parsing @font-face descriptors.
This implements the parsing of strings assigned to the descriptor IDL
attributes on FontFace objects, and sets the corresponding value in
mDetails if parsing succeeded.  As mentioned in the comments, this
doesn't cause the user font set to update yet; this will be handled in a
followup bug.
Attachment #8448642 - Flags: review?(jdaggett)
This gives the user font set the ability to create gfxProxyFontEntrys
(or look up existing gfxFontEntrys from the user font cache) for
FontFaceDataSource objects that wrap a FontFace object.
Attachment #8448644 - Flags: review?(jdaggett)
This adds on to gfxProxyFontEntry a pointer to the FontFace that caused
its creation.  The pointer is used to inform the FontFace about the
loading state as it progresses.
Attachment #8448649 - Flags: review?(jdaggett)
Comment on attachment 8448652 [details] [diff] [review]
Part 20: Support loading of fonts from ArrayBuffer{,View}s.

We add a new gfxFontFaceSrc type to represent a buffer of font data
that can be loaded immediately.  gfxFontFaceBufferSource is used
to abstract that, and a FontFace provides a concrete instance of
a gfxFontFaceBufferSource to expose its ArrayBuffer/ArrayBufferView
contents.
John, if there are IDL/stylerule/DOM/events parts of the patches that you're not comfortable reviewing, can you please flag Boris to review for those bits?  (I'll do that myself for a couple of the patches to start with.)
Comment on attachment 8448653 [details] [diff] [review]
Part 21: Implement FontFaceSet.{ready,status} and dispatch events.

Boris, in FontFaceSet::OnFontFaceStatusChanged in this patch, I'm wanting to fire a DOM event.  I found that I couldn't do it immediately, as in some contexts I got an assertion that it wasn't safe to do so.  So I'm currently dispatching a runnable to the event queue which then fires the DOM event.  Is there something else/better I should be doing?  Can you also look at the code that constructs and fires the DOM event?
Attachment #8448653 - Flags: review?(bzbarsky)
Depends on: 1032763
Comment on attachment 8448631 [details] [diff] [review]
Part 4: Implement CSSFontFaceLoadEvent constructor and fontfaces attribute.

>+  for (size_t i = 0; i < tmp->mFontfaces.Length(); i++) {

  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mFontfaces) 

should do the right thing.

>+  tmp->mFontfaces.Clear();

  NS_IMPL_CYCLE_COLLECTION_UNLINK(mFontfaces)

>+  mFontfaces.AppendElements(aFontfaces);

  mFontfaces = aFontfaces;

?

>+  aResult.Clear();
>+  aResult.AppendElements(mFontfaces);

aResult = mFontfaces;

All that said, is there a reason we're not using event codegen for this event?
Attachment #8448631 - Flags: review?(bzbarsky)
Flags: needinfo?(cam)
Comment on attachment 8448653 [details] [diff] [review]
Part 21: Implement FontFaceSet.{ready,status} and dispatch events.

What assertions do you get?  What does the spec say about when the event fired?

Note AsyncEventDispatcher, which has RunDOMEventWhenSafe and PostDOMEvent: the former runs once we unwind out of all pending script blockers while the former goes via an event loop task.  I think we should be using it here for whatever asynchrony we want.

>+  event->InitEvent(aType, true, false);

This shouldn't bubble, should it?

Also, the event should always be trusted, I'd think, based on the spec; that's not what DispatchLoadingFinishedEvent is doing.  Tests needed?

I guess the only reason you're using DispatchDOMEvent is because we have no signature for "DispatchEvent but I don't care if it throws"?
Attachment #8448653 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #28)
> Comment on attachment 8448631 [details] [diff] [review]
> Part 4: Implement CSSFontFaceLoadEvent constructor and fontfaces attribute.
> 
> All that said, is there a reason we're not using event codegen for this
> event?

It was because it wasn't working when I was trying to write the patch for the default sequence [] value myself, and then when I rebased on your patch for that I forgot to re-check if the event codegen worked.  Looks like it doesn't: filed bug 1033100 for that.
Flags: needinfo?(cam)
(In reply to Boris Zbarsky [:bz] from comment #29)
> Comment on attachment 8448653 [details] [diff] [review]
> Part 21: Implement FontFaceSet.{ready,status} and dispatch events.
> 
> What assertions do you get?  What does the spec say about when the event
> fired?

###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file /z/moz2/c/dom/events/EventDispatcher.cpp, line 455

The spec doesn't require the events be dispatched after an event loop run.

> Note AsyncEventDispatcher, which has RunDOMEventWhenSafe and PostDOMEvent:
> the former runs once we unwind out of all pending script blockers while the
> former goes via an event loop task.  I think we should be using it here for
> whatever asynchrony we want.

OK, sounds like RunDOMEventWhenSafe is what we want here then.

> >+  event->InitEvent(aType, true, false);
> 
> This shouldn't bubble, should it?

Oh yes.

> Also, the event should always be trusted, I'd think, based on the spec;
> that's not what DispatchLoadingFinishedEvent is doing.  Tests needed?

So just event->SetTrusted(true)?  Does the return value of event->Init(this) just tell me whether we have content script on the stack, and so if you know this event is being constructed due to an event constructor call or similar, that it should be marked as not trusted?

> I guess the only reason you're using DispatchDOMEvent is because we have no
> signature for "DispatchEvent but I don't care if it throws"?

No, it's just because I saw other code using it and just copied it.  Is DispatchEvent preferable?  If so, the one on nsIDOMEventTarget (which takes a bool outparam) or the one on EventTarget (which takes an ErrorResult)?  I guess it's moot if we use AsyncEventDispatcher anyway.
> filed bug 1033100 for that

Patch is up.

> So just event->SetTrusted(true)?

Yes.

> Does the return value of event->Init(this) just tell me whether we have content script
> on the stack

No, it actually examines the global the event is being associated with and returns whether that global's document is chrome (at least on main thread; in workers in in fact introspects the stack).  Olli, is this init() call even needed here?

> Is DispatchEvent preferable? 

Well, it would be nice to avoid passing all those nulls. We'd need to add a new signature, though, that takes no args other than the event.

> I guess it's moot if we use AsyncEventDispatcher anyway.

Yeah, true.
Depends on: 1033100
Does this look better re event dispatching?
Attachment #8448653 - Attachment is obsolete: true
Attachment #8448653 - Flags: review?(jdaggett)
Attachment #8449191 - Flags: review?(jdaggett)
Attachment #8449191 - Flags: review?(bzbarsky)
There's a leak I haven't figured out yet: https://tbpl.mozilla.org/?tree=Try&rev=eca1eadef8e2
Comment on attachment 8449191 [details] [diff] [review]
Part 21: Implement FontFaceSet.{ready,status} and dispatch events. (v2)

>+    if (mDocument->CSSLoader()) {

Please document that this might be null because we're in the middle of unlink?  Normally it can't be null.

>+    if (mReadyIsResolved) {

This seems moderately weird.  In particular, can't this lead to race conditions where someone might do .ready.then() and see this load or not depending on exact ordering?

>+FontFaceSet::HandleEvent(nsIDOMEvent* aEvent)
>+  Disconnect();

We don't want to disconnect from the CSSLoader here, do we?

I'm not sure how the mFonts document member works across presshell destruction and recreation.  What happens, exactly?  Do we create a new FontFaceSet?  Do we change the prescontext on the existing one?

r=me modulo those issues
Attachment #8449191 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #35)
> Comment on attachment 8449191 [details] [diff] [review]
> Part 21: Implement FontFaceSet.{ready,status} and dispatch events. (v2)
> 
> >+    if (mReadyIsResolved) {
> 
> This seems moderately weird.  In particular, can't this lead to race
> conditions where someone might do .ready.then() and see this load or not
> depending on exact ordering?

I'm not seeing where the race is.  Definitely if you wait a script turn before calling .ready.then() after you initiate a load, you could have called the new Promise object's then(), if the original font load completed and then a new one started.  But you'll still get called, once the new font finished loading.  Can you detail the situation you're worried about?

> >+FontFaceSet::HandleEvent(nsIDOMEvent* aEvent)
> >+  Disconnect();
> 
> We don't want to disconnect from the CSSLoader here, do we?

No, well spotted.

> I'm not sure how the mFonts document member works across presshell
> destruction and recreation.  What happens, exactly?  Do we create a new
> FontFaceSet?  Do we change the prescontext on the existing one?

I have not thought about this.  When does presshell recreation happen exactly?  When you move tabs between windows for example?  I think we can't recreate a FontFaceSet, since we really need document.fonts to be the same object.  Should I re-set mFonts->mPresContext when a new presshell is set, and if so, should I just do it in nsDocument::DoCreateShell (and maybe also clear mFonts->mPresContext in DeleteShell)?  Or maybe don't store mPresContext on FontFaceSet and always get it from mDocument's shell?
> Can you detail the situation you're worried about?

Is OnFontFaceStatusChanged basically only called off the event loop?  If so, then I think you're right and there is no problem.

> When does presshell recreation happen exactly?

When an iframe goes to display:none and back, basically.

> When you move tabs between windows for example?

No, we keep the presshell alive in that case, very carefully.

> Or maybe don't store mPresContext on FontFaceSet and always get it from mDocument's
> shell?

I think this is probably the way to go.  We could also maintain it via DeleteShell/DoCreateShell, but that might be a bit more annoying.  In either case we need to make sure the behavior when there is no prescontext (i.e. display:none iframe) makes sense.
(In reply to Boris Zbarsky [:bz] from comment #37)
> > Can you detail the situation you're worried about?
> 
> Is OnFontFaceStatusChanged basically only called off the event loop?  If so,
> then I think you're right and there is no problem.

There is one place where it is called with script on the stack: in Load().  So if you call load() on a FontFace in the FontFaceSet and it is now the only one loading, mReady will be replaced.

This will work:

  var face = new FontFace(...);
  document.fonts.add(face);
  document.fonts.ready.then(...);

This won't:

  var face = new FontFace(...);
  document.fonts.ready.then(...);
  document.fonts.add(face);

But I think that makes sense.

> > Or maybe don't store mPresContext on FontFaceSet and always get it from mDocument's
> > shell?
> 
> I think this is probably the way to go.  We could also maintain it via
> DeleteShell/DoCreateShell, but that might be a bit more annoying.  In either
> case we need to make sure the behavior when there is no prescontext (i.e.
> display:none iframe) makes sense.

OK cool.
> But I think that makes sense.

Yes, agreed.
Changed to use codegen for CSSFontFaceLoadEvent.
Attachment #8448629 - Attachment is obsolete: true
Attachment #8448631 - Attachment is obsolete: true
Attachment #8450663 - Flags: review?(jdaggett)
Changed to store only mDocument on the FontFaceSet, not mPresContext, per comment 37.
Attachment #8448634 - Attachment is obsolete: true
Attachment #8448634 - Flags: review?(jdaggett)
Attachment #8450664 - Flags: review?(jdaggett)
Minor changes to rebase on top of revised earlier patches.
Attachment #8448647 - Attachment is obsolete: true
Attachment #8448647 - Flags: review?(jdaggett)
Attachment #8450665 - Flags: review?(jdaggett)
More minor changes.
Attachment #8448650 - Attachment is obsolete: true
Attachment #8448650 - Flags: review?(jdaggett)
Attachment #8450667 - Flags: review?(jdaggett)
More minor changes.
Attachment #8448652 - Attachment is obsolete: true
Attachment #8448652 - Flags: review?(jdaggett)
Attachment #8450668 - Flags: review?(jdaggett)
Address remaining review comments from Boris.
Attachment #8449191 - Attachment is obsolete: true
Attachment #8449191 - Flags: review?(jdaggett)
Attachment #8450672 - Flags: review?(jdaggett)
More minor changes.
Attachment #8448654 - Attachment is obsolete: true
Attachment #8448654 - Flags: review?(jdaggett)
Attachment #8450674 - Flags: review?(jdaggett)
Depends on: 1034803
Update for some slightly changed enum types.
Attachment #8448627 - Attachment is obsolete: true
Attachment #8451249 - Flags: review?(jdaggett)
Update for the updated IDL patch.
Attachment #8450663 - Attachment is obsolete: true
Attachment #8450663 - Flags: review?(jdaggett)
Attachment #8451250 - Flags: review?(jdaggett)
* Fix some cycle collection definition problems.
* Don't accept empty string family values in the FontFace constructor.
Attachment #8448641 - Attachment is obsolete: true
Attachment #8448641 - Flags: review?(jdaggett)
Attachment #8451252 - Flags: review?(jdaggett)
Clear gfxProxyFontEntry::mFontFace when its loading is finished, to help the FontFace be released.
Attachment #8448649 - Attachment is obsolete: true
Attachment #8448649 - Flags: review?(jdaggett)
Attachment #8451253 - Flags: review?(jdaggett)
* Make sure we notify the FontFace if an ArrayBuffer-sourced face fails to load.
* Reject ArrayBuffer-sourced faces with a SyntaxError rather than a NetworkError.
Attachment #8450668 - Attachment is obsolete: true
Attachment #8450668 - Flags: review?(jdaggett)
Attachment #8451254 - Flags: review?(jdaggett)
* Change to initializing FontFaceSet.mReady with a resolved Promise:
    http://lists.w3.org/Archives/Public/www-style/2014Jul/0047.html
* Ensure we check to dispatch loaded events after delete() and clear() calls.
* Change to dispatching a loaded event even if the FontFaceSet is empty:
    http://lists.w3.org/Archives/Public/www-style/2014Jul/0062.html
Attachment #8450672 - Attachment is obsolete: true
Attachment #8450672 - Flags: review?(jdaggett)
Attachment #8451256 - Flags: review?(jdaggett)
* Clear mFontFace on remaining gfxProxyFontEntrys when we're destroying the nsUserFontSet.
* Handle non-existent descriptors on @font-face rules a bit better.
Attachment #8450674 - Attachment is obsolete: true
Attachment #8450674 - Flags: review?(jdaggett)
Attachment #8451257 - Flags: review?(jdaggett)
Attached patch Part 23: Tests. (obsolete) — Splinter Review
I just copied one of my fonts from layout/reftests/fonts/ into layout/style/test/.  Not sure if there's anything better to use instead.
Attachment #8451258 - Flags: review?(jdaggett)
Duplicate of this bug: fontloader
Attachment #8448626 - Flags: review?(jdaggett) → review+
relnote-firefox is enough to get our attention for the release notes.
relnote-firefox: --- → ?
Keywords: relnote
Comment on attachment 8451258 [details] [diff] [review]
Part 23: Tests.

Couple comments here. If it's not too much trouble it would be useful to have these tests in testharness.js format, such that we could run this against chrome and submit them for use with the CSS Font Loading API spec test suite. Differences with chrome might indicate parts of the spec that need to be tightened.

It would also be nice to have a reftest or two, just for sanity sake...
Rebased over Promise constructor changes.
Attachment #8451250 - Attachment is obsolete: true
Attachment #8451250 - Flags: review?(jdaggett)
Attachment #8460018 - Flags: review?(jdaggett)
Rebased over Promise constructor changes.
Attachment #8450664 - Attachment is obsolete: true
Attachment #8450664 - Flags: review?(jdaggett)
Attachment #8460019 - Flags: review?(jdaggett)
Rebased over Promise constructor changes.
Attachment #8448635 - Attachment is obsolete: true
Attachment #8448635 - Flags: review?(jdaggett)
Attachment #8460020 - Flags: review?(jdaggett)
Rebased over Promise constructor changes.
Attachment #8451252 - Attachment is obsolete: true
Attachment #8451252 - Flags: review?(jdaggett)
Attachment #8460021 - Flags: review?(jdaggett)
Rebased over Promise constructor changes.
Attachment #8448645 - Attachment is obsolete: true
Attachment #8448645 - Flags: review?(jdaggett)
Attachment #8460022 - Flags: review?(jdaggett)
Rebased over Promise constructor changes.
Attachment #8451256 - Attachment is obsolete: true
Attachment #8451256 - Flags: review?(jdaggett)
Attachment #8460023 - Flags: review?(jdaggett)
Attachment #8460023 - Attachment description: Implement FontFaceSet.{ready,status} and dispatch events. (v5) r=bzbarsky → Part 21: Implement FontFaceSet.{ready,status} and dispatch events. (v5) r=bzbarsky
Rebased over Promise constructor changes.
Attachment #8451257 - Attachment is obsolete: true
Attachment #8451257 - Flags: review?(jdaggett)
Attachment #8460024 - Flags: review?(jdaggett)
As noted in bug 1031206 and other bugs, I don't think the pattern of using "MaybeXXX" for method names is a good one. Be more specific about what's being done. Here I think in most cases you'll end up with XXXorYYY like names.

e.g. MaybeReject, MaybeCreateFontFaceFromSource, MaybeResolve, MaybePendingFontLoads

Part-11-Implement-FontFace-constructor-a.patch:+    mLoaded->MaybeReject(NS_ERROR_DOM_SYNTAX_ERR);
Part-14-Handle-FontFace-objects-when-bui.patch:+  record.mFontEntry = MaybeCreateFontFaceFromSource(fontfamily, aSource);
Part-17-Update-FontFace.status-and-FontF.patch:+  mLoaded->MaybeResolve(this);
Part-21-Implement-FontFaceSet.-ready-sta.patch:+FontFaceSet::MaybePendingFontLoads()
MaybeReject/MaybeResolve are methods on mozilla::dom::Promise, so I'll leave those.  That's where I got my naming idea from. ;)
(In reply to Cameron McCormack (:heycam) from comment #67)
> MaybeReject/MaybeResolve are methods on mozilla::dom::Promise, so I'll leave
> those.  That's where I got my naming idea from. ;)

Argh, ok then.
MaybeReject is called that because it will reject the promise, unless the promise is already settled, in which case it does nothing at all.

So we could call it RejectOrDoNothing, I guess...
Please set again the relnotes flags when the feature has landed.
relnote-firefox: ? → ---
Comment on attachment 8451249 [details] [diff] [review]
Part 2: Add Web IDL interfaces for CSS Font Loading API. (v2)

Review of attachment 8451249 [details] [diff] [review]:
-----------------------------------------------------------------

Feedback below, pushing the review to bz as a DOM peer.

::: dom/webidl/FontFace.webidl
@@ +47,5 @@
> +  readonly attribute FontFaceLoadStatus status;
> +
> +  // Promise<FontFace>
> +  [NewObject]
> +  Promise load();

So this depends on a revision to our WebIDL bindings to support promise type? Probably a good idea to note that in a comment I think.

@@ +50,5 @@
> +  [NewObject]
> +  Promise load();
> +
> +  // Promise<boolean>
> +  readonly attribute Promise loaded;

Here too.

::: dom/webidl/FontFaceSet.webidl
@@ +25,5 @@
> +  // Iterator entries();
> +  // Iterator keys();
> +  // Iterator values();
> +  // void forEach(ForEachCallback cb, optional any thisArg);
> +  // FontFace iterator;

Based on our an IRC discussion with cam, there's some contention as to whether these methods are needed or not. I think we probably want to log a bug to work out the exact set of set-like methods need to be supported once the spec issue is worked out.

www-style discussion:
http://lists.w3.org/Archives/Public/www-style/2014Jun/thread.html#msg476

@@ +55,5 @@
> +// XXX Do not land with this.
> +partial interface FontFaceSet {
> +  getter FontFace (unsigned long index);
> +  readonly attribute unsigned long length;
> +};

Hmm, is this work for a follow-on patch/bug?

::: dom/webidl/FontFaceSource.webidl
@@ +15,5 @@
> +  readonly attribute FontFaceSet fonts;
> +};
> +
> +Document implements FontFaceSource;
> +// WorkerGlobalScope implements FontFaceSource;

Bug for implementing this in WorkerGlobalScope?
Attachment #8451249 - Flags: review?(jdaggett)
Attachment #8451249 - Flags: review?(bzbarsky)
Attachment #8451249 - Flags: feedback+
Attachment #8448640 - Flags: review?(jdaggett) → review+
Comment on attachment 8451249 [details] [diff] [review]
Part 2: Add Web IDL interfaces for CSS Font Loading API. (v2)

> So this depends on a revision to our WebIDL bindings to support promise type?

There's nothing to support functionally: the type annotation on Promise return values is purely advisory and has no impact on the behavior of anything.

We can update the IDL parser to accept it and treat it as identical to just Promise, but it hasn't been a high priority, especially because how the parser treats Promise will need to change once Promises move into SpiderMonkey.

>+  [Pure, Cached, Constant, Frozen] readonly attribute sequence<FontFace> fontfaces;

[Constant] implies [Pure], so drop the [Pure].

There is nothing in the spec to indicate this should be [Frozen].  On the other hand, the spec doesn't actually define the behavior of this getter at all (e.g. the getter IDL, doesn't link to anything)...  Why is the [Frozen] there?

Also, we still need webidl spec updates to allow this IDL at all, right?  Or change the fontface spec to return "object" with manual conversions.  That's a spec issue, not an issue for our code, of course.

>+typedef (ArrayBuffer or ArrayBufferView) BinaryData;

This is annoying.  We already have a CryptoOperationData defined like so:

  typedef (ArrayBufferView or ArrayBuffer) CryptoOperationData;

Can you check whether we end up generating two distinct union types?  If so, we should consider either aligning the two specs in terms of ordering or changing union codegen to sort the union members (though the latter would make it harder to figure out what consumers of the union should use, so I'd prefer to align the two specs).

That's assuming we ever use BinaryData directly.  If it's only used in other unions, then there's no point aligning.

>+// [Exposed=Window,Worker]

That's not what the new Exposed syntax looks like.

>+  // Promise<boolean>
>+  readonly attribute Promise loaded;

The spec is moderately on crack here.  It says:

  loaded, of type Promise, readonly
      This attribute reflects the [[FontStatusPromise]] of the font face. 

But [[FontStatusPromise]] is resolved with a FontFace, not a boolean.

I don't see why we have both load() and loaded on this interface.  Please raise spec issues as needed.

>+  // readonly attribute unsigned long size;

How come commented out?  Are there followup bugs to implement it?  Please file as needed and put the bug numbers in comments here.

>+// XXX Do not land with this.

You mean do not pref on?  This needs a bug number.

>+Document implements FontFaceSource;

This line needs to be in Document.webidl for dep builds to work right.  We really need to make this fail codegen when placed in the wrong file.  :(  Can you please file a bug on that?

r=me with those addressed
Attachment #8451249 - Flags: review?(bzbarsky) → review+
Comment on attachment 8460023 [details] [diff] [review]
Part 21: Implement FontFaceSet.{ready,status} and dispatch events. (v5) r=bzbarsky

I don't think the logic in FontFaceSet::CheckLoadingFinished is quite right. For both the ready promise and the 'loadingdone' event, the logic is *not* simply whether there are no pending loads, it's whether there are no pending loads after all reflows have taken place.  Our current font code automatically loads all fonts in a fontlist but this is neither optimal or desired.

Ex:

  @font-face { font-family: A; ... }
  @font-face { font-family: B; ... }
  @font-face { font-family: C; ... }

  body { font-family: A, B, C; }

For a given character that's not supported by fonts A, B or C, the 'loadingdone' event should fire after font C loads.  Right now our code is automatically loading all three but it really should only be loading A (see bug 754215 and bug 998869 for more discussion). So the logic really needs to be "wait until all reflows have completed, then check load status".

The spec actually notes this:

http://dev.w3.org/csswg/css-font-loading/#font-face-set-ready

Put another way, the check for "all loads complete" can only occur after pending layout reflows have completed. Right now the code is not making that distinction.
Attachment #8460018 - Flags: review?(jdaggett) → review+
(In reply to John Daggett (:jtd) from comment #73)
> So the logic really needs to
> be "wait until all reflows have completed, then check load status".

Are you saying that the HasPendingRestyles() call in FontFaceSet::MaybePendingFontLoads needs to be something like HasPendingReflows() or HasPendingRestylesOrReflows() instead?

Is it possible for there to be no font loads in progress, no restyles pending, but there is a reflow pending, and that reflow could influence whether further font loads get kicked off?
(In reply to John Daggett (:jtd) from comment #73)
> ...
> For a given character that's not supported by fonts A, B or C, the
> 'loadingdone' event should fire after font C loads.  Right now our code is
> automatically loading all three but it really should only be loading A
> ...
How will you know whether a character is supported by font A, B or C without loading them? In other words, in case none of them supports the character you have to load all three (not just A) in sequence and check on each load whether it's supported until you know that.

Sebastian
So I'm not really happy with the division of labor between objects here. I think the design for FontFace/FontFaceSet is getting shoehorned into the existing code structure in ways that aren't really ideal. I think I'd prefer to modify our existing code structure to make FontFaceSet/FontFace more of a natural part of our userfont code.

To begin with, I think we should make the FontFace object to be the handler of all loading behavior. We should change the existing gfxMixedFontFamily/gfxProxyFontEntry to be simply gfxUserFontFamily/gfxUserFontEntry. 

primary roles for different objects:

gfx/thebes
==========

gfxUserFontFamily (was gfxMixedFontFamily)
  - handle the <family,style> ==> <platform font> lookup

gfxUserFontEntry (was gfxProxyFontEntry)
  - container of style descriptors used in font matching process
  - points to underlying FontFace which handles loading

gfxUserFontSet
  - handle <family name> ==> gfxUserFontFamily lookup
  - interaction with OTS
  - interaction with userfont cache

layout/style
============

FontFace
  - implements font loading (previously handled by gfxProxyFontEntry)
  - interactions with script such as explicit loading
  - promise fulfillment upon load/error
  - derived class for CSS-connected (@font-face) fonts

FontFaceSet
  - subclass of gfxUserFontSet (merge in nsUserFontSet methods)
  - handle interactions with CSS @font-face rules
  - interactions with script

nsFontFaceLoader
  - font data stream handler

Rather than use 'FontFaceDataSource' objects, let's make the CSS-connected FontFace objects be a derived class of FontFace (or vice versa) so that generic loading code doesn't need to know about the distinction between the two flavors of fonts (script created vs. @font-face rule created).

I think this structure will eliminate some of the oddities in the existing patches, like the "pre-created font entry" concept in FontFace. nsPresContext objects would construct FontFaceSet's rather than gfxUserFontSet objects. Not sure if that helps use eliminate some of the use of deferred runnables but I think it might.

I think I would suggest first trying to morph existing code into a form that supported existing @font-face rule handling, then add in the ability to create fonts from script with or without a FontFaceSet.
(In reply to Sebastian Zartner from comment #75)
> (In reply to John Daggett (:jtd) from comment #73)
> > ...
> > For a given character that's not supported by fonts A, B or C, the
> > 'loadingdone' event should fire after font C loads.  Right now our code is
> > automatically loading all three but it really should only be loading A
> > ...
> How will you know whether a character is supported by font A, B or C without
> loading them? In other words, in case none of them supports the character
> you have to load all three (not just A) in sequence and check on each load
> whether it's supported until you know that.

Right you need to load each, then check the cmap of the loaded font. We currently always load all three fonts, whether or not they are needed. We plan to fix that such that if only A is needed, then B and C will never be loaded. So the key metric to determine whether to fire 'loadingdone' or not is not just "all font loads completed" but "all font loads completed after reflow completes".
(In reply to Cameron McCormack (:heycam) from comment #74)
> (In reply to John Daggett (:jtd) from comment #73)
> > So the logic really needs to
> > be "wait until all reflows have completed, then check load status".
> 
> Are you saying that the HasPendingRestyles() call in
> FontFaceSet::MaybePendingFontLoads needs to be something like
> HasPendingReflows() or HasPendingRestylesOrReflows() instead?
> 
> Is it possible for there to be no font loads in progress, no restyles
> pending, but there is a reflow pending, and that reflow could influence
> whether further font loads get kicked off?

I think that's correct, yes. The content can dictate which fonts need to be loaded, not just the styles.
Just a thought but one thing that might make sense here is to spin off the core FontFace/FontFaceSet integration into a separate infrastructure bug. Land that, then use this bug to integrate the JS/promises interfaces.
Depends on: 1062058
(In reply to John Daggett (:jtd) from comment #76)
> FontFace
>   - implements font loading (previously handled by gfxProxyFontEntry)
>   - interactions with script such as explicit loading
>   - promise fulfillment upon load/error
>   - derived class for CSS-connected (@font-face) fonts

The spec allows FontFace objects to go from being CSS-connected to non-CSS-connected, if you hold a reference to a CSS-connected FontFace object and then remove the @font-face rule from the style sheet.  So for that reason I think I'm going to have have a single class represent connected and unconnected FontFaces.
I had planned to make FontFace a derived class of gfxUserFontEntry, but gfxUserFontEntry is refcounted (through gfxFontEntry) while FontFace needs to be cycle collected (due to it being a DOM object).  So I think I'll need to go back to having a separate FontFace object wrapping gfxUserFontEntry objects, unfortunately.
Depends on: 1069065
Attachment #8448633 - Flags: review?(jdaggett)
Attachment #8448636 - Flags: review?(jdaggett)
Attachment #8448639 - Flags: review?(jdaggett)
Attachment #8448642 - Flags: review?(jdaggett)
Attachment #8448643 - Flags: review?(jdaggett)
Attachment #8448644 - Flags: review?(jdaggett)
Attachment #8448651 - Flags: review?(jdaggett)
Attachment #8450665 - Flags: review?(jdaggett)
Attachment #8450667 - Flags: review?(jdaggett)
Attachment #8451253 - Flags: review?(jdaggett)
Attachment #8451254 - Flags: review?(jdaggett)
Attachment #8451258 - Flags: review?(jdaggett)
Attachment #8460019 - Flags: review?(jdaggett)
Attachment #8460020 - Flags: review?(jdaggett)
Attachment #8460021 - Flags: review?(jdaggett)
Attachment #8460022 - Flags: review?(jdaggett)
Attachment #8460023 - Flags: review?(jdaggett)
Attachment #8460024 - Flags: review?(jdaggett)
Bulk clearing review flags until the revised set of patches.
Depends on: 998869
(In reply to Cameron McCormack (:heycam) from comment #80)
> (In reply to John Daggett (:jtd) from comment #76)
> > FontFace
> >   - implements font loading (previously handled by gfxProxyFontEntry)
> >   - interactions with script such as explicit loading
> >   - promise fulfillment upon load/error
> >   - derived class for CSS-connected (@font-face) fonts
> 
> The spec allows FontFace objects to go from being CSS-connected to
> non-CSS-connected, if you hold a reference to a CSS-connected FontFace
> object and then remove the @font-face rule from the style sheet.  So for
> that reason I think I'm going to have have a single class represent
> connected and unconnected FontFaces.

Hmmm, I really dislike all the complicated edge cases the spec allows. So the implication would be that holding a reference to a FontFace, then deleting the @font-face rule, and adding it back into the FontFaceSet would make it unconnected?
(In reply to Cameron McCormack (:heycam) from comment #81)
> I had planned to make FontFace a derived class of gfxUserFontEntry, but
> gfxUserFontEntry is refcounted (through gfxFontEntry) while FontFace needs
> to be cycle collected (due to it being a DOM object).  So I think I'll need
> to go back to having a separate FontFace object wrapping gfxUserFontEntry
> objects, unfortunately.

Mmmm, yeah. The one thing that I think is important is to push all loading mechanisms into gfxUserFontEntry and gfxUserFontSet and reserve FontFace functionality to dealing with the script interactions. Sounds like we'll need to have FontFace observe gfxUserFontEntry load state changes.
(In reply to John Daggett (:jtd) from comment #83)
> Hmmm, I really dislike all the complicated edge cases the spec allows. So
> the implication would be that holding a reference to a FontFace, then
> deleting the @font-face rule, and adding it back into the FontFaceSet would
> make it unconnected?

Yes.  Well, it becomes unconnected as soon as the @font-face rule is removed from the style sheet.  This is explicitly called out in the last para of http://dev.w3.org/csswg/css-font-loading/#font-face-css-connection.
(In reply to John Daggett (:jtd) from comment #84)
> Mmmm, yeah. The one thing that I think is important is to push all loading
> mechanisms into gfxUserFontEntry and gfxUserFontSet and reserve FontFace
> functionality to dealing with the script interactions. Sounds like we'll
> need to have FontFace observe gfxUserFontEntry load state changes.

Yesterday I hit upon still extending gfxUserFontEntry with a version that knows about FontFaceSets but which isn't FontFace itself.  Say, FontFace::Entry.  Then we can still make FontFaceSet's "make me an entry" function (which'll be virtual on gfxUserFontSet) create a FontFace::entry, passing it the FontFaceSet it's part of.  Then the state change method can be made virtual, and FontFace::Entry can hook into it there.  How does that sound?
(In reply to Cameron McCormack (:heycam) from comment #86)
> Yesterday I hit upon still extending gfxUserFontEntry with a version that
> knows about FontFaceSets but which isn't FontFace itself.  Say,
> FontFace::Entry.  Then we can still make FontFaceSet's "make me an entry"
> function (which'll be virtual on gfxUserFontSet) create a FontFace::entry,
> passing it the FontFaceSet it's part of.  Then the state change method can
> be made virtual, and FontFace::Entry can hook into it there.  How does that
> sound?

Yeah, that sounds like it might work.
Depends on: 1069761
Depends on: 1070260
Depends on: 1070316
Depends on: 1071490
No longer depends on: 1071490
Blocks: 1072101
Blocks: 1072102
Blocks: 1072107
(In reply to Boris Zbarsky [:bz] from comment #72)
> Comment on attachment 8451249 [details] [diff] [review]
> Part 2: Add Web IDL interfaces for CSS Font Loading API. (v2)
> 
> > So this depends on a revision to our WebIDL bindings to support promise type?
> 
> There's nothing to support functionally: the type annotation on Promise
> return values is purely advisory and has no impact on the behavior of
> anything.
> 
> We can update the IDL parser to accept it and treat it as identical to just
> Promise, but it hasn't been a high priority, especially because how the
> parser treats Promise will need to change once Promises move into
> SpiderMonkey.

This got implemented in the intervening months.

> >+  [Pure, Cached, Constant, Frozen] readonly attribute sequence<FontFace> fontfaces;
> 
> [Constant] implies [Pure], so drop the [Pure].
> 
> There is nothing in the spec to indicate this should be [Frozen].  On the
> other hand, the spec doesn't actually define the behavior of this getter at
> all (e.g. the getter IDL, doesn't link to anything)...  Why is the [Frozen]
> there?

I've dropped it now.

> Also, we still need webidl spec updates to allow this IDL at all, right?  Or
> change the fontface spec to return "object" with manual conversions.  That's
> a spec issue, not an issue for our code, of course.

Yep.

> >+typedef (ArrayBuffer or ArrayBufferView) BinaryData;
> 
> This is annoying.  We already have a CryptoOperationData defined like so:
> 
>   typedef (ArrayBufferView or ArrayBuffer) CryptoOperationData;
> 
> Can you check whether we end up generating two distinct union types?  If so,
> we should consider either aligning the two specs in terms of ordering or
> changing union codegen to sort the union members (though the latter would
> make it harder to figure out what consumers of the union should use, so I'd
> prefer to align the two specs).
> 
> That's assuming we ever use BinaryData directly.  If it's only used in other
> unions, then there's no point aligning.

We are only using it in another union.  Looks like the Crypto API spec has switched the order around to match the Font Loading API.  But it would be good to have a common definition that other specs can use rather than duplicating the union, so I've added that here:

  http://heycam.github.io/webidl/#common-ArrayBufferData

> >+// [Exposed=Window,Worker]
> 
> That's not what the new Exposed syntax looks like.

Fixed.

> >+  // Promise<boolean>
> >+  readonly attribute Promise loaded;
> 
> The spec is moderately on crack here.  It says:
> 
>   loaded, of type Promise, readonly
>       This attribute reflects the [[FontStatusPromise]] of the font face. 
> 
> But [[FontStatusPromise]] is resolved with a FontFace, not a boolean.

Spec got fixed.

> >+  // readonly attribute unsigned long size;
> 
> How come commented out?  Are there followup bugs to implement it?  Please
> file as needed and put the bug numbers in comments here.

Added followup bug numbers.

> >+// XXX Do not land with this.
> 
> You mean do not pref on?  This needs a bug number.

Added.

> >+Document implements FontFaceSource;
> 
> This line needs to be in Document.webidl for dep builds to work right.  We
> really need to make this fail codegen when placed in the wrong file.  :( 
> Can you please file a bug on that?

Oh yeah I forgot you already told me about this. :)
Comment on attachment 8448640 [details] [diff] [review]
Part 10: Add public nsCSSParser function for parsing @font-face descriptors.

I migrated this patch over to bug 1070260 and landed it there.
Attachment #8448640 - Attachment is obsolete: true
I'm about to obsolete all the patches and upload new ones.  The patch numbering no longer matches up after the "combined part 3 & 4" part, fyi, so I'm not going to try to keep them or the patch version numbers in sync.
Attachment #8448626 - Attachment is obsolete: true
Attachment #8451249 - Attachment is obsolete: true
Attachment #8460018 - Attachment is obsolete: true
Attachment #8448633 - Attachment is obsolete: true
Attachment #8460019 - Attachment is obsolete: true
Attachment #8460020 - Attachment is obsolete: true
Attachment #8448636 - Attachment is obsolete: true
Attachment #8448639 - Attachment is obsolete: true
Attachment #8460021 - Attachment is obsolete: true
Attachment #8448642 - Attachment is obsolete: true
Attachment #8448643 - Attachment is obsolete: true
Attachment #8448644 - Attachment is obsolete: true
Attachment #8460022 - Attachment is obsolete: true
Attachment #8450665 - Attachment is obsolete: true
Attachment #8451253 - Attachment is obsolete: true
Attachment #8450667 - Attachment is obsolete: true
Attachment #8448651 - Attachment is obsolete: true
Attachment #8451254 - Attachment is obsolete: true
Attachment #8460023 - Attachment is obsolete: true
Attachment #8460024 - Attachment is obsolete: true
Attachment #8451258 - Attachment is obsolete: true
These new patches are based on top of the bug 998869 patch queue that Jonathan Kew pushed to try and came up green.
(Carrying over r=jdaggett.)

We start with this preffed off even in Nightly builds since we'll need
to support iterators on FontFaceSet objects (and remove the indexed
getter) before exposing it to authors.
Re-requesting review since I've made changes to test_interfaces.html that should be checked.
Attachment #8494312 - Flags: review?(bzbarsky)
This is the first of three patches -- parts 4.1, 4.2 and 4.3 -- that I intend to squash and land together, but which I've separated out for ease of review.

This first part moves the ownership of the gfxUserFontSet into FontFaceSet.
Attachment #8494316 - Flags: review?(jdaggett)
This second part simply moves the definitions of nsUserFontSet from nsFontFaceLoader.{h,cpp} into FontFaceSet.{h,cpp} without making any changes to the code.  This won't compile.
Attachment #8494318 - Flags: review?(jdaggett)
This third part moves nearly all of the functionality of nsUserFontSet into FontFaceSet.  As mentioned in a comment above, we can't make FontFaceSet inherit from gfxUserFontSet directly, so the next best thing I thought was to have a small class that inherits from gfxUserFontSet that overrides some virtual methods and forwards things on to the FontFaceSet.
Attachment #8494320 - Flags: review?(jdaggett)
Attachment #8494324 - Flags: review?(bzbarsky)
We're in a similar situation with gfxUserFontEntry -- we can't make FontFace inherit from it.  So instead, we have a small class that inherits from it that will override its virtual methods and forward things on to the FontFace object.

We make gfxUserFontSet::CreateFontFace virtual so we can override it to produce instances of our gfxUserFontEntry subclass.
Attachment #8494328 - Flags: review?(jdaggett)
Every nsCSSFontFaceRule that is used will get a FontFace object to reflect it in the FontFaceSet.  Here we add storage on the nsCSSFontFaceRule for its FontFace, although it will be the FontFaceSet's responsibility to set it.  We also add a static constructor function on FontFace to create one that reflects a rule.
Attachment #8494329 - Flags: review?(jdaggett)
We make gfxUserFontEntry::SetLoadState virtual so that we can hook into changes and update FontFace::mStatus.  We can't just reflect the gfxUserFontEntry's value in FontFace::Status() since the spec has requirements about when exactly the status is set.
Attachment #8494334 - Flags: review?(jdaggett)
Attachment #8494334 - Attachment description: 0015-Bug-1028497-Part-9-Implement-FontFace.status.patch → Part 9: Implement FontFace.status.
We add an mDescriptors that will be used to store the descriptor values of an unconnected FontFace object.  For CSS-connected FontFace objects, the descriptors are stored in the nsCSSFontFaceRule::mDecl.mDescriptors.  (Both of these use the same type, though, CSSFontFaceDescriptors.)

I did want to try having the descriptors always stored on the FontFace, and for the nsCSSFontFaceStyleDecl go and fetch them from the FontFace, but it turned out to be impossible to ensure that a FontFace could be created whenever a nsCSSFontFaceStyleDecl was cloned, since it needs access to the window object to create the FontFace's promise objects!  Oh well.
Attachment #8494338 - Flags: review?(jdaggett)
Blocks: 1072149
These set the descriptors, but don't cause anything to update, just like setting descriptors on an @font-face rule's style declaration doesn't do anything yet.  I've filed bug 1072149 as a followup to do that.
Attachment #8494339 - Flags: review?(jdaggett)
The awkwardness around checking that mLoaded isn't null is because creating a Promise object can fail if the global object is about to go away (I think).  So we null check any use of the Promise objects, and throw an exception from .load() and .loaded if we weren't able to create one.
Attachment #8494341 - Flags: review?(jdaggett)
This makes .load() work for CSS-connected FontFace objects (which at this point in the patch queue are the only type of FontFace objects we really support).
Attachment #8494342 - Flags: review?(jdaggett)
How that we have a class named "FontFace", it's a bit confusing for some of the gfxUserFontSet methods to have "FontFace" in their names, so I'm renaming them to mention "UserFontEntry" instead.
Attachment #8494343 - Flags: review?(jdaggett)
Here we change FontFaceSet's records array to associate gfxUserFontEntry pointers with FontFace pointers, rather than with nsCSSFontFaceRule pointers.  This will make it more uniform to handle both CSS-connected and unconnected FontFace objects when rebuilding the user font entries under UpdateRules.
Attachment #8494344 - Flags: review?(jdaggett)
We can no longer call FontFaceSet::DestroyUserFontSet (what used to be nsUserFontSet::Destroy) in nsPresContext::FlushUserFontSet, since we need to keep tracking FontFace objects even if the list of @font-face rules is empty.
Attachment #8494345 - Flags: review?(jdaggett)
I'm going to want to get the family name like this in two places, so factor it out into a method on FontFace.
Attachment #8494346 - Flags: review?(jdaggett)
This changes InsertRule, which looked at the descriptors on an nsCSSFontFaceRule to create a user font entry and add it to a family, into InsertConnectedFontFace, which can do the same but for the FontFace that reflects the rule.
Attachment #8494347 - Flags: review?(jdaggett)
This adds support for a CSS-connected FontFace to be disconnected from its rule.  This causes it to get its own copy of the descriptors on the nsCSSFontFaceStyleDecl, and for the pointers between the FontFace and the nsCSSFontFaceRule to be nulled out.

We start tracking now whether a given FontFace is in the FontFaceSet (in the sense that it will appear on the DOM FontFaceSet object if we inspect it with script).  All FontFace objects created though, whether they are currently "in" the FontFaceSet or not, are still tracked by the FontFaceSet.  We use the new mUnavailableFaces array on the FontFaceSet for that.

We need to track these FontFaces that aren't in the FontFaceSet as that's where we store their user font entry -- important if we call load() on a FontFace before adding it to the FontFaceSet.
Attachment #8494350 - Flags: review?(jdaggett)
We add a third array on FontFaceSet, mOtherFaces, which stores unconnected FontFace objects that have been added to the FontFaceSet.  We reflect them in the indexed properties and also create user font entries for them.

Part 23 will actually allow us to add some of these FontFaces to mOtherFaces.
Attachment #8494351 - Flags: review?(jdaggett)
This implements the bulk of the FontFace JS constructor, which parses the descriptors passed in.  We need a notion now of whether a FontFace is "initialized", since the spec requires us to go through the event loop before parsing the 'src' descriptor.  So a couple of places now have to check whether the FontFace is fully initialized, and we have a method to inform the FontFaceSet when a FontFace becomes initialized, in case we added it to the FontFaceSet before it was initialized (easy to do with |document.fonts.add(new FontFace(...))|.
Attachment #8494354 - Flags: review?(jdaggett)
This makes Load() work properly on unconnected FontFace objects.  We now give the FontFaceSet the responsibility to call Load() on the user font entry.  So the same mFontFaceSet->LoadFontFace() call works with both CSS-connected and unconnected FontFaces.
Attachment #8494355 - Flags: review?(jdaggett)
Implementing Add, Delete and Clear involves shuffling FontFace objects between mOtherFaces and mUnavailableFaces.
Attachment #8494358 - Flags: review?(jdaggett)
Boris, you looked at a previous incarnation of this patch for the style flushing noticing parts and the CSSLoader observer and DOMContentLoaded listener, but I'm re-requesting review as I've added layout flushing noticing too.  We need to ensure FontFaceSet::mReady isn't resolved until there are no more style or layout flushes pending (as well as checking that the FontFace objects involved have finished loading).  I'm not sure I've inserted my code into the best places -- I'm exposing nsIPresShell::mReflowScheduled to see if there is a reflow pending, and inserting a notification call into nsRefreshDriver::Tick, where we are setting nsIPresShell::mReflowScheduled to false.  Please let me know if there's a better way or place to do this.

John, you can review the rest. :)  I'm tracking the number of FontFaces we have in mConnectedFaces and mOtherFaces by their load status (unloaded, loading or loaded/error) and using that to determine when to dispatch the CSSFontFaceLoad events.
Attachment #8494362 - Flags: review?(jdaggett)
Attachment #8494362 - Flags: review?(bzbarsky)
This implements support for passing an ArrayBuffer or ArrayBufferView as the source to the FontFace JS constructor.
Attachment #8494363 - Flags: review?(jdaggett)
Attached patch Part 26: Tests. (obsolete) — Splinter Review
Let me know if you have any better idea than copying a random small-ish font into layout/style/test/ to use here.
Attachment #8494364 - Flags: review?(jdaggett)
Comment on attachment 8494312 [details] [diff] [review]
Part 2: Add Web IDL interfaces for CSS Font Loading API. (v3)

>+    {name: "CSSFontFaceRule", pref: "layout.css.font-loading-api.enabled"},

The pref bit should be on CSSFontFaceLoadEvent, not on CSSFontFaceRule, right?

>+// [Exposed=(Window,Worker)]

This comment should have a bug number for the bug to expose this in workers, right?

>+// [Constructor(sequence<FontFace> initialFaces)]

Possibly mention the relevant bug here too?

>+// get iterators working (bug 1072101).  Don't enable the pref for the CSS 

How about putting this comment next to the pref too, if it's not there already?

Or adding a test that will fail if the pref is enabled by default but iteration is not supported, if the pref is not enabled by default in the test profile?

>+[NoInterfaceObject, Pref="layout.css.font-loading-api.enabled"]
>+interface FontFaceSource {

This will fail codegen on tip; it doesn't make sense to conditionally expose something that's NoInterfaceObject already.  Drop one of those two annotations, depending on the behavior you actually want?

If you want to conditionally expose the _attribute_, the Pref annotation needs to go on the attribute.

r=me with those fixes.
Attachment #8494312 - Flags: review?(bzbarsky) → review+
Comment on attachment 8494324 [details] [diff] [review]
Part 5: Implement document.fonts.

r=me, though we should think a bit about how this should behave in display:none iframes or data documents.  What does the spec says to do in those cases?  Followup bug fine for that.
Attachment #8494324 - Flags: review?(bzbarsky) → review+
> creating a Promise object can fail if the global object is about to go away (I think)

It can only really fail on OOM in the JS engine as far as I can tell.

It would not be completely insane to just abort on failure there if desired.
Comment on attachment 8494354 [details] [diff] [review]
Part 21: Implement the FontFace constructor's parsing of descriptors.

What keeps mSourceBuffer alive until the runnable runs?

That should perhaps be a PersistentRooted, not a Heap, right?
Comment on attachment 8494363 [details] [diff] [review]
Part 25: Support loading of fonts from ArrayBuffer{,View}s.

>+  AutoSafeJSContext cx;

This is deprecated.  If you just need this for the Rooted, use the runtime instead?  That will also happen to work well off the main thread if you do it like so: CycleCollectedJSRuntime::Get()->Runtime().

If there are other patches in this queue in addition to this one and part 21 that involve JS value or jsapi, please ask me for review on those, ok?
Blocks: 1072762
(In reply to Boris Zbarsky [:bz] from comment #121)
> Comment on attachment 8494312 [details] [diff] [review]
> Part 2: Add Web IDL interfaces for CSS Font Loading API. (v3)
> 
> >+    {name: "CSSFontFaceRule", pref: "layout.css.font-loading-api.enabled"},
> 
> The pref bit should be on CSSFontFaceLoadEvent, not on CSSFontFaceRule,
> right?

Oops, yep, copy-paste fail.

> >+// [Exposed=(Window,Worker)]
> 
> This comment should have a bug number for the bug to expose this in workers,
> right?

Added.

> >+// [Constructor(sequence<FontFace> initialFaces)]
> 
> Possibly mention the relevant bug here too?

OK.

> >+// get iterators working (bug 1072101).  Don't enable the pref for the CSS 
> 
> How about putting this comment next to the pref too, if it's not there
> already?
> 
> Or adding a test that will fail if the pref is enabled by default but
> iteration is not supported, if the pref is not enabled by default in the
> test profile?

I've put the comment next to the pref, since I am turning the pref on in the test profile.

> >+[NoInterfaceObject, Pref="layout.css.font-loading-api.enabled"]
> >+interface FontFaceSource {
> 
> This will fail codegen on tip; it doesn't make sense to conditionally expose
> something that's NoInterfaceObject already.  Drop one of those two
> annotations, depending on the behavior you actually want?
> 
> If you want to conditionally expose the _attribute_, the Pref annotation
> needs to go on the attribute.

Ah yep, that's what I want.
Blocks: 1072767
(In reply to Boris Zbarsky [:bz] from comment #122)
> r=me, though we should think a bit about how this should behave in
> display:none iframes or data documents.  What does the spec says to do in
> those cases?  Followup bug fine for that.

Filed bug 1072767.  What do you mean by "data documents"?
Blocks: 1072770
(In reply to Boris Zbarsky [:bz] from comment #124)
> Comment on attachment 8494354 [details] [diff] [review]
> Part 21: Implement the FontFace constructor's parsing of descriptors.
> 
> What keeps mSourceBuffer alive until the runnable runs?
> 
> That should perhaps be a PersistentRooted, not a Heap, right?

Oh.  So I tried to get this right by reading https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/GC_Rooting_Guide but that doesn't seem to mention PersistentRooted.  Maybe I misunderstood and thought Heap<> would be sufficient to keep the object alive.  Is replacing Heap with PersistentRooted all that I need?
(In reply to Cameron McCormack (:heycam) from comment #128)
> Is replacing Heap with PersistentRooted all that I need?

Ah I need to pass in the runtime too.
(In reply to Boris Zbarsky [:bz] from comment #125)
> If there are other patches in this queue in addition to this one and part 21
> that involve JS value or jsapi, please ask me for review on those, ok?

It's just those two.  I'll put new versions up in a minute.
Is this right?  And is the reason we use PersistentRooted that we're storing a pointer to the ArrayBuffer on a non-cycle collected object (the runnable), whereas with mData in part 21 we're on FontFace so we use Heap?
Attachment #8495058 - Flags: review?(jdaggett)
Attachment #8495058 - Flags: review?(bzbarsky)
Attachment #8494354 - Attachment is obsolete: true
Attachment #8494363 - Attachment is obsolete: true
Attachment #8494354 - Flags: review?(jdaggett)
Attachment #8494363 - Flags: review?(jdaggett)
Attachment #8495060 - Flags: review?(jdaggett)
Attachment #8495060 - Flags: review?(bzbarsky)
Attachment #8495060 - Attachment description: 0031-Bug-1028497-Part-25-Support-loading-of-fonts-from-Ar.patch → Part 25: Support loading of fonts from ArrayBuffer{,View}s. (v2)
Comment on attachment 8494316 [details] [diff] [review]
Part 4.1: Move the nsUserFontSet object into FontFaceSet.

Review of attachment 8494316 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with fixup of associated member variable.

::: layout/base/nsPresContext.cpp
@@ -241,1 @@
>    mUserFontSetDirty = true;

Shouldn't the xxxDirty member variable change to?
Attachment #8494316 - Flags: review?(jdaggett) → review+
Attachment #8494318 - Flags: review?(jdaggett) → review+
(In reply to John Daggett (:jtd) from comment #133)
> Shouldn't the xxxDirty member variable change to?

Rename it to mFontFaceSetDirty you mean?  Or move it into FontFaceSet (and then just call it mDirty)?
Comment on attachment 8494320 [details] [diff] [review]
Part 4.3: Move nsUserFontSet functionality into FontFaceSet, and have a FontFaceSet::UserFontSet class extending gfxUserFontSet just to override virtual methods.

Review of attachment 8494320 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with minor cleanup

::: layout/style/FontFaceSet.cpp
@@ +35,4 @@
>  using namespace mozilla::dom;
>  
> +#ifdef PR_LOGGING
> +static PRLogModuleInfo* 

trailing whitespace (probably my fault)
Attachment #8494320 - Flags: review?(jdaggett) → review+
Attachment #8494328 - Flags: review?(jdaggett) → review+
Comment on attachment 8495058 [details] [diff] [review]
Part 21: Implement the FontFace constructor's parsing of descriptors. (v2)

r=me on the JS bits here.
Attachment #8495058 - Flags: review?(bzbarsky) → review+
(In reply to Cameron McCormack (:heycam) from comment #134)

> Rename it to mFontFaceSetDirty you mean?  Or move it into FontFaceSet (and
> then just call it mDirty)?

Either is fine, your call.
Comment on attachment 8495060 [details] [diff] [review]
Part 25: Support loading of fonts from ArrayBuffer{,View}s. (v2)

>+  T buf;

This _might_ need to be a RootedTypedArray<T>, if buf.Init() or ComputeLenghtAndData() can GC.  Please do a try run to make sure the gc static analysis test is green with this patch.

Past that, the only thing to make sure of is that the pointer you get out of GetData() is not used across possible JS execution, because if JS runs that pointer can become invalid.  It might be simplest to copy the data if we're not sure we can guarantee that invariant...

r=me modulo those issues.
Attachment #8495060 - Flags: review?(bzbarsky) → review+
> What do you mean by "data documents"?

Documents that can't have a presentation because they're not loaded in a browsing context.  For example, the .responseXML of an XHR, or a document created via DOMImplementation.createDocument.

Typically there are no subresource loads from such documents, so maybe this is all well-defined already in the spec, but worth checking.

> So I tried to get this right by reading ...

Ah, I see.  The important part is kinda incospicuous: "Heap pointers must also continue to be traced in the normal way, which is not covered here."

I'll see about making the docs clearer, but the short summary is that a Heap<T> needs to be traced.

For your case, where there can't be any cycles through the object because it's a transient runnable, there's no need to do the tracing approach (which plays nice with cycle collection).  You can just make your member be a GC root instead.  So I think PersistentRooted would work fine here.
Oh, and if you do end up needing RootedTypedArray here, you'll want to just use an AutoJSAPI here, and file a followup bug to make it possible to root those with a JSRuntime, I guess.
Blocks: 1073332
Comment on attachment 8494329 [details] [diff] [review]
Part 7: Add ability to create a FontFace for a @font-face rule; store it on the nsCSSFontFaceRule.

r+ but boris should review the cycle collection stuff
Attachment #8494329 - Flags: review?(bzbarsky)
Attachment #8494329 - Flags: review?(jdaggett) → review+
(In reply to Boris Zbarsky [:bz] from comment #138)
> Comment on attachment 8495060 [details] [diff] [review]
> Part 25: Support loading of fonts from ArrayBuffer{,View}s. (v2)
> 
> >+  T buf;
> 
> This _might_ need to be a RootedTypedArray<T>, if buf.Init() or
> ComputeLenghtAndData() can GC.  Please do a try run to make sure the gc
> static analysis test is green with this patch.

The "H" build on try turned out green.  I assume that's the one you mean.

> Past that, the only thing to make sure of is that the pointer you get out of
> GetData() is not used across possible JS execution, because if JS runs that
> pointer can become invalid.  It might be simplest to copy the data if we're
> not sure we can guarantee that invariant...

Oh, so we do go around the event loop with a NS_DispatchToMainThread and grab the data out of the ArrayBuffer object then.  I was hoping to avoid the need for making a copy of the data, but I'll just do that.
Comment on attachment 8494330 [details] [diff] [review]
Part 8: Implement Length and array access on FontFaceSet.

Review of attachment 8494330 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/FontFaceSet.cpp
@@ +159,5 @@
>  FontFaceSet::IndexedGetter(uint32_t aIndex, bool& aFound)
>  {
> +  // XXX Not sure if this is needed.
> +  // mPresContext->FlushUserFontSet();
> +

Hmmm, good question. I think Jonathan probably has a better understanding of this.
Attachment #8494330 - Flags: review?(jdaggett) → review?(jfkthame)
Comment on attachment 8494329 [details] [diff] [review]
Part 7: Add ability to create a FontFace for a @font-face rule; store it on the nsCSSFontFaceRule.

r=me on the CC bits, but maybe add comments in the nsFontFaceRuleContainer and FontFaceRuleRecord about how if they ever end up with strong references to any CCed objects those references will need to be added to the CC impl of FontFaceSet?

I assume that gfxUserFontEntry is not CCed; might be worth adding a comment to that effect as well.
Attachment #8494329 - Flags: review?(bzbarsky) → review+
> The "H" build on try turned out green.  I assume that's the one you mean.

Yep.

> and grab the data out of the ArrayBuffer object then.

That part is fine, assuming you mean the runnable with the PersistentRooted<JSObject*> member.

The important part is that no JS run between when you pull the uin8_t* out of the JSObject and when you stop looking at the memory that uint8_t* points to.
(In reply to Boris Zbarsky [:bz] from comment #145)
> That part is fine, assuming you mean the runnable with the
> PersistentRooted<JSObject*> member.
> 
> The important part is that no JS run between when you pull the uin8_t* out
> of the JSObject and when you stop looking at the memory that uint8_t* points
> to.

Ah OK.  That should be fine then -- after the call to FontFace::GetData in gfxUserFontEntry::LoadNextSrc, which extracts the buffer point out of the ArrayBuffer, the data is immediately parsed and the buffer pointer isn't used after that.
(In reply to Cameron McCormack (:heycam) from comment #146)
> (In reply to Boris Zbarsky [:bz] from comment #145)
> > That part is fine, assuming you mean the runnable with the
> > PersistentRooted<JSObject*> member.
> > 
> > The important part is that no JS run between when you pull the uin8_t* out
> > of the JSObject and when you stop looking at the memory that uint8_t* points
> > to.
> 
> Ah OK.  That should be fine then -- after the call to FontFace::GetData in
> gfxUserFontEntry::LoadNextSrc, which extracts the buffer point out of the
> ArrayBuffer, the data is immediately parsed and the buffer pointer isn't
> used after that.

I think we definitely need to carefully comment in the code about lifetimes like this.
Attachment #8494334 - Flags: review?(jdaggett) → review+
Comment on attachment 8494338 [details] [diff] [review]
Part 10: Implement FontFace descriptor getters.

Review of attachment 8494338 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with added assertions

::: layout/style/FontFace.cpp
@@ +254,5 @@
> +      aResult.AssignLiteral("normal");
> +    }
> +  } else {
> +    value.AppendToString(aPropID, aResult, nsCSSValue::eNormalized);
> +  }

Seems like we should assert here that family/src are not null.
Attachment #8494338 - Flags: review?(jdaggett) → review+
Attachment #8494339 - Flags: review?(jdaggett) → review+
(In reply to John Daggett (:jtd) from comment #148)
> Seems like we should assert here that family/src are not null.

I think they can validly be null if you're getting the value if say you have

  @font-face {
    font-family: X;
    src: rubbish;
  }

and you try to look up the src descriptor.  In that case the method here will return the empty string.
Comment on attachment 8494346 [details] [diff] [review]
Part 17: Factor out FontFace family-name getting.

Review of attachment 8494346 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comment updated

::: layout/style/FontFace.h
@@ +74,5 @@
> +  /**
> +   * Gets the family name of the FontFace as a plain string (as opposed to
> +   * GetFamily, which returns a CSS string).  Returns whether a valid family
> +   * name was available.
> +   */

I think the distinction is probably "raw string" vs. "CSS escaped string", or something like that. I think just "CSS string" is sort of vague.
Attachment #8494346 - Flags: review?(jdaggett) → review+
> the data is immediately parsed

And this process doesn't involve getting possibly-implemented-in-JS services or anything like that?
Comment on attachment 8494341 [details] [diff] [review]
Part 12: Implement FontFace.loaded.

Review of attachment 8494341 [details] [diff] [review]:
-----------------------------------------------------------------

r? to boris for the WebIDL stuff

::: layout/style/FontFace.cpp
@@ +83,3 @@
>    nsRefPtr<FontFace> obj = new FontFace(aGlobal, aPresContext);
>    obj->mRule = aRule;
> +  obj->SetStatus(LoadStateToStatus(aUserFontEntry->LoadState()));

This needs to be null-checked but looks like you do that in a later patch. ;)

::: layout/style/FontFace.h
@@ +91,5 @@
>    void SetFeatureSettings(const nsAString& aValue, mozilla::ErrorResult& aRv);
>  
>    mozilla::dom::FontFaceLoadStatus Status();
> +  mozilla::dom::Promise* Load(mozilla::ErrorResult& aRv);
> +  mozilla::dom::Promise* GetLoaded(mozilla::ErrorResult& aRv);

My nominee for Most Unfortunate Method Name of 2014...
Attachment #8494341 - Flags: review?(jdaggett)
Attachment #8494341 - Flags: review?(bzbarsky)
Attachment #8494341 - Flags: review+
(In reply to Boris Zbarsky [:bz] from comment #151)
> And this process doesn't involve getting possibly-implemented-in-JS services
> or anything like that?

Can PRLog stuff can be backed by JS-implemented objects?  https://hg.mozilla.org/try/file/a07001d1718b/gfx/thebes/gfxUserFontSet.cpp#l542 is the only function that does something with the data:

* gfxFontUtils::DetermineFontDataType just inspects the bytes
* SanitizeOpenTypeData is self contained
* gfxFontUtils::GetFullNameFromSFNT just calls into harfbuzz
* gfxPlatform::GetPlatform()->MakePlatformFont I haven't checked all of the backends to see if they don't use any services

Do you think it's just safer to make a copy of the data here?  I'm feeling like we could accidentally break things even if it's OK at the moment.
Comment on attachment 8494341 [details] [diff] [review]
Part 12: Implement FontFace.loaded.

>   Promise<FontFace> load();
>   readonly attribute Promise<boolean> loaded;

But as implemented, it's the same Promise, right?  So how can it be resolved with both a boolean and a FontFace, exactly?

I looks like it's actually resolved with the FontFace, right?  Assuming MaybeResolve doesn't end up treating it as a boolean, but I think we fixed that bug...

In any case, either the IDL or the implementation is wrong here.
Attachment #8494341 - Flags: review?(bzbarsky) → review-
> Can PRLog stuff can be backed by JS-implemented objects? 

No.

> Do you think it's just safer to make a copy of the data here?

Definitely.  If it's not too prohibitive performance-wise, I think we should just do that...
(In reply to Boris Zbarsky [:bz] from comment #154)
> Comment on attachment 8494341 [details] [diff] [review]
> Part 12: Implement FontFace.loaded.
> 
> >   Promise<FontFace> load();
> >   readonly attribute Promise<boolean> loaded;
> 
> But as implemented, it's the same Promise, right?  So how can it be resolved
> with both a boolean and a FontFace, exactly?
> 
> I looks like it's actually resolved with the FontFace, right?  Assuming
> MaybeResolve doesn't end up treating it as a boolean, but I think we fixed
> that bug...

Gosh, I thought I fixed that earlier.  Sorry, it should be Promise<FontFace>.  I guess there's no easy way to statically assert that my C++ functions implement the Web IDL stuff use the right parameter for the Promise objects...
Comment on attachment 8494342 [details] [diff] [review]
Part 13: Implement FontFace.load().

Review of attachment 8494342 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/FontFace.cpp
@@ +241,5 @@
> +  gfxUserFontEntry* entry =
> +    mPresContext->Fonts()->FindUserFontEntryForFontFace(this);
> +  if (entry) {
> +    entry->Load();
> +  }

So I don't quite know all of what is needed for FontFaceSet but it seems like a FontFace should have a reference to the FontFace::Entry. That would obviate the need for global lookup methods like this.
(In reply to Cameron McCormack (:heycam) (away October) from comment #156)
> > I looks like it's actually resolved with the FontFace, right?  Assuming
> > MaybeResolve doesn't end up treating it as a boolean, but I think we fixed
> > that bug...
> 
> Gosh, I thought I fixed that earlier.  Sorry, it should be
> Promise<FontFace>.  I guess there's no easy way to statically assert that my
> C++ functions implement the Web IDL stuff use the right parameter for the
> Promise objects...

Of course, mozilla::dom::Promise isn't a templated type anyway, so there's no information there to assert on.
Attachment #8494343 - Flags: review?(jdaggett) → review+
Attachment #8494345 - Flags: review?(jdaggett) → review?(jfkthame)
Comment on attachment 8494341 [details] [diff] [review]
Part 12: Implement FontFace.loaded.

> I guess there's no easy way to statically assert that my C++ functions
> implement the Web IDL stuff use the right parameter for the Promise objects

Not right now, yeah.  :(

r=me if the IDL is changed to say Promise<FontFace> for .loaded.
Attachment #8494341 - Flags: review- → review+
I updated my part 2 patch locally to fix the IDL to use Promise<FontFace>.  So this part is unchanged except for its context.
Attachment #8494341 - Attachment is obsolete: true
Attachment #8495695 - Flags: review?(bzbarsky)
Attachment #8495695 - Attachment is obsolete: true
Attachment #8495695 - Flags: review?(bzbarsky)
Comment on attachment 8494341 [details] [diff] [review]
Part 12: Implement FontFace.loaded.

Thanks.
Attachment #8494341 - Attachment is obsolete: false
Comment on attachment 8494330 [details] [diff] [review]
Part 8: Implement Length and array access on FontFaceSet.

Review of attachment 8494330 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/FontFaceSet.cpp
@@ +159,5 @@
>  FontFaceSet::IndexedGetter(uint32_t aIndex, bool& aFound)
>  {
> +  // XXX Not sure if this is needed.
> +  // mPresContext->FlushUserFontSet();
> +

I suspect it is needed, at least in principle, as we try to be lazy about updating the user font set's list of rules (on style changes, or state changes that could affect styling). So there's probably some risk that if you don't call FlushUserFontSet here, mRules may be stale.

I haven't tried to run this whole patch set; what happens, for example, if you enable or disable a stylesheet that contains @font-face rules, and then immediately (before we have a chance to reflow) use this getter, or the length method below?

@@ +173,5 @@
>  uint32_t
>  FontFaceSet::Length()
>  {
> +  // XXX Not sure if this is needed.
> +  // mPresContext->FlushUserFontSet();

And likewise here.
Attachment #8494345 - Flags: review?(jfkthame) → review+
(In reply to John Daggett (:jtd) from comment #157)
> Comment on attachment 8494342 [details] [diff] [review]
> Part 13: Implement FontFace.load().
> 
> So I don't quite know all of what is needed for FontFaceSet but it seems
> like a FontFace should have a reference to the FontFace::Entry. That would
> obviate the need for global lookup methods like this.

Sounds reasonable.  I'm going to leave the lookup here in part 13, and then in a new part 15.1 I'll switch over to storing the user font entry on FontFace objects.  Then this lookup method here will be simplified to a coupple of pointer dereferences.
(In reply to Jonathan Kew (:jfkthame) from comment #162)
> I suspect it is needed, at least in principle, as we try to be lazy about
> updating the user font set's list of rules (on style changes, or state
> changes that could affect styling). So there's probably some risk that if
> you don't call FlushUserFontSet here, mRules may be stale.

I just tried with this document:

<!DOCTYPE html>
<style>
@font-face { font-family: x; src: url(x); }
</style>
<script>
var fonts = document.fonts;
alert(fonts.length);
document.querySelector("style").disabled = true;
alert(fonts.length);
</script>

and it alerts "1" twice, so you're right we do need it.  I think that means we need it on every FontFace and FontFaceSet method.  For example if you had a hold of a FontFace object that reflected a rule, then you disable the style sheet, and then you assign to FontFace.weight, say, then you don't want that to affect the rule; the rule should be disconnected from the FontFace first.
(In reply to Cameron McCormack (:heycam) (away October) from comment #164)

> I just tried with this document:
...
> and it alerts "1" twice, so you're right we do need it.

And just to confirm -- if you call something like document.body.offsetTop before the second alert(), you'll presumably get the expected "0"?

(It'd be worth making sure your eventual tests cover this kind of thing.)

> I think that means we need it on every FontFace and FontFaceSet method.

Basically, any time you look at mRules, I guess. Perhaps it'd be safer to have a GetRules() accessor that encapsulates this, and use that whenever a method needs to look at the "current" rules.
(In reply to Jonathan Kew (:jfkthame) from comment #165)
> And just to confirm -- if you call something like document.body.offsetTop
> before the second alert(), you'll presumably get the expected "0"?

Yes it does.  And it does after I add the RebuildUserFontSet() call too.

> (It'd be worth making sure your eventual tests cover this kind of thing.)
> 
> > I think that means we need it on every FontFace and FontFaceSet method.
> 
> Basically, any time you look at mRules, I guess. Perhaps it'd be safer to
> have a GetRules() accessor that encapsulates this, and use that whenever a
> method needs to look at the "current" rules.

Maybe!  Although I think it might be better to keep the updating explicit -- there are various points where we access the three new arrays (what used to be the single array mRules) on FontFaceSet where we wouldn't want to be calling in to RebuildUserFontSet; when we're in UpdateRules for one.

I think it's like calling FlushPendingNotifications at the top of some DOM methods, which is a pattern we seem to use.
Attachment #8494330 - Attachment is obsolete: true
Attachment #8494330 - Flags: review?(jfkthame)
Attachment #8495947 - Flags: review?(jfkthame)
Attachment #8494342 - Attachment is obsolete: true
Attachment #8494342 - Flags: review?(jdaggett)
Attachment #8495948 - Flags: review?(jdaggett)
Attachment #8494347 - Attachment is obsolete: true
Attachment #8494347 - Flags: review?(jdaggett)
Attachment #8495950 - Flags: review?(jdaggett)
Attachment #8494350 - Attachment is obsolete: true
Attachment #8494350 - Flags: review?(jdaggett)
Attachment #8495951 - Flags: review?(jdaggett)
Attachment #8495058 - Attachment is obsolete: true
Attachment #8495058 - Flags: review?(jdaggett)
Attachment #8495953 - Flags: review?(jdaggett)
Attachment #8494355 - Attachment is obsolete: true
Attachment #8494355 - Flags: review?(jdaggett)
Attachment #8495954 - Flags: review?(jdaggett)
Attachment #8494358 - Attachment is obsolete: true
Attachment #8494358 - Flags: review?(jdaggett)
Attachment #8495955 - Flags: review?(jdaggett)
Attachment #8494362 - Attachment is obsolete: true
Attachment #8494362 - Flags: review?(jdaggett)
Attachment #8494362 - Flags: review?(bzbarsky)
Attachment #8495956 - Flags: review?(jdaggett)
Comment on attachment 8495957 [details] [diff] [review]
Part 25: Support loading of fonts from ArrayBuffer{,View}s. (v3)

Boris, my use of JSAPI stuff now is much smaller.  Can you check this?
Attachment #8495957 - Flags: review?(bzbarsky)
Attachment #8495953 - Attachment description: Part 21: Implement the FontFace constructor's parsing of descriptors. (v3) → Part 21: Implement the FontFace constructor's parsing of descriptors. (v3) r=bzbarsky
(In reply to Cameron McCormack (:heycam) (away October) from comment #163)
> (In reply to John Daggett (:jtd) from comment #157)
> > Comment on attachment 8494342 [details] [diff] [review]
> > Part 13: Implement FontFace.load().
> > 
> > So I don't quite know all of what is needed for FontFaceSet but it seems
> > like a FontFace should have a reference to the FontFace::Entry. That would
> > obviate the need for global lookup methods like this.
> 
> Sounds reasonable.  I'm going to leave the lookup here in part 13, and then
> in a new part 15.1 I'll switch over to storing the user font entry on
> FontFace objects.  Then this lookup method here will be simplified to a
> coupple of pointer dereferences.

The above bunch of new patches implement this.  So now FontFace has a strong pointer to its FontFace::Entry (i.e. its user font entry), and the FontFace::Entry has a weak pointer back to the FontFace.
Comment on attachment 8495957 [details] [diff] [review]
Part 25: Support loading of fonts from ArrayBuffer{,View}s. (v3)

>+GetDataFrom(JS::Handle<JSObject*> aValue, uint8_t*& aBuffer, uint32_t& aLength)

Why not make the first argument a const T& or const T*?  Now the callers actually have the ArrayBuffer or ArrayBufferView, so it's a little silly to create a new one.

So you'd have SetSource do:

  GetDataFrom(aArrayBuffer, mSourceBuffer, mSourceBufferLength)

and not need the RootedObject bit there or the Init() bit in GetDataFrom.  And I'd recommend changing SetSource to take references, not pointers, since you can never have null passed there.

>+  aBuffer = (uint8_t*) NS_Alloc(buf.Length());

This is an infallible allocation of a size controlled by the web page, right?

It would be better to use a fallible allocation here and propagate OOM back out as an exception.  Doing that in a followup is probably OK.

(The separate storage of buffer and length also makes my spidey-sense tingle; seems like it would be better to use a FallibleTArray and Swap() as needed, but the need to common up deallocation with other buffer sources might make that hard; it's at least worth a comment about why the storage is the way it is.)

Anyway, r=me on the JS bits with the templating fixed.
Attachment #8495957 - Flags: review?(bzbarsky) → review+
Attachment #8495947 - Flags: review?(jfkthame) → review+
(In reply to Boris Zbarsky [:bz] from comment #180)
> Comment on attachment 8495957 [details] [diff] [review]
> Part 25: Support loading of fonts from ArrayBuffer{,View}s. (v3)
> 
> >+GetDataFrom(JS::Handle<JSObject*> aValue, uint8_t*& aBuffer, uint32_t& aLength)
> 
> Why not make the first argument a const T& or const T*?  Now the callers
> actually have the ArrayBuffer or ArrayBufferView, so it's a little silly to
> create a new one.
> 
> So you'd have SetSource do:
> 
>   GetDataFrom(aArrayBuffer, mSourceBuffer, mSourceBufferLength)
> 
> and not need the RootedObject bit there or the Init() bit in GetDataFrom. 
> And I'd recommend changing SetSource to take references, not pointers, since
> you can never have null passed there.

Yep, all that makes sense.

> >+  aBuffer = (uint8_t*) NS_Alloc(buf.Length());
> 
> This is an infallible allocation of a size controlled by the web page, right?
> 
> It would be better to use a fallible allocation here and propagate OOM back
> out as an exception.  Doing that in a followup is probably OK.

I've change it to moz_malloc.

> (The separate storage of buffer and length also makes my spidey-sense
> tingle; seems like it would be better to use a FallibleTArray and Swap() as
> needed, but the need to common up deallocation with other buffer sources
> might make that hard; it's at least worth a comment about why the storage is
> the way it is.)

Added a comment.

> Anyway, r=me on the JS bits with the templating fixed.

Thanks.
Attachment #8495060 - Attachment is obsolete: true
Attachment #8495060 - Flags: review?(jdaggett)
Updating some patches for a couple of test failure fixes.
Attachment #8496534 - Flags: review?(jdaggett)
Attachment #8495949 - Attachment is obsolete: true
Attachment #8495949 - Flags: review?(jdaggett)
Attachment #8495950 - Attachment is obsolete: true
Attachment #8495950 - Flags: review?(jdaggett)
Attachment #8496535 - Flags: review?(jdaggett)
Attachment #8495953 - Attachment is obsolete: true
Attachment #8495953 - Flags: review?(jdaggett)
Attachment #8496536 - Flags: review?(jdaggett)
Attachment #8495955 - Attachment is obsolete: true
Attachment #8495955 - Flags: review?(jdaggett)
Attachment #8496537 - Flags: review?(jdaggett)
Attachment #8495956 - Attachment is obsolete: true
Attachment #8495956 - Flags: review?(jdaggett)
Attachment #8496538 - Flags: review?(jdaggett)
Attachment #8495957 - Attachment is obsolete: true
Attachment #8495957 - Flags: review?(jdaggett)
Attachment #8496539 - Flags: review?(jdaggett)
Attached patch Part 26: Tests. (v2) (obsolete) — Splinter Review
Attachment #8494364 - Attachment is obsolete: true
Attachment #8494364 - Flags: review?(jdaggett)
Attachment #8496540 - Flags: review?(jdaggett)
Whole patch queue can be downloaded here: http://mcc.id.au/temp/bug-1028497/
Comment on attachment 8496534 [details] [diff] [review]
Part 15.1: Store user font entries in FontFace objects. (v2)

Review of attachment 8496534 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comment added

::: layout/style/FontFace.h
@@ +51,5 @@
>  
>      virtual void SetLoadState(UserFontLoadState aLoadState) MOZ_OVERRIDE;
>  
>    protected:
> +    nsAutoTArray<FontFace*,1> mFontFaces;

I think a comment that explains why multiple faces might be associated with the same underlying userfont would be help. I'm assuming this is due to the userfont cache.
Attachment #8496534 - Flags: review?(jdaggett) → review+
Comment on attachment 8495948 [details] [diff] [review]
Part 13: Implement FontFace.load(). (v2)

Review of attachment 8495948 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but most of this patch is overwritten by later patches... :P
Attachment #8495948 - Flags: review?(jdaggett) → review+
Comment on attachment 8494344 [details] [diff] [review]
Part 15: Store FontFace objects on the FontFaceSet rather than nsCSSFontFaceRules.

Review of attachment 8494344 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with name changes suggested below

::: layout/style/FontFaceSet.cpp
@@ +378,5 @@
> +    //
> +    // XXX Now that it is possible for the author to hold on to a CSS-connected
> +    // FontFace object, we shouldn't cancel loading here; instead we should do
> +    // it when the FontFace is GCed, if we can detect that.
> +    size_t count = oldRecords.Length();

Agree, please file a follow-up bug for this.

::: layout/style/FontFaceSet.h
@@ +203,5 @@
>    nsTHashtable< nsPtrHashKey<nsFontFaceLoader> > mLoaders;
>  
> +  // The CSS-connected FontFace objects and their corresponding user
> +  // font entries.
> +  nsTArray<ConnectedFontFaceRecord> mConnectedFaces;

As per our IRC discussion, let's change the lingo here to avoid the use of the term "connected". I think the key here is that some FontFace's are associated with CSS rules and some are not. So something like mRuleFaces / mNonRuleFaces would be more descriptive I think. I'll let you work out the precise naming.
Attachment #8494344 - Flags: review?(jdaggett) → review+
Comment on attachment 8494344 [details] [diff] [review]
Part 15: Store FontFace objects on the FontFaceSet rather than nsCSSFontFaceRules.

r? to bz for the cycle collection bits
Attachment #8494344 - Flags: review?(bzbarsky)
Blocks: 1074058
(In reply to John Daggett (:jtd) from comment #192)
> ::: layout/style/FontFaceSet.cpp
> @@ +378,5 @@
> > +    //
> > +    // XXX Now that it is possible for the author to hold on to a CSS-connected
> > +    // FontFace object, we shouldn't cancel loading here; instead we should do
> > +    // it when the FontFace is GCed, if we can detect that.
> > +    size_t count = oldRecords.Length();
> 
> Agree, please file a follow-up bug for this.

Filed bug 1074058.
Comment on attachment 8496535 [details] [diff] [review]
Part 18: Create user font entries from FontFaces rather than nsCSSFontFaceRules. (v3)

Review of attachment 8496535 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/FontFaceSet.cpp
@@ +364,5 @@
> +  // Sometimes aRules has duplicate @font-face rules in it; we should make
> +  // that not happen, but in the meantime, don't try to insert the same
> +  // FontFace object more than once into mConnectedFaces.  We track which
> +  // ones we've handled in this table.
> +  nsTHashtable<nsPtrHashKey<nsCSSFontFaceRule>> handledRules;

The semantics of @font-face actually allow for rules such that ruleA == ruleB. Two identical @font-face rules would have this behavior. But since I think we'll end up linked to the same underlying userfont cache entry, I don't think it'll really matter.
Attachment #8496535 - Flags: review?(jdaggett) → review+
Comment on attachment 8495951 [details] [diff] [review]
Part 19: Support disconnecting FontFaces that reflect @font-face rules. (v2)

Review of attachment 8495951 [details] [diff] [review]:
-----------------------------------------------------------------

r? to bz for the cycle collection bits
Attachment #8495951 - Flags: review?(jdaggett)
Attachment #8495951 - Flags: review?(bzbarsky)
Attachment #8495951 - Flags: review+
Comment on attachment 8495952 [details] [diff] [review]
Part 20: Add storage for unconnected FontFace objects and create user font entries for them. (v2)

Review of attachment 8495952 [details] [diff] [review]:
-----------------------------------------------------------------

r? to bz for the cycle collection bits

::: layout/style/FontFaceSet.cpp
@@ +198,5 @@
>  
> +  // Web IDL objects can only expose array index properties up to INT32_MAX.
> +
> +  size_t total = mConnectedFaces.Length() + mOtherFaces.Length();
> +  return std::min<size_t>(total, INT32_MAX);

Hmm, just curious, is it actually possible to ever hit this limit?!? Seems like other code would break long before total > INT32_MAX
Attachment #8495952 - Flags: review?(jdaggett)
Attachment #8495952 - Flags: review?(bzbarsky)
Attachment #8495952 - Flags: review+
(In reply to John Daggett (:jtd) from comment #197)
> Hmm, just curious, is it actually possible to ever hit this limit?!? Seems
> like other code would break long before total > INT32_MAX

Yeah we probably would never get there, since FontFace objects are maybe 96 bytes on x86_64, so you'd need 384 GB of memory (or available swap space!) to reach it.
Comment on attachment 8496536 [details] [diff] [review]
Part 21: Implement the FontFace constructor's parsing of descriptors. (v4) r=bzbarsky

Review of attachment 8496536 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with minor tweaks/rename noted below

::: layout/style/FontFace.cpp
@@ +345,5 @@
> +  if (family.Length() != 0) {
> +    // The string length can be zero when the author passed an invalid
> +    // family name or an invalid descriptor to the JS FontFace constructor.
> +    nsStyleUtil::AppendEscapedCSSString(family, aResult);
> +  }

family.IsEmpty()

@@ +492,5 @@
> +    }
> +  } else {
> +    // This will cause the font to be loaded once it is initialized.
> +    mLoadPending = true;
> +  }

A similar comment should be in the header with the definition of the member variable.

::: layout/style/FontFace.h
@@ +233,5 @@
>    // Whether this FontFace appears in the FontFaceSet.
>    bool mInFontFaceSet;
> +
> +  bool mInitialized;
> +  bool mLoadPending;

I don't think this is really "load pending" but "need to load", since you're waiting on initialization to initiate the load. Rename to something appropriate and add a comment here to say what this is for, since it's sort of odd to need this when there's already a load status in the userfont.
Attachment #8496536 - Flags: review?(jdaggett) → review+
Comment on attachment 8495954 [details] [diff] [review]
Part 22: Make FontFace.load() work for unconnected FontFace objects. (v2)

Review of attachment 8495954 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/FontFaceSet.cpp
@@ +1128,5 @@
> +void
> +FontFaceSet::LoadFontFace(FontFace* aFontFace)
> +{
> +  MOZ_ASSERT(aFontFace->IsInitialized());
> +

The encapsulation feels wrong here. You're basically implementing a method of FontFace within FontFaceSet code. The pseudo-logic is:

  if (!userfontEntry) {
    create userfont entry;
  }
  userfontEntry->Load()

I think you can do this more cleanly in a FontFace::Load method and only defer to the FontFaceSet for the creation of a new userfont entry.
OK, so maybe I should

* push the family descriptor getting down into FontFaceSet::FindOrCreateUserFontEntryFromFontFace, since all callers of that method currently get the family just to pass in to it, and remove its family argument
* make its sheetType optional so we don't have to worry about it
* move all of FontFaceSet::LoadFontFace into FontFace::Load
* make FontFace a friend of FontFaceSet so that it can call FontFaceSet::FindOrCreateUserFontEntryFromFontFace

How does that sound?
Attachment #8496537 - Flags: review?(jdaggett) → review+
Attachment #8496538 - Flags: review?(jdaggett) → review?(roc)
Comment on attachment 8496538 [details] [diff] [review]
Part 24: Implement FontFaceSet.{ready,status} and dispatch events. (v3)

I dropped the r?bz I had before in comment 117.
Attachment #8496538 - Flags: review?(roc)
Attachment #8496538 - Flags: review?(jdaggett)
Attachment #8496538 - Flags: review?(bzbarsky)
Comment on attachment 8496539 [details] [diff] [review]
Part 25: Support loading of fonts from ArrayBuffer{,View}s. (v4) r=bzbarsky

Review of attachment 8496539 [details] [diff] [review]:
-----------------------------------------------------------------

r+ If possible, it would be nice to use an autoptr for the array buffer but if that doesn't work a refptr is fine.

::: gfx/thebes/gfxUserFontSet.h
@@ +53,5 @@
>      nsCOMPtr<nsIURI>       mURI;           // uri if url
>      nsCOMPtr<nsIURI>       mReferrer;      // referrer url if url
>      nsCOMPtr<nsIPrincipal> mOriginPrincipal; // principal if url
> +
> +    nsRefPtr<gfxFontFaceBufferSource> mBuffer;

Does this need to be a ref-counted resource? Would auto ptr work just as well?
Attachment #8496539 - Flags: review?(jdaggett) → review+
(In reply to Cameron McCormack (:heycam) (away October) from comment #201)
> OK, so maybe I should
> 
> * push the family descriptor getting down into
> FontFaceSet::FindOrCreateUserFontEntryFromFontFace, since all callers of
> that method currently get the family just to pass in to it, and remove its
> family argument
> * make its sheetType optional so we don't have to worry about it
> * move all of FontFaceSet::LoadFontFace into FontFace::Load
> * make FontFace a friend of FontFaceSet so that it can call
> FontFaceSet::FindOrCreateUserFontEntryFromFontFace
> 
> How does that sound?

Yeah, that sounds better.
Comment on attachment 8496538 [details] [diff] [review]
Part 24: Implement FontFaceSet.{ready,status} and dispatch events. (v3)

Review of attachment 8496538 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/FontFaceSet.cpp
@@ +508,5 @@
>  
> +void
> +FontFaceSet::UpdateCounts(FontFace* aFontFace, int32_t aDelta)
> +{
> +  switch (aFontFace->Status()) {

I'm sort of dubious about this function. It seems like you could just as easily refresh the counters without having to sprinkle calls to this function in lots of places (e.g. in UpdateRules). My concern is that this sort of thing is easy to get out of sync in particular sequences that may or may not be easy to duplicate.

@@ +1387,5 @@
> +  if (mLoadingCount == 1) {
> +    mStatus = FontFaceSetLoadStatus::Loading;
> +    (new AsyncEventDispatcher(this, NS_LITERAL_STRING("loading"),
> +                              false))->RunDOMEventWhenSafe();
> +  }

So I take it that you're assuming this is called at the point when the loading = 0 changes to loading = 1?

::: layout/style/FontFaceSet.h
@@ +309,5 @@
> +  // Counts of the FontFace objects in mConnectedFaces and mOtherFaces by their
> +  // loading status.
> +  int32_t mUnloadedCount;
> +  int32_t mLoadingCount;
> +  int32_t mLoadedAndErrorCount;

Hmmm, I'm confused. Both mUnloadedCount and mLoadedAndErrorCount are maintained but don't appear to be used within the code. What's the need for these?
Comment on attachment 8494344 [details] [diff] [review]
Part 15: Store FontFace objects on the FontFaceSet rather than nsCSSFontFaceRules.

Please add a comment to the FontFaceRecord struct telling people to adjust the relevant CC bits if more CCed members are added there.

r=me
Attachment #8494344 - Flags: review?(bzbarsky) → review+
Comment on attachment 8495951 [details] [diff] [review]
Part 19: Support disconnecting FontFaces that reflect @font-face rules. (v2)

r=me on the CC bits
Attachment #8495951 - Flags: review?(bzbarsky) → review+
Comment on attachment 8495952 [details] [diff] [review]
Part 20: Add storage for unconnected FontFace objects and create user font entries for them. (v2)

r=me on the CC bits
Attachment #8495952 - Flags: review?(bzbarsky) → review+
Comment on attachment 8496538 [details] [diff] [review]
Part 24: Implement FontFaceSet.{ready,status} and dispatch events. (v3)

>+++ b/layout/base/nsPresContext.h
>   void SetProcessingRestyles(bool aProcessing) {

Hmm.  So in RestyleManager::RebuildAllStyleData we set this to false before calling ProcessPendingRestyles (which will then set it to true and false again).

Is that a problem for the consumers here?

>+++ b/layout/base/nsRefreshDriver.cpp

So what are the exact semantics of "reflow flushing" we care about here, especially given interruptible reflow?

It seems to me that given a reflow interrupt we should wait until the reflow completes before we notify the fontset, and that this is not what this patch implements.  To do that you'd have to take into account whether the shell has a non-null mReflowContinueTimer, right?
Attachment #8496538 - Flags: review?(bzbarsky) → review-
Though the refresh driver code itself seems ok to me, I think...
(In reply to Boris Zbarsky [:bz] from comment #209)
> Hmm.  So in RestyleManager::RebuildAllStyleData we set this to false before
> calling ProcessPendingRestyles (which will then set it to true and false
> again).
> 
> Is that a problem for the consumers here?

Yeah, informing the FontFaceSet too early that restyles are done could mean that we resolve its mReady too soon.  I guess I'll need a separate counter variable that can handle being incremented more than once, and then inform the FontFaceSet when it gets back down to zero.

> >+++ b/layout/base/nsRefreshDriver.cpp
> So what are the exact semantics of "reflow flushing" we care about here,
> especially given interruptible reflow?
> 
> It seems to me that given a reflow interrupt we should wait until the reflow
> completes before we notify the fontset, and that this is not what this patch
> implements.  To do that you'd have to take into account whether the shell
> has a non-null mReflowContinueTimer, right?

I didn't really consider interruptible reflow.  We want to wait until we know there are no more reflows to come (since reflows create text runs, and text run building can kick off font loads) -- if we interrupt a reflow, we know there is more, so we should wait.

So I'm exposing this mReflowScheduled with a getter, and that's what I use in FontFaceSet to check if there are any reflows scheduled.  Going by the comment above its declaration, I guess that's insufficient and we need to be checking mReflowContinueTimer as well.  And we probably want to avoid calling mPresContext->NotifyFontFaceSetOnLayoutFlushed() if mReflowContinueTimer is not null after the FlushPendingNotifications call in nsRefreshDriver::Tick.

(In reply to Boris Zbarsky [:bz] from comment #210)
> Though the refresh driver code itself seems ok to me, I think...

Let me know if you think the NotifyFontFaceSetOnLayoutFlushed call should be done somewhere else, like in nsPresShell::FlushPendingNotifications.
You could do it in FlushPendingNotifications; compared to the refresh driver it would mean that some script that flushes layout then adds text would resolve the promise before we reflow the text, right?  But that might be OK given the wackiness of this concept in general: more text can get added to the document by script at any time, kicking off more font loads, right?
Yeah, though I think that's OK.  We'll resolve the promise right after the flush (though the promise callbacks won't be called until later, of course) and replace the promise the next time we reflow the text, if it needed to start a load.
That seems like it leads to pretty different behavior in different UAs in terms of what the page script observes...  Ah, well.
Yeah, since when the FontFace starts loading is pretty much entirely up to the UA.
(In reply to John Daggett (:jtd) from comment #203)
> Does this need to be a ref-counted resource? Would auto ptr work just as
> well?

I tried that, but since gfxFontFaceSrc objects are placed in nsTArray objects they want to be copy constructable, and that doesn't work well with the member being an nsAutoPtr, unfortunately.
I didn't end up moving the common get-family-name-from-FontFace code into the FindOrCreate... function, since at most call sites we needed the family name anyway.
Attachment #8495954 - Attachment is obsolete: true
Attachment #8495954 - Flags: review?(jdaggett)
Attachment #8497488 - Flags: review?(jdaggett)
John: I got rid of the loading count variables and instead just have a (dirtyable) boolean that stores whether we have any loading FontFace objects.

Boris: I merged the two FontFaceSet notification calls into the one, in nsRefreshDriver::Tick.  I awkwardly added a virtual function nsIPresShell::HasPendingReflow, since nsIPresShell has mReflowScheduled but mReflowContinueTimer is on PresShell.  I wasn't sure whether it was safe to static cast down to PresShell.
Attachment #8496538 - Attachment is obsolete: true
Attachment #8496538 - Flags: review?(jdaggett)
Attachment #8497491 - Flags: review?(jdaggett)
Attachment #8497491 - Flags: review?(bzbarsky)
Attachment #8496540 - Attachment is obsolete: true
Attachment #8496540 - Flags: review?(jdaggett)
Attachment #8497492 - Flags: review?(jdaggett)
The renaming requested in comment 192.
Attachment #8497493 - Flags: review?(jdaggett)
Patch queue updated at http://mcc.id.au/temp/bug-1028497/.