Bug 79630 – [Refactoring] Extract RenderBlock::updateFirstLetter() to FirstLetterUpdater class.

• Status: REOPENED
• Component: Layout and Rendering

[Hajime Morrita]

There are several  static function which is only used in a big function called updateFirstLetter().
That can be taken as a signal of should-be-a-class.

[Eric Seidel (no email)]

OK.

[Hajime Morrita]

Created attachment 128974 [details]
WIP

[Hajime Morrita]

Created attachment 128978 [details]
Patch

[Hajime Morrita]

Created attachment 128988 [details]
Patch for for 72440, mistakenly attached here

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 128988 [details]:

inspector/protocol/console-agent.html bug 79563 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 128988 [details]
Patch for for 72440, mistakenly attached here

Clearing flags on attachment: 128988

Committed r108980: <http://trac.webkit.org/changeset/108980>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

Reopening, the patch for bug 72440 was attached for landing (with the wrong bug number in the ChangeLog which is why it ended up here).

[Alexey Proskuryakov]

Why is this an improvement? "Updater" is not even a word that describes an entity.

[Hajime Morrita]

(In reply to comment #9)
> Why is this an improvement? "Updater" is not even a word that describes an entity.

This is an effort to split this giant class into smaller pieces. 
And updateFirstLetter() looks a good candidate for such split-out because 

- it has associated static functions which is used only from updateFirstLetter() and
- it method has such many lines.

But you're right. Updater doesn't look a good name.
I'll come back once the big picture of this kind of refactoring becomes clearer.

[Igor Trindade Oliveira]

(In reply to comment #10)
> (In reply to comment #9)
> > Why is this an improvement? "Updater" is not even a word that describes an entity.
> 
> This is an effort to split this giant class into smaller pieces. 
> And updateFirstLetter() looks a good candidate for such split-out because 
> 
> - it has associated static functions which is used only from updateFirstLetter() and
> - it method has such many lines.
> 
> But you're right. Updater doesn't look a good name.
> I'll come back once the big picture of this kind of refactoring becomes clearer.

Hajime,
I have been working in a not so intrusive changes for updateFirstLetter:

https://bugs.webkit.org/show_bug.cgi?id=80772

[Hajime Morrita]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Why is this an improvement? "Updater" is not even a word that describes an entity.
> > 
> > This is an effort to split this giant class into smaller pieces. 
> > And updateFirstLetter() looks a good candidate for such split-out because 
> > 
> > - it has associated static functions which is used only from updateFirstLetter() and
> > - it method has such many lines.
> > 
> > But you're right. Updater doesn't look a good name.
> > I'll come back once the big picture of this kind of refactoring becomes clearer.
> 
> Hajime,
> I have been working in a not so intrusive changes for updateFirstLetter:
> 
> https://bugs.webkit.org/show_bug.cgi?id=80772
Igor, thanks for letting me know  this! It looks good.