implement the remaining Set-like API of FontFaceSet

RESOLVED FIXED in Firefox 40

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

({dev-doc-needed})

Trunk
mozilla40
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(6 attachments)

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.
Blocks: 1088437
Depends on: 1123516
Depends on: 1146234
Depends on: 1146235
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.
Created attachment 8581455 [details] [diff] [review]
Part 1: Implement FontFaceSet.size.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8581455 - Flags: review?(bugs)
Created attachment 8581457 [details] [diff] [review]
Part 2: Implement FontFaceSet.{entries,values}.
Attachment #8581457 - Flags: review?(bugs)
Created attachment 8581459 [details] [diff] [review]
Part 3: Implement FontFaceSet.forEach.

This depends on bug 1146234.
Attachment #8581459 - Flags: review?(bugs)
Created attachment 8581460 [details] [diff] [review]
Part 4: Implement FontFaceSet.{keys,@@iterator}.

This depends on bug 1146235.
Attachment #8581460 - Flags: review?(bugs)
Created attachment 8581461 [details] [diff] [review]
Part 5: Remove indexed property access on FontFaceSet.
Attachment #8581461 - Flags: review?(bugs)
Comment on attachment 8581455 [details] [diff] [review]
Part 1: Implement FontFaceSet.size.

I think peterv should review all these.
Attachment #8581455 - Flags: review?(bugs) → review?(peterv)
Attachment #8581457 - Flags: review?(bugs) → review?(peterv)
Attachment #8581459 - Flags: review?(bugs) → review?(peterv)
Attachment #8581460 - Flags: review?(bugs) → review?(peterv)
Attachment #8581461 - Flags: review?(bugs) → review?(peterv)
No longer depends on: 1123516
Attachment #8581455 - Flags: review?(peterv) → review+
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?
Attachment #8581457 - Flags: review?(peterv) → review+
> > +  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 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?
Attachment #8581459 - Flags: review?(peterv) → review+
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 :-).
Attachment #8581460 - Flags: review?(peterv) → review+
Comment on attachment 8581461 [details] [diff] [review]
Part 5: Remove indexed property access on FontFaceSet.

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

\o/
Attachment #8581461 - Flags: review?(peterv) → review+
Created attachment 8584938 [details] [diff] [review]
Part 2: Implement FontFaceSet.{entries,values}. (v2)

I got rid of the wrapper cache, nsISupports inheritance, and cycle collection.
Attachment #8584938 - Flags: review?(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.
Attachment #8584938 - Flags: review?(peterv) → review+
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Keywords: dev-doc-needed
Depends on: 1183484
You need to log in before you can comment on or make changes to this bug.