Add word-spacing for each eligible space.
Created attachment 149197 [details] Patch
Comment on attachment 149197 [details] Patch Attachment 149197 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13039890 New failing tests: fast/css/word-space-extra.html
Created attachment 149199 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 150459 [details] Patch
Comment on attachment 150459 [details] Patch Shouldn’t the complex text code path be patched as well? r- for now.
(In reply to comment #5) > (From update of attachment 150459 [details]) > Shouldn’t the complex text code path be patched as well? r- for now. The complex text path is unique for each platform/port. It's not really possible for me to go there.
Created attachment 151297 [details] Patch
Comment on attachment 151297 [details] Patch Seems like you also need to patch RenderText::widthFromCache(), and possibly also RenderText::trimmedPrefWidths() and RenderText::computePreferredLogicalWidths(). I don’t understand why none of the code in line layout (specifically LineBreaker::nextLineBreak()) isn’t affected by this change.
(In reply to comment #8) > (From update of attachment 151297 [details]) > Seems like you also need to patch RenderText::widthFromCache(), and possibly also RenderText::trimmedPrefWidths() and RenderText::computePreferredLogicalWidths(). Yes, but there's clearly a lack of test coverage here. >I don’t understand why none of the code in line layout (specifically LineBreaker::nextLineBreak()) isn’t affected by this change. It is, but I guess there isn't any existing layout test in which word-spacing is decisive in creating a new line or causing a float to push down.
Created attachment 152042 [details] Patch
Comment on attachment 152042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152042&action=review > Source/WebCore/ChangeLog:33 > + (WebCore::RenderText::widthFromCache): This code-path is not covered by any existing layout tests! > + I've tried to come up with a test to hit it but have failed. Rather less dramatically - the clause altered here is a no-op for Chromium Linux as Font::isFixedPitch() is not implemented. This blocks me from testing it locally.
Hi Dan, Could you take a look at this one? Thanks, Robert
Comment on attachment 152042 [details] Patch Seems reasonable.
Committed r125430: <http://trac.webkit.org/changeset/125430>
Re-opened since this is blocked by 93881
This caused some failures on Apple Mountain Lion. https://bugs.webkit.org/show_bug.cgi?id=93882
I suspect css2.1/t1604-c541-word-sp-00-b-a.html may be invalid now but need to understand why it regressed on Mac before replacing it. http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r125430%20(2289)/results.html http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r125430%20(21773)/results.html
Hi Eric, Rolled out because it broke css2.1/t1604-c541-word-sp-00-b-a.htm on Mac builds, including Chromium Mac. I plan to replace css2.1/t1604-c541-word-sp-00-b-a.htm with a more up-to-date version but would still expect it to fail unless I remove the speculative code changes I made to platform/graphics/mac/ComplexTextController.cpp and WebCore::RenderText::widthFromCache. Would you like to re-review before I attempt to land a second time tomorrow? Thanks, Robert
Created attachment 158085 [details] Patch
Committed r125578: <http://trac.webkit.org/changeset/125578>
(In reply to comment #20) > Committed r125578: <http://trac.webkit.org/changeset/125578> Quite remarkably, this change did not include any changes to ComplexTextController (such as the ones that were in <http://trac.webkit.org/r125430>). As a result, there are now new discrepancies between the fast path and the complex path. It seems like comment #5 was just ignored.
<http://trac.webkit.org/r125578> also neglected to patch RenderText, which introduced a serious discrepancy in this case: <span style="font-size: 36px; word-spacing: 50px; border: solid red;">a b</span>
(In reply to comment #22) > <http://trac.webkit.org/r125578> also neglected to patch RenderText, which introduced a serious discrepancy in this case: > > <span style="font-size: 36px; word-spacing: 50px; border: solid red;">a b</span> Filed bug 96856.
Sorry, my bad. I make a bad habit of clicking on the review links directly. I can roll this out, but Robert is really the man to talk to here. Again, my apologies for inadvertently overriding your r- here.
(In reply to comment #21) > (In reply to comment #20) > > Committed r125578: <http://trac.webkit.org/changeset/125578> > > Quite remarkably, this change did not include any changes to ComplexTextController (such as the ones that were in <http://trac.webkit.org/r125430>). As a result, there are now new discrepancies between the fast path and the complex path. It seems like comment #5 was just ignored. Filed bug 96857.
(In reply to comment #20) > Committed r125578: <http://trac.webkit.org/changeset/125578> This caused bug 96865.
(In reply to comment #20) > Committed r125578: <http://trac.webkit.org/changeset/125578> This also caused bug 96869.
(In reply to comment #24) > Sorry, my bad. I make a bad habit of clicking on the review links directly. > > I can roll this out, but Robert is really the man to talk to here. Again, my apologies for inadvertently overriding your r- here. The patch Eric r+'d did address Mitz's comments re the complex path. However I had to roll it out as it broke a couple of tests. I fixed those tests and re-landed it per comments 17 and 18. I wish I hadn't re-landed now obviously - the platform-specific code paths for the complext text case and the fact that Chromium doesn't use the monospace code path mean that I stand very little chance of getting this right given my knowledge of this code. Sorry.