Bug 108384

Summary: TextAutosizing: refactor the cluster related method parameters into a single struct
Product: WebKit Reporter: Anton Vayvod <avayvod>
Component: Layout and RenderingAssignee: Anton Vayvod <avayvod>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jchaffraix, johnme, kenneth, ojan.autocc, timvolodine, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107300, 108411, 109573    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Anton Vayvod 2013-01-30 14:23:05 PST
TextAutosizing: refactor the cluster related method parameters into a single struct
Comment 1 Anton Vayvod 2013-01-30 14:30:20 PST
Created attachment 185575 [details]
Patch
Comment 2 John Mellor 2013-01-30 18:29:30 PST
Comment on attachment 185575 [details]
Patch

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

Seems reasonable. Just some minor nits.

> Source/WebCore/rendering/TextAutosizer.cpp:58
> +    const RenderBlock* blockContainingAllText;

Nit: Is there a reason why one of these is const, but not the other?

> Source/WebCore/rendering/TextAutosizer.cpp:150
> +void TextAutosizer::processContainer(float multiplier, RenderBlock* container, const TextAutosizingClusterInfo& cluster, RenderObject* subtreeRoot, const TextAutosizingWindowInfo& windowInfo)

Nit: You seem to alternate between calling variables of type TextAutosizingClusterInfo a cluster or a clusterInfo. If possible, pick one and stick with it :)

> Source/WebCore/rendering/TextAutosizer.h:64
> +    void processCluster(TextAutosizingClusterInfo*, RenderBlock* container, RenderObject* subtreeRoot, const TextAutosizingWindowInfo&);

Nit: Sometimes you pass a TextAutosizingClusterInfo*, and sometimes a TextAutosizingClusterInfo&. Best to be consistent, since it doesn't seem to be optional here. (I forget which WebKit prefers).
Comment 3 Anton Vayvod 2013-01-30 18:32:27 PST
Julien, Kenneth, could one of you take a look, please?
Resolving John's nits now.
Comment 4 Anton Vayvod 2013-01-30 18:45:02 PST
Created attachment 185655 [details]
Patch
Comment 5 Anton Vayvod 2013-01-30 18:51:27 PST
Comment on attachment 185575 [details]
Patch

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

>> Source/WebCore/rendering/TextAutosizer.cpp:58
>> +    const RenderBlock* blockContainingAllText;
> 
> Nit: Is there a reason why one of these is const, but not the other?

We pass |root| to methods that change the node itself or its descendants. blockContainingAllText is computed once per cluster and doesn't change with time.

>> Source/WebCore/rendering/TextAutosizer.cpp:150
>> +void TextAutosizer::processContainer(float multiplier, RenderBlock* container, const TextAutosizingClusterInfo& cluster, RenderObject* subtreeRoot, const TextAutosizingWindowInfo& windowInfo)
> 
> Nit: You seem to alternate between calling variables of type TextAutosizingClusterInfo a cluster or a clusterInfo. If possible, pick one and stick with it :)

Done.

>> Source/WebCore/rendering/TextAutosizer.h:64
>> +    void processCluster(TextAutosizingClusterInfo*, RenderBlock* container, RenderObject* subtreeRoot, const TextAutosizingWindowInfo&);
> 
> Nit: Sometimes you pass a TextAutosizingClusterInfo*, and sometimes a TextAutosizingClusterInfo&. Best to be consistent, since it doesn't seem to be optional here. (I forget which WebKit prefers).

Changed to TextAutosizingClusterInfo* (sometimes const pointer but it will change in the next patch).
Comment 6 John Mellor 2013-01-30 20:13:43 PST
Comment on attachment 185655 [details]
Patch

Looks good. Up to Julien/Kenneth now.
Comment 7 Kenneth Rohde Christiansen 2013-01-31 02:26:22 PST
Comment on attachment 185655 [details]
Patch

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

im ok with this, but I also dont see any big value in the change. Could you explain what you will be adding in the future?

> Source/WebCore/ChangeLog:5
> +        TextAutosizing: refactor the cluster related method parameters into a single struct.
> +        This anticipates adding more cluster related information in the following patches.
> +        https://bugs.webkit.org/show_bug.cgi?id=108384

You should only have the bugtitle here and then the bug link.
Comment 8 Build Bot 2013-01-31 02:43:36 PST
Comment on attachment 185655 [details]
Patch

Attachment 185655 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16270098
Comment 9 Anton Vayvod 2013-01-31 12:48:16 PST
Created attachment 185842 [details]
Patch
Comment 10 Anton Vayvod 2013-01-31 12:49:00 PST
Comment on attachment 185655 [details]
Patch

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

>> Source/WebCore/ChangeLog:5
>> +        https://bugs.webkit.org/show_bug.cgi?id=108384
> 
> You should only have the bugtitle here and then the bug link.

Done.
Comment 11 Anton Vayvod 2013-01-31 12:52:26 PST
I'm going to add more fields to the struct (e.g. a list of all narrower descendants of the cluster that we'd like to combine into a single one so that the multiplier is the same or a maximum width difference between the cluster and its descendants, see the bug 108411).
Comment 12 WebKit Review Bot 2013-01-31 14:03:48 PST
Comment on attachment 185842 [details]
Patch

Rejecting attachment 185842 [details] from review queue.

avayvod@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 13 WebKit Review Bot 2013-01-31 14:04:29 PST
Comment on attachment 185842 [details]
Patch

Rejecting attachment 185842 [details] from commit-queue.

avayvod@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 14 WebKit Review Bot 2013-01-31 14:33:18 PST
Comment on attachment 185842 [details]
Patch

Clearing flags on attachment: 185842

Committed r141489: <http://trac.webkit.org/changeset/141489>
Comment 15 WebKit Review Bot 2013-01-31 14:33:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Julien Chaffraix 2013-02-06 11:07:29 PST
Comment on attachment 185842 [details]
Patch

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

> Source/WebCore/rendering/TextAutosizer.cpp:125
> +void TextAutosizer::processCluster(TextAutosizingClusterInfo* clusterInfo, RenderBlock* container, RenderObject* subtreeRoot, const TextAutosizingWindowInfo& windowInfo)

I am sad that this landed with a raw pointer when we assume that it can't be NULL.

Raw pointers are better used for *optional* parameters and references when you *require* the parameters. Here the code always makes a pointer out of a local copy, which makes reference the obvious choice.