Bug 108391 – RenderCounter and RenderQuote should always be anonymous

• Status: NEW
• Component: New Bugs

[Elliott Sprehn]

RenderCounter and RenderQuote should always be anonymous

[Elliott Sprehn]

Created attachment 185595 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 185595 [details]
Patch

Seems reasonable.  I was not aware "createAnonymous" was the pattern.  I assume this matches how we construct other anonymous renderers?  (And why not just pass the document as the node?)

[Eric Seidel (no email)]

Bah.  Sorry ojan, I didn't mean to ovewrite your review.

[Elliott Sprehn]

Created attachment 185599 [details]
Patch

[Ojan Vafai]

(In reply to comment #2)
> (From update of attachment 185595 [details])
> Seems reasonable.  I was not aware "createAnonymous" was the pattern.  I assume this matches how we construct other anonymous renderers?  (And why not just pass the document as the node?)

This was a recent refactoring to allow tightening types so we can pass an Element instead of a Node. RenderText wasn't tightened. This seems like a step in that direction.

[Elliott Sprehn]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 185595 [details] [details])
> > Seems reasonable.  I was not aware "createAnonymous" was the pattern.  I assume this matches how we construct other anonymous renderers?  (And why not just pass the document as the node?)
> 
> This was a recent refactoring to allow tightening types so we can pass an Element instead of a Node. RenderText wasn't tightened. This seems like a step in that direction.

See https://trac.webkit.org/changeset/140244

[Build Bot]

Comment on attachment 185599 [details]
Patch

Attachment 185599 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16187992

New failing tests:
css2.1/t1202-counter-01-b.html
accessibility/render-counter-text.html
css2.1/t1202-counter-02-b.html
css2.1/t1202-counter-04-b.html
css2.1/t1202-counter-05-b.html
accessibility/crash-determining-aria-role-when-label-present.html
editing/execCommand/remove-format-elements.html
dom/xhtml/level2/html/HTMLQuoteElement02.xhtml
dom/html/level2/html/HTMLQuoteElement02.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/query-format-block.html
fast/block/line-layout/line-break-obj-removal-crash.html
editing/selection/home-end.html
fast/block/float/float-not-removed-from-pre-block.html
css3/calc/transition-crash3.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
fast/block/child-not-removed-from-parent-lineboxes-crash.html
dom/xhtml/level2/html/HTMLQuoteElement01.xhtml
css2.1/t1202-counter-06-b.html
css2.1/t1202-counter-03-b.html
dom/html/level2/html/HTMLQuoteElement01.html
css2.1/t1202-counter-00-b.html

[Elliott Sprehn]

(In reply to comment #7)
> (From update of attachment 185599 [details])
> Attachment 185599 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16187992
> 
> New failing tests:
> ...

This doesn't work because of the line:

view()->frameView()->incrementVisuallyNonEmptyCharacterCount(m_text.length());

It's unfortunate we're passing null for the Node now for anonymous renderers since it means you can't access view(), frame() or document() inside the constructor.

[Elliott Sprehn]

(In reply to comment #8)
> ...
> 
> This doesn't work because of the line:
> 
> view()->frameView()->incrementVisuallyNonEmptyCharacterCount(m_text.length());
> 
> It's unfortunate we're passing null for the Node now for anonymous renderers since it means you can't access view(), frame() or document() inside the constructor.

I don't I can make this change easily. I think it might be a mistake to pass null for the node for anonymous renderers because it prevents accessing most things from inside the constructor. It also means you can't really call any methods on the renderer until someone calls setDocumentForAnonymous because if any of those methods calls view(), document(), or frame() we'd crash.

We should reevaluate that change.

[WebKit Review Bot]

Comment on attachment 185599 [details]
Patch

Attachment 185599 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16271023

New failing tests:
css2.1/t1202-counter-01-b.html
css2.1/t1202-counter-08-b.html
accessibility/render-counter-text.html
css2.1/t1202-counters-06-b.html
css2.1/t1202-counter-07-b.html
css2.1/t1202-counter-02-b.html
css2.1/t1202-counter-04-b.html
accessibility/crash-determining-aria-role-when-label-present.html
css2.1/t1202-counter-15-b.html
css2.1/t1202-counters-09-b.html
css2.1/t1202-counters-11-b.html
css2.1/t1202-counter-16-f.html
css2.1/t1202-counters-04-b.html
css2.1/t1202-counters-03-b.html
css2.1/t1202-counter-12-b.html
css2.1/t1202-counters-07-b.html
css2.1/t1202-counter-09-b.html
css2.1/t1202-counters-08-b.html
css2.1/t1202-counter-14-b.html
css2.1/t1202-counter-11-b.html
css3/calc/transition-crash3.html
css2.1/t1202-counters-01-b.html
css2.1/t1202-counter-13-b.html
css2.1/t1202-counter-05-b.html
css2.1/t1202-counter-06-b.html
css2.1/t1202-counter-03-b.html
css2.1/t1202-counters-02-b.html
css2.1/t1202-counters-00-b.html
css2.1/t1202-counter-00-b.html
css2.1/t1202-counters-05-b.html

[Build Bot]

Comment on attachment 185599 [details]
Patch

Attachment 185599 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16271159

[Ojan Vafai]

(In reply to comment #9)
> (In reply to comment #8)
> > ...
> > 
> > This doesn't work because of the line:
> > 
> > view()->frameView()->incrementVisuallyNonEmptyCharacterCount(m_text.length());
> > 
> > It's unfortunate we're passing null for the Node now for anonymous renderers since it means you can't access view(), frame() or document() inside the constructor.
> 
> I don't I can make this change easily. I think it might be a mistake to pass null for the node for anonymous renderers because it prevents accessing most things from inside the constructor. It also means you can't really call any methods on the renderer until someone calls setDocumentForAnonymous because if any of those methods calls view(), document(), or frame() we'd crash.
> 
> We should reevaluate that change.

I'm inclined to agree. While I'm a big fan of the type tightening in general, making it so that an anonymous renderer can accidentally not have a Document seems like it's not worth it.

[Antti Koivisto]

(In reply to comment #12)
> I'm inclined to agree. While I'm a big fan of the type tightening in general, making it so that an anonymous renderer can accidentally not have a Document seems like it's not worth it.

An accident like that is unlikely to happen in practice as you crash very soon without document pointer.

If you need view/document/frame access you might be doing something that you shouldn't be doing in the constructor in the first place. Why couldn't that action be taken when the renderer is added to the tree for example? If there is a real need, the right fix is to add another constructor parameter for passing the document.

[Elliott Sprehn]

(In reply to comment #13)
> (In reply to comment #12)
> > I'm inclined to agree. While I'm a big fan of the type tightening in general, making it so that an anonymous renderer can accidentally not have a Document seems like it's not worth it.
> 
> An accident like that is unlikely to happen in practice as you crash very soon without document pointer.
> 
> If you need view/document/frame access you might be doing something that you shouldn't be doing in the constructor in the first place. Why couldn't that action be taken when the renderer is added to the tree for example? If there is a real need, the right fix is to add another constructor parameter for passing the document.

The actions can't be taken when added to the tree since that requires a virtual method call to do a type check, or a virtual method call for the dispatch. I suppose we can pass the Document pointer, but that seems somewhat contorted. It all ends up in m_node anyway.

What's your eventual plan for this?

[Build Bot]

Comment on attachment 185599 [details]
Patch

Attachment 185599 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16342867

New failing tests:
css2.1/t1202-counter-01-b.html
fast/block/float/float-not-removed-from-pre-block.html
accessibility/render-counter-text.html
fast/css/counters/2displays.html
css2.1/t1202-counter-02-b.html
css2.1/t1202-counter-04-b.html
css2.1/t1202-counter-05-b.html
accessibility/crash-determining-aria-role-when-label-present.html
fast/block/colspan-under-button-crash.html
http/tests/security/view-source-no-refresh.html
editing/execCommand/remove-format-elements.html
dom/xhtml/level2/html/HTMLQuoteElement02.xhtml
dom/html/level2/html/HTMLQuoteElement02.html
editing/execCommand/remove-format-multiple-elements.html
editing/execCommand/query-format-block.html
fast/block/line-layout/line-break-obj-removal-crash.html
editing/selection/home-end.html
fast/css-generated-content/005.html
fast/css/getComputedStyle/computed-style-properties.html
fast/css/content/content-quotes-02.html
css3/calc/transition-crash3.html
http/tests/security/view-source-no-javascript-url.html
fast/css/content/content-quotes-01.html
fast/block/child-not-removed-from-parent-lineboxes-crash.html
dom/xhtml/level2/html/HTMLQuoteElement01.xhtml
editing/selection/move-left-right.html
css2.1/t1202-counter-03-b.html
fast/css/beforeSelectorOnCodeElement.html
dom/html/level2/html/HTMLQuoteElement01.html
css2.1/t1202-counter-00-b.html

[Ahmad Saleem]

I checked on Webkit Github with this bug number and it didn't land and I think there were test failures.

Is this needed anymore? Thanks!