Bug 145860

Created attachment 254686 [details] Patch

Comment on attachment 254686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254686&action=review Looking good but I think we can improve the tokenization... > Source/WebCore/css/CSSParser.cpp:5994 > + areaName.append(text[i]); Instead of appending one by one, I think it would be better to keep track of the index of the start of the area name, and then just simply do a substring() call (perhaps even substringSharingImpl()) as gridRowNames will be valid throughout the whole life of columnNames I think) > LayoutTests/fast/css-grid-layout/grid-template-areas-get-set.html:80 > +} Although you already consider some of them I think we should have separate test cases for the following situations: a) ....A b) A.. c) ...A.. d) A.B e) ..A.B f) A.B.. g) ..A..B...

Created attachment 254720 [details] Patch

Thanks for the review! (In reply to comment #2) > > Source/WebCore/css/CSSParser.cpp:5994 > > + areaName.append(text[i]); > > Instead of appending one by one, I think it would be better to keep track of > the index of the start of the area name, and then just simply do a > substring() call (perhaps even substringSharingImpl()) as gridRowNames will > be valid throughout the whole life of columnNames I think) Yeah I think this approach makes the code a bit simpler. > > LayoutTests/fast/css-grid-layout/grid-template-areas-get-set.html:80 > > +} > > Although you already consider some of them I think we should have separate > test cases for the following situations: > a) ....A > b) A.. > c) ...A.. > d) A.B > e) ..A.B > f) A.B.. > g) ..A..B... I've added more cases like these ones and other too.

Comment on attachment 254720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254720&action=review > Source/WebCore/css/CSSParser.cpp:5971 > + while (index < text.length()) { let's cache text.length() in a local variable in order not to repeat the same call over and over again. > Source/WebCore/css/CSSParser.cpp:5975 > + index++; I think it's ok to use a single line for this. > Source/WebCore/css/CSSParser.cpp:5982 > + index++; Ditto. > LayoutTests/fast/css-grid-layout/grid-template-areas-get-set-expected.txt:21 > +PASS getComputedStyle(gridWithDotsNoSpace).getPropertyValue('-webkit-grid-template-areas') is "\". title . nav . . main test\"" Seems like you forgot to upload all the new expectations?

Comment on attachment 254720 [details] Patch Attachment 254720 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5739303049101312 New failing tests: fast/css-grid-layout/grid-template-areas-get-set.html

Created attachment 254722 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Comment on attachment 254720 [details] Patch Attachment 254720 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4917405222436864 New failing tests: fast/css-grid-layout/grid-template-areas-get-set.html

Created attachment 254723 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5

Created attachment 254735 [details] Patch

Attachment 254735 [details] did not pass style-queue: ERROR: Source/WebCore/css/CSSParser.cpp:5975: More than one command on the same line in while [whitespace/parens] [4] ERROR: Source/WebCore/css/CSSParser.cpp:5981: More than one command on the same line in while [whitespace/parens] [4] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 254754 [details] Patch

Thanks for the review. (In reply to comment #5) > Comment on attachment 254720 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254720&action=review > > > Source/WebCore/css/CSSParser.cpp:5971 > > + while (index < text.length()) { > > let's cache text.length() in a local variable in order not to repeat the > same call over and over again. Done. > > Source/WebCore/css/CSSParser.cpp:5975 > > + index++; > > I think it's ok to use a single line for this. It seems the style checker didn't like it. > > LayoutTests/fast/css-grid-layout/grid-template-areas-get-set-expected.txt:21 > > +PASS getComputedStyle(gridWithDotsNoSpace).getPropertyValue('-webkit-grid-template-areas') is "\". title . nav . . main test\"" > > Seems like you forgot to upload all the new expectations? True :-)

Comment on attachment 254754 [details] Patch Clearing flags on attachment: 254754 Committed r185492: <http://trac.webkit.org/changeset/185492>

All reviewed patches have been landed. Closing bug.

(In reply to comment #14) > Comment on attachment 254754 [details] > Patch > > Clearing flags on attachment: 254754 > > Committed r185492: <http://trac.webkit.org/changeset/185492> It caused 11 assertions on Apple's debug bots: https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/12379

Re-opened since this is blocked by bug 145927

Created attachment 254817 [details] Patch

(In reply to comment #16) > (In reply to comment #14) > > Comment on attachment 254754 [details] > > Patch > > > > Clearing flags on attachment: 254754 > > > > Committed r185492: <http://trac.webkit.org/changeset/185492> > > It caused 11 assertions on Apple's debug bots: > https://build.webkit.org/builders/ > Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/12379 Yes it was a silly mistake "index < length" was the last condition at: while (text[index] != ' ' && text[index] != '.' && index < length) When it should be the first one. :-(

Comment on attachment 254817 [details] Patch Clearing flags on attachment: 254817 Committed r185520: <http://trac.webkit.org/changeset/185520>

All reviewed patches have been landed. Closing bug.

Comment on attachment 254817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254817&action=review > Source/WebCore/css/CSSParser.cpp:5964 > +static Vector<String> parseGridTemplateAreasColumnNames(const String& gridRowNames) Could save some memory allocation by making this take a StringView and return a Vector<StringView>. Then can convert to a string when we need to. Probably a bit of a project, though.