Bug 205770

Created attachment 386773 [details] Test case

This causes a failure (unrelated to what it is intending to test) in http://wpt.live/WebIDL/current-realm.html

An older version of the spec dictated that we should throw. The spec got updated. We should fix this bug in WebKit. Should be easy.

Created attachment 389749 [details] Patch

Comment on attachment 389749 [details] Patch r- for red EWS bubbles.

Created attachment 390334 [details] Patch

Created attachment 390338 [details] Patch

Comment on attachment 390338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390338&action=review > Source/WebCore/ChangeLog:10 > + No longer throws, rejects promise, sets status to error, and "(No longer throws), (rejects promise), (sets status to error)" ..... or "No longer (throws, rejects promise, sets status to error)" ? > Source/WebCore/css/CSSFontFace.cpp:719 > + setStatus(Status::Loading); > + setStatus(Status::Failure); > + m_inErrorState = true; Is this legal regardless of what state the CSSFontFace happens to be in? It seems wrong to not even consult the current state. Perhaps ASSERT that there is a path from whatever the current state is to ::Failure. > Source/WebCore/css/CSSFontFace.h:86 > + const Optional<CSSValueList*> families() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<CSSValueList*>>(m_families.get()); } > + Optional<FontSelectionRange> weight() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeWeight()); } > + Optional<FontSelectionRange> stretch() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeWidth()); } > + Optional<FontSelectionRange> italic() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeSlope()); } > + Optional<FontSelectionCapabilities> fontSelectionCapabilities() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionCapabilities>>(m_fontSelectionCapabilities.computeFontSelectionCapabilities()); } > + const Optional<Vector<UnicodeRange>> ranges() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<Vector<UnicodeRange>>>(m_ranges); } > + const Optional<FontFeatureSettings> featureSettings() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontFeatureSettings>>(m_featureSettings); } > + Optional<FontLoadingBehavior> loadingBehavior() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontLoadingBehavior>>(m_loadingBehavior); } I'd appreciate a comment describing what it means for these accessors to return nullopt. (It means that the fields represent the empty string, but the backing store can't actually represent the empty string, so we use optionals here.) If the backing store *can* represent an empty string, there's no need to use an Optional here. > Source/WebCore/css/CSSFontFace.h:196 > + bool m_inErrorState { false }; The spec doesn't say anything about a new state. We already have CSSFontFace::Status::Failure. Why do we need a new, super-failure, state? What's the difference between this bool and being in the failure state? Why do these two situations need to be distinct? I also think this doesn't quite follow the spec. It says "If any of them fail to parse correctly, ... set font face’s corresponding attributes to the empty string." I think "corresponding attributes" means "just the ones that failed to parse." There should be tests added for this (hopefully contributed to WPT). > Source/WebCore/css/CSSFontFaceSet.cpp:163 > + if (!familiesWrapped.hasValue()) I would just merge this with the (!face.families()) guard above. > Source/WebCore/css/CSSFontFaceSet.cpp:244 > + if (face.families().hasValue()) You removed the check for whether or not the CSSValueList* is null. Please add a test which would have caught this bug. > Source/WebCore/css/CSSFontFaceSet.cpp:461 > + if (!fontSelectionCapabilitiesWrapped.hasValue()) > + continue; This should just be an ASSERT(). You already made sure that nothing gets added to candidateFontFaces which has a nullopt fontSelectionCapabilities. > Source/WebCore/css/CSSFontFaceSet.cpp:472 > + auto firstCapabilitiesWrapped = first.fontSelectionCapabilities(); > + auto secondCapabilitiesWrapped = second.fontSelectionCapabilities(); > + if (!firstCapabilitiesWrapped.hasValue()) > + return false; > + if (!secondCapabilitiesWrapped.hasValue()) > + return false; Ditto. > Source/WebCore/css/CSSSegmentedFontFace.cpp:123 > + if (!selectionCapabilitiesWrapped.hasValue()) > + continue; You should be able to ASSERT(selectionCapabilitiesWrapped.hasValue()) here. The code you wrote in CSSFontFaceSet::fontFace() should disallow any error-ed fonts to make it into this collection. > Source/WebCore/css/FontFace.cpp:271 > + auto familiesUnrwapped = families.value(); typo: unrwapped > Source/WebCore/css/FontFace.cpp:390 > + auto featureSettings = featureSettingsWrapped.value(); auto& No need to make a copy Also applicable to everywhere above > LayoutTests/http/tests/css/font-face-constructor-expected.txt:11 > + > + > + > + > + > + > + This indicates a bad design. A test should say PASS for each check it makes. > LayoutTests/http/tests/css/font-face-constructor.html:26 > + debug(testFontFace.family); > + debug(testFontFace.style); > + debug(testFontFace.weight); > + debug(testFontFace.stretch); > + debug(testFontFace.unicodeRange); > + debug(testFontFace.featureSettings); > + debug(testFontFace.display); > + document.fonts.add(testFontFace); > + debug("PASS: Did not throw"); Instead of "debug" you should use "shouldBeEqualToString" instead.

Created attachment 390423 [details] Patch

Comment on attachment 390423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390423&action=review > Source/WebCore/css/CSSFontFace.cpp:728 > + setStatus(Status::Failure); What if it's already in the failure state? I don't think the state diagram has loops in it. > Source/WebCore/css/FontFace.cpp:374 > + return "U+0-10FFFF"; Shouldn't this be the empty string? > Source/WebCore/css/FontFace.cpp:389 > + return "normal"_s; Ditto.

Created attachment 390529 [details] Patch

Created attachment 390800 [details] Patch

Comment on attachment 390800 [details] Patch Rejecting attachment 390800 [details] from commit-queue. nmouchtaris@apple.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

The commit-queue encountered the following flaky tests while processing attachment 390800 [details]: The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 390800 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 390800 [details] Patch Rejecting attachment 390800 [details] from commit-queue. New failing tests: imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html Full output: https://webkit-queues.webkit.org/results/13323017

Created attachment 390825 [details] Archive of layout-test-results from webkit-cq-01 for mac-mojave The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-01 Port: mac-mojave Platform: Mac OS X 10.14.6

Very nice comment art!

The commit-queue encountered the following flaky tests while processing attachment 390800 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 390800 [details] Patch Clearing flags on attachment: 390800 Committed r256659: <https://trac.webkit.org/changeset/256659>

All reviewed patches have been landed. Closing bug.

<rdar://problem/59479370>