WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
121221
RenderBR should not be RenderText
https://bugs.webkit.org/show_bug.cgi?id=121221
Summary
RenderBR should not be RenderText
Antti Koivisto
Reported
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.
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Antti Koivisto
Comment 1
2013-09-12 03:39:11 PDT
Created
attachment 211415
[details]
for bots
WebKit Commit Bot
Comment 2
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.
Early Warning System Bot
Comment 3
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
Antti Koivisto
Comment 4
2013-09-12 03:47:43 PDT
Created
attachment 211416
[details]
for bots 2
Build Bot
Comment 5
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
Build Bot
Comment 6
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
Build Bot
Comment 7
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
Build Bot
Comment 8
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
Ryosuke Niwa
Comment 9
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.
Antti Koivisto
Comment 10
2013-09-12 14:54:37 PDT
Yeah, working on it.
Antti Koivisto
Comment 11
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).
Darin Adler
Comment 12
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.
Antti Koivisto
Comment 13
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.
Build Bot
Comment 14
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
Build Bot
Comment 15
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
Build Bot
Comment 16
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
Build Bot
Comment 17
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
Build Bot
Comment 18
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
Build Bot
Comment 19
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
Antti Koivisto
Comment 20
2013-09-16 12:21:30 PDT
Created
attachment 211817
[details]
more bots
Build Bot
Comment 21
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
Build Bot
Comment 22
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
Build Bot
Comment 23
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
Build Bot
Comment 24
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
Antti Koivisto
Comment 25
2013-09-17 03:47:10 PDT
Created
attachment 211881
[details]
another
Early Warning System Bot
Comment 26
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
Early Warning System Bot
Comment 27
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
EFL EWS Bot
Comment 28
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
Build Bot
Comment 29
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
Build Bot
Comment 30
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
Antti Koivisto
Comment 31
2013-09-17 06:07:08 PDT
Created
attachment 211893
[details]
patch
Early Warning System Bot
Comment 32
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
Early Warning System Bot
Comment 33
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
Antti Koivisto
Comment 34
2013-09-17 07:05:36 PDT
Created
attachment 211899
[details]
with shouted OVERRIDE for qt bots
Darin Adler
Comment 35
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.
Antti Koivisto
Comment 36
2013-09-17 08:13:11 PDT
https://trac.webkit.org/r155957
Jessie Berlin
Comment 37
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
Dave Hyatt
Comment 38
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.
Antti Koivisto
Comment 39
2013-09-17 10:40:41 PDT
Fixed assertion in
https://trac.webkit.org/r155972
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug