Closed
Bug 1072101
Opened 10 years ago
Closed 9 years ago
implement the remaining Set-like API of FontFaceSet
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(6 files)
1.66 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
8.61 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
742 bytes,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
1.79 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
7.78 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8581457 -
Flags: review?(bugs)
Assignee | ||
Comment 4•9 years ago
|
||
This depends on bug 1146234.
Attachment #8581459 -
Flags: review?(bugs)
Assignee | ||
Comment 5•9 years ago
|
||
This depends on bug 1146235.
Attachment #8581460 -
Flags: review?(bugs)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8581461 -
Flags: review?(bugs)
Comment 7•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8581457 -
Flags: review?(bugs) → review?(peterv)
Updated•9 years ago
|
Attachment #8581459 -
Flags: review?(bugs) → review?(peterv)
Updated•9 years ago
|
Attachment #8581460 -
Flags: review?(bugs) → review?(peterv)
Updated•9 years ago
|
Attachment #8581461 -
Flags: review?(bugs) → review?(peterv)
Updated•9 years ago
|
Attachment #8581455 -
Flags: review?(peterv) → review+
Comment 8•9 years ago
|
||
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+
Comment 9•9 years ago
|
||
> > + 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•9 years ago
|
||
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 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Assignee | ||
Comment 13•9 years ago
|
||
I got rid of the wrapper cache, nsISupports inheritance, and cycle collection.
Attachment #8584938 -
Flags: review?(peterv)
Comment 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
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•9 years ago
|
||
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
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•