Cameron McCormack (:heycam) Assignee
	Description • 10 years ago

      No description provided.

	Cameron McCormack (:heycam) Assignee
	Comment 1 • 10 years ago

Current patch queue at: http://mcc.id.au/temp/bug-1028497/

	Cameron McCormack (:heycam) Assignee
	Comment 2 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 4 • 10 years ago

Not requesting review on this until we work out what to do with the Set-like API.

	Cameron McCormack (:heycam) Assignee
	Comment 7 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 9 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 10 • 10 years ago

This adds storage for the descriptor values on a FontFace and implements
the getters for them.

	Cameron McCormack (:heycam) Assignee
	Comment 14 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 16 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 19 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 23 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 26 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 27 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 10 years ago

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"?

	Cameron McCormack (:heycam) Assignee
	Comment 30 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 31 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 33 • 10 years ago

Does this look better re event dispatching?

	Cameron McCormack (:heycam) Assignee
	Comment 34 • 10 years ago

There's a leak I haven't figured out yet: https://tbpl.mozilla.org/?tree=Try&rev=eca1eadef8e2

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 36 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 38 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 10 years ago

> But I think that makes sense.

Yes, agreed.

	Cameron McCormack (:heycam) Assignee
	Comment 40 • 10 years ago

Changed to use codegen for CSSFontFaceLoadEvent.

	Cameron McCormack (:heycam) Assignee
	Comment 41 • 10 years ago

Changed to store only mDocument on the FontFaceSet, not mPresContext, per comment 37.

	Cameron McCormack (:heycam) Assignee
	Comment 42 • 10 years ago

Minor changes to rebase on top of revised earlier patches.

	Cameron McCormack (:heycam) Assignee
	Comment 43 • 10 years ago

More minor changes.

	Cameron McCormack (:heycam) Assignee
	Comment 44 • 10 years ago

More minor changes.

	Cameron McCormack (:heycam) Assignee
	Comment 45 • 10 years ago

Address remaining review comments from Boris.

	Cameron McCormack (:heycam) Assignee
	Comment 46 • 10 years ago

More minor changes.

	Cameron McCormack (:heycam) Assignee
	Comment 47 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=8be4ea66dc93

	Cameron McCormack (:heycam) Assignee
	Comment 48 • 10 years ago

Update for some slightly changed enum types.

	Cameron McCormack (:heycam) Assignee
	Comment 49 • 10 years ago

Update for the updated IDL patch.

	Cameron McCormack (:heycam) Assignee
	Comment 50 • 10 years ago

* Fix some cycle collection definition problems.
* Don't accept empty string family values in the FontFace constructor.

	Cameron McCormack (:heycam) Assignee
	Comment 51 • 10 years ago

Clear gfxProxyFontEntry::mFontFace when its loading is finished, to help the FontFace be released.

	Cameron McCormack (:heycam) Assignee
	Comment 52 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 53 • 10 years ago

* 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

	Cameron McCormack (:heycam) Assignee
	Comment 54 • 10 years ago

* Clear mFontFace on remaining gfxProxyFontEntrys when we're destroying the nsUserFontSet.
* Handle non-existent descriptors on @font-face rules a bit better.

	Cameron McCormack (:heycam) Assignee
	Comment 55 • 10 years ago

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.

	Sylvestre Ledru [:Sylvestre]
	Comment 57 • 10 years ago

relnote-firefox is enough to get our attention for the release notes.

	John Daggett (:jtd)
	Comment 58 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 59 • 10 years ago

Rebased over Promise constructor changes.

	Cameron McCormack (:heycam) Assignee
	Comment 60 • 10 years ago

Rebased over Promise constructor changes.

	Cameron McCormack (:heycam) Assignee
	Comment 61 • 10 years ago

Rebased over Promise constructor changes.

	Cameron McCormack (:heycam) Assignee
	Comment 62 • 10 years ago

Rebased over Promise constructor changes.

	Cameron McCormack (:heycam) Assignee
	Comment 63 • 10 years ago

Rebased over Promise constructor changes.

	Cameron McCormack (:heycam) Assignee
	Comment 64 • 10 years ago

Rebased over Promise constructor changes.

	Cameron McCormack (:heycam) Assignee
	Comment 65 • 10 years ago

Rebased over Promise constructor changes.

	John Daggett (:jtd)
	Comment 66 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 67 • 10 years ago

MaybeReject/MaybeResolve are methods on mozilla::dom::Promise, so I'll leave those.  That's where I got my naming idea from. ;)

	John Daggett (:jtd)
	Comment 68 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 69 • 10 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 70 • 10 years ago

Please set again the relnotes flags when the feature has landed.

	John Daggett (:jtd)
	Comment 71 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 72 • 10 years ago

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

	John Daggett (:jtd)
	Comment 73 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 74 • 10 years ago

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

	Sebastian Zartner [:sebo]
	Comment 75 • 10 years ago

(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

	John Daggett (:jtd)
	Comment 76 • 10 years ago

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.

	John Daggett (:jtd)
	Comment 77 • 10 years ago

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

	John Daggett (:jtd)
	Comment 78 • 10 years ago

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

	John Daggett (:jtd)
	Comment 79 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 80 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 81 • 10 years ago

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.

	John Daggett (:jtd)
	Comment 82 • 10 years ago

Bulk clearing review flags until the revised set of patches.

	John Daggett (:jtd)
	Comment 83 • 10 years ago

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

	John Daggett (:jtd)
	Comment 84 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 85 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 86 • 10 years ago

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

	John Daggett (:jtd)
	Comment 87 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 88 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 89 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 90 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 91 • 10 years ago

These new patches are based on top of the bug 998869 patch queue that Jonathan Kew pushed to try and came up green.

	Cameron McCormack (:heycam) Assignee
	Comment 92 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 93 • 10 years ago

Re-requesting review since I've made changes to test_interfaces.html that should be checked.

	Cameron McCormack (:heycam) Assignee
	Comment 94 • 10 years ago

Carrying over r=jdaggett.

	Cameron McCormack (:heycam) Assignee
	Comment 95 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 96 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 97 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 99 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 100 • 10 years ago

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.