Update the first letter when the first line is changed by adding a new text at its start.

Update the first letter when the first line is changed by adding a new text at its start.

Currently, CSS first-letter property does not work properly when the first letter is changed 
by DOM scripting. This patch allows us to update the first letter and remaining text when the 
first letter is changed.

BUG=92797
TEST=fast/css/first-letter-block-change.html

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

[joone, 2013-04-26 21:44:39]

Hello, I'd like you to review my patch.

[abarth-chromium, 2013-04-26 22:43:55]

I'm not the right reviewer for this CL.  Ojan or esprehn can probably point you
towards the right reviewer (or could review it themselves).

[eseidel, 2013-04-26 23:28:06]

Elliot is your man.

[joone, 2013-05-04 00:02:50]

Hi Elliot,
Could you take a look at my patch?

[Elliot Glaysher, 2013-05-06 18:25:27]

Eric, I suspect you have the wrong Elliot.

-erg
+esprehn

[joone, 2013-05-06 20:07:55]

On 2013/05/06 18:25:27, Elliot Glaysher wrote:
> Eric, I suspect you have the wrong Elliot.
> 
> -erg
> +esprehn

Sorry, I will ask Elliot Sprehn for a review.

[joone, 2013-05-06 20:09:38]

Hi Elliot,
Could you take a look at my patch?

[esprehn, 2013-05-06 20:43:58]

This looks unsafe, it'll crash for counters or quotes and I don't think it's
safe to do parent()->nextSibling()->nextSibling()

https://codereview.chromium.org/14113040/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/14113040/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:6624: static inline unsigned
whiteSpaceLength(PassRefPtr<StringImpl> oldText)
Just take a String not a RefPtr<StringImpl>

https://codereview.chromium.org/14113040/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:6666: RefPtr<StringImpl> oldText =
textObj->originalText();
Change to String

https://codereview.chromium.org/14113040/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:6746: RefPtr<StringImpl> updatedTextImp =
remainingText->originalText();
Just use String. A String is just a RefPtr to a StringImpl. There's no reason to
ever create your own RefPtr<StringImpl>

https://codereview.chromium.org/14113040/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:6747: ASSERT(updatedTextImp);
This assert should be inside whiteSpaceLength

https://codereview.chromium.org/14113040/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:6749: if (updatedTextImp &&
updatedTextImp->length() > 0)
This check should be inside whiteSpaceLength() and you want String isEmpty() not
an explicit length() check. Also checking if updatedTextImpl is null doesn't
make sense, you assert right above this.

unsigned length = whiteSpaceLength(updatedText)

https://codereview.chromium.org/14113040/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:6755: RenderObject* oldRemainingText =
currChild->parent()->nextSibling()->nextSibling();
This doesn't look safe at all. How do you know your parent has two siblings?

https://codereview.chromium.org/14113040/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:6759: RefPtr<StringImpl> oldString =
toText(oldRemainingText->node())->dataImpl();
This is going to crash if it's an anonymous text renderer like that created from
a RenderQuote or a RenderCounter.

[joone, 2013-09-06 17:51:53]

The updated patch allows to check if the existing first-letter is no longer the
first-letter. In this case, it deletes the old first-letter object and creates a
new one.

For the remaining text, I wanted to replace the
oldRemainingText(RenderTextFragment) object with a new RenderText, but it causes
a problem to add the new RenderText as a child of the firstLetterBlock object.

Instead, the oldRemainingText object is used again for containing the full
text(first letter + remaining text). It seems more efficient because we don't
need to create a new RenderText and delete the old RenderTextFragment.

I'd appreciate any feedback you may have.

[eseidel, 2013-09-09 20:20:34]

Wow.  We totally dropped the ball here.  I recommend tracking down esprehn in
#blink for more real-time feedback.

[esprehn, 2013-09-09 21:22:57]

Can you explain this patch further to me? What destroys the original
RenderTextFragment and why is it safe to go parent()->nextSibling()?

https://codereview.chromium.org/14113040/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/14113040/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:6760: RenderObject* remainingText =
currChild->parent()->nextSibling();
How do you know the currentCuild->parent() has a nextSibling and that it's not
wrapped in an anonymous thing?

https://codereview.chromium.org/14113040/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:6768:
createFirstLetterRenderer(firstLetterBlock, remainingText);
What destroyed the original RenderTextFragment that was there?

[joone, 2013-09-09 22:36:25]

https://codereview.chromium.org/14113040/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/14113040/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:6760: RenderObject* remainingText =
currChild->parent()->nextSibling();
On 2013/09/09 21:22:57, esprehn wrote:
> How do you know the currentCuild->parent() has a nextSibling and that it's not
> wrapped in an anonymous thing?

The currentChild is the firstLetter(RenderTextFragment) so its
parent(RenderInLine)'s next sibling should be the
remainingText(RenderTextFragment).The RenderInLine has the firstLetter object.
You can understand this from the createFirstLetterRenderer method.

https://codereview.chromium.org/14113040/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:6768:
createFirstLetterRenderer(firstLetterBlock, remainingText);
On 2013/09/09 21:22:57, esprehn wrote:
> What destroyed the original RenderTextFragment that was there?

When setting a new text to RenderTextFragment(RemainingText), the firstLetter's
parent(RenderInLine) is destroyed. You can see the code in
RenderTextFragment::setText

[esprehn, 2013-09-09 22:40:54]

Okay, this seems like it might be okay, but you definitely need to add a test.
Saying "follow the bug description" isn't enough since someone will just break
this code later if you don't add a LayoutTest.

[joone, 2013-09-09 22:56:17]

On 2013/09/09 22:40:54, esprehn wrote:
> Okay, this seems like it might be okay, but you definitely need to add a test.
> Saying "follow the bug description" isn't enough since someone will just break
> this code later if you don't add a LayoutTest.

Okay, thank you for the review. I will add a test case.

[joone, 2013-09-17 22:38:24]

Could you review the test case I added?

[esprehn, 2013-09-20 22:05:04]

LGTM

[commit-bot: I haz the power, 2013-09-20 22:11:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joone.hur@intel.com/14113040/18001

[commit-bot: I haz the power, 2013-09-21 00:06:26]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[esprehn, 2013-09-27 18:30:14]

lgtm, Please fix the test before landing.

https://codereview.chromium.org/14113040/diff/38001/LayoutTests/fast/css/firs...
File LayoutTests/fast/css/first-letter-block-change.html (right):

https://codereview.chromium.org/14113040/diff/38001/LayoutTests/fast/css/firs...
LayoutTests/fast/css/first-letter-block-change.html:12: setTimeout(function()
{addTextNode()}, 0);
This doesn't need to use setTimeout, just do:

onload = function() {
  document.body.offsetTop;
  addTextNode();
}

and then the test doesn't need to be async either.

https://codereview.chromium.org/14113040/diff/38001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/14113040/diff/38001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:5854: if (oldRemainingText)
We usually write these as one statement:

if (RenderObject* oldRemainingText =
toRenderBoxModelObject(currChild->parent())->firstLetterRemainingText())
 
toRenderText(oldRemainingText)->setText(toText(oldRemainingText->node())->data().impl());

but it's not a big deal.

[commit-bot: I haz the power, 2013-09-27 21:20:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joone.hur@intel.com/14113040/47001

[commit-bot: I haz the power, 2013-09-28 01:09:29]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-09-28 15:08:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joone.hur@intel.com/14113040/60001

[commit-bot: I haz the power, 2013-09-28 16:12:43]

Change committed as 158496