Bug 121221 - RenderBR should not be RenderText
Summary: RenderBR should not be RenderText
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-12 03:37 PDT by Antti Koivisto
Modified: 2013-09-17 10:40 PDT (History)
9 users (show)

See Also:


Attachments
for bots (22.92 KB, patch)
2013-09-12 03:39 PDT, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
for bots 2 (23.30 KB, patch)
2013-09-12 03:47 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (491.30 KB, application/zip)
2013-09-12 08:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (497.58 KB, application/zip)
2013-09-12 08:55 PDT, Build Bot
no flags Details
bots again (51.60 KB, patch)
2013-09-13 11:55 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (649.67 KB, application/zip)
2013-09-13 13:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (659.21 KB, application/zip)
2013-09-13 17:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (894.99 KB, application/zip)
2013-09-13 18:05 PDT, Build Bot
no flags Details
more bots (69.77 KB, patch)
2013-09-16 12:21 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (1.20 MB, application/zip)
2013-09-16 13:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (1.21 MB, application/zip)
2013-09-16 13:37 PDT, Build Bot
no flags Details
another (73.44 KB, patch)
2013-09-17 03:47 PDT, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch (79.49 KB, patch)
2013-09-17 06:07 PDT, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
with shouted OVERRIDE for qt bots (79.49 KB, patch)
2013-09-17 07:05 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2013-09-12 03:37:12 PDT
<br> is one of the two cases where an Element type has a RenderText subclass as a renderer. RenderBR should be a RenderInline instead. As a text renderer it is heavily specialised and does not really behave like one.
Comment 1 Antti Koivisto 2013-09-12 03:39:11 PDT
Created attachment 211415 [details]
for bots
Comment 2 WebKit Commit Bot 2013-09-12 03:40:46 PDT
Attachment 211415 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/accessibility/AccessibilityRenderObject.cpp', u'Source/WebCore/dom/ContainerNode.cpp', u'Source/WebCore/rendering/InlineBox.cpp', u'Source/WebCore/rendering/InlineBox.h', u'Source/WebCore/rendering/InlineFlowBox.cpp', u'Source/WebCore/rendering/InlineFlowBox.h', u'Source/WebCore/rendering/InlineIterator.h', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/RenderBR.cpp', u'Source/WebCore/rendering/RenderBR.h', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp', u'Source/WebCore/rendering/RenderInline.cpp', u'Source/WebCore/rendering/RenderMenuList.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderText.cpp', u'Source/WebCore/rendering/RenderTreeAsText.cpp', u'Source/WebCore/rendering/RootInlineBox.cpp']" exit_code: 1
Source/WebCore/rendering/RenderBlockLineLayout.cpp:320:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Early Warning System Bot 2013-09-12 03:47:18 PDT
Comment on attachment 211415 [details]
for bots

Attachment 211415 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1800081
Comment 4 Antti Koivisto 2013-09-12 03:47:43 PDT
Created attachment 211416 [details]
for bots 2
Comment 5 Build Bot 2013-09-12 08:26:13 PDT
Comment on attachment 211416 [details]
for bots 2

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

New failing tests:
fast/forms/menulist-separator-painting.html
fast/repaint/selection-gap-transformed-fixed-child.html
fast/repaint/selection-gap-flipped-absolute-child.html
fast/forms/select-baseline.html
fast/replaced/three-selects-break.html
fast/multicol/table-vertical-align.html
editing/selection/end-of-document.html
fast/css-intrinsic-dimensions/max-width-unconstrained.html
fast/forms/HTMLOptionElement_label06.html
fast/forms/selectlist-minsize.html
fast/text/capitalize-boundaries.html
editing/pasteboard/5006779.html
fast/flexbox/clear-overflow-before-scroll-update.html
editing/selection/move-by-word-visually-mac.html
fast/css/word-space-extra.html
fast/table/005.html
platform/mac/editing/input/firstrectforcharacterrange-caret-in-br.html
editing/selection/move-by-word-visually-multi-line.html
fast/css/focus-ring-detached.html
fast/repaint/selection-gap-absolute-child.html
fast/text/whitespace/pre-wrap-spaces-after-newline.html
fast/repaint/selection-gap-fixed-child.html
fast/forms/select-empty-option-height.html
fast/forms/form-element-geometry.html
fast/repaint/selection-gap-flipped-fixed-child.html
fast/block/child-not-removed-from-parent-lineboxes-crash.html
fast/css-intrinsic-dimensions/width-avoid-floats.html
fast/forms/HTMLOptionElement_label07.html
fast/repaint/selection-gap-transformed-absolute-child.html
fast/canvas/canvas-measureText-ideographicSpace.html
Comment 6 Build Bot 2013-09-12 08:26:15 PDT
Created attachment 211431 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 7 Build Bot 2013-09-12 08:55:47 PDT
Comment on attachment 211416 [details]
for bots 2

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

New failing tests:
fast/forms/menulist-separator-painting.html
fast/repaint/selection-gap-transformed-fixed-child.html
fast/repaint/selection-gap-flipped-absolute-child.html
platform/mac/accessibility/webarea-size-equals-content-size.html
fast/table/005.html
fast/replaced/three-selects-break.html
fast/multicol/table-vertical-align.html
editing/selection/end-of-document.html
fast/css-intrinsic-dimensions/max-width-unconstrained.html
fast/forms/HTMLOptionElement_label06.html
fast/forms/selectlist-minsize.html
fast/text/capitalize-boundaries.html
editing/pasteboard/5006779.html
fast/flexbox/clear-overflow-before-scroll-update.html
editing/selection/move-by-word-visually-mac.html
fast/css/word-space-extra.html
fast/forms/select-baseline.html
editing/selection/move-by-word-visually-multi-line.html
fast/css/focus-ring-detached.html
fast/repaint/selection-gap-absolute-child.html
fast/text/whitespace/pre-wrap-spaces-after-newline.html
fast/repaint/selection-gap-fixed-child.html
fast/forms/select-empty-option-height.html
fast/forms/form-element-geometry.html
fast/repaint/selection-gap-flipped-fixed-child.html
fast/block/child-not-removed-from-parent-lineboxes-crash.html
fast/css-intrinsic-dimensions/width-avoid-floats.html
fast/forms/HTMLOptionElement_label07.html
fast/repaint/selection-gap-transformed-absolute-child.html
fast/canvas/canvas-measureText-ideographicSpace.html
Comment 8 Build Bot 2013-09-12 08:55:49 PDT
Created attachment 211433 [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.4
Comment 9 Ryosuke Niwa 2013-09-12 14:30:17 PDT
There is a whole bunch of editing code that relies on the fact RenderBR is a RenderText.  You need to update all those places.
Comment 10 Antti Koivisto 2013-09-12 14:54:37 PDT
Yeah, working on it.
Comment 11 Antti Koivisto 2013-09-13 11:55:07 PDT
Created attachment 211575 [details]
bots again

Now RenderBR is a RenderBoxModelObject instead of RenderInline making it much more lightweight (lighter than it was as RenderText).
Comment 12 Darin Adler 2013-09-13 12:05:24 PDT
Comment on attachment 211575 [details]
bots again

View in context: https://bugs.webkit.org/attachment.cgi?id=211575&action=review

> Source/WebCore/dom/Document.cpp:5847
> -                if (!curr->node() || !curr->node()->isElementNode() || curr->isText())
> +                if (!curr->node() || !curr->node()->isElementNode() || curr->isText() || curr->isBR())
>                      continue;

Wonder why this isText check is needed. I assume a text renderer would not have an element.

> Source/WebCore/dom/Position.cpp:63
> +static bool hasInlineBoxWrapper(RenderObject& renderer)
> +{
> +    if (renderer.isBox() && toRenderBox(renderer).inlineBoxWrapper())
> +        return true;
> +    if (renderer.isText() && toRenderText(renderer).firstTextBox())
> +        return true;
> +    if (renderer.isBR() && toRenderBR(renderer).inlineBoxWrapper())
> +        return true;
> +    return false;
> +}

Looks like box and br have something in common. Maybe some day they can have a common base class to fix this sort of thing?

> Source/WebCore/editing/ApplyStyleCommand.cpp:1024
> +    if (node->renderer()->isBR() && node->renderer()->style()->isCollapsibleWhiteSpace('\n'))
> +        return;

I think there's another way to write this where we ask the style the question more directly without actually passing '\n', but on the other hand this should be pretty fast so maybe that's overkill.

> Source/WebCore/editing/Editor.cpp:3079
> +        if (node->renderer()->isBR())
> +            return node;

WTF is "markable"? Any why does a text node that might be completely collapsed maybe or a <br> automatically qualify?

> Source/WebCore/rendering/InlineBox.cpp:161
> +    if (renderer().isBR() && !isText())
> +        return 0;

Why is the !isText() needed here?

> Source/WebCore/rendering/InlineBox.cpp:195
> +        if (m_renderer.isBox())
> +            toRenderBox(renderer()).setInlineBoxWrapper(0);
> +        else if (renderer().isBR())
> +            toRenderBR(renderer()).setInlineBoxWrapper(0);

Here’s that same box/br polymorphism I was wondering about earlier.

> Source/WebCore/rendering/InlineBox.h:72
> +    virtual bool isLineBreak() const { return renderer().isBR(); }

Add override? Or maybe this is not an override.

> Source/WebCore/rendering/InlineTextBox.cpp:368
> -    return renderer().isBR() || (renderer().style()->preserveNewline() && len() == 1 && (*textRenderer().text())[start()] == '\n');
> +    return renderer().style()->preserveNewline() && len() == 1 && (*textRenderer().text())[start()] == '\n';

Not sure why it's right to remove it here.

> Source/WebCore/rendering/RenderBR.h:3
> + * Copyright (C) 2013 Apple Computer, Inc.

Apple Inc.

> Source/WebCore/rendering/RenderBR.h:31
> +class RenderBR FINAL : public RenderBoxModelObject {

Seems like some of the functions in this class end up a little “copy and pastey”. Wonder if there are ways to share a little more of the code with RenderText. Just some inline functions maybe? Or maybe not worth it.

> Source/WebCore/rendering/RenderBR.h:50
> +    virtual VisiblePosition positionForPoint(const LayoutPoint&) OVERRIDE;

We don't have to shout OVERRIDE any more. We can just say override these days.
Comment 13 Antti Koivisto 2013-09-13 12:20:29 PDT
> Wonder why this isText check is needed. I assume a text renderer would not have an element.

It is shouldn't be needed anymore actually. It was the former isBR() test.

> Looks like box and br have something in common. Maybe some day they can have a common base class to fix this sort of thing?

Yeah, there would room for a common base.

> WTF is "markable"? Any why does a text node that might be completely collapsed maybe or a <br> automatically qualify?

No idea.

> Why is the !isText() needed here?

inlineBox->isText() is not the same is inlineBox->renderer().isText(). It just has a confusing name.

> Add override? Or maybe this is not an override.

Nope, it is the base class.

> > Source/WebCore/rendering/InlineTextBox.cpp:368
> > -    return renderer().isBR() || (renderer().style()->preserveNewline() && len() == 1 && (*textRenderer().text())[start()] == '\n');
> > +    return renderer().style()->preserveNewline() && len() == 1 && (*textRenderer().text())[start()] == '\n';
> 
> Not sure why it's right to remove it here.

RenderBR now generates InlineBoxes not InlineTextBoxes.

> Seems like some of the functions in this class end up a little “copy and pastey”. Wonder if there are ways to share a little more of the code with RenderText. Just some inline functions maybe? Or maybe not worth it.

RenderText versions are generally much more complicated, I doubt there is much useful sharing.
Comment 14 Build Bot 2013-09-13 13:39:31 PDT
Comment on attachment 211575 [details]
bots again

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

New failing tests:
fast/forms/menulist-separator-painting.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-002.html
fast/dom/element-attribute-js-null.html
fast/replaced/three-selects-break.html
editing/execCommand/insert-lists-inside-another-list.html
fast/multicol/table-vertical-align.html
css3/selectors3/xhtml/css3-modsel-179a.xml
fast/forms/HTMLOptionElement_label06.html
fast/forms/selectlist-minsize.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-inset-rectangle-001.html
editing/style/5228141.html
fast/encoding/utf-16-little-endian.html
editing/inserting/insert-div-023.html
fast/flexbox/clear-overflow-before-scroll-update.html
editing/selection/move-by-word-visually-mac.html
fast/forms/select-baseline.html
css3/selectors3/html/css3-modsel-179a.html
editing/selection/move-by-word-visually-multi-line.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-ellipse-000.html
fast/forms/HTMLOptionElement_label07.html
css3/selectors3/xml/css3-modsel-179a.xml
csswg/submitted/shapes/shape-outside/shape-outside-floats-circle-000.html
fast/forms/select-empty-option-height.html
fast/forms/form-element-geometry.html
fast/encoding/utf-16-big-endian.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-001.html
editing/pasteboard/paste-text-006.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-003.html
Comment 15 Build Bot 2013-09-13 13:39:33 PDT
Created attachment 211585 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 16 Build Bot 2013-09-13 17:27:21 PDT
Comment on attachment 211575 [details]
bots again

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

New failing tests:
fast/forms/menulist-separator-painting.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-002.html
fast/dom/element-attribute-js-null.html
fast/replaced/three-selects-break.html
editing/execCommand/insert-lists-inside-another-list.html
fast/multicol/table-vertical-align.html
css3/selectors3/xhtml/css3-modsel-179a.xml
fast/forms/HTMLOptionElement_label06.html
fast/forms/selectlist-minsize.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-inset-rectangle-001.html
editing/style/5228141.html
fast/encoding/utf-16-little-endian.html
editing/inserting/insert-div-023.html
fast/flexbox/clear-overflow-before-scroll-update.html
editing/selection/move-by-word-visually-mac.html
fast/forms/select-baseline.html
css3/selectors3/html/css3-modsel-179a.html
editing/selection/move-by-word-visually-multi-line.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-ellipse-000.html
fast/forms/HTMLOptionElement_label07.html
css3/selectors3/xml/css3-modsel-179a.xml
csswg/submitted/shapes/shape-outside/shape-outside-floats-circle-000.html
fast/forms/select-empty-option-height.html
fast/forms/form-element-geometry.html
fast/encoding/utf-16-big-endian.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-001.html
editing/pasteboard/paste-text-006.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-003.html
Comment 17 Build Bot 2013-09-13 17:27:22 PDT
Created attachment 211607 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 18 Build Bot 2013-09-13 18:05:50 PDT
Comment on attachment 211575 [details]
bots again

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

New failing tests:
fast/forms/menulist-separator-painting.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-002.html
fast/dom/element-attribute-js-null.html
fast/replaced/three-selects-break.html
editing/execCommand/insert-lists-inside-another-list.html
fast/multicol/table-vertical-align.html
css3/selectors3/xhtml/css3-modsel-179a.xml
fast/forms/HTMLOptionElement_label06.html
fast/forms/selectlist-minsize.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-inset-rectangle-001.html
editing/style/5228141.html
fast/encoding/utf-16-little-endian.html
editing/inserting/insert-div-023.html
fast/flexbox/clear-overflow-before-scroll-update.html
editing/selection/move-by-word-visually-mac.html
fast/forms/select-baseline.html
css3/selectors3/html/css3-modsel-179a.html
editing/selection/move-by-word-visually-multi-line.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-ellipse-000.html
fast/forms/HTMLOptionElement_label07.html
css3/selectors3/xml/css3-modsel-179a.xml
csswg/submitted/shapes/shape-outside/shape-outside-floats-circle-000.html
fast/forms/select-empty-option-height.html
fast/forms/form-element-geometry.html
fast/encoding/utf-16-big-endian.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-001.html
editing/pasteboard/paste-text-006.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-003.html
Comment 19 Build Bot 2013-09-13 18:05:52 PDT
Created attachment 211608 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 20 Antti Koivisto 2013-09-16 12:21:30 PDT
Created attachment 211817 [details]
more bots
Comment 21 Build Bot 2013-09-16 13:15:56 PDT
Comment on attachment 211817 [details]
more bots

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

New failing tests:
fast/forms/textarea-scrolled-type.html
fast/images/image-map-anchor-children.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-circle-000.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-003.html
fast/forms/textarea-no-scroll-on-blur.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-ellipse-000.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-001.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-002.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-inset-rectangle-001.html
editing/pasteboard/paste-text-006.html
fast/text/whitespace/pre-wrap-spaces-after-newline.html
Comment 22 Build Bot 2013-09-16 13:15:58 PDT
Created attachment 211826 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 23 Build Bot 2013-09-16 13:37:01 PDT
Comment on attachment 211817 [details]
more bots

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

New failing tests:
fast/forms/textarea-scrolled-type.html
fast/images/image-map-anchor-children.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-circle-000.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-003.html
fast/forms/textarea-no-scroll-on-blur.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-ellipse-000.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-001.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-rounded-rectangle-002.html
csswg/submitted/shapes/shape-outside/shape-outside-floats-inset-rectangle-001.html
editing/pasteboard/paste-text-006.html
fast/text/whitespace/pre-wrap-spaces-after-newline.html
Comment 24 Build Bot 2013-09-16 13:37:03 PDT
Created attachment 211827 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 25 Antti Koivisto 2013-09-17 03:47:10 PDT
Created attachment 211881 [details]
another
Comment 26 Early Warning System Bot 2013-09-17 03:55:01 PDT
Comment on attachment 211881 [details]
another

Attachment 211881 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1872335
Comment 27 Early Warning System Bot 2013-09-17 03:55:13 PDT
Comment on attachment 211881 [details]
another

Attachment 211881 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1869371
Comment 28 EFL EWS Bot 2013-09-17 04:05:05 PDT
Comment on attachment 211881 [details]
another

Attachment 211881 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1869374
Comment 29 Build Bot 2013-09-17 04:15:34 PDT
Comment on attachment 211881 [details]
another

Attachment 211881 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1836423
Comment 30 Build Bot 2013-09-17 04:30:11 PDT
Comment on attachment 211881 [details]
another

Attachment 211881 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1880308
Comment 31 Antti Koivisto 2013-09-17 06:07:08 PDT
Created attachment 211893 [details]
patch
Comment 32 Early Warning System Bot 2013-09-17 06:13:36 PDT
Comment on attachment 211893 [details]
patch

Attachment 211893 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1893268
Comment 33 Early Warning System Bot 2013-09-17 06:15:01 PDT
Comment on attachment 211893 [details]
patch

Attachment 211893 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1924068
Comment 34 Antti Koivisto 2013-09-17 07:05:36 PDT
Created attachment 211899 [details]
with shouted OVERRIDE for qt bots
Comment 35 Darin Adler 2013-09-17 07:54:24 PDT
Comment on attachment 211899 [details]
with shouted OVERRIDE for qt bots

View in context: https://bugs.webkit.org/attachment.cgi?id=211899&action=review

Sorry, I later figured out I was wrong about OVERRIDE, but forgot to tell you.

> Source/WebCore/dom/Range.cpp:1651
> +        if (r->isBR()) {
> +            r->absoluteQuads(quads, &isFixed);
> +        } else if (r->isText()) {

Normally our code style says to omit the braces in a case like this.

> Source/WebCore/rendering/RenderBR.cpp:3
> + * Copyright (C) 2006, 2013 Apple Computer, Inc.

Should be Apple Inc. Should say All rights reserved.

> Source/WebCore/rendering/RenderBR.cpp:192
> +    IntRect boundingBox = linesBoundingBox();
> +    return IntRect(0, 0, boundingBox.width(), boundingBox.height());

I think you can do it like one of these two:

    return IntRect(IntPoint(), linesBoundingBox().size();
    return IntRect(IntPoint(0, 0), linesBoundingBox().size();

I like those slightly better than the two line form.

> LayoutTests/ChangeLog:16
> +            Changes in render tree dumb that don't affect rendering.

dump, not dumb

> LayoutTests/ChangeLog:25
> +            Changes in render tree dumb that don't affect rendering.

Ditto.
Comment 36 Antti Koivisto 2013-09-17 08:13:11 PDT
https://trac.webkit.org/r155957
Comment 37 Jessie Berlin 2013-09-17 10:13:24 PDT
(In reply to comment #36)
> https://trac.webkit.org/r155957

This appears to have caused some assertion failures in the following tests:

fast/repaint/selection-rl.html
fast/writing-mode/horizontal-bt-replaced-selection.html
fast/writing-mode/vertical-rl-replaced-selection.html

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r155959%20(12652)/results.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010ca2080a WTFCrash + 42 (Assertions.cpp:342)
1   com.apple.WebCore             	0x000000010e2105b1 WebCore::toRenderBox(WebCore::RenderObject&) + 65 (RenderBox.h:711)
2   com.apple.WebCore             	0x000000010e20f9f8 WebCore::InlineBox::nodeAtPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) + 136 (InlineBox.cpp:261)
3   com.apple.WebCore             	0x000000010e21623e WebCore::InlineFlowBox::nodeAtPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) + 910 (InlineFlowBox.cpp:1040)
4   com.apple.WebCore             	0x000000010ed86aad WebCore::RootInlineBox::nodeAtPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::LayoutUnit, WebCore::LayoutUnit) + 381 (RootInlineBox.cpp:229)
5   com.apple.WebCore             	0x000000010ebff5ee WebCore::RenderLineBoxList::hitTest(WebCore::RenderBoxModelObject*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::HitTestAction) const + 1134 (RenderLineBoxList.cpp:302)
6   com.apple.WebCore             	0x000000010eaa256e WebCore::RenderBlock::hitTestContents(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::HitTestAction) + 158 (RenderBlock.cpp:4300)
7   com.apple.WebCore             	0x000000010eaa18f0 WebCore::RenderBlock::nodeAtPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::HitTestAction) + 1344 (RenderBlock.cpp:4142)
8   com.apple.WebCore             	0x000000010eaa2639 WebCore::RenderBlock::hitTestContents(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::HitTestAction) + 361 (RenderBlock.cpp:4309)
9   com.apple.WebCore             	0x000000010eaa18f0 WebCore::RenderBlock::nodeAtPoint(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::HitTestAction) + 1344 (RenderBlock.cpp:4142)
10  com.apple.WebCore             	0x000000010ec40bd9 WebCore::RenderObject::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::LayoutPoint const&, WebCore::HitTestFilter) + 121 (RenderObject.cpp:2905)
11  com.apple.WebCore             	0x000000010ebc0e57 WebCore::RenderLayer::hitTestContents(WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, WebCore::HitTestFilter) const + 279 (RenderLayer.cpp:4879)
12  com.apple.WebCore             	0x000000010ebc0d01 WebCore::RenderLayer::hitTestContentsForFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow> const&, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::HitTestLocation const&, WebCore::HitTestFilter, bool&) const + 273 (RenderLayer.cpp:4786)
13  com.apple.WebCore             	0x000000010ebbef88 WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, bool, WebCore::HitTestingTransformState const*, double*) + 3192 (RenderLayer.cpp:4729)
14  com.apple.WebCore             	0x000000010ebc0986 WebCore::RenderLayer::hitTestList(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow>*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, WebCore::HitTestingTransformState const*, double*, double*, WebCore::HitTestingTransformState const*, bool) + 518 (RenderLayer.cpp:4925)
15  com.apple.WebCore             	0x000000010ebbec56 WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::LayoutRect const&, WebCore::HitTestLocation const&, bool, WebCore::HitTestingTransformState const*, double*) + 2374 (RenderLayer.cpp:4698)
16  com.apple.WebCore             	0x000000010ebbe14a WebCore::RenderLayer::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestLocation const&, WebCore::HitTestResult&) + 442 (RenderLayer.cpp:4442)
17  com.apple.WebCore             	0x000000010ed3f26f WebCore::RenderView::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestLocation const&, WebCore::HitTestResult&) + 63 (RenderView.cpp:106)
18  com.apple.WebCore             	0x000000010ed3f224 WebCore::RenderView::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestResult&) + 68 (RenderView.cpp:101)
19  com.apple.WebCore             	0x000000010dcd245b WebCore::Document::prepareMouseEvent(WebCore::HitTestRequest const&, WebCore::LayoutPoint const&, WebCore::PlatformMouseEvent const&) + 203 (Document.cpp:2994)
20  com.apple.WebCore             	0x000000010de6774a WebCore::EventHandler::prepareMouseEvent(WebCore::HitTestRequest const&, WebCore::PlatformMouseEvent const&) + 202 (EventHandler.cpp:2204)
21  com.apple.WebCore             	0x000000010de67c5f WebCore::EventHandler::handleMouseMoveEvent(WebCore::PlatformMouseEvent const&, WebCore::HitTestResult*, bool) + 703 (EventHandler.cpp:1774)
22  com.apple.WebCore             	0x000000010de677e6 WebCore::EventHandler::mouseMoved(WebCore::PlatformMouseEvent const&) + 134 (EventHandler.cpp:1695)
23  com.apple.WebKit2             	0x000000010b09ff38 WebKit::handleMouseEvent(WebKit::WebMouseEvent const&, WebKit::WebPage*, bool) + 408 (WebPage.cpp:1656)
24  com.apple.WebKit2             	0x000000010b0a01e4 WebKit::WebPage::mouseEventSyncForTesting(WebKit::WebMouseEvent const&, bool&) + 596
Comment 38 Dave Hyatt 2013-09-17 10:20:00 PDT
Comment on attachment 211899 [details]
with shouted OVERRIDE for qt bots

View in context: https://bugs.webkit.org/attachment.cgi?id=211899&action=review

One change I'd like to see: Go ahead and add a new method, isTextOrBR() and use it. You have tons of call sites that have turned into isText() || isBR() and you might as well make a helper method that just handles that.

> Source/WebCore/ChangeLog:8
> +        Stop inhering RenderBR from RenderText and make it be a RenderBoxModelObject instead. RenderBR was one

Typo. "inheriting"

> Source/WebCore/ChangeLog:12
> +        didn't care about its text content at all. The new RenderText is also significatly more lightweight

Typo. "significantly"

> Source/WebCore/dom/Range.cpp:1649
> +        if (r->isBR()) {

Get rid of braces here.
Comment 39 Antti Koivisto 2013-09-17 10:40:41 PDT
Fixed assertion in https://trac.webkit.org/r155972