Bug 63987

Summary: Bridge RenderBoxModelObject::calculateBackgroundImageGeometry parameters into a class
Product: WebKit Reporter: Alexandru Chiculita <achicu>
Component: CSSAssignee: 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 Flags
Patch
none
Patch
morrita: review-, morrita: commit-queue-
Patch
tony: review-, tony: commit-queue-
Patch - fixes for Comment #6
none
Patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
Patch none

Alexandru Chiculita
Reported 2011-07-06 05:48:14 PDT
Makes it easier to add new parameters for RenderBoxModelObject::calculateBackgroundImageGeometry.
Attachments
Patch (9.44 KB, patch)
2011-07-06 05:50 PDT, Alexandru Chiculita 		no flags
DetailsFormatted DiffDiff
Patch (9.43 KB, patch)
2011-07-06 05:54 PDT, Alexandru Chiculita 		morrita: review-
morrita: commit-queue-
DetailsFormatted DiffDiff
Patch (10.83 KB, patch)
2011-07-12 04:08 PDT, Alexandru Chiculita 		tony: review-
tony: commit-queue-
DetailsFormatted DiffDiff
Patch - fixes for Comment #6 (11.14 KB, patch)
2011-07-13 02:29 PDT, Alexandru Chiculita 		no flags
DetailsFormatted DiffDiff
Patch (12.06 KB, patch)
2011-07-13 05:00 PDT, Alexandru Chiculita 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ec2-cr-linux-03 (5.97 MB, application/zip)
2011-07-13 05:25 PDT, WebKit Review Bot 		no flags
Details
Patch (12.19 KB, patch)
2011-07-13 05:45 PDT, Alexandru Chiculita 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Alexandru Chiculita
Comment 1 2011-07-06 05:50:43 PDT
Created attachment 99820 [details] Patch Moved the 3 reference arguments into a class called BackgroundImageGeometry.
WebKit Review Bot
Comment 2 2011-07-06 05:53:11 PDT
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.
Alexandru Chiculita
Comment 3 2011-07-06 05:54:52 PDT
Created attachment 99821 [details] Patch Fixed code style.
Hajime Morrita
Comment 4 2011-07-06 19:12:40 PDT
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.
Alexandru Chiculita
Comment 5 2011-07-12 04:08:55 PDT
Created attachment 100464 [details] Patch
Tony Chang
Comment 6 2011-07-12 10:26:23 PDT
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.
Alexandru Chiculita
Comment 7 2011-07-13 02:29:30 PDT
Created attachment 100646 [details] Patch - fixes for Comment #6 Thanks for review! I've updated the patch.
Hajime Morrita
Comment 8 2011-07-13 03:47:56 PDT
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.
Alexandru Chiculita
Comment 9 2011-07-13 05:00:10 PDT
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.
WebKit Review Bot
Comment 10 2011-07-13 05:25:02 PDT
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
WebKit Review Bot
Comment 11 2011-07-13 05:25:08 PDT
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
Alexandru Chiculita
Comment 12 2011-07-13 05:45:39 PDT
Created attachment 100664 [details] Patch Fixed layout tests.
Hajime Morrita
Comment 13 2011-07-13 05:47:49 PDT
Comment on attachment 100664 [details] Patch Looks fine assuming the build passes. Let's land and see how it goes.
WebKit Review Bot
Comment 14 2011-07-13 06:45:37 PDT
Comment on attachment 100664 [details] Patch Clearing flags on attachment: 100664 Committed r90912: <http://trac.webkit.org/changeset/90912>
WebKit Review Bot
Comment 15 2011-07-13 06:45:51 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.