1072101 - implement the remaining Set-like API of FontFaceSet

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

The CSS Font Loading API spec currently duplicates the JS Set API in IDL.  Bug 1028497 will land with a couple of these methods implemented.  We should implement the rest.  It's likely that Web IDL will allow defining an interface to be Set-like to gain all these members automatically, so the spec might change to use that new feature.

Comment 1

[Cameron McCormack (:heycam)]

As mentioned in bug 1123516 comment 42, I'm going to implement the setlike-interface manually, as it's going to be awkward to handle the spec's requirements for ordering of FontFace objects in the set and to store the other information that's on FontFaceSet::FontFaceRecord if I've got the FontFaces stored in the backing Set.  :qDot points out that iterable<> is really what I want, to gain forEach/entries/keys/values/@@iterator methods for free without the rest of the setlike interface.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 1: Implement FontFaceSet.size.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 2: Implement FontFaceSet.{entries,values}.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 3: Implement FontFaceSet.forEach.

This depends on bug 1146234.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 4: Implement FontFaceSet.{keys,@@iterator}.

This depends on bug 1146235.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Part 5: Remove indexed property access on FontFaceSet.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8581455 [details] [diff] [review]
Part 1: Implement FontFaceSet.size.

I think peterv should review all these.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 8581457 [details] [diff] [review]
Part 2: Implement FontFaceSet.{entries,values}.

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

Mostly looks good but I want to see it again without the wrappercache or nsISupports on FontFaceSetIterator (unless I've missed something and that's impossible somehow).

::: dom/webidl/FontFaceSet.webidl
@@ +36,4 @@
>    boolean has(FontFace font);
>    [Throws] boolean delete(FontFace font);
>    void clear();
> +  FontFaceSetIterator entries();

I think these should be NewObject.

::: layout/style/FontFaceSet.cpp
@@ +348,5 @@
> +FontFace*
> +FontFaceSet::IndexedGetter(uint32_t aIndex, bool& aFound)
> +{
> +  FontFace* f = GetFontFaceAt(aIndex);
> +  aFound = f;

!!f or f != nullptr

::: layout/style/FontFaceSetIterator.cpp
@@ +29,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(FontFaceSetIterator)
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +

If you mark entries and values NewObject then this doesn't need to be wrappercached and you don't need to inherit from nsISupports, which would simplify this class a bit I'd think.

@@ +77,5 @@
> +  GetOrCreateDOMReflector(aCx, face, &value);
> +
> +  if (mIsKeyAndValue) {
> +    JS::AutoValueArray<2> values(aCx);
> +    values[0].set(value);

This should be mNextIndex - 1, no?

Comment 9

[Peter Van der Beken [:peterv]]

> > +  if (mIsKeyAndValue) {
> > +    JS::AutoValueArray<2> values(aCx);
> > +    values[0].set(value);
> 
> This should be mNextIndex - 1, no?

Actually, I got confused with the mention of iterable<>, I guess if this is setlike then it's fine.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 8581459 [details] [diff] [review]
Part 3: Implement FontFaceSet.forEach.

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

::: dom/webidl/FontFaceSet.webidl
@@ +23,4 @@
>    FontFaceSetIteratorResult next();
>  };
>  
> +callback FontFaceSetForEachCallback = void (FontFace key, FontFace value, FontFaceSet set);

Nit: I'd reverse key and value. Maybe silly but it'd align more with Array and Map forEach.

::: layout/style/FontFaceSet.cpp
@@ +385,5 @@
> +  ErrorResult rv;
> +  for (size_t i = 0; i < Size(); i++) {
> +    FontFace* face = GetFontFaceAt(i);
> +    aCallback.Call(aThisArg, *face, *face, *this, rv);
> +    if (rv.Failed()) {

Shouldn't this use aRv instead of rv?

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 8581460 [details] [diff] [review]
Part 4: Implement FontFaceSet.{keys,@@iterator}.

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

Guess I should review the Alias patches :-).

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 8581461 [details] [diff] [review]
Part 5: Remove indexed property access on FontFaceSet.

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

\o/

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Part 2: Implement FontFaceSet.{entries,values}. (v2)

I got rid of the wrapper cache, nsISupports inheritance, and cycle collection.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 8584938 [details] [diff] [review]
Part 2: Implement FontFaceSet.{entries,values}. (v2)

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

::: layout/style/FontFaceSetIterator.cpp
@@ +47,5 @@
> +    return;
> +  }
> +
> +  JS::Rooted<JS::Value> value(aCx);
> +  GetOrCreateDOMReflector(aCx, face, &value);

Please use ToJSValue here instead if you can.

Comment 15

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d08a71f17e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0d4e6985ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f48e695f646
https://hg.mozilla.org/integration/mozilla-inbound/rev/9702f21c594f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bff1a42c8c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/64b82da4caf3

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8d08a71f17e
https://hg.mozilla.org/mozilla-central/rev/cc0d4e6985ae
https://hg.mozilla.org/mozilla-central/rev/1f48e695f646
https://hg.mozilla.org/mozilla-central/rev/9702f21c594f
https://hg.mozilla.org/mozilla-central/rev/6bff1a42c8c7
https://hg.mozilla.org/mozilla-central/rev/64b82da4caf3