Bug 40400 - Refactoring: Simple shadow elements should be factored out .
Summary: Refactoring: Simple shadow elements should be factored out .
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-09 21:50 PDT by Hajime Morrita
Modified: 2010-06-10 03:35 PDT (History)
1 user (show)

See Also:


Attachments
patch v0 (20.49 KB, patch)
2010-06-09 23:50 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1; fixed style violation (20.53 KB, patch)
2010-06-09 23:55 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v2; sorted file order on project file (40.41 KB, patch)
2010-06-10 00:25 PDT, Hajime Morrita
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-06-09 21:50:41 PDT
There are several simple shadow element classes defined under rendering/*.cpp.
And some of them (ProgressValueElement, ShadowInputElement, SliderThumbElement) are 
pretty simple, and have similar definitions.
I'm going to another one for Bug 40280, so we should factored out the definition from them,
instead of making another copy.
Comment 1 Hajime Morrita 2010-06-09 23:50:59 PDT
Created attachment 58334 [details]
patch v0
Comment 2 WebKit Review Bot 2010-06-09 23:53:35 PDT
Attachment 58334 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/RenderSlider.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/ShadowElement.cpp:22:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/rendering/ShadowElement.h:66:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Hajime Morrita 2010-06-09 23:55:20 PDT
Created attachment 58335 [details]
patch v1; fixed style violation
Comment 4 Kent Tamura 2010-06-10 00:05:36 PDT
Comment on attachment 58335 [details]
patch v1; fixed style violation

This is a good refactoring.

Please check if we can share code with rendering/TextControllInnerElements.{cpp,h} too.


WebCore/WebCore.xcodeproj/project.pbxproj:21765
 +  				A7E5D1D111C0AE25000A338E /* ShadowElement.cpp in Sources */,
We should sort lists in project.pbxproj.
As you saw, the current pbxproj is not sorted correctly.  So you might want to run WebKitTools/Scripts/sort-Xcode-project-file before this change.
Comment 5 Hajime Morrita 2010-06-10 00:25:24 PDT
Created attachment 58339 [details]
patch v2; sorted file order on project file
Comment 6 Hajime Morrita 2010-06-10 00:30:38 PDT
Hi Kent-san, thank you for reviewing!

> Please check if we can share code with rendering/TextControllInnerElements.{cpp,h} too.
Their lifecycle model is different a bit;
i.e. are created without parent node, then attached the Node after some operation.
Although it might be possible to adopt ShadowElement to them,
I would be better to keep as is for ShadowElement simplicity.

> 
> WebCore/WebCore.xcodeproj/project.pbxproj:21765
>  +                  A7E5D1D111C0AE25000A338E /* ShadowElement.cpp in Sources */,
> We should sort lists in project.pbxproj.
Sorted out with the script.
Comment 7 Kent Tamura 2010-06-10 00:42:45 PDT
Comment on attachment 58339 [details]
patch v2; sorted file order on project file

OK.
Please make sure all of layout tests pass on Mac before committing.
Comment 8 Hajime Morrita 2010-06-10 03:35:44 PDT
Committed r60951: <http://trac.webkit.org/changeset/60951>