Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
101543009:
Make cluster creation independent of fingerprinting. Keep track of current (Closed)
DescriptionMake cluster creation independent of fingerprinting. Keep track of current
cluster during layout without walking up the tree.
BUG=302005
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164800
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Address review comments. #
Total comments: 4
Patch Set 3 : Address review comments. #Patch Set 4 : Update TestExpectations #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 22 (0 generated)
FYI - this is super rough and doesn't even compile yet but I wanted to give you a heads up on the general direction I am thinking about... basically "cluster" is now closer to what it was in the old autosizer, fingerprints are optional and force different clusters to share the same multiplier. This should let us reuse a lot of the old autosizer's logic for separating independent descendants, without having to work it into the fingerprinter. It also eliminates the need to walk up the tree in inflate().
This is ready for an initial review. I had trouble coming up with a more object-oriented design. All the methods you'd think might be in a Cluster or Supercluster class also touch state in the autosizer, like the cluster map. Ideas welcome. I also haven't figured out why TextAutosizer::isIndependentDescendant hangs when called outside of layout.
I like where this is heading. I'd be in favor of landing a cleaned-up version of this patch so we can iterate in parallel. I think we may have to punt a little while on a great object oriented design for the supercluster concept. I'm not sure whether there will be a 1:N, 1:1, or N:1 mapping from fingerprint to supercluster, for example. Ideas for cleaning up the code: Updating the two maps for fingerprints sprinkles quite a bit of complexity around. Could you add an inner class called FingerprintMapper? FingerprintMapper could manage two maps and have methods like add(RenderBlock*), remove(RenderBlock*), etc. I think some functions can be moved onto Cluster. For example, computeMultiplier and clusterWantsAutosizing feel like implementation details of Cluster. https://codereview.chromium.org/101543009/diff/110001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/101543009/diff/110001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:870: textAutosizer->prepareForLayout(); I think we still need a mechanism for calling setNeedsLayout() on all blocks that may depend on a multiplier change. For example, imagine blocks A, B, and C all depend on a common multiplier but only blocks A and B get called for layout(). https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.cpp:115: { Could you add a preventative if(!enabled) check here? https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.cpp:163: FastTextAutosizer::Cluster* FastTextAutosizer::getCluster(RenderBlock* block) I was surprised to learn clusters are created in getCluster. getOrCreateCluster? https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... File Source/core/rendering/FastTextAutosizer.h (right): https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.h:88: Cluster* addSupercluster(WTF::HashSet<RenderBlock*>&, RenderBlock*); Nit: addSuperCluster (camel case) https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.h:111: ClusterMap m_clusters; Is this needed?
https://codereview.chromium.org/101543009/diff/110001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/101543009/diff/110001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:870: textAutosizer->prepareForLayout(); On 2013/12/21 01:46:57, pdr wrote: > I think we still need a mechanism for calling setNeedsLayout() on all blocks > that may depend on a multiplier change. For example, imagine blocks A, B, and C > all depend on a common multiplier but only blocks A and B get called for > layout(). I'm not sure I follow. There are three issues I can think of: (1) For global changes (autosizing enabled/disabled via inspector, font scale factor changed in preferences, window resize) we have to make sure the multipliers get recomputed. But I thought TextAutosizer::recalculateMultipliers would handle this already? (2) If there is a partial layout, we need to make sure the cluster stack has all the ancestor clusters of the element being processed. I think we talked about walking up the tree from beginLayout in this case? Let's sync tomorrow about this. (3) If one cluster in a supercluster is modified or removed, we won't recompute the multiplier on the other clusters. I'm not sure this is actually a problem, it just means we might ignore the fingerprints if a page is adding and removing stuff via script. https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.cpp:115: { On 2013/12/21 01:46:57, pdr wrote: > Could you add a preventative if(!enabled) check here? Done. https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.cpp:163: FastTextAutosizer::Cluster* FastTextAutosizer::getCluster(RenderBlock* block) On 2013/12/21 01:46:57, pdr wrote: > I was surprised to learn clusters are created in getCluster. getOrCreateCluster? Done. https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... File Source/core/rendering/FastTextAutosizer.h (right): https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.h:88: Cluster* addSupercluster(WTF::HashSet<RenderBlock*>&, RenderBlock*); On 2013/12/21 01:46:57, pdr wrote: > Nit: addSuperCluster (camel case) I'm not a fan of this capitalization unless it somehow becomes a subclass of Cluster (a cluster that happens to be "super"). Currently it is another thing entirely (set of clusters). https://codereview.chromium.org/101543009/diff/110001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.h:111: ClusterMap m_clusters; On 2013/12/21 01:46:57, pdr wrote: > Is this needed? Yes, this actually owns the Cluster objects.
On 2013/12/21 01:46:57, pdr wrote: > Updating the two maps for fingerprints sprinkles quite a bit of complexity > around. Could you add an inner class called FingerprintMapper? FingerprintMapper > could manage two maps and have methods like add(RenderBlock*), > remove(RenderBlock*), etc. Done. > I think some functions can be moved onto Cluster. For example, computeMultiplier > and clusterWantsAutosizing feel like implementation details of Cluster. They do feel that way, but I'm not sure about the best way to structure it. Right now we call both of these before creating the Cluster object, so that we can pass the right values into the ctor.
A few minor nits. Otherwise, LGTM! https://codereview.chromium.org/101543009/diff/160001/Source/core/rendering/F... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/101543009/diff/160001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.cpp:172: FastTextAutosizer::Cluster* FastTextAutosizer::addSupercluster(WTF::HashSet<RenderBlock*>& roots, RenderBlock* returnFor) Would it be cleaner to change this to: FastTextAutosizer::Cluster* FastTextAutosizer::addSupercluster(RenderBlock* block) and have the function look up the roots? https://codereview.chromium.org/101543009/diff/160001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.cpp:193: RenderBlock* FastTextAutosizer::deepestCommonAncestor(WTF::HashSet<RenderBlock*>& blocks) It may be clean to pull out the typedef for BlockSet and use it here (and above).
https://codereview.chromium.org/101543009/diff/160001/Source/core/rendering/F... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/101543009/diff/160001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.cpp:172: FastTextAutosizer::Cluster* FastTextAutosizer::addSupercluster(WTF::HashSet<RenderBlock*>& roots, RenderBlock* returnFor) On 2014/01/07 19:50:27, pdr wrote: > Would it be cleaner to change this to: > FastTextAutosizer::Cluster* FastTextAutosizer::addSupercluster(RenderBlock* > block) > and have the function look up the roots? How about passing the fingerprint in, and having it look up the BlockSet from that? (Note that the RenderBlock* param is a performance optimization to save us a lookup for the first cluster, it's incidental to the purpose of addSupercluster.) https://codereview.chromium.org/101543009/diff/160001/Source/core/rendering/F... Source/core/rendering/FastTextAutosizer.cpp:193: RenderBlock* FastTextAutosizer::deepestCommonAncestor(WTF::HashSet<RenderBlock*>& blocks) On 2014/01/07 19:50:27, pdr wrote: > It may be clean to pull out the typedef for BlockSet and use it here (and > above). This is a great idea :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/101543009/250001
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/101543009/460001
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/101543009/630001
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/101543009/630001
Failed to apply patch for LayoutTests/TestExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file LayoutTests/TestExpectations Hunk #1 FAILED at 875. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/TestExpectations.rej Patch: LayoutTests/TestExpectations Index: LayoutTests/TestExpectations diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations index ae69a94bfebb03a8907487da24d1948cafa381ae..d90f30f9a7a37ec0996bc7a0bd9cc8f25ecbdab9 100644 --- a/LayoutTests/TestExpectations +++ b/LayoutTests/TestExpectations @@ -875,6 +875,31 @@ crbug.com/322782 virtual/fasttextautosizing/fast/text-autosizing/oscillation-jav crbug.com/322782 virtual/fasttextautosizing/fast/text-autosizing/wide-child.html [ ImageOnlyFailure ] crbug.com/322782 virtual/fasttextautosizing/fast/text-autosizing/wide-in-narrow-overflow-scroll.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/cluster-with-narrow-lca-and-cluster.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/cluster-with-narrow-lca.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-and-overflow-hidden-ancestor.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-height-ancestor.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-maxheight-ancestor.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-maxheight.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-percent-maxheight.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-percent-of-viewport-maxheight.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-within-overflow-ancestor.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-button-input-elements.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-checkbox-input-element.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-radio-input-element.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-select-element.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-textfield-input-elements.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/hackernews-comments.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/header-li-links-autosizing.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/header-links-autosizing-different-fontsize.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/header-links-autosizing.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/nested-em-line-height.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/positioned-out-of-flow.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/resize-window.html [ Failure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/span-child.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/unwrappable-blocks.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/unwrappable-inlines.html [ ImageOnlyFailure ] + crbug.com/331898 mhtml/invalid-bad-boundary.mht [ NeedsRebaseline ] crbug.com/331898 mhtml/invalid-bad-boundary2.mht [ NeedsRebaseline ]
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/101543009/800001
Failed to apply patch for LayoutTests/TestExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file LayoutTests/TestExpectations Hunk #1 FAILED at 875. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/TestExpectations.rej Patch: LayoutTests/TestExpectations Index: LayoutTests/TestExpectations diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations index 7434c56a92d8f4a2044985bd562d65c7071b3177..bafb2b59e81ff3931c36d0837a096b19209de6a3 100644 --- a/LayoutTests/TestExpectations +++ b/LayoutTests/TestExpectations @@ -875,6 +875,31 @@ crbug.com/322782 virtual/fasttextautosizing/fast/text-autosizing/oscillation-jav crbug.com/322782 virtual/fasttextautosizing/fast/text-autosizing/wide-child.html [ ImageOnlyFailure ] crbug.com/322782 virtual/fasttextautosizing/fast/text-autosizing/wide-in-narrow-overflow-scroll.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/cluster-with-narrow-lca-and-cluster.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/cluster-with-narrow-lca.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-and-overflow-hidden-ancestor.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-height-ancestor.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-maxheight-ancestor.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-maxheight.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-percent-maxheight.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-percent-of-viewport-maxheight.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/constrained-within-overflow-ancestor.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-button-input-elements.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-checkbox-input-element.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-radio-input-element.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-select-element.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/form-controls-autosizing-textfield-input-elements.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/hackernews-comments.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/header-li-links-autosizing.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/header-links-autosizing-different-fontsize.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/header-links-autosizing.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/nested-em-line-height.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/positioned-out-of-flow.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/resize-window.html [ Failure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/span-child.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/unwrappable-blocks.html [ ImageOnlyFailure ] +crbug.com/302005 virtual/fasttextautosizing/fast/text-autosizing/unwrappable-inlines.html [ ImageOnlyFailure ] + crbug.com/331898 mhtml/invalid-bad-boundary.mht [ NeedsRebaseline ] crbug.com/331898 mhtml/invalid-bad-boundary2.mht [ NeedsRebaseline ]
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/101543009/840001
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
On 2014/01/09 03:05:04, I haz the power (commit-bot) wrote: > Retried try job too often on win_blink_rel for step(s) webkit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_... I think this is just due to hackernews. Can you add: virtual/fasttextautosizing/fast/text-autosizing/hackernews-comments.html [ NeedsRebaseline ]
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/101543009/1060001
Message was sent while issue was closed.
Change committed as 164800 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
