[webkit-reviews] review denied: [Bug 33089] Implement alphabetic CSS3 list-style-types : [Attachment 46019] Patch with test case
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 7 10:12:20 PST 2010
Darin Adler <darin at apple.com> has denied Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 33089: Implement alphabetic CSS3 list-style-types
https://bugs.webkit.org/show_bug.cgi?id=33089
Attachment 46019: Patch with test case
https://bugs.webkit.org/attachment.cgi?id=46019&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> // disc | circle | square | decimal | decimal-leading-zero |
lower-roman |
> // upper-roman | lower-greek | lower-alpha | lower-latin |
upper-alpha |
> - // upper-latin | hebrew | armenian | georgian | cjk-ideographic |
hiragana |
> - // katakana | hiragana-iroha | katakana-iroha | none | inherit
> + // upper-latin | afar | ethiopic-halehame-aa-et |
ethiopic-halehame-aa-er |
> + // amharic | ethiopic-halehame-am-et | amharic-abegede |
ethiopic-abegede-am-et |
> + // cjk-earthly-branch | cjk-heavenly-stem | ethiopic |
ethiopic-halehame-gez |
> + // ethiopic-abegede | ethiopic-abegede-gez | hangul-consonant |
hangul |
> + // lower-norwegian | oromo | ethiopic-halehame-om-et | sidama |
> + // ethiopic-halehame-sid-et | somali | ethiopic-halehame-so-et |
tigre |
> + // ethiopic-halehame-tig | tigrinya-er | ethiopic-halehame-ti-er |
> + // tigrinya-er-abegede | ethiopic-abegede-ti-er | tigrinya-et |
> + // ethiopic-halehame-ti-et | tigrinya-et-abegede |
ethiopic-abegede-ti-et |
> + // upper-greek | upper-norwegian | hebrew | armenian | georgian |
> + // cjk-ideographic | hiragana | katakana | hiragana-iroha |
katakana-iroha |
> + // none | inherit
So lame that you have to do this. This comments can be useful, but it's always
a pain to have one more place you have to edit! My apologies.
> +afar
> +ethiopic-halehame-aa-et
> +ethiopic-halehame-aa-er
> +amharic
> +ethiopic-halehame-am-et
> +amharic-abegede
> +ethiopic-abegede-am-et
> +cjk-earthly-branch
> +cjk-heavenly-stem
> +ethiopic
> +ethiopic-halehame-gez
> +ethiopic-abegede
> +ethiopic-abegede-gez
> +hangul-consonant
> +hangul
> +lower-norwegian
> +oromo
> +ethiopic-halehame-om-et
> +sidama
> +ethiopic-halehame-sid-et
> +somali
> +ethiopic-halehame-so-et
> +tigre
> +ethiopic-halehame-tig
> +tigrinya-er
> +ethiopic-halehame-ti-er
> +tigrinya-er-abegede
> +ethiopic-abegede-ti-er
> +tigrinya-et
> +ethiopic-halehame-ti-et
> +tigrinya-et-abegede
> +ethiopic-abegede-ti-et
> +upper-greek
> +upper-norwegian
Is there an order here? Could we sort these alphabetically so the order isn't
seemingly-random?
> - "upper-alpha", "upper-latin", "hebrew", "armenian", "georgian",
"cjk-ideographic", "hiragana", "katakana",
> - "hiragana-iroha", "katakana-iroha", "inline", "block", "list-item",
"run-in", "compact", "inline-block",
> - "table", "inline-table", "table-row-group", "table-header-group",
"table-footer-group", "table-row",
> + "upper-alpha", "upper-latin", "afar", "ethiopic-halehame-aa-et",
"ethiopic-halehame-aa-er", "amharic",
> + "ethiopic-halehame-am-et", "amharic-abegede",
"ethiopic-abegede-am-et", "cjk-earthly-branch", "cjk-heavenly-stem",
> + "ethiopic", "ethiopic-halehame-gez", "ethiopic-abegede",
"ethiopic-abegede-gez", "hangul-consonant", "hangul",
> + "lower-norwegian", "oromo", "ethiopic-halehame-om-et", "sidama",
"ethiopic-halehame-sid-et", "somali",
> + "ethiopic-halehame-so-et", "tigre", "ethiopic-halehame-tig",
"tigrinya-er", "ethiopic-halehame-ti-er",
> + "tigrinya-er-abegede", "ethiopic-abegede-ti-er", "tigrinya-et",
"ethiopic-halehame-ti-et", "tigrinya-et-abegede",
> + "ethiopic-abegede-ti-et", "upper-greek", "upper-norwegian",
"hebrew", "armenian", "georgian", "cjk-ideographic",
> + "hiragana", "katakana", "hiragana-iroha", "katakana-iroha",
"inline", "block", "list-item", "run-in", "compact",
> + "inline-block", "table", "inline-table", "table-row-group",
"table-header-group", "table-footer-group", "table-row",
Same question about sort order.
> + return 0x1366;
In most cases, we use names for characters rather than just putting the number
in. That's typically done by adding named constants to CharacterNames.h that
exactly match the names from the Unicode specification. The exception would be
in a table where there are so many characters it is not helpful. This seems
like a case where the name would be better.
> + static const UChar ethiopicHalehameAaErAlphabet[18] = {
> + 0x1200, 0x1208, 0x1210, 0x1218, 0x1228, 0x1230, 0x1260,
0x1270, 0x1290,
> + 0x12A0, 0x12A8, 0x12C8, 0x12D0, 0x12E8, 0x12F0, 0x1308,
0x1338, 0x1348
> + };
How confident are we that these tables are correct? I was concerned when I
considered doing this, because the CSS draft had a lot of cautions about not
knowing if the tables were correct.
> + return toAlphabetic(value, ethiopicHalehameAaErAlphabet, 18);
It's unfortunate to repeat the constants like "18" in the call sites like this.
I'd rather see us use a sizeof approach so the constant isn't repeated twice. I
believe someone had a patch to add a "number of array elements" macro or
function template to WTF. And I think there's a way to do this with a function
template so we don't even have to type the sizeof each time, although I'm not
sure. If all else fails I suggest using a macro.
> - switch (style()->listStyleType()) {
> + const EListStyleType type = style()->listStyleType();
For brevity, WebKit code doesn't use const for the values local variables, even
though we could for many; almost all. Please don't do it here.
> + const UChar suffix = listMarkerSuffix(type);
Same issue here.
> - const UChar periodSpace[2] = { '.', ' ' };
> - context->drawText(style()->font(), TextRun(periodSpace, 2),
marker.location() + IntSize(width, 0));
> + UChar suffixSpace[2];
> + suffixSpace[0] = suffix;
> + suffixSpace[1] = ' ';
You could use array initialization syntax here as the old code did. It's good
that you removed the const, by the way.
> - const UChar spacePeriod[2] = { ' ', '.' };
> - TextRun spacePeriodRun(spacePeriod, 2);
> - int width = font.width(spacePeriodRun);
> - context->drawText(style()->font(), spacePeriodRun,
marker.location());
> + UChar spaceSuffix[2];
> + spaceSuffix[0] = ' ';
> + spaceSuffix[1] = suffix;
> + TextRun spaceSuffixRun(spaceSuffix, 2);
> + int width = font.width(spaceSuffixRun);
> + context->drawText(style()->font(), spaceSuffixRun,
marker.location());
Same here.
> - const UChar periodSpace[2] = { '.', ' ' };
> - int periodSpaceWidth = font.width(TextRun(periodSpace, 2));
> - width = itemWidth + periodSpaceWidth;
> + UChar suffixSpace[2];
> + suffixSpace[0] = listMarkerSuffix(type);
> + suffixSpace[1] = ' ';
And here.
> + const EListStyleType type = style()->listStyleType();
Const here.
> - const UChar periodSpace[2] = { '.', ' ' };
> - int periodSpaceWidth = font.width(TextRun(periodSpace, 2));
> - return IntRect(x(), y() + font.ascent(), itemWidth +
periodSpaceWidth, font.height());
> + UChar suffixSpace[2];
> + suffixSpace[0] = listMarkerSuffix(type);
> + suffixSpace[1] = ' ';
> + int suffixSpaceWidth = font.width(TextRun(suffixSpace, 2));
> + return IntRect(x(), y() + font.ascent(), itemWidth +
suffixSpaceWidth, font.height());
Same array issue here again.
> unsigned _box_direction : 1; // EBoxDirection (CSS3 box_direction
property, flexible box layout module)
> - // 32 bits
> + // 33 bits
There is another comment a few lines later than also needs to be updated. The
one that says "39 bits". And then one further down, the one that says "48
bits".
> + Afar, EthiopicHalehameAaEt, EthiopicHalehameAaEr, Amharic,
> + EthiopicHalehameAmEt, AmharicAbegede, EthiopicAbegedeAmEt,
> + CjkEarthlyBranch, CjkHeavenlyStem, Ethiopic, EthiopicHalehameGez,
> + EthiopicAbegede, EthiopicAbegedeGez, HangulConsonant, Hangul,
> + LowerNorwegian, Oromo, EthiopicHalehameOmEt, Sidama,
> + EthiopicHalehameSidEt, Somali, EthiopicHalehameSoEt, Tigre,
> + EthiopicHalehameTig, TigrinyaEr, EthiopicHalehameTiEr,
> + TigrinyaErAbegede, EthiopicAbegedeTiEr, TigrinyaEt,
> + EthiopicHalehameTiEt, TigrinyaEtAbegede, EthiopicAbegedeTiEt,
> + UpperGreek, UpperNorwegian,
> Hebrew, Armenian, Georgian, CJKIdeographic,
> Hiragana, Katakana, HiraganaIroha, KatakanaIroha, NoneListStyle
At a certain point these lists are far better if sorted, and one item per line.
> + /*
> + The following styles are ordered as they appear in section 4.4. of
the
> + Draft 7 November 2002 draft of the CSS3 Lists module
<http://www.w3.org/TR/css3-lists/#alphabetic>.
> + */
Ah, the answer to my question about ordering. I'm not sure what's best.
By the way, we almost always prefer "//" comments everywhere, even in CSS and
test cases.
A test case is much more valuable if it can be a dumpAsText test rather than a
render-tree-based test, since those usually need different results
per-platform. That having been said, I can't think of a way to do this at the
moment. We should try to think of a way, though, because I think it makes
things easier to maintain and test later. It's also much better to have a test
where we can see if we're passing or failing even if we don't have fonts that
cover all the characters. Worth thinking on this too.
review- because I am sure that at least one of the comments above will lead to
a code change, if only the number of bits comments, for example
More information about the webkit-reviews
mailing list