Bug 63987 – Bridge RenderBoxModelObject::calculateBackgroundImageGeometry parameters into a class

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexandru Chiculita]

Makes it easier to add new parameters for RenderBoxModelObject::calculateBackgroundImageGeometry.

[Alexandru Chiculita]

Created attachment 99820 [details]
Patch

Moved the 3 reference arguments into a class called BackgroundImageGeometry.

[WebKit Review Bot]

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]

Created attachment 99821 [details]
Patch

Fixed code style.

[Hajime Morrita]

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]

Created attachment 100464 [details]
Patch

[Tony Chang]

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]

Created attachment 100646 [details]
Patch - fixes for Comment #6

Thanks for review! I've updated the patch.

[Hajime Morrita]

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]

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 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]

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]

Created attachment 100664 [details]
Patch

Fixed layout tests.

[Hajime Morrita]

Comment on attachment 100664 [details]
Patch

Looks fine assuming the build passes. Let's land and see how it goes.

[WebKit Review Bot]

Comment on attachment 100664 [details]
Patch

Clearing flags on attachment: 100664

Committed r90912: <http://trac.webkit.org/changeset/90912>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.