Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1760153002:
Check slot's distribution state more strictly (Closed)
|
Created:
4 years, 9 months ago by hayato Modified:
4 years, 9 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck slot's distribution state more strictly
- Add more asserts
- Introduce willUpdateDistribution and make clearDistribution, it is now a private function, call it
BUG=531990
Committed: https://crrev.com/5ff5b9d9a9f99edf1392f35558686f9b3edd2663
Cr-Commit-Position: refs/heads/master@{#379248}
Patch Set 1 #Patch Set 2 : update #
Total comments: 4
Messages
Total messages: 26 (11 generated)
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1760153002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1760153002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
update
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1760153002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1760153002/20001
Description was changed from ========== Check slot's distribution state more strictly BUG= ========== to ========== Check slot's distribution state more strictly - Add more asserts - Introduce willUpdateDistribution and make cleanDistribution call it BUG=531990 ==========
hayato@chromium.org changed reviewers: + kochi@chromium.org, tkent@chromium.org, yuzus@chromium.org
PTAL
lgtm
This doesn't seem to me a state split for checking on-going distribution state. Is the change clearDistribution() -> willUpdateDistribution() + didUpdateDistribution() expected? insertedInto()/removedFrom() call clearDistribution() and that behavior will change. https://codereview.chromium.org/1760153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/1760153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:276: void HTMLSlotElement::clearDistribution() Why this is not updateDistribution()? insertedInto()/removedFrom() will always update the distribution, as opposed to just clear distribution and calculate lazily later? https://codereview.chromium.org/1760153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLSlotElement.h (right): https://codereview.chromium.org/1760153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLSlotElement.h:59: void willUpdateDistribution(); nit: Looking at usages, I'd think renaming willUpdateDistribution/didUpdateDistribution to startUpdateDistribution()/finishUpdateDistribution()) is better, but it's up to you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Is the change clearDistribution() -> willUpdateDistribution() + didUpdateDistribution() expected? Yes > insertedInto()/removedFrom() call clearDistribution() and that behavior will change. No external behavior change, isn't it? (except for slotchange event which is just POC) https://codereview.chromium.org/1760153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/1760153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:276: void HTMLSlotElement::clearDistribution() On 2016/03/04 at 03:49:07, kochi wrote: > Why this is not updateDistribution()? Could you elaborate? > > insertedInto()/removedFrom() will always update the distribution, > as opposed to just clear distribution and calculate lazily later? Yes.
https://codereview.chromium.org/1760153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/1760153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:276: void HTMLSlotElement::clearDistribution() On 2016/03/04 05:15:14, hayato wrote: > On 2016/03/04 at 03:49:07, kochi wrote: > > Why this is not updateDistribution()? > > Could you elaborate? The previous one was obvious that it "clears" m_distributedNodes etc. But now they are cleared, then completes update. So to me, "clear"ing "distribution" sounds strange. I thought this function does the full update process of distribution.
I understand the point. However, it's now a private function. This naming strange-ness should not be a serious issue. If you have any better idea, we can update the name later.
The CQ bit was checked by hayato@chromium.org
The CQ bit was unchecked by hayato@chromium.org
Description was changed from ========== Check slot's distribution state more strictly - Add more asserts - Introduce willUpdateDistribution and make cleanDistribution call it BUG=531990 ========== to ========== Check slot's distribution state more strictly - Add more asserts - Introduce willUpdateDistribution and make clearDistribution, it is now a private function, call it BUG=531990 ==========
The CQ bit was checked by hayato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1760153002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1760153002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
On 2016/03/04 08:25:31, hayato wrote: > I understand the point. However, it's now a private function. This naming > strange-ness should not be a serious issue. > If you have any better idea, we can update the name later. alright, lgtm.
Message was sent while issue was closed.
Description was changed from ========== Check slot's distribution state more strictly - Add more asserts - Introduce willUpdateDistribution and make clearDistribution, it is now a private function, call it BUG=531990 ========== to ========== Check slot's distribution state more strictly - Add more asserts - Introduce willUpdateDistribution and make clearDistribution, it is now a private function, call it BUG=531990 Committed: https://crrev.com/5ff5b9d9a9f99edf1392f35558686f9b3edd2663 Cr-Commit-Position: refs/heads/master@{#379248} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5ff5b9d9a9f99edf1392f35558686f9b3edd2663 Cr-Commit-Position: refs/heads/master@{#379248} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
