Bug 79630 - [Refactoring] Extract RenderBlock::updateFirstLetter() to FirstLetterUpdater class.
Summary: [Refactoring] Extract RenderBlock::updateFirstLetter() to FirstLetterUpdater ...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-26 23:12 PST by Hajime Morrita
Modified: 2012-03-12 18:10 PDT (History)
8 users (show)

See Also:


Attachments
WIP (35.85 KB, patch)
2012-02-27 00:06 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (40.39 KB, patch)
2012-02-27 00:47 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for for 72440, mistakenly attached here (6.39 KB, patch)
2012-02-27 02:02 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-02-26 23:12:44 PST
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.
Comment 1 Eric Seidel (no email) 2012-02-26 23:30:22 PST
OK.
Comment 2 Hajime Morrita 2012-02-27 00:06:24 PST
Created attachment 128974 [details]
WIP
Comment 3 Hajime Morrita 2012-02-27 00:47:55 PST
Created attachment 128978 [details]
Patch
Comment 4 Hajime Morrita 2012-02-27 02:02:11 PST
Created attachment 128988 [details]
Patch for for 72440, mistakenly attached here
Comment 5 WebKit Review Bot 2012-02-27 03:11:38 PST
The commit-queue encountered the following flaky tests while processing attachment 128988 [details]:

inspector/protocol/console-agent.html bug 79563 (authors: caseq@chromium.org, loislo@chromium.org, and pfeldman@chromium.org)
The commit-queue is continuing to process your patch.
Comment 6 WebKit Review Bot 2012-02-27 03:14:01 PST
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>
Comment 7 WebKit Review Bot 2012-02-27 03:14:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Julien Chaffraix 2012-02-27 07:35:17 PST
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).
Comment 9 Alexey Proskuryakov 2012-02-27 10:43:26 PST
Why is this an improvement? "Updater" is not even a word that describes an entity.
Comment 10 Hajime Morrita 2012-02-27 16:14:59 PST
(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.
Comment 11 Igor Trindade Oliveira 2012-03-12 10:38:00 PDT
(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
Comment 12 Hajime Morrita 2012-03-12 18:10:51 PDT
(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.