| Summary: | The "font" property is not a shorthand | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||||||
| Component: | CSS | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | abarth, darin, dglazkov, jchaffraix, luiz, macpherson, simon.fraser, timothy, webkit.review.bot | ||||||||||||||||||
| Priority: | P2 | Keywords: | HasReduction | ||||||||||||||||||
| Version: | 523.x (Safari 3) | ||||||||||||||||||||
| Hardware: | Mac | ||||||||||||||||||||
| OS: | OS X 10.4 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Eric Seidel (no email)
2007-10-21 12:48:05 PDT
Created attachment 16771 [details]
test case
Created attachment 92601 [details]
[PATCH] Suggested fix, initial version
Comment on attachment 92601 [details] [PATCH] Suggested fix, initial version Attachment 92601 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8598068 New failing tests: canvas/philip/tests/2d.text.draw.align.left.html canvas/philip/tests/2d.text.draw.space.basic.html canvas/philip/tests/2d.text.draw.align.right.html canvas/philip/tests/2d.text.draw.align.start.rtl.html canvas/philip/tests/2d.text.draw.fontface.notinpage.html canvas/philip/tests/2d.text.draw.align.start.ltr.html canvas/philip/tests/2d.text.draw.baseline.alphabetic.html canvas/philip/tests/2d.text.draw.fill.maxWidth.bound.html canvas/philip/tests/2d.text.draw.align.end.ltr.html canvas/philip/tests/2d.text.draw.fontface.html canvas/philip/tests/2d.text.draw.fontface.repeat.html fast/canvas/2d.fillText.gradient.html canvas/philip/tests/2d.text.measure.width.basic.html canvas/philip/tests/2d.text.draw.align.center.html canvas/philip/tests/2d.text.draw.align.end.rtl.html Created attachment 92626 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Hmm... I wonder if these are unrelated failures. (In reply to comment #5) > Hmm... I wonder if these are unrelated failures. Yeah, dunno. The logs from the bot look legit. I guess we'll find out if/when the patch lands. Comment on attachment 92601 [details] [PATCH] Suggested fix, initial version View in context: https://bugs.webkit.org/attachment.cgi?id=92601&action=review I am not an official reviewer but here goes some thoughts. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:317 > + { > + const CSSProperty* property = findPropertyWithId(CSSPropertyFontVariant); > + if (!property) > + return ""; // All longhands must have at least implicit values if "font" is specified. > + if (!property->isImplicit()) { > + if (!result.isEmpty()) > + result += " "; > + result += property->value()->cssText(); > + } > + } This code seems to be replicated several times, sometimes with minor differences but many times almost unchanged. Would not it be possible to create a function and avoid code replication? > Source/WebCore/css/CSSParser.cpp:595 > +void CSSParser::addPropertyImplicit(int propId, PassRefPtr<CSSValue> value, bool important, bool implicit) Boolean parameter reduce readability. Would not it be better to create enumerations with descriptive names? > Source/WebCore/css/CSSParser.cpp:3619 > + // do nothing, it's the initial value for all three I think that comments should start with capital letters and finish with a period. (In reply to comment #7) > (From update of attachment 92601 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92601&action=review > > I am not an official reviewer but here goes some thoughts. > > > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:317 > > + { > > + const CSSProperty* property = findPropertyWithId(CSSPropertyFontVariant); > > + if (!property) > > + return ""; // All longhands must have at least implicit values if "font" is specified. > > + if (!property->isImplicit()) { > > + if (!result.isEmpty()) > > + result += " "; > > + result += property->value()->cssText(); > > + } > > + } > > This code seems to be replicated several times, sometimes with minor differences but many times almost unchanged. > Would not it be possible to create a function and avoid code replication? I explicitly didn't want to clutter the method namespace but I agree that the current implementation looks quite awkward. Fixed. > > Source/WebCore/css/CSSParser.cpp:595 > > +void CSSParser::addPropertyImplicit(int propId, PassRefPtr<CSSValue> value, bool important, bool implicit) > > Boolean parameter reduce readability. Would not it be better to create enumerations with descriptive names? It is an ad-hoc method which is not going to be used in many places around, so I don't think it warrants another enum (which is of course a very good thing for simplifying APIs). > > Source/WebCore/css/CSSParser.cpp:3619 > > + // do nothing, it's the initial value for all three > > I think that comments should start with capital letters and finish with a period. Cleaned up all comments in the method. Created attachment 92972 [details]
[PATCH] Comments addressed
Comment on attachment 92972 [details] [PATCH] Comments addressed Attachment 92972 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8685055 New failing tests: canvas/philip/tests/2d.text.draw.align.left.html canvas/philip/tests/2d.text.draw.space.basic.html canvas/philip/tests/2d.text.draw.align.right.html canvas/philip/tests/2d.text.draw.align.start.rtl.html canvas/philip/tests/2d.text.draw.fontface.notinpage.html canvas/philip/tests/2d.text.draw.align.start.ltr.html canvas/philip/tests/2d.text.draw.baseline.alphabetic.html canvas/philip/tests/2d.text.draw.fill.maxWidth.bound.html canvas/philip/tests/2d.text.draw.align.end.ltr.html canvas/philip/tests/2d.text.draw.fontface.html canvas/philip/tests/2d.text.draw.fontface.repeat.html fast/canvas/2d.fillText.gradient.html canvas/philip/tests/2d.text.measure.width.basic.html canvas/philip/tests/2d.text.draw.align.center.html canvas/philip/tests/2d.text.draw.align.end.rtl.html Created attachment 92989 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 92972 [details] [PATCH] Comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=92972&action=review review- because this seems to affect normal property value fetching, not just the inspector functions, and that should be tested and because this needs to use StringBuilder, not String. > Source/WebCore/ChangeLog:5 > + font property does not show up as "shorthand" in inspector I know you discovered this while working on the inspector but the code change here does not seem at all inspector-specific. Can we add some non-inspector CSS test cases to demonstrate this bug and fix? > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:298 > +bool CSSMutableStyleDeclaration::appendMandatoryPropertyValueIfExplicit(const CSSProperty* property, const String& stringBefore, String* result) const For WebKit coding style, result should be a reference, not a pointer. For a code path that is appending, result needs to be a StringBuilder, not a String. Since stringBefore is always a string constant, it should be a const char*, since StringBuilder already has an optimized code path for appending those. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:312 > + DEFINE_STATIC_LOCAL(String, spaceString, (" ")); Should get rid of this and just use a space after making the change I suggested above. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:313 > + String result(""); Needs to be a StringBuilder. > Source/WebCore/css/CSSParser.h:76 > void addProperty(int propId, PassRefPtr<CSSValue>, bool important); > + void addPropertyImplicit(int propId, PassRefPtr<CSSValue>, bool important, bool implicit); I think this would be clearer as a default argument rather than a separate function. All the callers are already passing a boolean argument, not a constant, and it could simply default to true. Created attachment 111791 [details]
[PATCH] Darin's comments addressed
(In reply to comment #12) > (From update of attachment 92972 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92972&action=review > > review- because this seems to affect normal property value fetching, not just the inspector functions, and that should be tested and because this needs to use StringBuilder, not String. Good point. However, the affected test has nothing to do with web inspector (not sure why it resides in the "inspector-support" directory) and tests exactly the modified functionality: retrieves applicable style rules and examines their properties. In my case, the "font" property has gone in favor of its longhands ("font-family" et al), and that's what the expected result has been updated to reflect. > > Source/WebCore/ChangeLog:5 > > + font property does not show up as "shorthand" in inspector > > I know you discovered this while working on the inspector but the code change here does not seem at all inspector-specific. Can we add some non-inspector CSS test cases to demonstrate this bug and fix? Please see above. If you suggest that there is a better way to test this change than to directly query the CSS style for its properties, I will surely add/modify a proper test. > > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:298 > > +bool CSSMutableStyleDeclaration::appendMandatoryPropertyValueIfExplicit(const CSSProperty* property, const String& stringBefore, String* result) const > > For WebKit coding style, result should be a reference, not a pointer. > For a code path that is appending, result needs to be a StringBuilder, not a String. Fixed both. > Since stringBefore is always a string constant, it should be a const char*, since StringBuilder already has an optimized code path for appending those. > > > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:312 > > + DEFINE_STATIC_LOCAL(String, spaceString, (" ")); > > Should get rid of this and just use a space after making the change I suggested above. Done. > > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:313 > > + String result(""); > > Needs to be a StringBuilder. Done. > > Source/WebCore/css/CSSParser.h:76 > > void addProperty(int propId, PassRefPtr<CSSValue>, bool important); > > + void addPropertyImplicit(int propId, PassRefPtr<CSSValue>, bool important, bool implicit); > > I think this would be clearer as a default argument rather than a separate function. All the callers are already passing a boolean argument, not a constant, and it could simply default to true. Done. @jchaffraix: I see you made a related fix a while ago to avoid crashes by means of calling updateFont(). I have moved this call just in front of the affected computeLength() call for the CSS_EXS case (since fontMetrics() are not required in other cases). Do you think this particular change is safe enough given the invoking code modification? Comment on attachment 111791 [details] [PATCH] Darin's comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=111791&action=review > Source/WebCore/css/CSSStyleSelector.cpp:2592 > + updateFont(); I think it is an overkill to move the updateFont here. The case covered by the bug was specific to <canvas> where we cheat by having parentStyle == m_style. The rest of the code would use a different parentStyle which would not be modified in-flight. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1888 > + styleSelector->applyPropertyToCurrentStyle(CSSPropertyFontWeight, tempDecl->getPropertyCSSValue(CSSPropertyFontWeight).get()); Now that you have split the different steps, adding the call to updateFont() just after this line would be the best option. Comment on attachment 111791 [details] [PATCH] Darin's comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=111791&action=review review- because of the issue Julien mentions > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:315 > + return ""; A more efficient empty string is emptyAtom. Is empty string really right for this? I don’t see any test cases covering this. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:319 > + return ""; Ditto. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:321 > + return ""; Ditto. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:323 > + return ""; Ditto. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:328 > + return ""; Ditto. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:330 > + return ""; Ditto. > Source/WebCore/css/CSSParser.cpp:3959 > - // do nothing, it's the inital value for all three > + // Do nothing, it's the initial value for all three. > + switch (valueOrdinal) { > + case 0: > + styleImplicit = false; > + break; > + case 1: > + variantImplicit = false; > + break; > + case 2: > + weightImplicit = false; > + break; > + } The comment says “do nothing”, but now the code does something, so “do nothing” no longer makes sense. > Source/WebCore/css/CSSParser.cpp:4039 > + // Default value, nothing to do You didn’t add a period. I’m not sure “nothing to do” really describes code that does something. May need better wording. (In reply to comment #16) > (From update of attachment 111791 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111791&action=review > > > Source/WebCore/css/CSSStyleSelector.cpp:2592 > > + updateFont(); > > I think it is an overkill to move the updateFont here. The case covered by the bug was specific to <canvas> where we cheat by having parentStyle == m_style. The rest of the code would use a different parentStyle which would not be modified in-flight. Thanks Julien, I've reverted this change. > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1888 > > + styleSelector->applyPropertyToCurrentStyle(CSSPropertyFontWeight, tempDecl->getPropertyCSSValue(CSSPropertyFontWeight).get()); > > Now that you have split the different steps, adding the call to updateFont() just after this line would be the best option. Had to make updateFont() public for this but seems to work fine! (In reply to comment #17) > (From update of attachment 111791 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111791&action=review > > review- because of the issue Julien mentions > > > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:315 > > + return ""; > > A more efficient empty string is emptyAtom. Is empty string really right for this? I don’t see any test cases covering this. Sorry for not articulating the intention of these empty strings properly. In fact, ALL mandatory "font" longhands must exist in the style (at least implicitly), so the absence of ANY of those is an error. I have added ASSERT_NOT_REACHED() for this case. > > Source/WebCore/css/CSSParser.cpp:3959 > > - // do nothing, it's the inital value for all three > > + // Do nothing, it's the initial value for all three. > > + switch (valueOrdinal) { > > + case 0: > > + styleImplicit = false; > > + break; > > + case 1: > > + variantImplicit = false; > > + break; > > + case 2: > > + weightImplicit = false; > > + break; > > + } > > The comment says “do nothing”, but now the code does something, so “do nothing” no longer makes sense. > > > Source/WebCore/css/CSSParser.cpp:4039 > > + // Default value, nothing to do > > You didn’t add a period. I’m not sure “nothing to do” really describes code that does something. May need better wording. Fixed the comments to be a bit more verbose (and correct). Also, fixed the issue Julien was concerned about. Created attachment 111929 [details]
[PATCH] Comments addressed, take 3
Comment on attachment 111929 [details] [PATCH] Comments addressed, take 3 View in context: https://bugs.webkit.org/attachment.cgi?id=111929&action=review This looks good to me, especially as all bots are green now. I'd still like to see a test which doesn't use Inspector at all, that examines the font shorthand. Some comments remaining, that still lead to r-: > Source/WebCore/ChangeLog:7 > + This needs more explaination. The inline comments are very helpful, but still the whole context is missing. An important fix deserves a longer description! > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:315 > + return ""; return emptyString(); > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:319 > + success &= appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontStyle), "", result); Hm, I'd rather pass CSSPropertyFontStyle to appendMandatoryPropertyValueIfExplicit, instead of the CSSProperty*. There's no need then to pass in stringBefore, as the value can be determined by the property. static inline void appendStringBefore(CSSPropertyID id, StringBuilder& result) (a better name should be found!) { switch (id) { case CSSPropertyFontSize: break; case CSSPropertyFontVariant: result.append(' '); return; ... }; This will also use StringBuilder::append(char) rather than const char* and avoids an unncessary strlen. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:320 > + success &= appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontVariant), " ", result); This would just read success &= appendMandatoryPropertyValueIfExplicit(CSSPropertyFontVariant, result) then. > Source/WebCore/css/FontValue.cpp:40 > + result.append(" "); Use append(' ') > Source/WebCore/css/FontValue.cpp:45 > + result.append(" "); Ditto. > Source/WebCore/css/FontValue.cpp:50 > + result.append(" "); Ditto. > Source/WebCore/css/FontValue.cpp:55 > + result.append(" "); Ditto. > Source/WebCore/css/FontValue.cpp:56 > + result.append("/"); '/' > Source/WebCore/css/FontValue.cpp:61 > + result.append(" "); ' ' > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1888 > + styleSelector->updateFont(); Why is this needed now? This needs to be documented. Created attachment 115139 [details]
Patch
WildFox's comments addressed
(In reply to comment #21) > (From update of attachment 111929 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111929&action=review > > This looks good to me, especially as all bots are green now. I'd still like to see a test which doesn't use Inspector at all, that examines the font shorthand. Some comments remaining, that still lead to r-: Test added. > > Source/WebCore/ChangeLog:7 > > + > > This needs more explaination. The inline comments are very helpful, but still the whole context is missing. > An important fix deserves a longer description! Described what the patch actually does. > > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:315 > > + return ""; > > return emptyString(); Done. > > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:319 > > + success &= appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontStyle), "", result); > > Hm, I'd rather pass CSSPropertyFontStyle to appendMandatoryPropertyValueIfExplicit, instead of the CSSProperty*. > > There's no need then to pass in stringBefore, as the value can be determined by the property. > static inline void appendStringBefore(CSSPropertyID id, StringBuilder& result) (a better name should be found!) > { > switch (id) { > case CSSPropertyFontSize: > break; > case CSSPropertyFontVariant: > result.append(' '); > return; > ... > }; > > This will also use StringBuilder::append(char) rather than const char* and avoids an unncessary strlen. Merged the two suggested methods into one, since there were no other clients of appendMandatoryPropertyValueIfExplicit than the "font" property builder. > > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:320 > > + success &= appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontVariant), " ", result); > > This would just read > success &= appendMandatoryPropertyValueIfExplicit(CSSPropertyFontVariant, result) > then. Done. > > Source/WebCore/css/FontValue.cpp:40 > > + result.append(" "); > > Use append(' ') > > > Source/WebCore/css/FontValue.cpp:45 > > + result.append(" "); > > Ditto. > > > Source/WebCore/css/FontValue.cpp:50 > > + result.append(" "); > > Ditto. > > > Source/WebCore/css/FontValue.cpp:55 > > + result.append(" "); > > Ditto. > > > Source/WebCore/css/FontValue.cpp:56 > > + result.append("/"); > > '/' > > > Source/WebCore/css/FontValue.cpp:61 > > + result.append(" "); > > ' ' Fixed all instances. > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1888 > > + styleSelector->updateFont(); > > Why is this needed now? This needs to be documented. Added an explanation. Comment on attachment 115139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115139&action=review Looks great, r=me! Assuming everything turns green, it's ready to land. > Source/WebCore/css/CSSStyleSelector.cpp:2396 > +void CSSStyleSelector::applyPropertyToCurrentStyle(int id, CSSValue *value) "CSSValue* value". No idea why style bot doesn't complain here :-) > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1999 > + // As described in BUG66291, setting font-size on a font may entail a CSSPrimitiveValue::computeLengthDouble call, > + // which assumes the fontMetrics are available for the affected font, otherwise a crash occurs (see http://trac.webkit.org/changeset/96122). > + // The updateFont() call below updates the fontMetrics and ensures the proper setting of font-size. Excellent, that's the information you want to read :-) Committed r100273: <http://trac.webkit.org/changeset/100273> This broke a test: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100277%20(34712)/platform/mac/plugins/jsobjc-dom-wrappers-pretty-diff.html (In reply to comment #26) > This broke a test: > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100277%20(34712)/platform/mac/plugins/jsobjc-dom-wrappers-pretty-diff.html Test fixed in http://trac.webkit.org/changeset/100451 ("font" is no longer mapped to DOMCSSValue). I believe this change (correctly) caused all of the CSSFontProperty code in StyleResolver::applyProperty to be dead, and now could be removed. :) Well, almost all the CSSPropertyFont code. Blink removed the dead code in: https://codereview.chromium.org/19678002/ |