Bug 79630

Summary: [Refactoring] Extract RenderBlock::updateFirstLetter() to FirstLetterUpdater class.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: Layout and RenderingAssignee: Hajime Morrita <morrita>
Status: REOPENED    
Severity: Normal CC: ap, eric, hyatt, igor.oliveira, jchaffraix, mitz, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Patch for for 72440, mistakenly attached here none

Hajime Morrita
Reported 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.
Attachments
WIP (35.85 KB, patch)
2012-02-27 00:06 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Patch (40.39 KB, patch)
2012-02-27 00:47 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Patch for for 72440, mistakenly attached here (6.39 KB, patch)
2012-02-27 02:02 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2012-02-26 23:30:22 PST
OK.
Hajime Morrita
Comment 2 2012-02-27 00:06:24 PST
Created attachment 128974 [details] WIP
Hajime Morrita
Comment 3 2012-02-27 00:47:55 PST
Created attachment 128978 [details] Patch
Hajime Morrita
Comment 4 2012-02-27 02:02:11 PST
Created attachment 128988 [details] Patch for for 72440, mistakenly attached here
WebKit Review Bot
Comment 5 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.
WebKit Review Bot
Comment 6 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>
WebKit Review Bot
Comment 7 2012-02-27 03:14:06 PST
All reviewed patches have been landed. Closing bug.
Julien Chaffraix
Comment 8 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).
Alexey Proskuryakov
Comment 9 2012-02-27 10:43:26 PST
Why is this an improvement? "Updater" is not even a word that describes an entity.
Hajime Morrita
Comment 10 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.
Igor Trindade Oliveira
Comment 11 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
Hajime Morrita
Comment 12 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.
Note You need to log in before you can comment on or make changes to this bug.