[webkit-reviews] review denied: [Bug 18926] dt:after generate a line break. Boost documentation page renders poorly : [Attachment 48313] Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements.

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Yuzo Fujishima <yuzo at google.com>'s request for
review:
Bug 18926: dt:after generate a line break. Boost documentation page renders
poorly
https://bugs.webkit.org/show_bug.cgi?id=18926

Attachment 48313: Do not assume that text is broken at pseudo/inline element
boundaries, i.e., after :before, before :after, and before/after inline
elements.
https://bugs.webkit.org/attachment.cgi?id=48313&action=review

------- Additional Comments from mitz at webkit.org
> --- 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.