[Webkit-unassigned] [Bug 18926] dt:after generate a line break. Boost documentation page renders poorly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=18926


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48313|review?                     |review-
               Flag|                            |




--- Comment #19 from mitz at webkit.org  2010-02-08 10:08:34 PST ---
(From update of attachment 48313)
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,20 @@
> +2010-02-07  Yuzo Fujishima  <yuzo at google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements.
> +        Tests css2.1/t0803-c5504-imrgn-l-02-b-ag-expected.html and css2.1/t0804-c5509-ipadn-l-02-b-ag-expected.html have been rebaselined because the original expected values are incorrect, judging from the test description and how IE 8.0, Firefox 3.6, and Opera 10.10 render the pages.
> +        Test fast/dom/clone-node-dynamic-style-expected.html have been also rebaselined because the original expected values assumes a line break.
> +        Expected values under platform/qt need not be updated because the tests are in the Skipped file for the platform.
> +        https://bugs.webkit.org/show_bug.cgi?id=18926
> +        https://bugs.webkit.org/show_bug.cgi?id=7276
> +
> +        Tests: fast/css/inline-element-line-break.html
> +               fast/css/pseudo-element-line-break.html
> +
> +        * rendering/RenderBlockLineLayout.cpp:
> +        (WebCore::RenderBlock::findNextLineBreak):
> +

The code change looks fine, but I have to say that this is not a good change
log entry. There is no need to mention the test result changes in the WebCore
change log. It is wrong to call out pseudoelements, :before and :after—there is
nothing in the bug or in the patch that is specific to those. I also don’t
understand “do not assume that text is broken”—it doesn’t look like the old
code was assuming that. Perhaps a better way to write this change log would be
to start with the titles and URLs of the bugs you’re fixing, then list the
tests you’ve added. Then you can explain what the root cause of the bug was:
findNextLineBreak() unconditionally allowed lines to break between elements
when no other line breaking opportunity had been found, but that was
unnecessary and led to incorrect layout, and you changed the code to not allow
those line breaks unconditionally. You can put that explanation before the
changed methods list or as a description of the change to findNextLineBreak().

Marking r- because of the change log.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.