Bug 63987 - Bridge RenderBoxModelObject::calculateBackgroundImageGeometry parameters into a class
Summary: Bridge RenderBoxModelObject::calculateBackgroundImageGeometry parameters into...
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
Depends on:
Blocks: 27570
  Show dependency treegraph
Reported: 2011-07-06 05:48 PDT by Alexandru Chiculita
Modified: 2011-07-13 06:45 PDT (History)
6 users (show)

See Also:

Patch (9.44 KB, patch)
2011-07-06 05:50 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch (9.43 KB, patch)
2011-07-06 05:54 PDT, Alexandru Chiculita
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
Patch (10.83 KB, patch)
2011-07-12 04:08 PDT, Alexandru Chiculita
tony: review-
tony: commit-queue-
Details | Formatted Diff | Diff
Patch - fixes for Comment #6 (11.14 KB, patch)
2011-07-13 02:29 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch (12.06 KB, patch)
2011-07-13 05:00 PDT, Alexandru Chiculita
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2011-07-06 05:48:14 PDT
Makes it easier to add new parameters for RenderBoxModelObject::calculateBackgroundImageGeometry.
Comment 1 Alexandru Chiculita 2011-07-06 05:50:43 PDT
Created attachment 99820 [details]

Moved the 3 reference arguments into a class called BackgroundImageGeometry.
Comment 2 WebKit Review Bot 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.
Comment 3 Alexandru Chiculita 2011-07-06 05:54:52 PDT
Created attachment 99821 [details]

Fixed code style.
Comment 4 Hajime Morrita 2011-07-06 19:12:40 PDT
Comment on attachment 99821 [details]

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.
Comment 5 Alexandru Chiculita 2011-07-12 04:08:55 PDT
Created attachment 100464 [details]
Comment 6 Tony Chang 2011-07-12 10:26:23 PDT
Comment on attachment 100464 [details]

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.
Comment 7 Alexandru Chiculita 2011-07-13 02:29:30 PDT
Created attachment 100646 [details]
Patch - fixes for Comment #6

Thanks for review! I've updated the patch.
Comment 8 Hajime Morrita 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.
Comment 9 Alexandru Chiculita 2011-07-13 05:00:10 PDT
Created attachment 100658 [details]

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 10 WebKit Review Bot 2011-07-13 05:25:02 PDT
Comment on attachment 100658 [details]

Attachment 100658 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9019581

New failing tests:
Comment 11 WebKit Review Bot 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
Comment 12 Alexandru Chiculita 2011-07-13 05:45:39 PDT
Created attachment 100664 [details]

Fixed layout tests.
Comment 13 Hajime Morrita 2011-07-13 05:47:49 PDT
Comment on attachment 100664 [details]

Looks fine assuming the build passes. Let's land and see how it goes.
Comment 14 WebKit Review Bot 2011-07-13 06:45:37 PDT
Comment on attachment 100664 [details]

Clearing flags on attachment: 100664

Committed r90912: <http://trac.webkit.org/changeset/90912>
Comment 15 WebKit Review Bot 2011-07-13 06:45:51 PDT
All reviewed patches have been landed.  Closing bug.