TextAutosizing: refactor the cluster related method parameters into a single struct
Created attachment 185575 [details] Patch
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).
Julien, Kenneth, could one of you take a look, please? Resolving John's nits now.
Created attachment 185655 [details] Patch
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 on attachment 185655 [details] Patch Looks good. Up to Julien/Kenneth now.
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 on attachment 185655 [details] Patch Attachment 185655 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16270098
Created attachment 185842 [details] Patch
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.
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 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 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 on attachment 185842 [details] Patch Clearing flags on attachment: 185842 Committed r141489: <http://trac.webkit.org/changeset/141489>
All reviewed patches have been landed. Closing bug.
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.