Bug 98395

Created attachment 167066 [details] proof of concept patch

Please do send an announcement to webkit-dev when ready. Sounds like a good feature to me.

(In reply to comment #2) > Please do send an announcement to webkit-dev when ready. Sounds like a good feature to me. Thank you for the response. Yes, I'll announce to webkit-dev before starting.

I took over this work from Bashi.

do you expect to prefix these types, events, etc until the spec gets to CR? i expect it will evolve as it goes through more WDs, LC, then CR

I think we should do prefix them. So I will do.

Created attachment 186593 [details] Patch

Created attachment 186757 [details] Updated patch

Comment on attachment 186757 [details] Updated patch mitz: Would you review this patch? Let me know if this patch is too large to review. I'm happy to break it into smaller pieces.

Comment on attachment 186757 [details] Updated patch Attachment 186757 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16395062 New failing tests: fast/frames/seamless/seamless-nested.html

Hi mitz, Did you have a chance to look at this? Failure of seamless-nested.html should be unrelated to the change.

Just a quick note to let you know the interface defined in the spec is still in flux, so you'll probably need to review the changes later to match the eventual spec design when CSS3 Fonts goes to CR. It would be especially helpful to post any questions you have about spec details to www-style. Just to be clear, the methods such as loadFont/checkFont are *not* limited to loading a single font, the defined behavior is to load all fonts in the font list passed in: For example: document.fontloader.loadFont({font: "16px fontA, fontB, fontC", onsuccess: drawStuff, onerror: handleError}); The function 'drawStuff' will be called *after* the default face for families fontA, fontB and fontC have all been downloaded and activated.

Thanks for the comments, John. Yes I understand the spec is not fixed yet. We will keep the feature prefixed until it goes to CR. And I guess you took a look at the patch - right, it doesn't handle multiple fonts case correctly. I will update the patch. Thanks!

I should also point out that the load events are more than just "loading font data is done", there's an interaction with layout code that's somewhat tricky to handle. For example, the "loadingdone" event should only fire after layout operations have completed and no new font loads occurred. Consider this example: @font-face { font-family: general-serif; src: url(generalserif.woff) format("woff"); /* contains no kanji/kana */ } @font-face { font-family: japanesebodytext; src: url(japanesefont.woff) format("woff"); } body { font-family: general-serif, japanesebodytext; } <p>納豆嫌い</p> The "loadingdone" event should fire after the 'japanesefont.woff' is downloaded, not after 'generalserif.woff' completes. That's the reason behind the sentence in the spec "when the user agent completes the final font load for document doc, after all pending layout operations that might affect font selection have completed and no font loads are pending...".

Thank you for pointing it out. I will update my patch accordingly.

Created attachment 188820 [details] Updated patch

Comment on attachment 188820 [details] Updated patch Attachment 188820 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16614175 New failing tests: fast/css/ex-unit-with-no-x-height.html

Created attachment 188984 [details] Updated patch, fix test failure

I think the issues John pointed out are addressed in the current patch. There are still some FIXMEs in the code, but I would like to ask for a review before completing details. BTW font loader has moved to a separate spec. http://dev.w3.org/csswg/css-font-load-events-3/ http://lists.w3.org/Archives/Public/www-style/2013Feb/0433.html

https://lists.webkit.org/pipermail/webkit-dev/2013-January/023541.html was the announcement, for posterity.

Comment on attachment 188984 [details] Updated patch, fix test failure View in context: https://bugs.webkit.org/attachment.cgi?id=188984&action=review I think this is a fantastic feature. I need to read through the patch again, and it's possible that some of the other CSS folks will want to too. If you schedule some time on my calendar you and I could go over this via VC and I suspect we'd have fewer review iterations needed. > Source/WebCore/ChangeLog:9 > + This patch adds two interfaces, WebKitFontLoader and WebKitCSSFontFaceLoadEvent, and a document attribute webkitFontloader to WebCore. > + The webkitFontloader implements the FontLoader interface defined in CSS Fonts Module Level 3.[1] My understanding was that the standards community was moving away from using prefixes. If this is spec'd, we should just use the real names? > Source/WebCore/bindings/js/JSDictionary.cpp:236 > +void JSDictionary::convertValue(JSC::ExecState*, JSC::JSValue value, RefPtr<CSSFontFaceRule>& result) > +{ > + result = toCSSFontFaceRule(value); > +} I'm confused as to what these do or why they're needed? Some artifact of how JSC bindings are implemented? > Source/WebCore/bindings/v8/Dictionary.cpp:519 > + CSSFontFaceRule* source = 0; > + if (v8Value->IsObject()) { > + v8::Handle<v8::Object> wrapper = v8::Handle<v8::Object>::Cast(v8Value); > + v8::Handle<v8::Object> fontface = wrapper->FindInstanceInPrototypeChain(V8CSSFontFaceRule::GetTemplate(m_isolate)); > + if (!fontface.IsEmpty()) > + source = V8CSSFontFaceRule::toNative(fontface); Same question. I'm confused by what this custom bindings code is doing/why it's needed? > Source/WebCore/css/WebKitFontLoader.cpp:67 > + if (!m_numLoading) { I believe webkit style is to early return instead of makign a long block. > Source/WebCore/css/WebKitFontLoader.cpp:150 > + scheduleEvent(WebKitCSSFontFaceLoadEvent::createForFontFaceRule(eventNames().loadingEvent, rule)); > + scheduleEvent(WebKitCSSFontFaceLoadEvent::createForFontFaceRule(eventNames().loadstartEvent, rule)); Thank you for making this async!

Comment on attachment 188984 [details] Updated patch, fix test failure View in context: https://bugs.webkit.org/attachment.cgi?id=188984&action=review Thanks Eric! I've scheduled a VC tomorrow. >> Source/WebCore/ChangeLog:9 >> + The webkitFontloader implements the FontLoader interface defined in CSS Fonts Module Level 3.[1] > > My understanding was that the standards community was moving away from using prefixes. If this is spec'd, we should just use the real names? I just thought it's safer to use prefixes until the spec goes to CR because fontloader spec is still actively being developed, as jdaggett noted on this thread. I'm not sure of WebKit policy about prefixing so let me know if we should use real names. >> Source/WebCore/bindings/js/JSDictionary.cpp:236 >> +} > > I'm confused as to what these do or why they're needed? Some artifact of how JSC bindings are implemented? Yeah they are needed when we extract values of these types from JS dictionary. See the comment below in WebKitFontLoader.cpp. >> Source/WebCore/bindings/v8/Dictionary.cpp:519 >> + source = V8CSSFontFaceRule::toNative(fontface); > > Same question. I'm confused by what this custom bindings code is doing/why it's needed? See below. >> Source/WebCore/css/WebKitFontLoader.cpp:67 >> + if (!m_numLoading) { > > I believe webkit style is to early return instead of makign a long block. Will fix, thanks. > Source/WebCore/css/WebKitFontLoader.cpp:215 > + params.get("onsuccess", onsuccess); JSDictionary::convertValue (in JSC) and Dictionary::get (in V8) are used here, to extract VoidCallback value from a Dictionary. Similarly, extract methods for CSSFontFaceRule and DOMError are needed by the generated code for WebKitCSSFontFaceLoadEvent.

(In reply to comment #22) > (From update of attachment 188984 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188984&action=review > >> Source/WebCore/ChangeLog:9 > >> + The webkitFontloader implements the FontLoader interface defined in CSS Fonts Module Level 3.[1] > > > > My understanding was that the standards community was moving away from using prefixes. If this is spec'd, we should just use the real names? > > I just thought it's safer to use prefixes until the spec goes to CR because fontloader spec is still actively being developed, as jdaggett noted on this thread. I'm not sure of WebKit policy about prefixing so let me know if we should use real names. Current best-practice is to use the real names, but to guard everything behind a runtime flag. That runtime flag will be off by default, and turning it on for stable chrome is guarded by a separate feature process which we're working on documenting. :) We're trying to avoid further web-fragmentation from webdevs depending on prefixed features in shipping browsers.

(In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 188984 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=188984&action=review > > >> Source/WebCore/ChangeLog:9 > > >> + The webkitFontloader implements the FontLoader interface defined in CSS Fonts Module Level 3.[1] > > > > > > My understanding was that the standards community was moving away from using prefixes. If this is spec'd, we should just use the real names? > > > > I just thought it's safer to use prefixes until the spec goes to CR because fontloader spec is still actively being developed, as jdaggett noted on this thread. I'm not sure of WebKit policy about prefixing so let me know if we should use real names. > > Current best-practice is to use the real names, but to guard everything behind a runtime flag. That runtime flag will be off by default, and turning it on for stable chrome is guarded by a separate feature process which we're working on documenting. :) We're trying to avoid further web-fragmentation from webdevs depending on prefixed features in shipping browsers. This does not match my understanding of the prefix policy. Please discuss in webkit-dev

Comment on attachment 188984 [details] Updated patch, fix test failure View in context: https://bugs.webkit.org/attachment.cgi?id=188984&action=review This patch looks great. Thank you for taking the time to speak with me this afternoon. For non-chromium ports, this will be off by default (the #define will be false). For Chromium, this will be enabled, but we need to guard it with a feature flag. In order to test your feature, you will need to create a setting and expose it via internal settings, similar to what we did with the threaded HTML parser. I recommend that you post and land the feature flag and setting first. That will ensure that your tests require the setting to expose this to the web. Once we believe this feature is stable and ready for release we'll change the runtime flag to default to true. A little while later, we'll remove the flag. Unfortunately this process is not quite as refined as we'd like, so adding the setting is a bit of boiler plate code, but much less code than you've already written for this. Thank you again for your work here. Many websites will be very happy to see this! I've marked this r- for now, as we should land this with the real names and with the runtime flag. However I'm happy to r+ any follow-up patch immediately with those fixes. > Source/WebCore/css/CSSFontFace.cpp:172 > + Document* document = (*m_segmentedFontFaces.begin())->fontSelector()->document(); I don't think the () are necessary? They don't hurt, but I don't think c++ requires them.

(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > (From update of attachment 188984 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=188984&action=review > > > >> Source/WebCore/ChangeLog:9 > > > >> + The webkitFontloader implements the FontLoader interface defined in CSS Fonts Module Level 3.[1] > > > > > > > > My understanding was that the standards community was moving away from using prefixes. If this is spec'd, we should just use the real names? > > > > > > I just thought it's safer to use prefixes until the spec goes to CR because fontloader spec is still actively being developed, as jdaggett noted on this thread. I'm not sure of WebKit policy about prefixing so let me know if we should use real names. > > > > Current best-practice is to use the real names, but to guard everything behind a runtime flag. That runtime flag will be off by default, and turning it on for stable chrome is guarded by a separate feature process which we're working on documenting. :) We're trying to avoid further web-fragmentation from webdevs depending on prefixed features in shipping browsers. > > This does not match my understanding of the prefix policy. Please discuss in webkit-dev You're correct that the webkit project does not seem to have a coherent policy for vendor prefixes. You're correct that the CSS WG is very clear on their policy for prefixes with Working Draft specs: http://wiki.csswg.org/spec/vendor-prefixes We've been discussing this some as of late, and the current thinking is that for chromium-developed features, we would like to try and follow mozilla's lead on reducing vendor prefixing as much as possible. Which means implementing the feature without prefixes, and guarding them with runtime flags to allow us to test interoperably with other implementations w/o prefixes, while still ensuring these features are off in stable browsers. As I'm sure you're aware, this is a hotly discussed topic (on lists that you likely read, including http://lists.w3.org/Archives/Public/www-style/2012May/0324.html). I'm less interested in trying to bring cohesion to our currently divergent feature policies, but rather take this approach for this approach for this feature. Obviously this means that we cannot ship this feature in stable Chrome in compliance with the current CSS WG recommendations. Hence the requirement of a runtime flag so that we can continue to ship with this feature off by default. I think we should have another discussion about vendor prefixing at the next webkit meeting: http://trac.webkit.org/wiki/Deprecating%20features%20and%20vendor%20prefixes but I don't think that should stop this patch from landing before then.

(In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 188984 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=188984&action=review > > >> Source/WebCore/ChangeLog:9 > > >> + The webkitFontloader implements the FontLoader interface defined in CSS Fonts Module Level 3.[1] > > > > > > My understanding was that the standards community was moving away from using prefixes. If this is spec'd, we should just use the real names? > > > > I just thought it's safer to use prefixes until the spec goes to CR because fontloader spec is still actively being developed, as jdaggett noted on this thread. I'm not sure of WebKit policy about prefixing so let me know if we should use real names. > > Current best-practice is to use the real names, but to guard everything behind a runtime flag. That runtime flag will be off by default, and turning it on for stable chrome is guarded by a separate feature process which we're working on documenting. :) We're trying to avoid further web-fragmentation from webdevs depending on prefixed features in shipping browsers. The normal policy for the CSS WG is that a prefix should be used until the spec [1] is in CR or until the WG otherwise declares it sufficiently mature to unprefix. [1] http://drafts.csswg.org/css-font-load-events-3/ For the Font Loader and related interfaces defined by CSS3 Fonts, I would recommend that the following prefixed forms be used: * document.webkitFontLoader * WebKitCSSFontFaceLoadEventInit * WebKitLoadFontParameters * WebKitCSSFontFaceLoadEvent * WebKitCSSFontsReadyCallback * WebKitFontLoader You might also need to prefix any event type strings that are newly introduced by the spec [1] that don't already have a standard use in HTML5. See also [2] for guidance. [2] https://trac.webkit.org/wiki/PrefixedAPIs

> The normal policy for the CSS WG is that a prefix should be used until the spec [1] is in CR or until the WG otherwise declares it sufficiently mature to unprefix. That's the policy for shipping a feature, not for writing code. The plan here is keep the code disabled until it's ready to ship.

(In reply to comment #28) > > The normal policy for the CSS WG is that a prefix should be used until the spec [1] is in CR or until the WG otherwise declares it sufficiently mature to unprefix. > > That's the policy for shipping a feature, not for writing code. The plan here is keep the code disabled until it's ready to ship. That may be your Chromium policy decision, but it's not a WebKit policy decision.

Created attachment 192168 [details] Updated patch

Comment on attachment 188984 [details] Updated patch, fix test failure View in context: https://bugs.webkit.org/attachment.cgi?id=188984&action=review Changes from previous patch: - Removed prefixes (we can use ImplementedAs IDL attribute to expose prefixed names if necessary) - Added guards by the runtime flag - Enabled the runtime flag for TestShell - Updated compiler flag name (ENABLE_FONT_LOADER -> ENABLE_FONT_LOAD_EVENTS) >> Source/WebCore/css/CSSFontFace.cpp:172 >> + Document* document = (*m_segmentedFontFaces.begin())->fontSelector()->document(); > > I don't think the () are necessary? They don't hurt, but I don't think c++ requires them. It doesn't compile without the (); -> is stronger than *.

Apologies Kunihiko for failing to follow-up sooner. Thank you for adding the runtime and compile flags for this feature. Thank you also for changing from using prefixed class names, to unprefixed ones, of all the prefixing changes having the implementation classes unprefixed is most important to me. (Prefixes on the names of CPP files just makes work for us later when we decide to remove them.) I'm not particularly interested in using this bug as a platform for arguing whether JS/CSS features should be prefixed in WebKit IDL files or not. As you and I discussed offline, toggling the prefixing of this feature is a trivial change to our IDL files, thanks to our implementedAs IDL attribute: http://trac.webkit.org/wiki/WebKitIDL#ImplementedAs For Chromium prefixes are not particularly useful as we lean heavily on our runtime flags instead of prefixes to avoid shipping incomplete features to the web during development. Other consumers of WebKit may follow different policies for their own products, and thus it may be desirable for us to now or later change these few IDL attributes/classes to have implementedAs attributes, whether gated by platform #ifdefs or not. I will look again at the actual implementation of your patch in the morning. Thank you again for your hard work on this.

I asked on chromium-dev, and am told we should add a line to chromestatus.com for this feature. I believe you should be able to do so using your @chromium account and the link to the spreadsheet. That will help make sure we're publicly accountable, and tracking the feature so we don't ship it before other browsers or the w3c are ready, etc. See: https://docs.google.com/document/d/1jrSlM4Yhae7XCJ8BuasWx71CvDEMMbSKbXwx7hoh1Co/edit

Also, as I mentioned last night, I'll review your patch again shortly. Apologies for all the process goop.

Comment on attachment 192168 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=192168&action=review I need to read this again, but hopefully these comments are helpful. I'm most concerned about possible reference-cycles, but I think I've mostly convinced myself that we don't have any here. Also, I want to check in my next pass to make sure that we're using all the internal "Style" classes instead of the "CSS" classes when possible. CSS classes implement the CSS Object Model, where as the Style classes are all our internal data structures. I suspect this patch got that all correct, but I will need to re-read to make sure. > Source/WebCore/bindings/v8/Dictionary.cpp:508 > +bool Dictionary::get(const String& key, RefPtr<CSSFontFaceRule>& value) const I've filed bug 111911 about all this boilerplate that we sadly have to write for each change like this. :( > Source/WebCore/css/CSSFontFace.h:95 > + CSSFontFace(FontTraitsMask traitsMask, PassRefPtr<CSSFontFaceRule> rule, bool isLocalFallback) I assume the ownership is such here that this does not create a cycle? I guess the StyleResolver is the only thing which holds onto CSSFontFaces, so we should be OK? https://code.google.com/p/chromium/codesearch#search/&q=RefPtr%3CCSSFontFace%3E&sq=package:chromium&type=cs > Source/WebCore/css/CSSFontFaceLoadEvent.h:49 > +struct CSSFontFaceLoadEventInit : public EventInit { > + RefPtr<CSSFontFaceRule> fontface; > + RefPtr<DOMError> error; > +}; It's kinda unfortunate that we even need this sort of thing in C++ at all. These types of objects would be much easier to create directly in JavaScript if we had support for such. :) > Source/WebCore/css/CSSFontSelector.h:59 > + CSSSegmentedFontFace* getFontFace(const FontDescription&, const AtomicString&); Might want to label that the AtomicString is the familyName. > Source/WebCore/css/FontLoader.cpp:56 > + LoadFontCallback(int numLoading, PassRefPtr<VoidCallback> loadCallback, PassRefPtr<VoidCallback> errorCallback) : m_numLoading(numLoading), m_errorOccured(false), m_loadCallback(loadCallback), m_errorCallback(errorCallback) { } This woudl be nicer written out in many lines, IMO. LoadFontCallback(int numLoading, PassRefPtr<VoidCallback> loadCallback, PassRefPtr<VoidCallback> errorCallback) : m_numLoading(numLoading) , m_errorOccured(false) , m_loadCallback(loadCallback) , m_errorCallback(errorCallback) { } > Source/WebCore/css/FontLoader.cpp:210 > + // FIXME: the text member of params is ignored. Nit: This should start with a capital letter since it's a sentence. > Source/WebCore/css/FontLoader.cpp:214 > + RefPtr<VoidCallback> onsuccess; > + RefPtr<VoidCallback> onerror; > + if (!params.get("font", fontString)) It seems all this callback lookup stuff could move into the same function which does the LoadFontCallback create. Maybe just a helper function (static inline or member function on FontLoader) which takes the params, and returns a FontLoadCallback or nullptr. > Source/WebCore/css/FontLoader.cpp:217 > + params.get("onsuccess", onsuccess); > + params.get("onerror", onerror); Very odd that this uses a mutable RefPtr instead of being smart enough to return a PassRefPTr. These are things which could easily be fixed by autogeneraton when we get there.. > Source/WebCore/css/FontLoader.cpp:225 > + for (const FontFamily* f = &font.family(); f; f = f->next()) > + numFamilies++; I'm surprised we don't already have a method for counting how long a font fallback list is. > Source/WebCore/css/FontLoader.cpp:242 > + // FIXME: the second parameter (text) is ignored. Same nit. > Source/WebCore/css/FontLoader.cpp:254 > +bool FontLoader::resolveFontStyle(const String& fontString, Font& font) This is a surprisingly long function. > Source/WebCore/css/FontLoader.cpp:263 > + if (fontValue == "inherit" || fontValue == "initial") I'm surprised we don't have AtomicString contstants for these values. > Source/WebCore/css/FontLoader.cpp:285 > + styleResolver->applyPropertyToCurrentStyle(CSSPropertyFontStyle, parsedStyle->getPropertyCSSValue(CSSPropertyFontStyle).get()); > + styleResolver->applyPropertyToCurrentStyle(CSSPropertyFontVariant, parsedStyle->getPropertyCSSValue(CSSPropertyFontVariant).get()); > + styleResolver->applyPropertyToCurrentStyle(CSSPropertyFontWeight, parsedStyle->getPropertyCSSValue(CSSPropertyFontWeight).get()); If we don't already have a helper function, we shoudl make one. applyPropertyToCurrentStyle(styleResolver, CSSPropertyFontStyle, parsedStyle) is much shorter. > Source/WebCore/css/FontLoader.h:77 > + Document* document() { return m_document; } const > Source/WebCore/dom/EventNames.h:88 > + macro(loading) \ > + macro(loadingdone) \ It seems odd that CSS didn't just reuse "load" to mean "loading done".

Created attachment 192437 [details] Updated patch

Comment on attachment 192168 [details] Updated patch Thanks for the comments! I've added a line to the chromestatus spreadsheet. View in context: https://bugs.webkit.org/attachment.cgi?id=192168&action=review >> Source/WebCore/bindings/v8/Dictionary.cpp:508 >> +bool Dictionary::get(const String& key, RefPtr<CSSFontFaceRule>& value) const > > I've filed bug 111911 about all this boilerplate that we sadly have to write for each change like this. :( Thanks! >> Source/WebCore/css/CSSFontFace.h:95 >> + CSSFontFace(FontTraitsMask traitsMask, PassRefPtr<CSSFontFaceRule> rule, bool isLocalFallback) > > I assume the ownership is such here that this does not create a cycle? I guess the StyleResolver is the only thing which holds onto CSSFontFaces, so we should be OK? > https://code.google.com/p/chromium/codesearch#search/&q=RefPtr%3CCSSFontFace%3E&sq=package:chromium&type=cs Yeah only StyleResolver (transitively) holds CSSFontFace, and I don't think CSSFontFaceRule holds StyleResolver. >> Source/WebCore/css/CSSFontSelector.h:59 >> + CSSSegmentedFontFace* getFontFace(const FontDescription&, const AtomicString&); > > Might want to label that the AtomicString is the familyName. Done. >> Source/WebCore/css/FontLoader.cpp:56 >> + LoadFontCallback(int numLoading, PassRefPtr<VoidCallback> loadCallback, PassRefPtr<VoidCallback> errorCallback) : m_numLoading(numLoading), m_errorOccured(false), m_loadCallback(loadCallback), m_errorCallback(errorCallback) { } > > This woudl be nicer written out in many lines, IMO. > > LoadFontCallback(int numLoading, PassRefPtr<VoidCallback> loadCallback, PassRefPtr<VoidCallback> errorCallback) > : m_numLoading(numLoading) > , m_errorOccured(false) > , m_loadCallback(loadCallback) > , m_errorCallback(errorCallback) > { } Done. >> Source/WebCore/css/FontLoader.cpp:210 >> + // FIXME: the text member of params is ignored. > > Nit: This should start with a capital letter since it's a sentence. Done. >> Source/WebCore/css/FontLoader.cpp:214 >> + if (!params.get("font", fontString)) > > It seems all this callback lookup stuff could move into the same function which does the LoadFontCallback create. Maybe just a helper function (static inline or member function on FontLoader) which takes the params, and returns a FontLoadCallback or nullptr. Moved to LoadFontCallback::createFromParams(). >> Source/WebCore/css/FontLoader.cpp:242 >> + // FIXME: the second parameter (text) is ignored. > > Same nit. Done. >> Source/WebCore/css/FontLoader.cpp:254 >> +bool FontLoader::resolveFontStyle(const String& fontString, Font& font) > > This is a surprisingly long function. Most part of this function was copied from CanvasRenderingContext2D::setFont, which is longer than this... >> Source/WebCore/css/FontLoader.cpp:285 >> + styleResolver->applyPropertyToCurrentStyle(CSSPropertyFontWeight, parsedStyle->getPropertyCSSValue(CSSPropertyFontWeight).get()); > > If we don't already have a helper function, we shoudl make one. > > applyPropertyToCurrentStyle(styleResolver, CSSPropertyFontStyle, parsedStyle) is much shorter. Done. >> Source/WebCore/css/FontLoader.h:77 >> + Document* document() { return m_document; } > > const Done. >> Source/WebCore/dom/EventNames.h:88 >> + macro(loadingdone) \ > > It seems odd that CSS didn't just reuse "load" to mean "loading done". That is because the spec already uses "load" as per-font load event. http://dev.w3.org/csswg/css-font-load-events-3/#events

Comment on attachment 192437 [details] Updated patch Attachment 192437 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17111434

Created attachment 192442 [details] Updated patch, fix build error

Comment on attachment 192442 [details] Updated patch, fix build error View in context: https://bugs.webkit.org/attachment.cgi?id=192442&action=review This patch looks great. Please reply to my comments before landing. As I mentioned before, we may need to make a follow-up patch to use implemntedAs for some ports, if other ports want to turn this on for testing before other browsers have implemented this. > Source/WebCore/css/CSSFontFaceLoadEvent.cpp:43 > + : Event(type, false, false) We need to fix these to not use Bools some day. I assume that means this event doesn't bubble? > Source/WebCore/css/CSSFontSelector.cpp:232 > + // FIXME: This CSSFontFaceRule has no parent. Could you file a bug and link it from here, or otherwise explain more about how this should be fixed? I assume this method would need more context to construct the rule properly? > Source/WebCore/css/CSSFontSelector.cpp:503 > + String family = familyName.string(); I think this can be const String&? I'm confused why it even needs to be a String and not an AtomicString. > Source/WebCore/css/CSSFontSelector.cpp:505 > + Vector<RefPtr<CSSFontFace> >* familyFontFaces = m_fontFaces.get(family); Isn't familyName only used on this line? Can we just get rid of the local? > Source/WebCore/css/CSSSegmentedFontFace.cpp:80 > + Vector<RefPtr<LoadFontCallback> > callbacks; > + m_callbacks.swap(callbacks); I assume we're doing this so that modifications to the fontLoader during a notify do not cause additional notifies to fire, correct? > Source/WebCore/css/CSSSegmentedFontFace.cpp:175 > + getFontData(fontDescription); Why does this not return anything? "get" seems like a poor name for thsi function (obviously this pre-dates your patch, and I'm not asknig you to change it... just noticing). This seems like more of an "ensure" function? > Source/WebCore/css/FontLoader.cpp:45 > +static const int defaultFontSize = 10; > +static const char* const defaultFontFamily = "sans-serif"; I assume these are dictacted by the spec and should not be shared with some other portion of our engine? Also, I am surprised we don't already have a global AtomicString for sans-serif. maybe it's just used as a function-local static other places. > Source/WebCore/css/FontLoader.cpp:54 > + static PassRefPtr<LoadFontCallback> createFromParams(const Dictionary& params, const FontFamily& family) Oooh, this is nice. Thank you. > Source/WebCore/css/FontLoader.cpp:125 > +EventTargetData* FontLoader::eventTargetData() > +{ > + return &m_eventTargetData; > +} > + > +EventTargetData* FontLoader::ensureEventTargetData() > +{ > + return &m_eventTargetData; > +} This feels a bit odd. I guess no caller ever holds onto this data? Should these return const EventTargetData*? > Source/WebCore/css/FontLoader.cpp:189 > + String errorName = (source && source->isDecodeError()) ? "InvalidFontDataError" : "NotFoundError"; I'm a little surprised these strings don't already exist somewhere we should share. > Source/WebCore/css/FontLoader.cpp:237 > + RefPtr<LoadFontCallback> callback = LoadFontCallback::createFromParams(params, font.family()); This is much nicer, thank you.

Created attachment 192681 [details] Updated patch

Comment on attachment 192442 [details] Updated patch, fix build error View in context: https://bugs.webkit.org/attachment.cgi?id=192442&action=review >> Source/WebCore/css/CSSFontFaceLoadEvent.cpp:43 >> + : Event(type, false, false) > > We need to fix these to not use Bools some day. I assume that means this event doesn't bubble? Right. It doesn't bubble, not cancelable. >> Source/WebCore/css/CSSFontSelector.cpp:232 >> + // FIXME: This CSSFontFaceRule has no parent. > > Could you file a bug and link it from here, or otherwise explain more about how this should be fixed? I assume this method would need more context to construct the rule properly? Done. >> Source/WebCore/css/CSSFontSelector.cpp:503 >> + String family = familyName.string(); > > I think this can be const String&? I'm confused why it even needs to be a String and not an AtomicString. Apparently there is no need for String. Got rid of this. >> Source/WebCore/css/CSSSegmentedFontFace.cpp:80 >> + m_callbacks.swap(callbacks); > > I assume we're doing this so that modifications to the fontLoader during a notify do not cause additional notifies to fire, correct? Correct. >> Source/WebCore/css/CSSSegmentedFontFace.cpp:175 >> + getFontData(fontDescription); > > Why does this not return anything? "get" seems like a poor name for thsi function (obviously this pre-dates your patch, and I'm not asknig you to change it... just noticing). This seems like more of an "ensure" function? Yeah this is to start the font load if not started yet. I agree this looks a little odd so I've added a comment here. >> Source/WebCore/css/FontLoader.cpp:45 >> +static const char* const defaultFontFamily = "sans-serif"; > > I assume these are dictacted by the spec and should not be shared with some other portion of our engine? Also, I am surprised we don't already have a global AtomicString for sans-serif. maybe it's just used as a function-local static other places. These are coming from the canvas spec. The font load events spec says: "These values must be parsed using the same syntax as values for the CSS ‘font’ property, the same way the font attribute of the CanvasRenderingContext2D is interpreted." Maybe I should create a function used by FontLoader and CanvasRenderingContext2D that resolves font styles, but not sure where that function should be placed. >> Source/WebCore/css/FontLoader.cpp:125 >> +} > > This feels a bit odd. I guess no caller ever holds onto this data? Should these return const EventTargetData*? These are kind of boilerplate code that EventTarget subclasses need. Caller modifies EventTargetData (adding/removing listeners), hence non-const. >> Source/WebCore/css/FontLoader.cpp:189 >> + String errorName = (source && source->isDecodeError()) ? "InvalidFontDataError" : "NotFoundError"; > > I'm a little surprised these strings don't already exist somewhere we should share. There is a table of standard DOMErrors in dom/DOMCoreException.cpp. Changed to use that. "InvalidFontDataError" is specific to the font load events, so I would like to leave it as a string literal for now.

Comment on attachment 192681 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=192681&action=review Thank you for all your hard work on this. > Source/WebCore/css/CSSFontFace.cpp:140 > + if (RuntimeEnabledFeatures::fontLoadEventsEnabled() && m_loadState == Loading) { You could have saved yourself a bit of typing by putting these runtimeenabledfeatures checks into the notify* methods. But this is fine too.

Comment on attachment 192681 [details] Updated patch Clearing flags on attachment: 192681 Committed r145787: <http://trac.webkit.org/changeset/145787>

All reviewed patches have been landed. Closing bug.