Summary: | Bridge RenderBoxModelObject::calculateBackgroundImageGeometry parameters into a class | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||||||||||||
Component: | CSS | Assignee: | Alexandru Chiculita <achicu> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | aboxhall, dglazkov, eric, morrita, simon.fraser, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 27570 | ||||||||||||||||||
Attachments: |
|
Description
Alexandru Chiculita
2011-07-06 05:48:14 PDT
Created attachment 99820 [details]
Patch
Moved the 3 reference arguments into a class called BackgroundImageGeometry.
Attachment 99820 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderBoxModelObject.h:144: The parameter name "geometry" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 99821 [details]
Patch
Fixed code style.
Comment on attachment 99821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99821&action=review > Source/WebCore/rendering/RenderBoxModelObject.h:44 > +public: Could you make fields private and add accessors for them? Few new classes have public member variables. Created attachment 100464 [details]
Patch
Comment on attachment 100464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100464&action=review > Source/WebCore/rendering/RenderBoxModelObject.h:43 > +class BackgroundImageGeometry { Since this class is only used by RenderBoxModelObject and RenderBox, maybe it should be declared within the protected section of RenderBoxModelObject? > Source/WebCore/rendering/RenderBoxModelObject.h:75 > + IntRect m_destRect; > + IntPoint m_phase; > + IntSize m_tileSize; These should be LayoutRect, LayoutPoint and LayoutSize. The getters/setters should also be updated. Created attachment 100646 [details] Patch - fixes for Comment #6 Thanks for review! I've updated the patch. Comment on attachment 100646 [details] Patch - fixes for Comment #6 View in context: https://bugs.webkit.org/attachment.cgi?id=100646&action=review Hi Alex, Thanks for the update. It looks almost fine! I added some comments for a small point. > Source/WebCore/rendering/RenderBoxModelObject.cpp:777 > RefPtr<Image> image = bgImage->image(clientForBackgroundImage, tileSize); Is it possible to eliminate redundant local variables like destOrigin, destRect, phase, tileSize? - For "phase += destRect.location() - destOrigin", we can just provide an method on BackgroundImageGeometry to get the same value. - For destOrigin, we can also have a shorthand accessor for that. Created attachment 100658 [details]
Patch
Removed locals. Also dest origin was saved before the clipping was done, so now it also maintains the dest origin in the BackgroundImageGeometry class.
Comment on attachment 100658 [details] Patch Attachment 100658 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9019581 New failing tests: http/tests/misc/acid2-pixel.html tables/mozilla/bugs/bug12008.html fast/css/acid2-pixel.html Created attachment 100663 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 100664 [details]
Patch
Fixed layout tests.
Comment on attachment 100664 [details]
Patch
Looks fine assuming the build passes. Let's land and see how it goes.
Comment on attachment 100664 [details] Patch Clearing flags on attachment: 100664 Committed r90912: <http://trac.webkit.org/changeset/90912> All reviewed patches have been landed. Closing bug. |