See the chromium bug http://crbug.com/89609 for details. The specification (http://www.w3.org/TR/css3-fonts/#src-desc) mentions--referring to local(): > The name can optionally be enclosed in quotes. And later gives this example: /* bold face of Gentium */ @font-face { font-family: MyGentium; src: local(Gentium Bold), /* full font name */ local(Gentium-Bold), /* Postscript name */ url(GentiumBold.ttf); /* otherwise, download it */ font-weight: bold; }
Created attachment 101271 [details] Patch V0
Created attachment 116155 [details] Patch
(In reply to comment #2) > Created an attachment (id=116155) [details] > Patch Just rebased to ToT.
Comment on attachment 116155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116155&action=review > Source/WebCore/ChangeLog:6 > + Allows unquoted local font names contains spaces. "local font names with spaces" or "local font names containing spaces". > Source/WebCore/css/CSSParser.cpp:4366 > + if (args->current()->unit == CSSPrimitiveValue::CSS_STRING) { > + parsedValue = CSSFontFaceSrcValue::createLocal(args->current()->string); > + } else if (args->current()->unit == CSSPrimitiveValue::CSS_IDENT) { No { } around a single statement. > Source/WebCore/css/CSSParser.cpp:4370 > + failed = true; Are you sure this is correct? What if we had a valid token after local() e.g. local(~~ non-ident) src(valid value)
Created attachment 116865 [details] Patch
Comment on attachment 116155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116155&action=review Thanks for review! Revised the patch. >> Source/WebCore/ChangeLog:6 >> + Allows unquoted local font names contains spaces. > > "local font names with spaces" or "local font names containing spaces". Done. >> Source/WebCore/css/CSSParser.cpp:4366 >> + } else if (args->current()->unit == CSSPrimitiveValue::CSS_IDENT) { > > No { } around a single statement. Done. >> Source/WebCore/css/CSSParser.cpp:4370 >> + failed = true; > > Are you sure this is correct? What if we had a valid token after local() e.g. local(~~ non-ident) src(valid value) I couldn't find how should we treat that case in the spec draft, but Firefox rejects it. I followed Firefox's manner.
(In reply to comment #6) > >> Source/WebCore/css/CSSParser.cpp:4370 > >> + failed = true; > > > > Are you sure this is correct? What if we had a valid token after local() e.g. local(~~ non-ident) src(valid value) > > I couldn't find how should we treat that case in the spec draft, but Firefox rejects it. I followed Firefox's manner. Please add a test case for this, and also mention it in the change log.
Comment on attachment 116865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116865&action=review > Source/WebCore/css/CSSParser.cpp:4380 > } else if (val->unit == CSSParserValue::Function) { You should put this entire block in a helper function. > Source/WebCore/css/CSSParser.cpp:4386 > + // There are two allowed functions: local() and format(). A better way to document this is by asserting that. > Source/WebCore/css/CSSParser.cpp:4404 > + parsedValue = CSSFontFaceSrcValue::createLocal(builder.toString()); You should check that failed is false here. Otherwise, we don't bail out early on line 4426.
Comment on attachment 116865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116865&action=review Thank you for the review! I have one question (please see below). >> Source/WebCore/css/CSSParser.cpp:4380 >> } else if (val->unit == CSSParserValue::Function) { > > You should put this entire block in a helper function. Umm, the block contains many references to local variables that are used entire the function so I think just putting this block in a helper function doesn't make things clear. It might be better to refactor the whole function, but I'm not sure it's worth to do it just for fixing this problem. Should I do this? If so, I'll try it. >> Source/WebCore/css/CSSParser.cpp:4386 >> + // There are two allowed functions: local() and format(). > > A better way to document this is by asserting that. Asserting isn't good here because the input comes from stylesheets and could contain arbitrary values. >> Source/WebCore/css/CSSParser.cpp:4404 >> + parsedValue = CSSFontFaceSrcValue::createLocal(builder.toString()); > > You should check that failed is false here. Otherwise, we don't bail out early on line 4426. Will do.
Comment on attachment 116865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116865&action=review >>> Source/WebCore/css/CSSParser.cpp:4380 >>> } else if (val->unit == CSSParserValue::Function) { >> >> You should put this entire block in a helper function. > > Umm, the block contains many references to local variables that are used entire the function so I think just putting this block in a helper function doesn't make things clear. It might be better to refactor the whole function, but I'm not sure it's worth to do it just for fixing this problem. Should I do this? If so, I'll try it. Could be a follow up patch. >>> Source/WebCore/css/CSSParser.cpp:4386 >>> + // There are two allowed functions: local() and format(). >> >> A better way to document this is by asserting that. > > Asserting isn't good here because the input comes from stylesheets and could contain arbitrary values. That means this comment is incorrect, right? If you meant to say that we only accept local and format functions here, then that's just redundant; the code already tells us that much.
Created attachment 117115 [details] Patch
Created attachment 117117 [details] Patch for landing
Comment on attachment 117117 [details] Patch for landing Clearing flags on attachment: 117117 Committed r101504: <http://trac.webkit.org/changeset/101504>
All reviewed patches have been landed. Closing bug.