Convert first letter into a pseudo element.

Convert first letter into a pseudo element.

Currently, first letter renderers are created and destroyed through the updateFirstLetters() call in
RenderBlock. This has, historically, been problematic as we may miss places where the call
was required, leading to accessing invalid memory.

This CL converts the first letter code to use a PseudoElement for the first-letter instead of
creating classes during layout.

With the PseudoElement implementation the two tests which are currently in
LayoutTests/fast/css/first-letter-removed-added.html work as expected.

BUG=391288
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183913

[dsinclair, 2014-09-27 03:21:20]

dsinclair@chromium.org changed reviewers:
+ eseidel@chromium.org, esprehn@chromium.org, jchaffraix@chromium.org

[dsinclair, 2014-09-27 03:21:22]

PTAL.

This is ready for review now.

The only outstanding thing that I know of at this point is figuring out if I'm
hitting the ASSERT I added to the hit testing code. I'm going to test this out
on Monday.

I did a trybot run in patchset 13 and fixed the 3 actual failures in subsequent
tests. The bulk of the other failures are just changing generated to
pseudo:first-letter and removing #text in a few places.

https://codereview.chromium.org/571603003/diff/320001/LayoutTests/fast/css/dy...
File LayoutTests/fast/css/dynamic-class-pseudo-elements.html (right):

https://codereview.chromium.org/571603003/diff/320001/LayoutTests/fast/css/dy...
LayoutTests/fast/css/dynamic-class-pseudo-elements.html:84:
shouldBe("internals.updateStyleAndReturnAffectedElementCount()", "10");
What does this check do? I'm not sure why the number went up, but I also don't
understand where the number came from?

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/H...
File Source/core/rendering/HitTestResult.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/H...
Source/core/rendering/HitTestResult.cpp:148: ASSERT_NOT_REACHED();
This doesn't trip on any layout tests. I'm going to see if I can trigger it
manually ...

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2671: // FIXME: IS this right? Doing this
gives us the same behaviour as the non-pseudoElement first letter ....
This was a fix for a failing layout test, basically, act like the previous code.
 Is this the right thing to do to figure out the style?

[dsinclair, 2014-09-27 16:15:58]

dsinclair@chromium.org changed reviewers:
+ leviw@chromium.org, pdr@chromium.org

[esprehn, 2014-09-30 09:00:31]

This seems like it's moving in the right direction, but those
SubtreeStyleChanges are bad, we can't force an entire subtree to do a recalc any
time something related to a first-letter changes.

https://codereview.chromium.org/571603003/diff/320001/LayoutTests/fast/css/dy...
File LayoutTests/fast/css/dynamic-class-pseudo-elements.html (right):

https://codereview.chromium.org/571603003/diff/320001/LayoutTests/fast/css/dy...
LayoutTests/fast/css/dynamic-class-pseudo-elements.html:84:
shouldBe("internals.updateStyleAndReturnAffectedElementCount()", "10");
On 2014/09/27 at 03:21:21, dsinclair wrote:
> What does this check do? I'm not sure why the number went up, but I also don't
understand where the number came from?

It's the number of Element instances that went through ::recalcStyle(). So
you're causing 2 more elements to go through recalcStyle in this test. Given the
number of first letters, is that correct?

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1757: clearFirstLetterPseudoElement();
Destroying it on every child mutation doesn't seem right, if you have:

<div><span>Text</span></div>

then the child mutations will be in the <span> and it won't have the first
letter pseudo, the div will.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2513: if (pseudoId == FIRST_LETTER && element &&
!element->firstLetterTextRenderer()) {
Why can't we handle this in the code below?

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2519: // test_span_2.
Why? This comment explains what you're doing but not why. This code also doesn't
seem right, if you're always going to force it when you should just always pass
Force to the caller.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2526: if (pseudoId == FIRST_LETTER &&
element->firstLetterTextRenderer() != element->remainingTextRenderer()) {
firstLetterTextRenderer() doesn't seem like something that should be on Element,
neither should remainingTextRenderer().

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2589: setNeedsStyleRecalc(SubtreeStyleChange);
Woah, this is crazy, why does removing it cause a recalcStyle for the entire
subtree? That's not good. Especially not since you do this on every mutation of
the child list (which doesn't seem right either).

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:3321: static inline bool
isPunctuationForFirstLetter(UChar c)
Lets put all this code in it's own class.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:3343: unsigned Element::firstLetterLength(const
String& text) const
ditto. Own class, own files.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:3377: RenderObject*
Element::firstLetterTextRenderer() const
ditto

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:104:
ASSERT(m_remainingTextRenderer->node()->isTextNode());
toText() already asserts this, and RenderText can only have Text* as the node
anyway, so you don't need this assert.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:129: removeChildren();
This doesn't make sense, PseudoElement never has real DOM children.
removeChildren() doesn't do anything.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:130: this->attachFirstLetterTextRenderers();
no this->

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:190: this->renderer()->setStyle(pseudoStyle);
I don't think you need this-> anywhere in here.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:205:
this->renderer()->parent()->addChild(remainingText, nextSibling);
Woah this is scary, did you write this stuff or did it already exist?

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:234: renderer->setStyle(pseudoStyle);
Setting the style on yourself doesn't make sense, it should have already
happened.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:75: return style && style->display() != NONE
This statement has gotten confusing, can we break it into multiple if statements
and returns instead?

if (!style)
  return false;
if (style->display() == NONE)
  return false;
if (...)
  ...
return true;

https://codereview.chromium.org/571603003/diff/320001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:772: RenderObject* pseudoElement =
firstLetterPseudoElement(renderer);
Move this into the if (RenderObject* pseudoElement = ...) {

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/H...
File Source/core/rendering/HitTestResult.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/H...
Source/core/rendering/HitTestResult.cpp:148: ASSERT_NOT_REACHED();
On 2014/09/27 at 03:21:21, dsinclair wrote:
> This doesn't trip on any layout tests. I'm going to see if I can trigger it
manually ...

I don't think this can happen now, any hit that would hit a PseudoElement is
always retargetted at the parent node. See the code in here checking
isPseudoElement().

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2671: // FIXME: IS this right? Doing this
gives us the same behaviour as the non-pseudoElement first letter ....
On 2014/09/27 at 03:21:21, dsinclair wrote:
> This was a fix for a failing layout test, basically, act like the previous
code.  Is this the right thing to do to figure out the style?

Not sure what this is doing, have an example? Also please propagate the !, it's
hard to understand long if statements with negations at the start of groups like
this.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:54: Text* e = associatedTextNode();
text =, no single letter variables.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:76: if (RenderObject* previous =
previousSibling()) {
This feels sketchy, we can't keep an explicit RefPtr to the PseudoElement
instead?

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:80:
previous->node()->parentElement()->setNeedsStyleRecalc(SubtreeStyleChange);
This is really bad, you're causing a subtree recalc every time the text value
changes.

ex.

<div>Text here
  ... thousands of divs ...
  <div></div>
  ...
</div>

if you reach in and touch the text node with "Text here" and change the value
you'll force a recalc style of thousands of divs.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:120: Node* e = node();
node = this->node(); Not sure what e is, but no single letter vars.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:131: return (e && e->isTextNode())
? toText(e) : nullptr;
This code seems overly cautious, how can firstLetterTextRenderer ever not be a
text node? node() is actually a Text* anyway (it must be).

[dsinclair, 2014-09-30 21:46:34]

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1757: clearFirstLetterPseudoElement();
On 2014/09/30 09:00:30, esprehn wrote:
> Destroying it on every child mutation doesn't seem right, if you have:
> 
> <div><span>Text</span></div>
> 
> then the child mutations will be in the <span> and it won't have the first
> letter pseudo, the div will.


Removed. Re-ran the layout tests with this removed and it no-longer is needed.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2513: if (pseudoId == FIRST_LETTER && element &&
!element->firstLetterTextRenderer()) {
On 2014/09/30 09:00:30, esprehn wrote:
> Why can't we handle this in the code below?

Done.

Moved it into the if (change >= UpdatePseudoElements) below.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2519: // test_span_2.
On 2014/09/30 09:00:30, esprehn wrote:
> Why? This comment explains what you're doing but not why. This code also
doesn't
> seem right, if you're always going to force it when you should just always
pass
> Force to the caller.

The caller is now doing a Force if the children have changed. This will handle
the first letter.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2526: if (pseudoId == FIRST_LETTER &&
element->firstLetterTextRenderer() != element->remainingTextRenderer()) {
On 2014/09/30 09:00:30, esprehn wrote:
> firstLetterTextRenderer() doesn't seem like something that should be on
Element,
> neither should remainingTextRenderer().

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2589: setNeedsStyleRecalc(SubtreeStyleChange);
On 2014/09/30 09:00:30, esprehn wrote:
> Woah, this is crazy, why does removing it cause a recalcStyle for the entire
> subtree? That's not good. Especially not since you do this on every mutation
of
> the child list (which doesn't seem right either).

Done. This call has been removed.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:3321: static inline bool
isPunctuationForFirstLetter(UChar c)
On 2014/09/30 09:00:30, esprehn wrote:
> Lets put all this code in it's own class.

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:3343: unsigned Element::firstLetterLength(const
String& text) const
On 2014/09/30 09:00:30, esprehn wrote:
> ditto. Own class, own files.

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/Element...
Source/core/dom/Element.cpp:3377: RenderObject*
Element::firstLetterTextRenderer() const
On 2014/09/30 09:00:30, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:104:
ASSERT(m_remainingTextRenderer->node()->isTextNode());
On 2014/09/30 09:00:30, esprehn wrote:
> toText() already asserts this, and RenderText can only have Text* as the node
> anyway, so you don't need this assert.

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:129: removeChildren();
On 2014/09/30 09:00:30, esprehn wrote:
> This doesn't make sense, PseudoElement never has real DOM children.
> removeChildren() doesn't do anything.

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:130: this->attachFirstLetterTextRenderers();
On 2014/09/30 09:00:30, esprehn wrote:
> no this->

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:190: this->renderer()->setStyle(pseudoStyle);
On 2014/09/30 09:00:30, esprehn wrote:
> I don't think you need this-> anywhere in here.

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:205:
this->renderer()->parent()->addChild(remainingText, nextSibling);
On 2014/09/30 09:00:30, esprehn wrote:
> Woah this is scary, did you write this stuff or did it already exist?

I did, the previous version already had the firstLetterContainer and addChild()
to that container.

What would be the better solution here? Use nextSibling->parent()? or do I just
need to if (renderer()->parent()) in case we aren't in the tree for some reason?

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:234: renderer->setStyle(pseudoStyle);
On 2014/09/30 09:00:30, esprehn wrote:
> Setting the style on yourself doesn't make sense, it should have already
> happened.

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:75: return style && style->display() != NONE
On 2014/09/30 09:00:30, esprehn wrote:
> This statement has gotten confusing, can we break it into multiple if
statements
> and returns instead?
> 
> if (!style)
>   return false;
> if (style->display() == NONE)
>   return false;
> if (...)
>   ...
> return true;

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/H...
File Source/core/rendering/HitTestResult.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/H...
Source/core/rendering/HitTestResult.cpp:148: ASSERT_NOT_REACHED();
On 2014/09/30 09:00:31, esprehn wrote:
> On 2014/09/27 at 03:21:21, dsinclair wrote:
> > This doesn't trip on any layout tests. I'm going to see if I can trigger it
> manually ...
> 
> I don't think this can happen now, any hit that would hit a PseudoElement is
> always retargetted at the parent node. See the code in here checking
> isPseudoElement().


Done. I removed the m_isFirstLetter variable since it wouldn't be needed anymore
which simplified this function a bunch.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2671: // FIXME: IS this right? Doing this
gives us the same behaviour as the non-pseudoElement first letter ....
On 2014/09/30 09:00:31, esprehn wrote:
> On 2014/09/27 at 03:21:21, dsinclair wrote:
> > This was a fix for a failing layout test, basically, act like the previous
> code.  Is this the right thing to do to figure out the style?
> 
> Not sure what this is doing, have an example? Also please propagate the !,
it's
> hard to understand long if statements with negations at the start of groups
like
> this.


Logic cleaned up.

For an example, with fast/css/first-line-style-for-before-after-content.html, if
we don't have this extra bit of logic, we lose the first-letter styling on each
of the lines. The FIRST_LINE style is used and we lose the colour from the
FIRST_LETTER.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:54: Text* e = associatedTextNode();
On 2014/09/30 09:00:31, esprehn wrote:
> text =, no single letter variables.

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:76: if (RenderObject* previous =
previousSibling()) {
On 2014/09/30 09:00:31, esprehn wrote:
> This feels sketchy, we can't keep an explicit RefPtr to the PseudoElement
> instead?

I can do that, if it's preferred, I was more afraid of those pointers going
stale or the renderer having been removed already then just getting the sibling.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:80:
previous->node()->parentElement()->setNeedsStyleRecalc(SubtreeStyleChange);
On 2014/09/30 09:00:31, esprehn wrote:
> This is really bad, you're causing a subtree recalc every time the text value
> changes.
> 
> ex.
> 
> <div>Text here
>   ... thousands of divs ...
>   <div></div>
>   ...
> </div>
> 
> if you reach in and touch the text node with "Text here" and change the value
> you'll force a recalc style of thousands of divs.


Changed to LocalStyleChange. I had to add a second call to
previous->node()->setNeedsStyleRecalc() to force the first-letter pseudoElement
to also recalc style.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:120: Node* e = node();
On 2014/09/30 09:00:31, esprehn wrote:
> node = this->node(); Not sure what e is, but no single letter vars.

Done.

https://codereview.chromium.org/571603003/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:131: return (e && e->isTextNode())
? toText(e) : nullptr;
On 2014/09/30 09:00:31, esprehn wrote:
> This code seems overly cautious, how can firstLetterTextRenderer ever not be a
> text node? node() is actually a Text* anyway (it must be).


I believe I broke that invariant (if it is invariant?). In
PseudoElement::attachFirstLetterTextRenderers(), when I create the
RenderTextFragment for the letter, I set the node to 'this'. There is wackyness
that happens if you create two renderers with the same node, if the node gets
removed from the tree bad things were happening. So, I changed the node for the
letter renderer to be the pseudoElement.

[Julien - ping for review, 2014-10-01 21:14:48]

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterHelper.cpp (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.cpp:83: // Keep looking allowed punctuation
for the :first-letter.
Keep looking *for* allowed punctuation?

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.cpp:90: length = scanLength + 1;
It seems wasteful to do this update in the loop instead of just outside when we
have bailed out.

This is just copied from the old code so we will just want to add a FIXME.

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.cpp:123: 
While helpful, those extra spaces are not consistent with the rest of the Blink
code so I would drop them off.

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.cpp:126: // the first letter, we'll not apply
the correct style to it.
Could you give an example of when this would happen?

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.cpp:126: // the first letter, we'll not apply
the correct style to it.
Could you give an example of when this would happen (ideally in the comment)?

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterHelper.h (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.h:1: /*
This should be uploaded without copy detection as the diff is wrong.

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.h:40: FirstLetterHelper();
Maybe we should also use MAKE_NON_COPYABLE? Alternatively maybe we would want to
put a  = delete (based on the outcome of the blink-dev thread on that feature)?

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:107: }
Why do we need this code as we are destroying m_remainingTextRenderer?

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:67: RenderTextFragment* m_remainingTextRenderer;
This screams adding a specialized class and so is the FirstLetterHelper class
that should just be added to the new class IMHO.

https://codereview.chromium.org/571603003/diff/360001/Source/core/rendering/R...
File Source/core/rendering/RenderObjectChildList.cpp (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/rendering/R...
Source/core/rendering/RenderObjectChildList.cpp:49: firstChild()->remove(); //
List markers are owned by their enclosing list and so don't get destroyed by
this container.
Nit: I would move that above the isListMarker check to match the style below.

https://codereview.chromium.org/571603003/diff/360001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:89: previous->node()->detach();
It's interesting that detach doesn't trigger the
dont-modify-the-tree-during-layout-assert as we still call it during layout
(unless I missed the part of the patch that fixes this). It does modify the tree
by removing some child and have some side effects that we don't want during
layout.

[dsinclair, 2014-10-04 02:01:35]

I'm getting a crash with the current code that I don't understand. Any idea what
causes:


ASSERTION FAILED: ThreadState::isAnyThreadInGC()
../../third_party/WebKit/Source/platform/heap/Heap.cpp(2811) : static void
blink::Heap::addPageMemoryRegion(blink::PageMemoryRegion *)
1   0x7f8220d44be2 blink::Heap::addPageMemoryRegion(blink::PageMemoryRegion*)
2   0x7f8220d52132 blink::ThreadState::prepareRegionTree()
3   0x7f8220d5202d blink::ThreadState::detachMainThread()
4   0x7f82164a1c06 blink::shutdown()
5   0x7f821b2e0688 content::RenderThreadImpl::Shutdown()
6   0x7f821b2e06cc non-virtual thunk to content::RenderThreadImpl::Shutdown()
7   0x7f821ad9bb08 content::ChildProcess::~ChildProcess()


Specifically, it started after the last change, so it's something to do with me
calling elementRareData()->setPseudoElement(FIRST_LETTER, nullptr). Am I somehow
cleaning things up incorrectly?

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterHelper.cpp (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.cpp:83: // Keep looking allowed punctuation
for the :first-letter.
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> Keep looking *for* allowed punctuation?

Done.

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.cpp:90: length = scanLength + 1;
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> It seems wasteful to do this update in the loop instead of just outside when
we
> have bailed out.
> 
> This is just copied from the old code so we will just want to add a FIXME.

Fixed the code. We don't need scanLength at all as we can just increment length
as we iterate the loop instead.

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.cpp:123: 
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> While helpful, those extra spaces are not consistent with the rest of the
Blink
> code so I would drop them off.

Done.

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.cpp:126: // the first letter, we'll not apply
the correct style to it.
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> Could you give an example of when this would happen (ideally in the comment)?

I don't know. This is from the original code. I'll see if I can figure out an
example where this is an issue.

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterHelper.h (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.h:1: /*
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> This should be uploaded without copy detection as the diff is wrong.

Acknowledged.

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterHelper.h:40: FirstLetterHelper();
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> Maybe we should also use MAKE_NON_COPYABLE? Alternatively maybe we would want
to
> put a  = delete (based on the outcome of the blink-dev thread on that
feature)?

That was the macro I couldn't remember, thanks. (Was looking for
DISALLOW_COPY_AND_ASSIGN() from the chromium side)

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.cpp:107: }
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> Why do we need this code as we are destroying m_remainingTextRenderer?

We aren't destroying m_remainingTextRenderer.  We clean out our pointer just to
be safe, but m_remainingTextRenderer doesn't have the pseudoElement renderer as
a parent, it's a sibling.

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:67: RenderTextFragment* m_remainingTextRenderer;
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> This screams adding a specialized class and so is the FirstLetterHelper class
> that should just be added to the new class IMHO.

Done.

https://codereview.chromium.org/571603003/diff/360001/Source/core/rendering/R...
File Source/core/rendering/RenderObjectChildList.cpp (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/rendering/R...
Source/core/rendering/RenderObjectChildList.cpp:49: firstChild()->remove(); //
List markers are owned by their enclosing list and so don't get destroyed by
this container.
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> Nit: I would move that above the isListMarker check to match the style below.

Done.

https://codereview.chromium.org/571603003/diff/360001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/360001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:89: previous->node()->detach();
On 2014/10/01 21:14:47, Julien Chaffraix - PST wrote:
> It's interesting that detach doesn't trigger the
> dont-modify-the-tree-during-layout-assert as we still call it during layout
> (unless I missed the part of the patch that fixes this). It does modify the
tree
> by removing some child and have some side effects that we don't want during
> layout.

Done. This should be cleaned up when we do the recalc style as triggered above.

[dsinclair, 2014-10-04 03:39:02]

Figured it out, I was just sync'd to a bad point in the tree. Issue had already
been reverted.

[dsinclair, 2014-10-04 16:00:44]

PTAL. esprehn@, jchaffraix@ I believe I've got everything worked out after the
review changes.

[Julien - ping for review, 2014-10-06 17:47:34]

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2512: if (!remainingTextRenderer ||
remainingTextRenderer !=
toFirstLetterPseudoElement(element)->remainingTextRenderer()
Wouldn't |element| be an arbitrary Element and not necessarily a
FirstLetterPseudoElement?

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:104: parentRenderer =
element.parentOrShadowHostElement()->renderer();
It seems like following shadow hosts could potentially allow first-letter to
cross shadow tree boundaries. I think we should use parentElement() to stop at
shadow tree boundaries.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:118: // want to consider that
renderer for our text renderer so we go the sibling.
we go *to* the sibling?

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:127: } else if
(firstLetterTextRenderer->isListMarker()) {
These conditions just seems like magic to me. It would be easier to read with
some 'why' comment to explain the special casing of certain renderers.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:127: } else if
(firstLetterTextRenderer->isListMarker()) {
These conditions just seems like magic to me. It would be easier to read with
some 'why' comment to explain the special casing of certain renderers.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:141: // Let the child handle it
when it's attached.
Not totally sure what this means: are you delegating the first-letter renderer
creation to the child (which child btw?) during attachment?

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:146: }
It really looks like this could go into a helper function for readability.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:191: RenderObject* styleContainer =
parentOrShadowHostElement()->renderer();
Same comment about the use of parentOrShadowHostElement() potentially bleeding
through shadow trees' boundaries.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:54: friend class FirstLetterPseudoElement; //
For constructor.
Let's make the constructor protected instead of a blanket friend declaration.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:77: return false;
This past 3 lines can be simplified to:

return style->contentData();

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:78: }
It seems like a switch on styleType() would make this code more readable as the
code was doing implicit casing.

https://codereview.chromium.org/571603003/diff/560001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:765: if (previousSibling &&
previousSibling->isPseudoElement() &&
previousSibling->node()->isFirstLetterPseudoElement())
Isn't the previousSibling->isPseudoElement() call redundant with
isFirstLetterPseudoElement()? (I haven't repeated this comment to other similar
patterns)

Also it's weird that this code doesn't use firstLetterPseudoElement like the
rest of the code.

https://codereview.chromium.org/571603003/diff/560001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:778: ASSERT(firstLetter->isText());
This ASSERT is redundant with the toRenderText below *and* is disabled in
Release contrary to toRenderText.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderInline.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderInline.cpp:786: 
Nit: Unneeded empty space.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (left):

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:117: // Don't reset first-letter
here because we are only transforming the truncated fragment.
Doesn't this comment still applies?

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:122: // FIXME(dsinclair): First
letter?
Let's put an actionable FIXME, preferably with a bug.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:143: 
Unneeded space to be removed.

[Julien - ping for review, 2014-10-06 20:11:46]

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2512: if (!remainingTextRenderer ||
remainingTextRenderer !=
toFirstLetterPseudoElement(element)->remainingTextRenderer()
On 2014/10/06 at 17:47:32, Julien Chaffraix - PST wrote:
> Wouldn't |element| be an arbitrary Element and not necessarily a
FirstLetterPseudoElement?

Actually nevermind, I thought |element| == this.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.h (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.h:72: FirstLetterPseudoElement*
m_firstLetterPseudoElement;
Shouldn't this be a RawPtrWillBeMember<FirstLetterPseudoElement> too? It would
seem fine to mark this member if it is set, which is what DOM classes usually do
to Node pointers.

[esprehn, 2014-10-07 00:17:40]

You're causing full document forced style recalcs, that's not right.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2519: createPseudoElementIfNeeded(pseudoId);
You should just reattach the first letter PseudoElement. There's no need to
destroy and recreate the DOM node.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2547: elementRareData()->setPseudoElement(pseudoId,
nullptr);
This is in the wrong side of the branch. You should fix the above if statement
instead or add a new case.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:99: RenderObject* parentRenderer;
= 0;

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:103: if
(element.isFirstLetterPseudoElement())
This doesn't make sense, why does this code think that you can call it on both
the PseudoElement and other random elements? That shouldn't happen.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:104: parentRenderer =
element.parentOrShadowHostElement()->renderer();
On 2014/10/06 at 17:47:33, Julien Chaffraix - PST wrote:
> It seems like following shadow hosts could potentially allow first-letter to
cross shadow tree boundaries. I think we should use parentElement() to stop at
shadow tree boundaries.

PseudoElements are never direct children of ShadowRoots, so this is safe.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.h:1: /*
new short copyright.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.h:52: friend class PseudoElement; //
For constructor.
remove friend, make the constructor protected.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:73: if (style->styleType() == BACKDROP)
I would just || the two style types together.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:90:
toFirstLetterPseudoElement(previous->node())->setIsStale();
setIsStale() should probably be something like setNeedsUpdate() and it should
schedule the style recalc.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:92:
document().setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::PseudoClass));
This is definitely not right, you're causing a _full document_ forced style
recalc!

[dsinclair, 2014-10-07 19:36:21]

PTAL.

esprehn@, how do I trigger a walk to recalc style of nodes without triggering a
full document recalc style?

I tried document().updateRenderTreeIfNeeded() and
document().updateLayoutIgnorePendingStylesheets() but neither of those did the
trick.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2512: if (!remainingTextRenderer ||
remainingTextRenderer !=
toFirstLetterPseudoElement(element)->remainingTextRenderer()
On 2014/10/06 20:11:45, Julien Chaffraix - PST wrote:
> On 2014/10/06 at 17:47:32, Julien Chaffraix - PST wrote:
> > Wouldn't |element| be an arbitrary Element and not necessarily a
> FirstLetterPseudoElement?
> 
> Actually nevermind, I thought |element| == this.


Right, it has to be otherwise something has gone very very very wrong since we
pull it from the pseudoElement() call.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2519: createPseudoElementIfNeeded(pseudoId);
On 2014/10/07 00:17:40, esprehn wrote:
> You should just reattach the first letter PseudoElement. There's no need to
> destroy and recreate the DOM node.

Done.

I was using FirstLetterPseudoElement::dispose() to put the first letter back
into the remaining text renderer. I've changed that to be ::detach instead.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2547: elementRareData()->setPseudoElement(pseudoId,
nullptr);
On 2014/10/07 00:17:40, esprehn wrote:
> This is in the wrong side of the branch. You should fix the above if statement
> instead or add a new case.

Done? I wasn't quite sure what you meant with the above, but is this what you
were looking for?

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:99: RenderObject* parentRenderer;
On 2014/10/07 00:17:40, esprehn wrote:
> = 0;

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:103: if
(element.isFirstLetterPseudoElement())
On 2014/10/07 00:17:40, esprehn wrote:
> This doesn't make sense, why does this code think that you can call it on both
> the PseudoElement and other random elements? That shouldn't happen.

This does get called on non-PseudoElements. We use this to find the remaining
text renderer, which has to start at the node the pseudoElement is attached too
and look from there. (The method is static)

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:104: parentRenderer =
element.parentOrShadowHostElement()->renderer();
On 2014/10/07 00:17:40, esprehn wrote:
> On 2014/10/06 at 17:47:33, Julien Chaffraix - PST wrote:
> > It seems like following shadow hosts could potentially allow first-letter to
> cross shadow tree boundaries. I think we should use parentElement() to stop at
> shadow tree boundaries.
> 
> PseudoElements are never direct children of ShadowRoots, so this is safe.

Acknowledged.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:118: // want to consider that
renderer for our text renderer so we go the sibling.
On 2014/10/06 17:47:32, Julien Chaffraix - PST wrote:
> we go *to* the sibling?

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:127: } else if
(firstLetterTextRenderer->isListMarker()) {
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> These conditions just seems like magic to me. It would be easier to read with
> some 'why' comment to explain the special casing of certain renderers.


This is from the original code (see RenderBlock). So, I don't know the exact
why. I've added a fixme to try to trackdown some meaning.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:141: // Let the child handle it
when it's attached.
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> Not totally sure what this means: are you delegating the first-letter renderer
> creation to the child (which child btw?) during attachment?

Done. Better?

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:146: }
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> It really looks like this could go into a helper function for readability.

I'm not sure how much that would help. The code would be very similar in another
method, we'd just be able to skip some of the parentRenderer checks.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:191: RenderObject* styleContainer =
parentOrShadowHostElement()->renderer();
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> Same comment about the use of parentOrShadowHostElement() potentially bleeding
> through shadow trees' boundaries.

Acknowledged.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.h:1: /*
On 2014/10/07 00:17:40, esprehn wrote:
> new short copyright.

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.h:52: friend class PseudoElement; //
For constructor.
On 2014/10/07 00:17:40, esprehn wrote:
> remove friend, make the constructor protected.

I can't in this case. We inherit from PseudoElement, not the other way around. I
can make it public, but I only wanted it to be create-able through the
PseudoElement::create call.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:54: friend class FirstLetterPseudoElement; //
For constructor.
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> Let's make the constructor protected instead of a blanket friend declaration.

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:73: if (style->styleType() == BACKDROP)
On 2014/10/07 00:17:40, esprehn wrote:
> I would just || the two style types together.

Went with the switch route.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:77: return false;
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> This past 3 lines can be simplified to:
> 
> return style->contentData();

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:78: }
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> It seems like a switch on styleType() would make this code more readable as
the
> code was doing implicit casing.

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:765: if (previousSibling &&
previousSibling->isPseudoElement() &&
previousSibling->node()->isFirstLetterPseudoElement())
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> Isn't the previousSibling->isPseudoElement() call redundant with
> isFirstLetterPseudoElement()? (I haven't repeated this comment to other
similar
> patterns)

No. I don't know if I have a node() unless I know if I'm a pseudoElement. It's
possible for previousSibling->node() to return a nullptr.



> Also it's weird that this code doesn't use firstLetterPseudoElement like the
> rest of the code.

The firstLetterPseudoElement didn't exist on RenderTextFragment when I wrote
this code. Fixed.

https://codereview.chromium.org/571603003/diff/560001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:778: ASSERT(firstLetter->isText());
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> This ASSERT is redundant with the toRenderText below *and* is disabled in
> Release contrary to toRenderText.

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderInline.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderInline.cpp:786: 
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> Nit: Unneeded empty space.

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (left):

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:117: // Don't reset first-letter
here because we are only transforming the truncated fragment.
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> Doesn't this comment still applies?

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:90:
toFirstLetterPseudoElement(previous->node())->setIsStale();
On 2014/10/07 00:17:40, esprehn wrote:
> setIsStale() should probably be something like setNeedsUpdate() and it should
> schedule the style recalc.

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:92:
document().setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::PseudoClass));
On 2014/10/07 00:17:40, esprehn wrote:
> This is definitely not right, you're causing a _full document_ forced style
> recalc!


How do I tell the system that it needs to run style recalc or certain nodes?
This worked, but as you said, is triggering a full recalc.

I tried document().updateRenderTreeIfNeeded() and
document().updateLayoutIgnorePendingStylesheets() but neither of those did the
trick.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:122: // FIXME(dsinclair): First
letter?
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> Let's put an actionable FIXME, preferably with a bug.


Leftover from when I was cutting things out, removed.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:143: 
On 2014/10/06 17:47:33, Julien Chaffraix - PST wrote:
> Unneeded space to be removed.

Done.

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.h (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.h:72: FirstLetterPseudoElement*
m_firstLetterPseudoElement;
On 2014/10/06 20:11:45, Julien Chaffraix - PST wrote:
> Shouldn't this be a RawPtrWillBeMember<FirstLetterPseudoElement> too? It would
> seem fine to mark this member if it is set, which is what DOM classes usually
do
> to Node pointers.

Done.

[esprehn, 2014-10-07 21:06:19]

You want to do setNeedsStyleRecalc(LocalStyleChange) on the specific nodes
you need a recalc for.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dsinclair, 2014-10-07 21:12:53]

On 2014/10/07 21:06:19, esprehn wrote:
> You want to do setNeedsStyleRecalc(LocalStyleChange) on the specific nodes
> you need a recalc for.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

Done. Try jobs running now to see if everything still works.

[dsinclair, 2014-10-08 19:44:57]

On 2014/10/07 21:12:53, dsinclair wrote:
> On 2014/10/07 21:06:19, esprehn wrote:
> > You want to do setNeedsStyleRecalc(LocalStyleChange) on the specific nodes
> > you need a recalc for.
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> Done. Try jobs running now to see if everything still works.


I went through the layout test results and everything looks good.

[Julien - ping for review, 2014-10-09 18:24:49]

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/560001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:104: parentRenderer =
element.parentOrShadowHostElement()->renderer();
On 2014/10/07 19:36:20, dsinclair wrote:
> On 2014/10/07 00:17:40, esprehn wrote:
> > On 2014/10/06 at 17:47:33, Julien Chaffraix - PST wrote:
> > > It seems like following shadow hosts could potentially allow first-letter
to
> > cross shadow tree boundaries. I think we should use parentElement() to stop
at
> > shadow tree boundaries.
> > 
> > PseudoElements are never direct children of ShadowRoots, so this is safe.
> 
> Acknowledged.

I don't deny that we don't use pseudo as direct children of shadow roots but I
would rather stick with less dangerous APIs as we have the choice.

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:3: // found in the LICENSE file.
I don't think the new header applied to old content as this file is mostly
copied from RenderBlock.cpp.

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:132: // FIXME: This black-list of
disallowed RenderText subclasses is fragile.
Could we do a white list instead?

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:76: default:
Let's avoid having a default case as it would allow the compiler to catch adding
more cases.

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
File Source/core/dom/RenderTreeBuilder.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
Source/core/dom/RenderTreeBuilder.cpp:56: if (m_renderingParent->needsAttach())
You're checking m_renderingParent->needsAttach() for an element that is *not*
the right parent and that's probably wrong. We could change the constructor to
point to the right parent and that would remove the code in parentRenderer().

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
Source/core/dom/RenderTreeBuilder.cpp:60: return
FirstLetterPseudoElement::firstLetterTextRenderer(*element);
If we keep the code in parentRenderer(), there is an inconsistency here as we
check for top layer in this function but not in the other one.

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:767: if
(renderer->firstLetterPseudoElement()) {
if (FirstLetterPseudoElement* firstLetterElement =
renderer->firstLetterPseudoElement()) {

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:771: // First letter pseudo elements only
have one child which is the text.
This comment sounds like the following 'if' is not needed but AFAICT
:first-letter may not apply so it's not unconditional.

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:843: RenderObject* pseudoElement =
toRenderTextFragment(renderer)->firstLetterPseudoElement()->renderer();
We do a lot of NULL-check against the :first-letter's renderer but do we have
coverage of that? I am assuming the following would trigger that:

:first-letter {
    display: none;
}

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:1523: RenderObject* pseudoElement =
fragment->firstLetterPseudoElement()->renderer();
s/pseudoElement/pseudoElementRenderer/

https://codereview.chromium.org/571603003/diff/660001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:808: ASSERT(firstLetterRenderer &&
firstLetterRenderer->isText());
toRenderText will also check for isText but in release too so this checks seems
redundant.

Also when you have some logical ors in ASSERT, it's a sign that it should be
split up in several ASSERTs.

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2659: &&
(!rendererForFirstLineStyle->node() ||
!rendererForFirstLineStyle->node()->isFirstLetterPseudoElement())) {
rendererForFirstLineStyle->node() will only return 0 for anonymous content and
the first close of the if checks for that. Thus I think that this check is
redundant and unneeded.

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:641: // This is the same as node() except
for renderers of :before, :after and :first-letter
Nit: How about putting the :first-letter on the following line. That would make
for a better split paragraph.

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:65: PassRefPtr<StringImpl>
RenderTextFragment::completeText() const
We now have completeText() and originalText(). The difference between the 2 is
pretty difficult to draw. Could we pick a naming that makes the distinction more
obvious?

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.h (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.h:67: virtual void
updateHitTestResult(HitTestResult&, const LayoutPoint&) override;
Unrelated but it's weird that updateHitTestResult can have side effects.

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderTreeAsText.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderTreeAsText.cpp:180: if (o.isPseudoElement() ||
(o.parent() && o.parent()->isPseudoElement() &&
toPseudoElement(o.parent()->node())->pseudoId() != FIRST_LETTER))
:-(

I would like us to remove this code upfront (I would happily rubber-stamp that).
At the very least a follow-up bug under Warden to remove this madness.

[dsinclair, 2014-10-09 21:14:23]

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:3: // found in the LICENSE file.
On 2014/10/09 18:24:47, Julien Chaffraix - PST wrote:
> I don't think the new header applied to old content as this file is mostly
> copied from RenderBlock.cpp.

Reverted to the old one to be safe.

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:132: // FIXME: This black-list of
disallowed RenderText subclasses is fragile.
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> Could we do a white list instead?


It _could_ be. But, it'd end up being like 50 entries long as I'd have to add
kind of renderer to start with. How's about a bug attached to the FIXME?

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:76: default:
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> Let's avoid having a default case as it would allow the compiler to catch
adding
> more cases.


There are a lot of these we don't actually care about (there are 21 items in the
PseudoId enum ....) most of which aren't PseudoElements (only first-letter,
backdrop, before and after).

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
File Source/core/dom/RenderTreeBuilder.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
Source/core/dom/RenderTreeBuilder.cpp:56: if (m_renderingParent->needsAttach())
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> You're checking m_renderingParent->needsAttach() for an element that is *not*
> the right parent and that's probably wrong. We could change the constructor to
> point to the right parent and that would remove the code in parentRenderer().


Are you saying this is an existing issue, or due to the way
FirstLetterPseudoElement is built?

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
Source/core/dom/RenderTreeBuilder.cpp:60: return
FirstLetterPseudoElement::firstLetterTextRenderer(*element);
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> If we keep the code in parentRenderer(), there is an inconsistency here as we
> check for top layer in this function but not in the other one.


In what cases is there a top layer? Is this something you think is specific to
first-letter or an existing issue?

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:767: if
(renderer->firstLetterPseudoElement()) {
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> if (FirstLetterPseudoElement* firstLetterElement =
> renderer->firstLetterPseudoElement()) {

Done.

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:771: // First letter pseudo elements only
have one child which is the text.
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> This comment sounds like the following 'if' is not needed but AFAICT
> :first-letter may not apply so it's not unconditional.


Changed it to an ASSERT as it should always exist.

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:843: RenderObject* pseudoElement =
toRenderTextFragment(renderer)->firstLetterPseudoElement()->renderer();
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> We do a lot of NULL-check against the :first-letter's renderer but do we have
> coverage of that? I am assuming the following would trigger that:
> 
> :first-letter {
>     display: none;
> }


The spec doesn't list display as valid in first-letter
(http://www.w3.org/TR/CSS2/selector.html#first-letter).

This check, probably, isn't necessary but I'm begin paranoid.

https://codereview.chromium.org/571603003/diff/660001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:1523: RenderObject* pseudoElement =
fragment->firstLetterPseudoElement()->renderer();
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> s/pseudoElement/pseudoElementRenderer/

Done.

https://codereview.chromium.org/571603003/diff/660001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:808: ASSERT(firstLetterRenderer &&
firstLetterRenderer->isText());
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> toRenderText will also check for isText but in release too so this checks
seems
> redundant.
> 
> Also when you have some logical ors in ASSERT, it's a sign that it should be
> split up in several ASSERTs.

Done (removed the assert).

So, in general, is:

ASSERT(firstLetterRenderer)
ASSERT(firstLetterRenderer->isText())

preferable to the &&'d version?

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2659: &&
(!rendererForFirstLineStyle->node() ||
!rendererForFirstLineStyle->node()->isFirstLetterPseudoElement())) {
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> rendererForFirstLineStyle->node() will only return 0 for anonymous content and
> the first close of the if checks for that. Thus I think that this check is
> redundant and unneeded.

Done.

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:641: // This is the same as node() except
for renderers of :before, :after and :first-letter
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> Nit: How about putting the :first-letter on the following line. That would
make
> for a better split paragraph.

Done.

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:65: PassRefPtr<StringImpl>
RenderTextFragment::completeText() const
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> We now have completeText() and originalText(). The difference between the 2 is
> pretty difficult to draw. Could we pick a naming that makes the distinction
more
> obvious?

Agreed.

What about changing completeText to originalText and originalText to
fragmentText?

I think that's closer to what these are, current completeText gives you all the
text from the text node and originalText gives you the text from the node,
possibly minus the first-letter (so the text originally set into
RenderTextFragment).

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.h (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.h:67: virtual void
updateHitTestResult(HitTestResult&, const LayoutPoint&) override;
On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> Unrelated but it's weird that updateHitTestResult can have side effects.

Acknowledged.

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderTreeAsText.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderTreeAsText.cpp:180: if (o.isPseudoElement() ||
(o.parent() && o.parent()->isPseudoElement() &&
toPseudoElement(o.parent()->node())->pseudoId() != FIRST_LETTER))
On 2014/10/09 18:24:49, Julien Chaffraix - PST wrote:
> :-(
> 
> I would like us to remove this code upfront (I would happily rubber-stamp
that).
> At the very least a follow-up bug under Warden to remove this madness.

Done.

https://codereview.chromium.org/645623003

[Julien - ping for review, 2014-10-10 14:47:07]

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:132: // FIXME: This black-list of
disallowed RenderText subclasses is fragile.
On 2014/10/09 21:14:22, dsinclair wrote:
> On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > Could we do a white list instead?
> 
> 
> It _could_ be. But, it'd end up being like 50 entries long as I'd have to add
> kind of renderer to start with. How's about a bug attached to the FIXME?

50 is too long indeed. Let's stick to a FIXME.

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:76: default:
On 2014/10/09 21:14:22, dsinclair wrote:
> On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > Let's avoid having a default case as it would allow the compiler to catch
> adding
> > more cases.
> 
> 
> There are a lot of these we don't actually care about (there are 21 items in
the
> PseudoId enum ....) most of which aren't PseudoElements (only first-letter,
> backdrop, before and after).

We looked at this with Dan and having a default with ASSERT_NOT_REACHED()
doesn't work here so it's better to revert to simple if's.

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
File Source/core/dom/RenderTreeBuilder.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
Source/core/dom/RenderTreeBuilder.cpp:56: if (m_renderingParent->needsAttach())
On 2014/10/09 21:14:22, dsinclair wrote:
> On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > You're checking m_renderingParent->needsAttach() for an element that is
*not*
> > the right parent and that's probably wrong. We could change the constructor
to
> > point to the right parent and that would remove the code in
parentRenderer().
> 
> 
> Are you saying this is an existing issue, or due to the way
> FirstLetterPseudoElement is built?

I was confused by the logic actually and we have the right parent. However the
following logic seems unneeded and Dan explained to me that this is to disable
creating the renderer.

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
Source/core/dom/RenderTreeBuilder.cpp:60: return
FirstLetterPseudoElement::firstLetterTextRenderer(*element);
On 2014/10/09 21:14:22, dsinclair wrote:
> On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > If we keep the code in parentRenderer(), there is an inconsistency here as
we
> > check for top layer in this function but not in the other one.
> 
> 
> In what cases is there a top layer? Is this something you think is specific to
> first-letter or an existing issue?

top layer is a DOM Element API so it won't happen on pseudo but I like to keep
the consistency to avoid being bitten back later.

https://codereview.chromium.org/571603003/diff/660001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:808: ASSERT(firstLetterRenderer &&
firstLetterRenderer->isText());
On 2014/10/09 21:14:22, dsinclair wrote:
> On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > toRenderText will also check for isText but in release too so this checks
> seems
> > redundant.
> > 
> > Also when you have some logical ors in ASSERT, it's a sign that it should be
> > split up in several ASSERTs.
> 
> Done (removed the assert).
> 
> So, in general, is:
> 
> ASSERT(firstLetterRenderer)
> ASSERT(firstLetterRenderer->isText())
> 
> preferable to the &&'d version?

From my perspective yes because if ASSERT(firstLetterRenderer &&
firstLetterRenderer->isText()) fails you don't know which clause caused it and
have to run through a debugger to know.

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:65: PassRefPtr<StringImpl>
RenderTextFragment::completeText() const
On 2014/10/09 21:14:22, dsinclair wrote:
> On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > We now have completeText() and originalText(). The difference between the 2
is
> > pretty difficult to draw. Could we pick a naming that makes the distinction
> more
> > obvious?
> 
> Agreed.
> 
> What about changing completeText to originalText and originalText to
> fragmentText?
> 
> I think that's closer to what these are, current completeText gives you all
the
> text from the text node and originalText gives you the text from the node,
> possibly minus the first-letter (so the text originally set into
> RenderTextFragment).

I like completeText better than originalText and fragmentText is fine. The
comments you put here would be useful in the header.

[dsinclair, 2014-10-10 18:06:53]

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:132: // FIXME: This black-list of
disallowed RenderText subclasses is fragile.
On 2014/10/10 14:47:07, Julien Chaffraix - PST wrote:
> On 2014/10/09 21:14:22, dsinclair wrote:
> > On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > > Could we do a white list instead?
> > 
> > 
> > It _could_ be. But, it'd end up being like 50 entries long as I'd have to
add
> > kind of renderer to start with. How's about a bug attached to the FIXME?
> 
> 50 is too long indeed. Let's stick to a FIXME.


Done. crbug.com/422336

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/PseudoE...
File Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/PseudoE...
Source/core/dom/PseudoElement.h:76: default:
On 2014/10/10 14:47:07, Julien Chaffraix - PST wrote:
> On 2014/10/09 21:14:22, dsinclair wrote:
> > On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > > Let's avoid having a default case as it would allow the compiler to catch
> > adding
> > > more cases.
> > 
> > 
> > There are a lot of these we don't actually care about (there are 21 items in
> the
> > PseudoId enum ....) most of which aren't PseudoElements (only first-letter,
> > backdrop, before and after).
> 
> We looked at this with Dan and having a default with ASSERT_NOT_REACHED()
> doesn't work here so it's better to revert to simple if's.

Done.

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
File Source/core/dom/RenderTreeBuilder.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
Source/core/dom/RenderTreeBuilder.cpp:56: if (m_renderingParent->needsAttach())
On 2014/10/10 14:47:07, Julien Chaffraix - PST wrote:
> On 2014/10/09 21:14:22, dsinclair wrote:
> > On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > > You're checking m_renderingParent->needsAttach() for an element that is
> *not*
> > > the right parent and that's probably wrong. We could change the
constructor
> to
> > > point to the right parent and that would remove the code in
> parentRenderer().
> > 
> > 
> > Are you saying this is an existing issue, or due to the way
> > FirstLetterPseudoElement is built?
> 
> I was confused by the logic actually and we have the right parent. However the
> following logic seems unneeded and Dan explained to me that this is to disable
> creating the renderer.

Acknowledged.

https://codereview.chromium.org/571603003/diff/660001/Source/core/dom/RenderT...
Source/core/dom/RenderTreeBuilder.cpp:60: return
FirstLetterPseudoElement::firstLetterTextRenderer(*element);
On 2014/10/10 14:47:07, Julien Chaffraix - PST wrote:
> On 2014/10/09 21:14:22, dsinclair wrote:
> > On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > > If we keep the code in parentRenderer(), there is an inconsistency here as
> we
> > > check for top layer in this function but not in the other one.
> > 
> > 
> > In what cases is there a top layer? Is this something you think is specific
to
> > first-letter or an existing issue?
> 
> top layer is a DOM Element API so it won't happen on pseudo but I like to keep
> the consistency to avoid being bitten back later.


Code removed from parentRenderer().

https://codereview.chromium.org/571603003/diff/660001/Source/core/inspector/I...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/inspector/I...
Source/core/inspector/InspectorCSSAgent.cpp:808: ASSERT(firstLetterRenderer &&
firstLetterRenderer->isText());
On 2014/10/10 14:47:07, Julien Chaffraix - PST wrote:
> On 2014/10/09 21:14:22, dsinclair wrote:
> > On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > > toRenderText will also check for isText but in release too so this checks
> > seems
> > > redundant.
> > > 
> > > Also when you have some logical ors in ASSERT, it's a sign that it should
be
> > > split up in several ASSERTs.
> > 
> > Done (removed the assert).
> > 
> > So, in general, is:
> > 
> > ASSERT(firstLetterRenderer)
> > ASSERT(firstLetterRenderer->isText())
> > 
> > preferable to the &&'d version?
> 
> From my perspective yes because if ASSERT(firstLetterRenderer &&
> firstLetterRenderer->isText()) fails you don't know which clause caused it and
> have to run through a debugger to know.

Acknowledged.

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/660001/Source/core/rendering/R...
Source/core/rendering/RenderTextFragment.cpp:65: PassRefPtr<StringImpl>
RenderTextFragment::completeText() const
On 2014/10/10 14:47:07, Julien Chaffraix - PST wrote:
> On 2014/10/09 21:14:22, dsinclair wrote:
> > On 2014/10/09 18:24:48, Julien Chaffraix - PST wrote:
> > > We now have completeText() and originalText(). The difference between the
2
> is
> > > pretty difficult to draw. Could we pick a naming that makes the
distinction
> > more
> > > obvious?
> > 
> > Agreed.
> > 
> > What about changing completeText to originalText and originalText to
> > fragmentText?
> > 
> > I think that's closer to what these are, current completeText gives you all
> the
> > text from the text node and originalText gives you the text from the node,
> > possibly minus the first-letter (so the text originally set into
> > RenderTextFragment).
> 
> I like completeText better than originalText and fragmentText is fine. The
> comments you put here would be useful in the header.


Added comments into the header. Can't rename originalText as it's an override
from RenderText.

[Julien - ping for review, 2014-10-10 22:47:23]

I think the next iteration is going to be it. My big issue with this change is
the use of initialized booleans that may hide something.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:119: if
(firstLetterTextRenderer->style() &&
firstLetterTextRenderer->style()->styleType() == FIRST_LETTER) {
If firstLetterTextRenderer->style() is NULL, several of the following 'else'
conditions are moot as we never set the boolean fields: e.g. isText() or
isFloatingOrOutOfFlowPositioned() are set in updateFromStyle() which is called
when setting the style.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:127: // FIXME: Need to try down why
these conditions are needed.
Do we really want to land that?

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:130: 
Unneeded empty space.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:188: RenderStyle*
FirstLetterPseudoElement::styleForFirstLetter(RenderObject* rendererContainer)
I don't think this function could return nullptr or else we would ASSERT in
setStyle (that all callers call).

That means that the early returns should be removed or ASSERT'ed.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:251: nextRenderer = remainingText;
That doesn't give us anything (but nice paranoia).

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
File Source/core/dom/RenderTreeBuilder.h (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:82: RawPtrWillBeMember<RenderObject>
m_renderingParent;
I would have liked this change to be in a separate refactoring. I also wonder if
m_renderingParentRenderer would be better but that's orthogonal.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/editing/Te...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/editing/Te...
Source/core/editing/TextIterator.cpp:1525: ASSERT(pseudoElementRenderer &&
pseudoElementRenderer->slowFirstChild());
Let's split this ASSERT in 2 (cf my rationale earlier).

[esprehn, 2014-10-11 04:46:20]

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:2510: if (!remainingTextRenderer ||
remainingTextRenderer !=
toFirstLetterPseudoElement(element)->remainingTextRenderer()
This feels like it should be a separate private method like updateFirstLetter()

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:2541: // We have a first-letter pseudoElement, but
we no longer should have one.
This comment doesn't seem like it adds value.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:272: if (!child->isText() &&
!child->isQuote() && !child->isImage())
This is code duplication? We should probably fix this in a future patch.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
File Source/core/dom/FirstLetterPseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.h:45: void setNeedsUpdate()
Put this in the cpp file, it doesn't need to be inline.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.h:57: friend class PseudoElement; //
For constructor.
I'd rather you moved the if (pseudoId == FIRST_LETTER) into StyleResolver and
removed this friend.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
File Source/core/dom/RenderTreeBuilder.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.cpp:59: if (m_renderingParent->node() &&
m_renderingParent->node()->needsAttach())
This is not correct.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.cpp:88: if (!isSVGSVGElement(*m_node) &&
!(m_renderingParent->node() && m_renderingParent->node()->isSVGElement()))
propagate the ! through it, it's very hard to read these cases.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
File Source/core/dom/RenderTreeBuilder.h (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:62: if (nextRenderer &&
nextRenderer->parent())
This should not be inline, this is way too much code to have in the inline
constructor.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:63: m_renderingParent =
nextRenderer->parent();
m_renderingParent is nullptr either way, so you can just do

if (nextRenderer)
  m_renderingParent = nextRenderer->parent();

you don't need to null check it first.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:66: ContainerNode* containerNode =
NodeRenderingTraversal::parent(node, &m_parentDetails);
else if (ContainerNode* ...)

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:82: RawPtrWillBeMember<RenderObject>
m_renderingParent;
Hmm, this seems bad. The parent is always supposed to have a node(). I guess we
can try to iron this out later.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/rendering/...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/rendering/...
Source/core/rendering/RenderTextFragment.cpp:137: PseudoElement* pseudo =
toPseudoElement(node);
toFirstLetterPseudoElement()

https://codereview.chromium.org/571603003/diff/1060001/Source/core/rendering/...
Source/core/rendering/RenderTextFragment.cpp:138: RenderObject* nextRenderer =
FirstLetterPseudoElement::firstLetterTextRenderer(*pseudo);
This should just be a method on FirstLetterPseudoElement.

pseudo->textRenderer(); or something.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/rendering/...
Source/core/rendering/RenderTextFragment.cpp:143: return (node &&
node->isTextNode()) ? toText(node) : nullptr;
How can this ever not be a Text node? RenderTextFragment must be a Text, and the
firstLetterTextRenderer seems like it better return RenderText too, which I
think means it must be a Text node as well.

[esprehn, 2014-10-11 04:49:35]

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
File Source/core/dom/RenderTreeBuilder.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.cpp:59: if (m_renderingParent->node() &&
m_renderingParent->node()->needsAttach())
On 2014/10/11 at 04:46:20, esprehn wrote:
> This is not correct.

Sorry, this is correct, it just seems really gross to have to deal with non
node() m_renderingParent only for first letter. We can land this as-is and
figure it out later though. This patch is a huge improvement either way.

[esprehn, 2014-10-11 04:51:31]

lgtm, please fix my comments before landing. This is a huge improvement, lets
land it and see where it takes us.

[dsinclair, 2014-10-14 14:31:28]

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:2510: if (!remainingTextRenderer ||
remainingTextRenderer !=
toFirstLetterPseudoElement(element)->remainingTextRenderer()
On 2014/10/11 04:46:19, esprehn wrote:
> This feels like it should be a separate private method like
updateFirstLetter()

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:2541: // We have a first-letter pseudoElement, but
we no longer should have one.
On 2014/10/11 04:46:19, esprehn wrote:
> This comment doesn't seem like it adds value.

Done. (Assuming you meant just the first line.) I kept the rest as I think it
helps clarify what the code is doing and points out the relevant test to see why
it's broken.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:119: if
(firstLetterTextRenderer->style() &&
firstLetterTextRenderer->style()->styleType() == FIRST_LETTER) {
On 2014/10/10 22:47:23, Julien Chaffraix - PST wrote:
> If firstLetterTextRenderer->style() is NULL, several of the following 'else'
> conditions are moot as we never set the boolean fields: e.g. isText() or
> isFloatingOrOutOfFlowPositioned() are set in updateFromStyle() which is called
> when setting the style.

I'd prefer to do this in a followup so I can go through each of the conditions
to see if they can be skipped or not.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:127: // FIXME: Need to try down why
these conditions are needed.
On 2014/10/10 22:47:23, Julien Chaffraix - PST wrote:
> Do we really want to land that?

Fixed the spelling, I think we want to keep it so we can track down why each of
these things is here.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:130: 
On 2014/10/10 22:47:23, Julien Chaffraix - PST wrote:
> Unneeded empty space.

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:188: RenderStyle*
FirstLetterPseudoElement::styleForFirstLetter(RenderObject* rendererContainer)
On 2014/10/10 22:47:23, Julien Chaffraix - PST wrote:
> I don't think this function could return nullptr or else we would ASSERT in
> setStyle (that all callers call).
> 
> That means that the early returns should be removed or ASSERT'ed.

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:251: nextRenderer = remainingText;
On 2014/10/10 22:47:23, Julien Chaffraix - PST wrote:
> That doesn't give us anything (but nice paranoia).

Done.

Left over from nextRenderer being m_nextRenderer.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.cpp:272: if (!child->isText() &&
!child->isQuote() && !child->isImage())
On 2014/10/11 04:46:19, esprehn wrote:
> This is code duplication? We should probably fix this in a future patch.

Acknowledged.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
File Source/core/dom/FirstLetterPseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.h:45: void setNeedsUpdate()
On 2014/10/11 04:46:19, esprehn wrote:
> Put this in the cpp file, it doesn't need to be inline.

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.h:57: friend class PseudoElement; //
For constructor.
On 2014/10/11 04:46:20, esprehn wrote:
> I'd rather you moved the if (pseudoId == FIRST_LETTER) into StyleResolver and
> removed this friend.

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
File Source/core/dom/RenderTreeBuilder.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.cpp:59: if (m_renderingParent->node() &&
m_renderingParent->node()->needsAttach())
On 2014/10/11 04:49:34, esprehn wrote:
> On 2014/10/11 at 04:46:20, esprehn wrote:
> > This is not correct.
> 
> Sorry, this is correct, it just seems really gross to have to deal with non
> node() m_renderingParent only for first letter. We can land this as-is and
> figure it out later though. This patch is a huge improvement either way.

Acknowledged.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.cpp:88: if (!isSVGSVGElement(*m_node) &&
!(m_renderingParent->node() && m_renderingParent->node()->isSVGElement()))
On 2014/10/11 04:46:20, esprehn wrote:
> propagate the ! through it, it's very hard to read these cases.

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
File Source/core/dom/RenderTreeBuilder.h (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:62: if (nextRenderer &&
nextRenderer->parent())
On 2014/10/11 04:46:20, esprehn wrote:
> This should not be inline, this is way too much code to have in the inline
> constructor.

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:63: m_renderingParent =
nextRenderer->parent();
On 2014/10/11 04:46:20, esprehn wrote:
> m_renderingParent is nullptr either way, so you can just do
> 
> if (nextRenderer)
>   m_renderingParent = nextRenderer->parent();
> 
> you don't need to null check it first.

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:66: ContainerNode* containerNode =
NodeRenderingTraversal::parent(node, &m_parentDetails);
On 2014/10/11 04:46:20, esprehn wrote:
> else if (ContainerNode* ...)

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:82: RawPtrWillBeMember<RenderObject>
m_renderingParent;
On 2014/10/11 04:46:20, esprehn wrote:
> Hmm, this seems bad. The parent is always supposed to have a node(). I guess
we
> can try to iron this out later.


So, the problem I have specifically, is if you have a :before and a
:first-letter, the parent renderer of the first element is an anonymous renderer
generated by the :before. So, that anonymous renderer doesn't have a node but we
need that renderer to attach the first letter renderer too.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/dom/Render...
Source/core/dom/RenderTreeBuilder.h:82: RawPtrWillBeMember<RenderObject>
m_renderingParent;
On 2014/10/10 22:47:23, Julien Chaffraix - PST wrote:
> I would have liked this change to be in a separate refactoring. I also wonder
if
> m_renderingParentRenderer would be better but that's orthogonal.

Acknowledged.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/editing/Te...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/editing/Te...
Source/core/editing/TextIterator.cpp:1525: ASSERT(pseudoElementRenderer &&
pseudoElementRenderer->slowFirstChild());
On 2014/10/10 22:47:23, Julien Chaffraix - PST wrote:
> Let's split this ASSERT in 2 (cf my rationale earlier).

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/rendering/...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/571603003/diff/1060001/Source/core/rendering/...
Source/core/rendering/RenderTextFragment.cpp:137: PseudoElement* pseudo =
toPseudoElement(node);
On 2014/10/11 04:46:20, esprehn wrote:
> toFirstLetterPseudoElement()

Done.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/rendering/...
Source/core/rendering/RenderTextFragment.cpp:138: RenderObject* nextRenderer =
FirstLetterPseudoElement::firstLetterTextRenderer(*pseudo);
On 2014/10/11 04:46:20, esprehn wrote:
> This should just be a method on FirstLetterPseudoElement.
> 
> pseudo->textRenderer(); or something.


We also call this when we don't have a FirstLetterPseudoElement yet, so it has
to be static. I can make a dummy method on first letter that just calls through
to the static, but the static will need to exist anyway.

https://codereview.chromium.org/571603003/diff/1060001/Source/core/rendering/...
Source/core/rendering/RenderTextFragment.cpp:143: return (node &&
node->isTextNode()) ? toText(node) : nullptr;
On 2014/10/11 04:46:20, esprehn wrote:
> How can this ever not be a Text node? RenderTextFragment must be a Text, and
the
> firstLetterTextRenderer seems like it better return RenderText too, which I
> think means it must be a Text node as well.

This was more to be safe then anything else. It is possible to have a RenderText
that doesn't point to a text node. The first letter renderer for example points
to the FirstLetterPseudoElement instead of the text node (this is because, we'd
end up with two renderers pointed at the same node which causes funkyness when
the renderer or node get destroyed).

[esprehn, 2014-10-14 14:38:55]

On 2014/10/14 at 14:31:28, dsinclair wrote:
> ...
> 
> This was more to be safe then anything else. It is possible to have a
RenderText that doesn't point to a text node. The first letter renderer for
example points to the FirstLetterPseudoElement instead of the text node (this is
because, we'd end up with two renderers pointed at the same node which causes
funkyness when the renderer or node get destroyed).

Hmm, that's not right then. A RenderText should never point to an Element, only
a Text or be anonymous. The RenderTextFragment for the first letter should be a
child of the FirstLetterPseudoElement. That makes sense too, since you can float
it or add margins and padding. :first-letter is a Box, not a Text.

RenderText should be asserting if you try to give it an Element, please don't
land this with that.

[Julien - ping for review, 2014-10-17 16:49:59]

lgtm

https://codereview.chromium.org/571603003/diff/1550001/Source/core/dom/FirstL...
File Source/core/dom/FirstLetterPseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/1550001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.h:48: bool needsUpdate() const {return
m_needsUpdate; }
Style: space after opening brace.

https://codereview.chromium.org/571603003/diff/1550001/Source/core/rendering/...
File Source/core/rendering/RenderText.cpp (left):

https://codereview.chromium.org/571603003/diff/1550001/Source/core/rendering/...
Source/core/rendering/RenderText.cpp:1413: 
Unrelated change.

[dsinclair, 2014-10-17 17:21:39]

https://codereview.chromium.org/571603003/diff/1550001/Source/core/dom/FirstL...
File Source/core/dom/FirstLetterPseudoElement.h (right):

https://codereview.chromium.org/571603003/diff/1550001/Source/core/dom/FirstL...
Source/core/dom/FirstLetterPseudoElement.h:48: bool needsUpdate() const {return
m_needsUpdate; }
On 2014/10/17 16:49:59, Julien Chaffraix - PST wrote:
> Style: space after opening brace.

Done.

https://codereview.chromium.org/571603003/diff/1550001/Source/core/rendering/...
File Source/core/rendering/RenderText.cpp (left):

https://codereview.chromium.org/571603003/diff/1550001/Source/core/rendering/...
Source/core/rendering/RenderText.cpp:1413: 
On 2014/10/17 16:49:59, Julien Chaffraix - PST wrote:
> Unrelated change.

Done.

[dsinclair, 2014-10-17 17:21:57]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2014-10-17 17:23:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/571603003/1570001

[commit-bot: I haz the power, 2014-10-17 18:34:53]

Committed patchset #49 (id:1570001) as 183913

[dsinclair, 2014-10-21 16:49:47]

A revert of this CL (patchset #49 id:1570001) has been created in
https://codereview.chromium.org/639863006/ by dsinclair@chromium.org.

The reason for reverting is: Causing clusterfuzz regressions..