http://dev.w3.org/csswg/css3-fonts/#unicode-range-desc says: "User agents that download fonts for characters outside the defined unicode-range are considered non-conformant." WebKit r63184 always downloads Web Fonts, even when all the characters are outside the unicode-range. For example, in opening the following, WebKit downloads myfont.ttf, which it must not. <style> @font-face { font-family:myfont; src: url(myfont.ttf); unicode-range: U+FFFF; } </style> <span style="font-family:myfont"> No characters here are in the unicode-range. </span> The behavior can be confirmed by either - Setting a breakpoint at WebCore/css/CSSFontFaceSource.cpp:181, - Checking the server log, or - Monitoring the network traffic.
Created attachment 64292 [details] Patch
Attachment 64292 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3767106
Created attachment 64301 [details] Patch 2
Comment on attachment 64301 [details] Patch 2 I don’t think that the render tree is the right level to implement this. I also suspect that this approach is incomplete, because not all text that gets drawn comes from RenderText. I think the right approach would be to trigger the load from getGlyphDataForCharacter. Glyph pages could be populated with “faults”, and getGlyphDataForCharacter could “fire the faults”, which would kick off loading, and meanwhile return the temporary glyph data. SimpleFontData or a subclass thereof could hold the knowledge of how to fire the fault, probably just by holding a pointer to an instance of an abstract class (which CSSFontFaceSource would inherit from) that has a single method for doing this.
Comment on attachment 64301 [details] Patch 2 r-ing based on mitz's comments.
Created attachment 67022 [details] Patch
Hi, I've created a new patch based on your comment. Can you take a look? (In reply to comment #4) > (From update of attachment 64301 [details]) > I don’t think that the render tree is the right level to implement this. I also suspect that this approach is incomplete, because not all text that gets drawn comes from RenderText. I think the right approach would be to trigger the load from getGlyphDataForCharacter. Glyph pages could be populated with “faults”, and getGlyphDataForCharacter could “fire the faults”, which would kick off loading, and meanwhile return the temporary glyph data. SimpleFontData or a subclass thereof could hold the knowledge of how to fire the fault, probably just by holding a pointer to an instance of an abstract class (which CSSFontFaceSource would inherit from) that has a single method for doing this.
Attachment 67022 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3968356
Created attachment 67140 [details] Fix Chromium and Win compilation
Attachment 67022 [details] did not build on win: Build output: http://queues.webkit.org/results/3912397
*** Bug 42571 has been marked as a duplicate of this bug. ***
Ping?
Comment on attachment 67140 [details] Fix Chromium and Win compilation View in context: https://bugs.webkit.org/attachment.cgi?id=67140&action=review I'm not commenting on the patch as such, rather on the test technique. > LayoutTests/fast/css/custom-font-xheight.html:61 > +function test() { > + // Wait until the font download completes. > + // If there is better alternative, we should not use timer here. > + window.setTimeout(testMain, 100); This looks really suspicious. I wonder if we should delay firing the onload event, in case there's a web font used. According to http://paulirish.com/2009/fighting-the-font-face-fout/, this is what IE and Firefox are doing.
Ping, reviewers? As for the onload event firing commented in #13, I think it's too big to handle in this patch, if we need to.
Sorry for the nagging, but can anyone review this?
(In reply to comment #15) > Sorry for the nagging, but can anyone review this? This sounds like something Mitz would love to review. Mieeeeetz! Mieeeeetz!
Comment on attachment 67140 [details] Fix Chromium and Win compilation View in context: https://bugs.webkit.org/attachment.cgi?id=67140&action=review > WebCore/platform/graphics/GlyphPageTreeNode.h:82 > + GlyphData glyphDataForCharacter(UChar32 c, bool mayLoad) const; An enum is always better than a Bool (since the callsites are much more readable). Seems we might also want a default value of DoNotTriggerLoads or similar?
Created attachment 75669 [details] Use enum TriggerLoad
Thank you for the review. Introduced enum TriggerLoad.
Attachment 75669 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6743065
Attachment 75669 [details] did not build on win: Build output: http://queues.webkit.org/results/6758067
Created attachment 75762 [details] Fix Win/Linux Compilation
Created attachment 78500 [details] Pre Source/WebCore move
Created attachment 78637 [details] Post Source/WebCore move
Comment on attachment 78637 [details] Post Source/WebCore move View in context: https://bugs.webkit.org/attachment.cgi?id=78637&action=review I decided to get things moving myself, and I think I understand what's happening. Comments below: > Source/WebCore/ChangeLog:53 > + * WebCore.xcodeproj/project.pbxproj: > + * css/CSSFontFaceSource.cpp: > + (WebCore::CSSFontFaceSource::getFontData): > + * platform/graphics/FontFallbackList.cpp: > + (WebCore::FontFallbackList::releaseFontData): > + (WebCore::FontFallbackList::loadingCustomFonts): > + (WebCore::FontFallbackList::beforeDeleteSegmentedFontData): > + (WebCore::FontFallbackList::fontDataAt): > + (WebCore::FontFallbackList::setPlatformFont): > + (WebCore::FontFallbackList::appendFontData): > + * platform/graphics/FontFallbackList.h: > + * platform/graphics/FontFastPath.cpp: > + (WebCore::Font::glyphDataForCharacter): > + * platform/graphics/GlyphPageTreeNode.cpp: > + (WebCore::GlyphPage::glyphDataForCharacter): > + * platform/graphics/GlyphPageTreeNode.h: > + * platform/graphics/SegmentedFontData.cpp: > + (WebCore::SegmentedFontData::~SegmentedFontData): > + (WebCore::SegmentedFontData::isLoading): > + (WebCore::SegmentedFontData::registerObserver): > + (WebCore::SegmentedFontData::unregisterObserver): > + * platform/graphics/SegmentedFontData.h: > + (WebCore::SegmentedFontDataObserver::~SegmentedFontDataObserver): > + * platform/graphics/SimpleFontData.cpp: > + (WebCore::SimpleFontData::SimpleFontData): > + (WebCore::SimpleFontData::initCharWidths): > + (WebCore::SimpleFontData::platformGlyphInit): > + (WebCore::SimpleFontData::beginLoadIfNeeded): > + * platform/graphics/SimpleFontData.h: > + (WebCore::SimpleFontData::isUsed): > + * platform/graphics/chromium/SimpleFontDataLinux.cpp: > + (WebCore::SimpleFontData::platformInit): > + * platform/graphics/mac/SimpleFontDataMac.mm: > + (WebCore::SimpleFontData::platformInit): > + * platform/graphics/win/SimpleFontDataCGWin.cpp: > + (WebCore::SimpleFontData::platformInit): For a non-trivial patch like this, it would be good to explain changes for each file/method. See http://trac.webkit.org/changeset/75543 for an example. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:833 > - 371F4FFC0D25E7F300ECE0D5 /* SegmentedFontData.h in Headers */ = {isa = PBXBuildFile; fileRef = 371F4FFA0D25E7F300ECE0D5 /* SegmentedFontData.h */; }; > + 371F4FFC0D25E7F300ECE0D5 /* SegmentedFontData.h in Headers */ = {isa = PBXBuildFile; fileRef = 371F4FFA0D25E7F300ECE0D5 /* SegmentedFontData.h */; settings = {ATTRIBUTES = (Private, ); }; }; Why is this necessary? > Source/WebCore/platform/graphics/FontFallbackList.cpp:176 > + static_cast<const SegmentedFontData*>(fontData)->registerObserver(const_cast<SegmentedFontDataObserver*>(static_cast<const SegmentedFontDataObserver*>(this))); This double-static_cast seems sad. Could this be made more elegant? And don't say const_cast :) > Source/WebCore/platform/graphics/FontFallbackList.h:41 > +class FontFallbackList : public RefCounted<FontFallbackList>, public SegmentedFontDataObserver { Sounds like FontFallbackList is the only implementation of SegmentedFontDataObserver? Are we expecting to have more? If not, maybe a whole different interface is not necessary? But maybe having a nice separation of concerns is a good thing. > Source/WebCore/platform/graphics/GlyphPageTreeNode.h:63 > +enum TriggerLoad { This could be more descriptive. LoadBehavior? I dunno. > Source/WebCore/platform/graphics/SegmentedFontData.h:87 > + virtual void beforeDeleteSegmentedFontData(const SegmentedFontData*) = 0; In WebKit, it's typical to use "will" prefix, as in "willDeleteSegmentedFontData". > Source/WebCore/platform/graphics/SimpleFontData.h:273 > + CachedResourceLoader* m_cachedResourceLoader; I am worried about the lifetime of CachedResourceLoader vs. SimpleFontData. Can you explain how it's safe to hold to a raw ptr here?
Created attachment 79258 [details] Address comment #25
Thank you very much for the review. (In reply to comment #25) > (From update of attachment 78637 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78637&action=review > > I decided to get things moving myself, and I think I understand what's happening. Comments below: > > > Source/WebCore/ChangeLog:53 > > + * WebCore.xcodeproj/project.pbxproj: > > + * css/CSSFontFaceSource.cpp: > > + (WebCore::CSSFontFaceSource::getFontData): > > + * platform/graphics/FontFallbackList.cpp: > > + (WebCore::FontFallbackList::releaseFontData): > > + (WebCore::FontFallbackList::loadingCustomFonts): > > + (WebCore::FontFallbackList::beforeDeleteSegmentedFontData): > > + (WebCore::FontFallbackList::fontDataAt): > > + (WebCore::FontFallbackList::setPlatformFont): > > + (WebCore::FontFallbackList::appendFontData): > > + * platform/graphics/FontFallbackList.h: > > + * platform/graphics/FontFastPath.cpp: > > + (WebCore::Font::glyphDataForCharacter): > > + * platform/graphics/GlyphPageTreeNode.cpp: > > + (WebCore::GlyphPage::glyphDataForCharacter): > > + * platform/graphics/GlyphPageTreeNode.h: > > + * platform/graphics/SegmentedFontData.cpp: > > + (WebCore::SegmentedFontData::~SegmentedFontData): > > + (WebCore::SegmentedFontData::isLoading): > > + (WebCore::SegmentedFontData::registerObserver): > > + (WebCore::SegmentedFontData::unregisterObserver): > > + * platform/graphics/SegmentedFontData.h: > > + (WebCore::SegmentedFontDataObserver::~SegmentedFontDataObserver): > > + * platform/graphics/SimpleFontData.cpp: > > + (WebCore::SimpleFontData::SimpleFontData): > > + (WebCore::SimpleFontData::initCharWidths): > > + (WebCore::SimpleFontData::platformGlyphInit): > > + (WebCore::SimpleFontData::beginLoadIfNeeded): > > + * platform/graphics/SimpleFontData.h: > > + (WebCore::SimpleFontData::isUsed): > > + * platform/graphics/chromium/SimpleFontDataLinux.cpp: > > + (WebCore::SimpleFontData::platformInit): > > + * platform/graphics/mac/SimpleFontDataMac.mm: > > + (WebCore::SimpleFontData::platformInit): > > + * platform/graphics/win/SimpleFontDataCGWin.cpp: > > + (WebCore::SimpleFontData::platformInit): > > For a non-trivial patch like this, it would be good to explain changes for each file/method. See http://trac.webkit.org/changeset/75543 for an example. Added comments. > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:833 > > - 371F4FFC0D25E7F300ECE0D5 /* SegmentedFontData.h in Headers */ = {isa = PBXBuildFile; fileRef = 371F4FFA0D25E7F300ECE0D5 /* SegmentedFontData.h */; }; > > + 371F4FFC0D25E7F300ECE0D5 /* SegmentedFontData.h in Headers */ = {isa = PBXBuildFile; fileRef = 371F4FFA0D25E7F300ECE0D5 /* SegmentedFontData.h */; settings = {ATTRIBUTES = (Private, ); }; }; > > Why is this necessary? SegmentedFontData.h is now included in FontFallbackList.h, which is copied to PrivateHeaders directory at build time. If we don't copy SegmentedFontData.h also, compilation errors would result. > > > Source/WebCore/platform/graphics/FontFallbackList.cpp:176 > > + static_cast<const SegmentedFontData*>(fontData)->registerObserver(const_cast<SegmentedFontDataObserver*>(static_cast<const SegmentedFontDataObserver*>(this))); > > This double-static_cast seems sad. Could this be made more elegant? And don't say const_cast :) Removed casting by not using SegmentedFontDataObserver interface. > > > Source/WebCore/platform/graphics/FontFallbackList.h:41 > > +class FontFallbackList : public RefCounted<FontFallbackList>, public SegmentedFontDataObserver { > > Sounds like FontFallbackList is the only implementation of SegmentedFontDataObserver? Are we expecting to have more? If not, maybe a whole different interface is not necessary? But maybe having a nice separation of concerns is a good thing. As mentioned above, stopped using the interface. > > > Source/WebCore/platform/graphics/GlyphPageTreeNode.h:63 > > +enum TriggerLoad { > > This could be more descriptive. LoadBehavior? I dunno. Renamed to CharacterSource, which the caller should know. (Caller may not have any idea if load will be triggered or not, on the other hand.) > > > Source/WebCore/platform/graphics/SegmentedFontData.h:87 > > + virtual void beforeDeleteSegmentedFontData(const SegmentedFontData*) = 0; > > In WebKit, it's typical to use "will" prefix, as in "willDeleteSegmentedFontData". Renamed as suggested. > > > Source/WebCore/platform/graphics/SimpleFontData.h:273 > > + CachedResourceLoader* m_cachedResourceLoader; > > I am worried about the lifetime of CachedResourceLoader vs. SimpleFontData. Can you explain how it's safe to hold to a raw ptr here? m_cachedResourceLoader has the same lifetime as the document, and SimpleFontData created here never outlives the document. (Some other SImpleFontData can be put into global cache and live longer than each document, though.) But I admit the assumption can be broken by future changes.
Any thoughts Hyatt? This seems like a relatively small patch.
See also bug 61561.
Comment on attachment 79258 [details] Address comment #25 View in context: https://bugs.webkit.org/attachment.cgi?id=79258&action=review Looks decent. You can avoid having to change SimpleFontData (and all the platform-specific files) with my suggestions below and make the patch much simpler/smaller. > Source/WebCore/css/CSSFontFaceSource.cpp:181 > + // Store cached font and cached resource loader in SimpleFontData to be used for on-demand web font loading. > + fontData.set(new SimpleFontData(tempData->platformData(), true, true, m_font.get(), fontSelector->cachedResourceLoader())); You don't need to cache the resource loader in the FontData. > Source/WebCore/platform/graphics/FontFastPath.cpp:79 > + GlyphData data = page->glyphDataForCharacter(c, CharacterFromDocument); Make a static helper function in FontFastPath.cpp that takes the Font and the Page and the character and hands you back the GlyphData. The Font is able to get to the cached resource loader, since the Font can get to the FontSelector. This way you don't have to cache the resource loader ever, and you also don't need to patch glyphDataForCharacter. > Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:57 > +GlyphData GlyphPage::glyphDataForCharacter(UChar32 c, CharacterSource characterSource) const > +{ > + unsigned index = indexForCharacter(c); > + > + if (characterSource == CharacterFromDocument) > + if (const SimpleFontData* fontData = m_glyphFontData[index]) > + fontData->beginLoadIfNeeded(); > + > + return GlyphData(m_glyphs[index], m_glyphFontData[index]); > +} This can just be a static function in FontFastPath.cpp, and you don't need this CharacterSource stuff. > Source/WebCore/platform/graphics/SimpleFontData.cpp:63 > + , m_isUsed(false) > + , m_font(font) > + , m_cachedResourceLoader(cachedResourceLoader) You shouldn't need any of this. > Source/WebCore/platform/graphics/SimpleFontData.cpp:219 > +void SimpleFontData::beginLoadIfNeeded() const > +{ > + if (!m_isUsed && isCustomFont() && isLoading() && m_font && m_cachedResourceLoader) { > + m_isUsed = true; > + m_font->beginLoadIfNeeded(m_cachedResourceLoader); > + } > +} Can just be in FontFastPath instead in the helper function. > Source/WebCore/platform/graphics/SimpleFontData.h:75 > + SimpleFontData(const FontPlatformData&, bool isCustomFont = false, bool isLoading = false, CachedFont* = 0, CachedResourceLoader* = 0); See previous comments, but you shouldn't have to change SimpleFontData at all.
Thank you very much for the review. (In reply to comment #30) > > Source/WebCore/platform/graphics/FontFastPath.cpp:79 > > + GlyphData data = page->glyphDataForCharacter(c, CharacterFromDocument); > > Make a static helper function in FontFastPath.cpp that takes the Font and the Page and the character and hands you back the GlyphData. The Font is able to get to the cached resource loader, since the Font can get to the FontSelector. This way you don't have to cache the resource loader ever, and you also don't need to patch glyphDataForCharacter. I think the following pieces of information would be needed in the helper function: 1. The URL of the font (or a CachedFont instance) 2. Whether the font is already scheduled for loading Can you advise me where to put them (or how to retrieve them)? Does it make sense to put them into SimpleFontData?
Another, probably more difficult, issue is how to know whether all the characters in the document have been scanned to retrieve the GlyphData. Without knowing that, it is hard to tell if a certain web font is not used in the document or characters that use the font are yet to be scanned. Put differently, it is hard to determine the return value of SimpleFontData::isLoading(). Perhaps we can determine the font usage at the completion of document loading. But I'm not sure if it is feasible. I'm also not sure if we can handle text manipulation by JavaScript. I would appreciate experts' advice on this.
I release the ownership of this bug.
I'm pretty sure this used to do the right thing in stable about a year ago. Regression?
*** Bug 135976 has been marked as a duplicate of this bug. ***
<rdar://problem/18030068>
rdar://18030068 was closed as a duplicate of rdar://17779042. I've asked for a status update since Chrome shipped this over a year ago and Firefox is about to ship it in version 44. I've updated my test page with the current support status: http://chris.improbable.org/experiments/browser/webfonts/universal-noto-sans/
rdar://problem/17779042
(In reply to comment #37) > rdar://18030068 was closed as a duplicate of rdar://17779042. I've asked for > a status update since Chrome shipped this over a year ago and Firefox is > about to ship it in version 44. > > I've updated my test page with the current support status: > > http://chris.improbable.org/experiments/browser/webfonts/universal-noto-sans/ Thanks for the test page! This will be super useful when implementing.
*** Bug 122851 has been marked as a duplicate of this bug. ***
Created attachment 278237 [details] Needs tests
Comment on attachment 278237 [details] Needs tests View in context: https://bugs.webkit.org/attachment.cgi?id=278237&action=review > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=42154 Needs the radar number
Comment on attachment 278237 [details] Needs tests View in context: https://bugs.webkit.org/attachment.cgi?id=278237&action=review > Source/WebCore/css/CSSSegmentedFontFace.cpp:106 > + ranges.appendRange(FontRanges::Range(unicodeRanges[j].from(), unicodeRanges[j].to(), WTFMove(fontAccessor))); This needs to copy the ref, not move it.
<rdar://problem/17779042>
Created attachment 278297 [details] WIP
Comment on attachment 278297 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=278297&action=review > Source/WebCore/css/CSSSegmentedFontFace.cpp:136 > + return WTFMove(fontRanges); This move also moves out of the cache, making the cache cache empty ranges!
Created attachment 278333 [details] WIP
Created attachment 278341 [details] Patch
Created attachment 278342 [details] Patch
Created attachment 278343 [details] Patch
Comment on attachment 278343 [details] Patch Attachment 278343 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1284221 New failing tests: fast/text/unicode-range-download.html
Created attachment 278346 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 278343 [details] Patch Attachment 278343 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1284234 New failing tests: fast/text/unicode-range-download.html
Created attachment 278347 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 278343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278343&action=review Looks like some tests are failing. Maybe null dereferences? > Source/WebCore/css/CSSSegmentedFontFace.cpp:62 > +class CSSFontAccessor : public FontAccessor { final? > Source/WebCore/css/CSSSegmentedFontFace.cpp:69 > + const Font* access() const override final instead of override? > Source/WebCore/css/CSSSegmentedFontFace.cpp:85 > + bool isLoading() const override final instead of override? > Source/WebCore/platform/graphics/FontRanges.cpp:45 > +class TrivialFontAccessor : public FontAccessor { final? > Source/WebCore/platform/graphics/FontRanges.cpp:47 > + static Ref<TrivialFontAccessor> create(RefPtr<Font>&& font) Does this need to be able to take a null? Maybe a Ref instead of a RefPfr for the argument? > Source/WebCore/platform/graphics/FontRanges.cpp:58 > + const Font* access() const override final instead of override? > Source/WebCore/platform/graphics/FontRanges.cpp:65 > + return m_font->isLoading(); final instead of override? > Source/WebCore/platform/graphics/FontRanges.cpp:85 > + if (const auto* font = range.font()) { I don’t think the const here has value and I would omit it. > Source/WebCore/platform/graphics/FontRanges.cpp:102 > - return m_ranges[0].font(); > + const Font* font = m_ranges[0].fontAccessor().access(); Should just be: auto* m_ranges[0].font(); No reason to write out the direct use of fontAccessor, I don’t think. > Source/WebCore/platform/graphics/FontRanges.h:49 > + : m_from(range.from()) > + , m_to(range.to()) It would be better style to get at the data members directly rather than using the accessor functions. Makes it more clear that the only reason this has to be defined is because of the explicit copyRef that is needed to copy a Ref. > Source/WebCore/platform/graphics/FontRanges.h:54 > + Range(Range&&) = default; There is no good reason to include this. It will be automatically generated whether we mention it or not. > Source/WebCore/platform/graphics/FontRanges.h:55 > + Range& operator=(const Range&) = delete; We don’t need to delete this. The Ref class already won’t support this so it can’t be called, even if we don’t explicitly delete it. > Source/WebCore/platform/graphics/FontRanges.h:56 > + Range& operator=(Range&&) = default; There is no good reason to include this. It will be automatically generated whether we mention it or not. > Source/WebCore/platform/graphics/FontRanges.h:74 > + FontRanges(const FontRanges&) = default; > + FontRanges& operator=(FontRanges&&) = default; Can we please omit these? It’s really peculiar to explicitly mention the *copy* constructor and *move* assignment operator. I believe that the move constructor and copy assignment operator will also work; the fact that those two are not mentioned has no effect and the fact that these other two two are mentioned also has no effect. > Source/WebCore/platform/graphics/FontRanges.h:80 > unsigned size() const { return m_ranges.size(); } > const Range& rangeAt(unsigned i) const { return m_ranges[i]; } It’s a little peculiar that we use a private vector here. There’s very little encapsulation provided by having this be a class. It might be better to either use a Vector directly and have some helper functions, or have this be a struct with a public ranges data member. The size/rangeAt functions won’t work with a “for” statement for example, or all the other helpful functions that take a vector. > Source/WebCore/platform/graphics/FontSelector.h:43 > + virtual const Font* access() const = 0; I’m not too comfortable with the name “access” here. Is there some other terminology we could use? I think just font() would probably be a better name.
Comment on attachment 278343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278343&action=review >> Source/WebCore/platform/graphics/FontRanges.h:74 >> + FontRanges& operator=(FontRanges&&) = default; > > Can we please omit these? It’s really peculiar to explicitly mention the *copy* constructor and *move* assignment operator. I believe that the move constructor and copy assignment operator will also work; the fact that those two are not mentioned has no effect and the fact that these other two two are mentioned also has no effect. The build fails if these are omitted.
(In reply to comment #55) > Comment on attachment 278343 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278343&action=review > > Looks like some tests are failing. Maybe null dereferences? The failing test is the one that I added in this patch, which uses testRunner.dumpResourceLoadCallbacks() to provide a log of font downloads (so we can make sure that not all the fonts downloaded). Unfortunately, the test downloads two fonts, which causes a race (which is by design). This is all fine, except that the resulting output may be interleaved in an unexpected way (because of the race). I will discuss how to solve this with Brady tomorrow. The patch is working correctly, I just need a way for a test to make sure "only n fonts were downloaded" without caring about the exact order of events.
Comment on attachment 278343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278343&action=review >> Source/WebCore/platform/graphics/FontRanges.h:80 >> const Range& rangeAt(unsigned i) const { return m_ranges[i]; } > > It’s a little peculiar that we use a private vector here. There’s very little encapsulation provided by having this be a class. It might be better to either use a Vector directly and have some helper functions, or have this be a struct with a public ranges data member. The size/rangeAt functions won’t work with a “for” statement for example, or all the other helpful functions that take a vector. This is a good idea, but I think it's out of scope for this patch. https://bugs.webkit.org/show_bug.cgi?id=157476
(In reply to comment #57) > (In reply to comment #55) > > Comment on attachment 278343 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=278343&action=review > > > > Looks like some tests are failing. Maybe null dereferences? > > The failing test is the one that I added in this patch, which uses > testRunner.dumpResourceLoadCallbacks() to provide a log of font downloads > (so we can make sure that not all the fonts downloaded). Unfortunately, the > test downloads two fonts, which causes a race (which is by design). This is > all fine, except that the resulting output may be interleaved in an > unexpected way (because of the race). > > I will discuss how to solve this with Brady tomorrow. The patch is working > correctly, I just need a way for a test to make sure "only n fonts were > downloaded" without caring about the exact order of events. Brady says to fix the test so resource completion ordering is deterministic.
Created attachment 278409 [details] Patch for committing
Attachment 278409 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:59: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:64: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:70: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:86: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 278410 [details] Patch for committing
Attachment 278410 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:59: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:64: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:70: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:86: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 278410 [details] Patch for committing Attachment 278410 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1294255 New failing tests: fast/text/unicode-range-download.html
Created attachment 278415 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 278410 [details] Patch for committing Attachment 278410 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1294298 New failing tests: fast/text/unicode-range-download.html
Created attachment 278417 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 278410 [details] Patch for committing Attachment 278410 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1294302 New failing tests: fast/text/unicode-range-download.html
Created attachment 278419 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 278410 [details] Patch for committing Attachment 278410 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1294303 New failing tests: fast/text/unicode-range-download.html
Created attachment 278420 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 278421 [details] Patch for committing
Attachment 278421 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:59: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:64: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:70: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:86: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 278421 [details] Patch for committing Attachment 278421 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1294492 New failing tests: fast/text/unicode-range-download.html
Created attachment 278427 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 278421 [details] Patch for committing Attachment 278421 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1294502 New failing tests: fast/text/unicode-range-download.html
Created attachment 278429 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 278421 [details] Patch for committing Attachment 278421 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1294560 New failing tests: fast/text/unicode-range-download.html
Created attachment 278432 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 278421 [details] Patch for committing Attachment 278421 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1294572 New failing tests: fast/text/unicode-range-download.html
Created attachment 278434 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 278450 [details] Patch for committing
Attachment 278450 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:59: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:64: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:70: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:86: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 278450 [details] Patch for committing Clearing flags on attachment: 278450 Committed r200601: <http://trac.webkit.org/changeset/200601>
Your test is failing: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r200610%20(5059)/results.html
The test fails if run after fast/text/font-face-set-javascript.html
<rdar://problem/26335045>
I see that this was already committed in https://trac.webkit.org/changeset/200601/webkit, not sure why this is still open?