Bug 153348

*** Bug 153642 has been marked as a duplicate of this bug. ***

Created attachment 270184 [details] Applied on top of attachment 270144 [details]

Created attachment 270290 [details] Applied on top of attachment 270144 [details]

Created attachment 270293 [details] Applied on top of attachment 270144 [details]

Created attachment 270415 [details] Applied on top of attachment 270144 [details]

We need to support iteration. Iteration must iterate in document-order, followed by non-CSS-connected fonts in insertion order.

Adding a font that is CSS-Connected needs to throw a InvalidModificationError

We need our loading/loaded status to match our constituent FontFaces. add() and remove() need to be updated to maintain this invariant.

We need to implement event triggering.

Created attachment 270455 [details] Applied on top of attachment 270144 [details]

Created attachment 270477 [details] Applied on top of attachment 270478 [details]

Created attachment 271512 [details] WIP

Created attachment 271522 [details] WIP

Created attachment 271529 [details] Patch

Comment on attachment 271529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271529&action=review > LayoutTests/fast/text/font-face-set-javascript.html:24 > +var item = iterator.next(); I should test that fonts are iterated in insertion order.

Comment on attachment 271529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271529&action=review > Source/WebCore/css/FontFaceSet.cpp:135 > + if (face.get().status() == CSSFontFace::Status::Failure) { This failure check should occur before we add anything to m_pendingPromises.

Comment on attachment 271529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271529&action=review > Source/WebCore/css/CSSFontFace.h:2 > - * Copyright (C) 2007, 2008 Apple Inc. All rights reserved. > + * Copyright (C) 2016 Apple Inc. All rights reserved. You should preserve the 2007,2008 dates. > Source/WebCore/css/CSSFontFace.h:52 > + static Ref<CSSFontFace> create(CSSFontSelector& fontSelector, FontFace* wrapper = nullptr, bool isLocalFallback = false) { return adoptRef(*new CSSFontFace(fontSelector, wrapper, isLocalFallback)); } Should not all be on one line. > Source/WebCore/css/CSSFontFace.h:96 > + virtual void kick(CSSFontFace&) { }; Kick is not a very descriptive name. Please choose a new one. > Source/WebCore/css/CSSFontFaceSet.cpp:88 > + for (size_t i = 0; i < m_faces.size(); ++i) { Use a modern loop? > Source/WebCore/css/CSSFontFaceSet.cpp:97 > + ASSERT_NOT_REACHED(); Is it really impossible to hit this, even with bad JS? > Source/WebCore/css/CSSFontFaceSet.cpp:112 > +static bool familiesIntersect(CSSFontFace& face, const CSSValueList& request) const CSSFontFace& > Source/WebCore/css/CSSFontFaceSet.h:49 > + bool has(const CSSFontFace&) const; I would prefer hasFace() > Source/WebCore/css/CSSFontFaceSet.h:50 > + size_t size() const { return m_faces.size(); } Never really happy with size() as a count, since it's ambiguous with byte size. > Source/WebCore/css/CSSFontFaceSet.h:73 > + Status m_status { Status::Loaded }; Seems odd that the initial state is Loaded? > Source/WebCore/css/CSSSegmentedFontFace.cpp:58 > +void CSSSegmentedFontFace::kick(CSSFontFace&) Donut know what "kick" means. > Source/WebCore/css/FontFaceSet.h:57 > + bool has(FontFace*) const; > + size_t size() const; Ditto. > Source/WebCore/css/FontFaceSet.h:88 > + using RefCounted<FontFaceSet>::ref; > + using RefCounted<FontFaceSet>::deref; Why do you need these?

Comment on attachment 271529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271529&action=review > Source/WebCore/css/CSSFontFaceSet.cpp:119 > + for (auto &family1 : faceFamilies) { auto& > Source/WebCore/css/CSSFontFaceSet.cpp:165 > + for (auto face : matchingFaces) auto&? > Source/WebCore/css/CSSFontFaceSet.cpp:175 > + for (auto face : matchingFaces) { auto&? > Source/WebCore/css/CSSFontFaceSet.h:44 > +class CSSFontFaceSet : public CSSFontFace::Client { final? > Source/WebCore/css/FontFace.h:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. should keep the old dates. > Source/WebCore/css/FontFaceSet.cpp:123 > + if (!matchingFaces.size()) { matchingFaces.isEmpty() is a tad more readable. > Source/WebCore/css/FontFaceSet.cpp:128 > + for (auto face : matchingFaces) auto& > Source/WebCore/css/FontFaceSet.cpp:133 > + for (auto face : matchingFaces) { auto& > Source/WebCore/css/FontFaceSet.cpp:142 > + auto& vec = m_pendingPromises.add(RefPtr<FontFace>(face.get().wrapper()), Vector<Ref<PendingPromise>>()).iterator->value; vector > Source/WebCore/css/FontFaceSet.h:46 > +class FontFaceSet : public RefCounted<FontFaceSet>, public CSSFontFaceSetClient, public EventTargetWithInlineData, public ActiveDOMObject { final?

Comment on attachment 271529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271529&action=review >> Source/WebCore/css/CSSFontFaceSet.cpp:88 >> + for (size_t i = 0; i < m_faces.size(); ++i) { > > Use a modern loop? I use "i" below. m_faces.remove(i); >> Source/WebCore/css/CSSFontFaceSet.cpp:97 >> + ASSERT_NOT_REACHED(); > > Is it really impossible to hit this, even with bad JS? Yes, FontFaceSet is an intermediary between JavaScript and this function, and it will only call this function if CSSFontFaceSet::has(). Like Darin says: "No aspirational ASSERT()s!" >> Source/WebCore/css/CSSFontFaceSet.cpp:165 >> + for (auto face : matchingFaces) > > auto&? Wow, my testing didn't catch this because "face" is a std::reference_wrapper :| >> Source/WebCore/css/CSSFontFaceSet.h:50 >> + size_t size() const { return m_faces.size(); } > > Never really happy with size() as a count, since it's ambiguous with byte size. I think it's more valuable to be consistent with C++ conventions rather than (weirdly) use Obj-C conventions. >> Source/WebCore/css/CSSFontFaceSet.h:73 >> + Status m_status { Status::Loaded }; > > Seems odd that the initial state is Loaded? The state is "loaded" iff there are 0 loads active. Otherwise, the state is "loading." >> Source/WebCore/css/FontFaceSet.h:88 >> + using RefCounted<FontFaceSet>::deref; > > Why do you need these? RefCounted has a ref() and deref() function, and EventTarget has a ref() and deref() function.

Comment on attachment 271529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271529&action=review >>> Source/WebCore/css/CSSFontFaceSet.h:50 >>> + size_t size() const { return m_faces.size(); } >> >> Never really happy with size() as a count, since it's ambiguous with byte size. > > I think it's more valuable to be consistent with C++ conventions rather than (weirdly) use Obj-C conventions. What do you think I should name it?

(In reply to comment #20) > Comment on attachment 271529 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271529&action=review > > >>> Source/WebCore/css/CSSFontFaceSet.h:50 > >>> + size_t size() const { return m_faces.size(); } > >> > >> Never really happy with size() as a count, since it's ambiguous with byte size. > > > > I think it's more valuable to be consistent with C++ conventions rather than (weirdly) use Obj-C conventions. > > What do you think I should name it? faceCount? numberOfFaces()?

Committed r196747: <http://trac.webkit.org/changeset/196747>