Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2842263004:
Fix assignedNodes({flatten:true}) to return fallback content in document tree (Closed)
DescriptionPatch Set 1 #Patch Set 2 : Fix. #Patch Set 3 : Remove layout test (upstreamed to WPT) #Patch Set 4 : rebase #Patch Set 5 : Fix TestExpectations #Patch Set 6 : Remove failure expected.txt. #
Total comments: 4
Patch Set 7 : fix for comments. #
Total comments: 5
Patch Set 8 : Fix for comments. #
Total comments: 1
Patch Set 9 : Rename GetDistributedNodes to GetDistributedNodesExcludingFallback #
Total comments: 9
Patch Set 10 : tentative #Patch Set 11 : tentative #
Messages
Total messages: 48 (27 generated)
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Add layout test for fallback content and distribution.
BUG=
==========
to
==========
Fix assignedNodes({flatten:true}) to return fallback content in document tree
BUG=715915
==========
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from
==========
Fix assignedNodes({flatten:true}) to return fallback content in document tree
BUG=715915
==========
to
==========
Fix assignedNodes({flatten:true}) to return fallback content in document tree
BUG=715915, 626703
==========
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/08 04:24:08, kochi wrote: > PTAL Ping?
kochi@chromium.org changed reviewers: + hayato@chromium.org
Oops, I forgot to add a reviewer..
https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:140: if (other.SupportsDistribution()) { Could you avoid if else here? It looks a functional duplication of GetDistributedNodesForBinding. https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.h (right): https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.h:110: const HeapVector<Member<Node>> GetDistributedNodesOfSlotInDocument() const; InDocument is not correct. It should be inNonShadowTrees or something. I would rather name this to more logical name, than leaking the condition. e.g. SupportDistribution() is a logical name, and it doesn't leak the internal condition.
https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:140: if (other.SupportsDistribution()) { On 2017/05/12 03:19:09, hayato wrote: > Could you avoid if else here? > It looks a functional duplication of GetDistributedNodesForBinding. I see, having inconsistent distributed_nodes_ and distributed_indices_ would be also problematic. Let me work on it.
https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:140: if (other.SupportsDistribution()) { On 2017/05/12 07:26:33, kochi wrote: > On 2017/05/12 03:19:09, hayato wrote: > > Could you avoid if else here? > > It looks a functional duplication of GetDistributedNodesForBinding. > > I see, having inconsistent distributed_nodes_ and distributed_indices_ > would be also problematic. Let me work on it. Hmm, this function is called when resolving distribution, to have a cached distributed nodes in |distributed_nodes_|. |distributed_nodes_| of slots in non-shadow tree are still empty. If we don't calculate |distributed_nodes_| for slots in shadow tree here, GetDistributedNodesForBinding() has to go though all |distributed_nodes_| to see if there is any slot that contains fallback content in it, and insert manually calculated distributed nodes. This will make assignedNodes({flatten:true}) slower. GetDistributedNodesForBinding() does manual calculation if |this| is a slot in non-shadow tree. So I don't think this is duplicate.
PTAL
https://codereview.chromium.org/2842263004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:140: const HeapVector<Member<Node>>& other_distributed_nodes = Avoid this temporary vector being allocated. It looks inefficient. https://codereview.chromium.org/2842263004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:142: : other.GetDistributedNodesInNonShadowTree(); As I mentioned before, I prefer more logical name for GetDistributedNodesInNonShadowTree(). - SupportsDistribution is a logical name, which doesn't leak an internal condition. - GetDistributedNodesInNonShadowTree() is not a logical name, which leaks an internal condition. Could you rename GetDistributedNodesInNonShadowTree so that it is consistent with the naming of SupportsDistribution()?
https://codereview.chromium.org/2842263004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:140: const HeapVector<Member<Node>>& other_distributed_nodes = On 2017/05/16 at 04:22:32, hayato wrote: > Avoid this temporary vector being allocated. It looks inefficient. Ah, it is a reference, and being used in L145. Ignore this comment.
It looks I forgot to see this comment. On 2017/05/12 at 07:53:14, kochi wrote: > https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): > > https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:140: if (other.SupportsDistribution()) { > On 2017/05/12 07:26:33, kochi wrote: > > On 2017/05/12 03:19:09, hayato wrote: > > > Could you avoid if else here? > > > It looks a functional duplication of GetDistributedNodesForBinding. > > > > I see, having inconsistent distributed_nodes_ and distributed_indices_ > > would be also problematic. Let me work on it. > > Hmm, this function is called when resolving distribution, to have > a cached distributed nodes in |distributed_nodes_|. > > |distributed_nodes_| of slots in non-shadow tree are still empty. > > If we don't calculate |distributed_nodes_| for slots in shadow > tree here, GetDistributedNodesForBinding() has to go though all > |distributed_nodes_| to see if there is any slot that contains > fallback content in it, and insert manually calculated distributed > nodes. This will make assignedNodes({flatten:true}) slower. > > GetDistributedNodesForBinding() does manual calculation if |this| > is a slot in non-shadow tree. So I don't think this is duplicate. Could you elaborate? I am afraid that I don't understand the point. In the latest CL, both, 1 and 2, look same to me literally. 1. the contents of GetDistributedNodesForBinding() const HeapVector<Member<Node>> HTMLSlotElement::GetDistributedNodesForBinding() { DCHECK(!NeedsDistributionRecalc()); if (SupportsDistribution()) return distributed_nodes_; return GetDistributedNodesInNonShadowTree(); } 2. const HeapVector<Member<Node>>& other_distributed_nodes = other.SupportsDistribution() ? other.distributed_nodes_ : other.GetDistributedNodesInNonShadowTree();
On 2017/05/16 04:46:05, hayato wrote: > It looks I forgot to see this comment. > > > On 2017/05/12 at 07:53:14, kochi wrote: > > > https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): > > > > > https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:140: if > (other.SupportsDistribution()) { > > On 2017/05/12 07:26:33, kochi wrote: > > > On 2017/05/12 03:19:09, hayato wrote: > > > > Could you avoid if else here? > > > > It looks a functional duplication of GetDistributedNodesForBinding. > > > > > > I see, having inconsistent distributed_nodes_ and distributed_indices_ > > > would be also problematic. Let me work on it. > > > > Hmm, this function is called when resolving distribution, to have > > a cached distributed nodes in |distributed_nodes_|. > > > > |distributed_nodes_| of slots in non-shadow tree are still empty. > > > > If we don't calculate |distributed_nodes_| for slots in shadow > > tree here, GetDistributedNodesForBinding() has to go though all > > |distributed_nodes_| to see if there is any slot that contains > > fallback content in it, and insert manually calculated distributed > > nodes. This will make assignedNodes({flatten:true}) slower. > > > > GetDistributedNodesForBinding() does manual calculation if |this| > > is a slot in non-shadow tree. So I don't think this is duplicate. > > Could you elaborate? I am afraid that I don't understand the point. > > In the latest CL, both, 1 and 2, look same to me literally. > > 1. the contents of GetDistributedNodesForBinding() > > const HeapVector<Member<Node>> > HTMLSlotElement::GetDistributedNodesForBinding() { > DCHECK(!NeedsDistributionRecalc()); > if (SupportsDistribution()) > return distributed_nodes_; > return GetDistributedNodesInNonShadowTree(); > } > > > 2. const HeapVector<Member<Node>>& other_distributed_nodes = > other.SupportsDistribution() ? other.distributed_nodes_ > : other.GetDistributedNodesInNonShadowTree(); Aha, I see, I missed your obvious point :-)
Patchset #8 (id:140001) has been deleted
On 2017/05/16 04:46:05, hayato wrote: > It looks I forgot to see this comment. > > > On 2017/05/12 at 07:53:14, kochi wrote: > > > https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): > > > > > https://codereview.chromium.org/2842263004/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:140: if > (other.SupportsDistribution()) { > > On 2017/05/12 07:26:33, kochi wrote: > > > On 2017/05/12 03:19:09, hayato wrote: > > > > Could you avoid if else here? > > > > It looks a functional duplication of GetDistributedNodesForBinding. > > > > > > I see, having inconsistent distributed_nodes_ and distributed_indices_ > > > would be also problematic. Let me work on it. > > > > Hmm, this function is called when resolving distribution, to have > > a cached distributed nodes in |distributed_nodes_|. > > > > |distributed_nodes_| of slots in non-shadow tree are still empty. > > > > If we don't calculate |distributed_nodes_| for slots in shadow > > tree here, GetDistributedNodesForBinding() has to go though all > > |distributed_nodes_| to see if there is any slot that contains > > fallback content in it, and insert manually calculated distributed > > nodes. This will make assignedNodes({flatten:true}) slower. > > > > GetDistributedNodesForBinding() does manual calculation if |this| > > is a slot in non-shadow tree. So I don't think this is duplicate. > > Could you elaborate? I am afraid that I don't understand the point. > > In the latest CL, both, 1 and 2, look same to me literally. > > 1. the contents of GetDistributedNodesForBinding() > > const HeapVector<Member<Node>> > HTMLSlotElement::GetDistributedNodesForBinding() { > DCHECK(!NeedsDistributionRecalc()); > if (SupportsDistribution()) > return distributed_nodes_; > return GetDistributedNodesInNonShadowTree(); > } > > > 2. const HeapVector<Member<Node>>& other_distributed_nodes = > other.SupportsDistribution() ? other.distributed_nodes_ > : other.GetDistributedNodesInNonShadowTree(); So it was duplicate because ResolveDistribution() is called while it is recalculating the distribution, so calling into GetDistributedNodesForBinding() causes hitting DCHECK(). I split the function into two parts.
PTAL https://codereview.chromium.org/2842263004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:140: const HeapVector<Member<Node>>& other_distributed_nodes = On 2017/05/16 04:26:44, hayato wrote: > On 2017/05/16 at 04:22:32, hayato wrote: > > Avoid this temporary vector being allocated. It looks inefficient. > > Ah, it is a reference, and being used in L145. Ignore this comment. Acknowledged. https://codereview.chromium.org/2842263004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:142: : other.GetDistributedNodesInNonShadowTree(); On 2017/05/16 04:22:32, hayato wrote: > As I mentioned before, I prefer more logical name for > GetDistributedNodesInNonShadowTree(). > > - SupportsDistribution is a logical name, which doesn't leak an internal > condition. > - GetDistributedNodesInNonShadowTree() is not a logical name, which leaks an > internal condition. > > Could you rename GetDistributedNodesInNonShadowTree so that it is consistent > with the naming of SupportsDistribution()? I thought "InNonShadowTree()" is logical in that it can be either true or false. Though I understand that you want more consistency with "SupportsDistribution()". I renamed this to "CollectDistributedNodesFromSlotWithoutDistribution()", but it went away with the other change.
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2842263004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:84: HTMLSlotElement::GetDistributedNodesWithFallback() const { The concept of DistributedNodes includes Fallback elements. So this name would be confusing. I rather name this just 'GetDistributedNodes'. Couldn't we unify this with GetDistributedNodesForBinding? Something is wrong if the only difference between them is DCHECK(). That implies that we should use DCHECK() for both cases. Could you pursue to unify them?
Patchset #9 (id:180001) has been deleted
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
On 2017/05/16 07:50:07, hayato wrote: > https://codereview.chromium.org/2842263004/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): > > https://codereview.chromium.org/2842263004/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:84: > HTMLSlotElement::GetDistributedNodesWithFallback() const { > The concept of DistributedNodes includes Fallback elements. So this name would > be confusing. > > I rather name this just 'GetDistributedNodes'. > > Couldn't we unify this with GetDistributedNodesForBinding? > Something is wrong if the only difference between them is DCHECK(). > That implies that we should use DCHECK() for both cases. > > Could you pursue to unify them? Okay, the original GetDistributedNodes() function name predates the introduction of fallback contents, and the name is no longer precise. On the other hand, GetDistributedNodesForBinding() is, although it's public, only called from assignedNodesForBinding() and not from IDL, and can be private. So I renamed GetDistributedNodes() -> GetDistributedNodesExcludingFallback() GetDistributedNodesForBinding() -> GetDistributedNodes() so these names semantically match what they return.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleEngine.cpp (right): https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleEngine.cpp:874: for (auto& node : slot.GetDistributedNodesExcludingFallback()) { Is this correct? Why doesn't this need to invalidate fallback elements? https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (left): https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:79: DCHECK(!NeedsDistributionRecalc()); Don't remove DCHECK(). DCHECK() is important and shouldn't be removed without a reason. If there is some code path which doesn't meet this condition, please fix the caller, instead of removing DCHECK(). At least, I suggest to explain what can justify removing DCHECK(), so that we can know the reason why DCHECK() was removed. https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:101: HTMLSlotElement::GetDistributedNodesExcludingFallback() { GetDistributedNodesExcludingFallback doesn't sound a good name to me. It is likely to exclude some elements from the current distributed nodes. e.g set difference. Here, we are returning distributed_nodes_ itself, not excluding any element from there.
https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleEngine.cpp (right): https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleEngine.cpp:874: for (auto& node : slot.GetDistributedNodesExcludingFallback()) { On 2017/05/18 06:58:25, hayato wrote: > Is this correct? Why doesn't this need to invalidate fallback elements? If this is not correct, I'll file another bug. This should not change from the previous behavior, because the old GetDistributedNodes() returned its internal distributed_nodes_ which doesn't include fallback content. https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (left): https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:79: DCHECK(!NeedsDistributionRecalc()); On 2017/05/18 06:58:25, hayato wrote: > Don't remove DCHECK(). DCHECK() is important and shouldn't be removed without a > reason. The only user of GetDistributedNodesForBinding() was from assignedNodesForBinding() previously, though the interface was public so something could call it, so DCHECK() made sense. Now this interface became private and called from two places: one from the same place, assignedNodesForBinding() as before, just after UpdateDistribution(). The other from AppendDistributedNodesFrom(), which are supposed to be called *during* distribution update, so DCHECK(!NeedsDistribution()) will fail. I deliberately removed DCHECK() here because the existing call path already guarantee that distribution is updated, and it should not DHCECK() for the new call path. > If there is some code path which doesn't meet this condition, please fix the > caller, instead of removing DCHECK(). > > At least, I suggest to explain what can justify removing DCHECK(), so that we > can know the reason why DCHECK() was removed. https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:101: HTMLSlotElement::GetDistributedNodesExcludingFallback() { On 2017/05/18 06:58:25, hayato wrote: > GetDistributedNodesExcludingFallback doesn't sound a good name to me. > It is likely to exclude some elements from the current distributed nodes. e.g > set difference. > Here, we are returning distributed_nodes_ itself, not excluding any element from > there. Hmm, then |distributed_nodes_| is a misnomer. I'm not really sure the benefit of not adding fallback content to distributed_nodes_ and recalculate on demand. I thought it was done for some optimization - do you remember why? It isn't obvious to me.
https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (left): https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:79: DCHECK(!NeedsDistributionRecalc()); Okay. Since it is now private, it can be acceptable. Though I still prefer DCHECK() as much as possible, with some kind of refactoring, we can tackle this issue as another issue. https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:101: HTMLSlotElement::GetDistributedNodesExcludingFallback() { We are adding fallback content to distributed_nodes_. See UpdateDistributedNodesWithFallback
https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/2842263004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:101: HTMLSlotElement::GetDistributedNodesExcludingFallback() { On 2017/05/19 04:05:36, hayato wrote: > We are adding fallback content to distributed_nodes_. See > UpdateDistributedNodesWithFallback Ah, sorry. I was misunderstanding. Let me rework.
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Closing this as upstream spec has been changed. See https://github.com/whatwg/dom/pull/459 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
