Summary: | REGRESSION(r149700): fast/css-generated-content/close-quote-negative-depth.html | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | Layout and Rendering | Assignee: | Darin Adler <darin> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, ap, benjamin, buildbot, cmarcelo, commit-queue, darin, eflews.bot, esprehn+autocc, glenn, gtk-ews, gyuyoung.kim, kling, philn, rniwa, roger_fong, webkit-ews, xan.lopez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2013-05-07 23:59:16 PDT
Added a test expectation in http://trac.webkit.org/changeset/149715. Please remove it when you land a fix. I’m fixing this along with other RenderQuote changes. Created attachment 201036 [details]
Patch
Comment on attachment 201036 [details] Patch Attachment 201036 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/342064 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 Not sure why the lower_bound code is not compiling on Qt. Anyone have ideas? 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 Comment on attachment 201036 [details] Patch Attachment 201036 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/329069 (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. 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. I should dump that QUOTES_LANG macro and just initialize the array without using macro. Comment on attachment 201036 [details] Patch Attachment 201036 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/266239 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. 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 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
Yes, I’ll put up a new patch with just the bug fix shortly. (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! Oh, one more thing: I think the #if around the checkNumberOfDistinctQuoteCharacters definition should be !ASSERT_DISABLED. 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 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
Created attachment 201080 [details]
Patch
Comment on attachment 201080 [details] Patch Clearing flags on attachment: 201080 Committed r149754: <http://trac.webkit.org/changeset/149754> All reviewed patches have been landed. Closing bug. |