Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2205523004:
Refactored UseCounter code for event-listener-firing. (Closed)
DescriptionRefactored UseCounter code for event-listener-firing.
BUG=629601
Committed: https://crrev.com/840adcf880f0246881f171ede42a3f0930743546
Cr-Commit-Position: refs/heads/master@{#410208}
Patch Set 1 #Patch Set 2 : Renamed the method. #Patch Set 3 : Removed TODO #
Total comments: 1
Patch Set 4 : Added a TODO for the suspicious |if|. #
Messages
Total messages: 21 (12 generated)
The CQ bit was checked by mustaq@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.
ptal
mustaq@chromium.org changed reviewers: + bokan@chromium.org
ptal
Description was changed from ========== Refactored UseCounter code for event-listener-firing. BUG= ========== to ========== Refactored UseCounter code for event-listener-firing. BUG=629601 ==========
bokan@chromium.org changed reviewers: + ojan@chromium.org
Looks like there's a bug in the existing code, could you confirm and fix? Otherwise, lgtm https://codereview.chromium.org/2205523004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/EventTarget.cpp (right): https://codereview.chromium.org/2205523004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/EventTarget.cpp:569: if (executingWindow->top()) I know you didn't add this but I think this is wrong - all frames will count as subframes here. It should be `if (executingWindow != executingWindow->top())`. It looks like this was accidentally changed in https://chromiumcodereview.appspot.com/22564003. +ojan@ to confirm/FYI since he added it.
On 2016/08/05 15:34:25, bokan wrote: > Looks like there's a bug in the existing code, could you confirm and fix? > Otherwise, lgtm > > https://codereview.chromium.org/2205523004/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/events/EventTarget.cpp (right): > > https://codereview.chromium.org/2205523004/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/events/EventTarget.cpp:569: if > (executingWindow->top()) > I know you didn't add this but I think this is wrong - all frames will count as > subframes here. It should be `if (executingWindow != executingWindow->top())`. > It looks like this was accidentally changed in > https://chromiumcodereview.appspot.com/22564003. +ojan@ to confirm/FYI since he > added it. Hmm, thanks for catching this. There could be another interpretation of the |if| statement from current impl of DOMWindow::top() and LocalDOMWindow::frame(): "If LocalDOMWindow::frame() is not null, count". It could be correct when combined with the description of that other CL saying: "In a coming change, newly added executingWindow() will walk up the import tree to return the master doc's DOMWindow." In any case, I think it makes sense to go commit CL and resolve the concern as a separate bug. Wdyt?
On 2016/08/05 17:25:36, mustaq wrote: > On 2016/08/05 15:34:25, bokan wrote: > > Looks like there's a bug in the existing code, could you confirm and fix? > > Otherwise, lgtm > > > > > https://codereview.chromium.org/2205523004/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/events/EventTarget.cpp (right): > > > > > https://codereview.chromium.org/2205523004/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/events/EventTarget.cpp:569: if > > (executingWindow->top()) > > I know you didn't add this but I think this is wrong - all frames will count > as > > subframes here. It should be `if (executingWindow != executingWindow->top())`. > > It looks like this was accidentally changed in > > https://chromiumcodereview.appspot.com/22564003. +ojan@ to confirm/FYI since > he > > added it. > > Hmm, thanks for catching this. > > There could be another interpretation of the |if| statement from current impl of > DOMWindow::top() and LocalDOMWindow::frame(): > > "If LocalDOMWindow::frame() is not null, count". > > It could be correct when combined with the description of that other CL saying: > "In a coming change, newly added executingWindow() will > walk up the import tree to return the master doc's DOMWindow." Perhaps I'm missing something. All we're doing right now is null checks though. Just because the executingWindow has a `top()` window doesn't mean that window isn't the `top()` itself, right? > In any case, I think it makes sense to go commit CL and resolve the concern as > a separate bug. Wdyt? Yup, sgtm, please add a TODO and bug# though.
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by mustaq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2205523004/#ps80001 (title: "Added a TODO for the suspicious |if|.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ewwww...yikes. That's definitely wrong. Please fix in a followup patch if you don't mind. If you don't have time, I can do it as well.
Message was sent while issue was closed.
Description was changed from ========== Refactored UseCounter code for event-listener-firing. BUG=629601 ========== to ========== Refactored UseCounter code for event-listener-firing. BUG=629601 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Refactored UseCounter code for event-listener-firing. BUG=629601 ========== to ========== Refactored UseCounter code for event-listener-firing. BUG=629601 Committed: https://crrev.com/840adcf880f0246881f171ede42a3f0930743546 Cr-Commit-Position: refs/heads/master@{#410208} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/840adcf880f0246881f171ede42a3f0930743546 Cr-Commit-Position: refs/heads/master@{#410208} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
