Bug 93747 - Reduce the size of empty NinePieceImage objects.
Summary: Reduce the size of empty NinePieceImage objects.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-10 15:25 PDT by Andreas Kling
Modified: 2012-08-13 15:45 PDT (History)
3 users (show)

See Also:


Attachments
A patch apart (10.93 KB, patch)
2012-08-10 15:27 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff
Patch for moon landing (10.88 KB, patch)
2012-08-13 14:13 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-08-10 15:25:43 PDT
Patch incoming.
Comment 1 Andreas Kling 2012-08-10 15:27:36 PDT
Created attachment 157819 [details]
A patch apart
Comment 2 WebKit Review Bot 2012-08-10 15:30:24 PDT
Attachment 157819 [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/style/NinePieceImage.h:61:  The parameter name "o" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/style/NinePieceImage.h:95:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Antti Koivisto 2012-08-10 16:45:01 PDT
Comment on attachment 157819 [details]
A patch apart

View in context: https://bugs.webkit.org/attachment.cgi?id=157819&action=review

> Source/WebCore/rendering/style/NinePieceImage.h:37
> -class NinePieceImage {
> +class NinePieceImageData {

I think it would be better to use the normal RenderStyle DataRef<>/SET_VAR pattern here. The advantage is that we can share the structure in cases where it already exists and some other surround property is modified. It is also more consistent.

We should also have separate version of SET_VAR for these two-level data structure cases as currently we end up creating unnecessary structures when a variable is set to the current value.
Comment 4 Antti Koivisto 2012-08-13 13:06:56 PDT
Comment on attachment 157819 [details]
A patch apart

Actually DataRefs in substructures don't make sense for non-inherited data as there is never anything to share (except the default value). Forgot what I mumbled above, the patch is fine.
Comment 5 Antti Koivisto 2012-08-13 13:09:11 PDT
It might be good idea to see if NinePieceImages are so rare compared to other properties in SurroundData it should go to rare data instead.
Comment 6 Andreas Kling 2012-08-13 14:13:27 PDT
Created attachment 158101 [details]
Patch for moon landing
Comment 7 WebKit Review Bot 2012-08-13 15:45:05 PDT
Comment on attachment 158101 [details]
Patch for moon landing

Clearing flags on attachment: 158101

Committed r125465: <http://trac.webkit.org/changeset/125465>
Comment 8 WebKit Review Bot 2012-08-13 15:45:08 PDT
All reviewed patches have been landed.  Closing bug.