Bug 115776 – REGRESSION(r149700): fast/css-generated-content/close-quote-negative-depth.html

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

fast/css-generated-content/close-quote-negative-depth.html is still failing:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcss-generated-content%2Fclose-quote-negative-depth.html

[Ryosuke Niwa]

Added a test expectation in http://trac.webkit.org/changeset/149715. Please remove it when you land a fix.

[Darin Adler]

I’m fixing this along with other RenderQuote changes.

[Darin Adler]

Created attachment 201036 [details]
Patch

[Early Warning System Bot]

Comment on attachment 201036 [details]
Patch

Attachment 201036 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/342064

[Early Warning System Bot]

Comment on attachment 201036 [details]
Patch

Attachment 201036 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/318291

[Darin Adler]

Not sure why the lower_bound code is not compiling on Qt. Anyone have ideas?

[EFL EWS Bot]

Comment on attachment 201036 [details]
Patch

Attachment 201036 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/420241

[EFL EWS Bot]

Comment on attachment 201036 [details]
Patch

Attachment 201036 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/329069

[Darin Adler]

(In reply to comment #6)
> Not sure why the lower_bound code is not compiling on Qt. Anyone have ideas?

This code seems to work on Mac clang and library, but not elsewhere with different versions of gcc and the library. Here are my theories so far about why:

1) Maybe the comparator needs to be a namespace level struct and can't be a struct inside a function.
2) Maybe the type of quoteTable needs to be a pointer, not an array.
3) Maybe the type of the buffer argument needs to be a pointer, not an array.

I don't have any other ideas.

[Darin Adler]

Anders, I’d really like your comments on this in at least two dimensions:

1) Should I split this patch up? The rework of depth that fixes the regression is fairly easily separable from the rework of the per-language quote lookup.

2) What do you think of the algorithms I chose? In particular, do you think it's OK to use lower_bound and strcmp like this or should I do something different?

I’d like to land this soon, but it might need some work.

[Darin Adler]

I should dump that QUOTES_LANG macro and just initialize the array without using macro.

[kov's GTK+ EWS bot]

Comment on attachment 201036 [details]
Patch

Attachment 201036 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/266239

[Ryosuke Niwa]

Can we split the fix?  It seems quite risky to do another refactoring & a bug fix in one patch given that r149700 and r149707 have already been quite massive.

[Build Bot]

Comment on attachment 201036 [details]
Patch

Attachment 201036 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/408360

New failing tests:
fast/css/content/content-quotes-06.html

[Build Bot]

Created attachment 201059 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Darin Adler]

Yes, I’ll put up a new patch with just the bug fix shortly.

[Anders Carlsson]

(In reply to comment #10)
> Anders, I’d really like your comments on this in at least two dimensions:
> 
> 1) Should I split this patch up? The rework of depth that fixes the regression is fairly easily separable from the rework of the per-language quote lookup.

Sounds like a good idea (and I think you're doing this already).

> 
> 2) What do you think of the algorithms I chose? In particular, do you think it's OK to use lower_bound and strcmp like this or should I do something different?

My plan was to just use bsearch - no need to inline all that std::lower_bound code.

How about we rename quoteCharacterString to stringForQuoteCharacter?

Looks great otherwise!

[Anders Carlsson]

Oh, one more thing: I think the #if around the checkNumberOfDistinctQuoteCharacters definition should be !ASSERT_DISABLED.

[Build Bot]

Comment on attachment 201036 [details]
Patch

Attachment 201036 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/408399

New failing tests:
fast/css/content/content-quotes-06.html

[Build Bot]

Created attachment 201074 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Darin Adler]

Created attachment 201080 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 201080 [details]
Patch

Clearing flags on attachment: 201080

Committed r149754: <http://trac.webkit.org/changeset/149754>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.