Bug 108384 – TextAutosizing: refactor the cluster related method parameters into a single struct

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Anton Vayvod]

TextAutosizing: refactor the cluster related method parameters into a single struct

[Anton Vayvod]

Created attachment 185575 [details]
Patch

[John Mellor]

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).

[Anton Vayvod]

Julien, Kenneth, could one of you take a look, please?
Resolving John's nits now.

[Anton Vayvod]

Created attachment 185655 [details]
Patch

[Anton Vayvod]

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).

[John Mellor]

Comment on attachment 185655 [details]
Patch

Looks good. Up to Julien/Kenneth now.

[Kenneth Rohde Christiansen]

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.

[Build Bot]

Comment on attachment 185655 [details]
Patch

Attachment 185655 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16270098

[Anton Vayvod]

Created attachment 185842 [details]
Patch

[Anton Vayvod]

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.

[Anton Vayvod]

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).

[WebKit Review Bot]

Comment on attachment 185842 [details]
Patch

Rejecting attachment 185842 [details] from review queue.

[e-mail] 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.

[WebKit Review Bot]

Comment on attachment 185842 [details]
Patch

Rejecting attachment 185842 [details] from commit-queue.

[e-mail] 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.

[WebKit Review Bot]

Comment on attachment 185842 [details]
Patch

Clearing flags on attachment: 185842

Committed r141489: <http://trac.webkit.org/changeset/141489>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

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.