Issue 849783002: Event.path should include Window

Issue 849783002: Event.path should include Window (Closed)

Created:
5 years, 11 months ago by kojii

Modified:
5 years, 11 months ago

Reviewers:
hayato, brucedawson

CC:
blink-reviews, arv+blink, Inactive

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Event.path should include Window It was pointed out at W3C bug 21066[1] that Event.path API should return an array of EventTarget, not NodeList, and also it should include Window object except for the 'load' event as described in the HTML spec. LayoutTests/fast/dom/shadow/event-path.html was changed to a script test to ensure not only types but also instances are correct. Exiting test expectations are updated to include the Window object, and added one test to test that the 'load' event does not include the Window object. One imported/web-platform-tests fails due to this spec change and marked as Failure, until the PR[2] is merged and imported. [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=21066 [2] https://github.com/w3c/web-platform-tests/pull/1524 BUG=442632 TEST=fast/dom/shadow/event-path-load.html, fast/dom/shadow/event-path-window-load.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188594

Patch Set 1 #

Patch Set 2 : WIP: tests updated, but Window is still wrong #

Patch Set 3 : Test fixed and cleanup #

Patch Set 4 : IDL sequence<T> to T[] #

Total comments: 7

Patch Set 5 : rebase-update #

Patch Set 6 : comments fixed #

Total comments: 6

Patch Set 7 : fix comments and window.onload #

Patch Set 8 : test expected update for http/tests/dom/crash-on-querying-event-path.html #

Total comments: 1

Created: 5 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+187 lines, -143 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path.html	View	1 2	2 chunks	+26 lines, -18 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path-empty-shadow-element-expected.txt	View		1 chunk	+11 lines, -11 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path-expected.txt	View	1 2	1 chunk	+0 lines, -11 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path-for-user-agent-shadow-tree-expected.txt	View		1 chunk	+7 lines, -7 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path-in-shadow-tree-expected.txt	View		1 chunk	+13 lines, -13 lines	0 comments	Download
A	LayoutTests/fast/dom/shadow/event-path-load.html	View	1 2	1 chunk	+20 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path-multiple-shadow-roots-2-expected.txt	View		1 chunk	+17 lines, -17 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path-multiple-shadow-roots-expected.txt	View		1 chunk	+18 lines, -18 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path-shadow-insertion-point-in-oldest-shadow-root-expected.txt	View		1 chunk	+9 lines, -9 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path-svg-expected.txt	View		1 chunk	+2 lines, -2 lines	0 comments	Download
A	LayoutTests/fast/dom/shadow/event-path-window-load.html	View	1 2 3 4 5 6	1 chunk	+16 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/event-path-with-dom-mutation-expected.txt	View		1 chunk	+13 lines, -13 lines	0 comments	Download
M	LayoutTests/fast/xmlhttprequest/xmlhttprequest-get-expected.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/http/tests/dom/crash-on-querying-event-path-expected.txt	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/events/Event.h	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/events/Event.cpp	View	1 2 3 4 5 6 7	1 chunk	+18 lines, -10 lines	0 comments	Download
M	Source/core/events/Event.idl	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/events/TreeScopeEventContext.h	View	1 2 3 4 5	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/events/TreeScopeEventContext.cpp	View	1 2 3 4 5 6	1 chunk	+10 lines, -8 lines	1 comment	Download

Messages

Total messages: 16 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

hayato

https://codereview.chromium.org/849783002/diff/60001/LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html File LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html (right): https://codereview.chromium.org/849783002/diff/60001/LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html#newcode1 LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html:1: <!DOCTYPE html> I think we don't have to update ...

5 years, 11 months ago (2015-01-15 04:15:28 UTC) #3

kojii

All fixed, PTAL! https://codereview.chromium.org/849783002/diff/60001/LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html File LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html (right): https://codereview.chromium.org/849783002/diff/60001/LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html#newcode1 LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html:1: <!DOCTYPE html> On 2015/01/15 04:15:28, hayato ...

5 years, 11 months ago (2015-01-15 06:56:44 UTC) #4

All fixed, PTAL!

https://codereview.chromium.org/849783002/diff/60001/LayoutTests/imported/web...
File
LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html
(right):

https://codereview.chromium.org/849783002/diff/60001/LayoutTests/imported/web...
LayoutTests/imported/web-platform-tests/shadow-dom/elements-and-dom-objects/extensions-to-event-interface/event-path-001.html:1:
<!DOCTYPE html>
On 2015/01/15 04:15:28, hayato wrote:
> I think we don't have to update the imported W3C test. I've never done that as
a
> part of blink's CL.
> Rather, I guess we have to treat imported tests as *read-only*.
> Could you confirm that we need to update this file or not?

The doc says the import process will overwrite, but the import is manual, and
the last run was more than a year ago.

Found that several imported tests are already marked as Failure, so agree,
changed to mark as Failure and not to take the PR in advance.

https://codereview.chromium.org/849783002/diff/60001/Source/core/events/TreeS...
File Source/core/events/TreeScopeEventContext.cpp (right):

https://codereview.chromium.org/849783002/diff/60001/Source/core/events/TreeS...
Source/core/events/TreeScopeEventContext.cpp:42: LocalDOMWindow* window =
path.windowEventContext().window();
On 2015/01/15 04:15:28, hayato wrote:
> Could you make sure that calling path.windowEventContext() is safe or not
here?
> I'm afraid that there is a case where path.windowEventContext() hits ASSERTION
> if we don't call path.ensureWindowEventContext() here.

Done.

https://codereview.chromium.org/849783002/diff/60001/Source/core/events/TreeS...
File Source/core/events/TreeScopeEventContext.h (right):

https://codereview.chromium.org/849783002/diff/60001/Source/core/events/TreeS...
Source/core/events/TreeScopeEventContext.h:65:
Vector<RefPtrWillBeRawPtr<EventTarget>>& ensureEventPath(EventPath&);
On 2015/01/15 04:15:28, hayato wrote:
> WillBeHeapVector?

Done.

hayato

https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Event.cpp File Source/core/events/Event.cpp (right): https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Event.cpp#newcode239 Source/core/events/Event.cpp:239: Vector<RefPtrWillBeRawPtr<EventTarget>> Event::path() const WillBeHeapVector? https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Event.h File Source/core/events/Event.h (right): https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Event.h#newcode182 ...

5 years, 11 months ago (2015-01-15 08:27:40 UTC) #5

kojii

On 2015/01/15 08:27:40, hayato wrote: > https://codereview.chromium.org/849783002/diff/100001/Source/core/events/TreeScopeEventContext.cpp#newcode42 > Source/core/events/TreeScopeEventContext.cpp:42: LocalDOMWindow* window = > path.hasWindowEventContext() ? ...

5 years, 11 months ago (2015-01-16 00:06:21 UTC) #6

hayato

On 2015/01/16 00:06:21, koji wrote: > On 2015/01/15 08:27:40, hayato wrote: > > > https://codereview.chromium.org/849783002/diff/100001/Source/core/events/TreeScopeEventContext.cpp#newcode42 ...

5 years, 11 months ago (2015-01-16 03:32:59 UTC) #7

kojii

All fixed, along with returning [window] for events that are dispatched directly to the window ...

5 years, 11 months ago (2015-01-16 08:37:23 UTC) #8

All fixed, along with returning [window] for events that are dispatched directly
to the window object. PTAL!

https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Even...
File Source/core/events/Event.cpp (right):

https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Even...
Source/core/events/Event.cpp:239: Vector<RefPtrWillBeRawPtr<EventTarget>>
Event::path() const
On 2015/01/15 08:27:39, hayato wrote:
> WillBeHeapVector?

Done.

https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Event.h
File Source/core/events/Event.h (right):

https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Even...
Source/core/events/Event.h:182: Vector<RefPtrWillBeRawPtr<EventTarget>> path()
const;
On 2015/01/15 08:27:39, hayato wrote:
> WillBeHeapVector?

Done.

https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Tree...
File Source/core/events/TreeScopeEventContext.cpp (right):

https://codereview.chromium.org/849783002/diff/100001/Source/core/events/Tree...
Source/core/events/TreeScopeEventContext.cpp:42: LocalDOMWindow* window =
path.hasWindowEventContext() ? path.windowEventContext().window() : nullptr;
On 2015/01/15 08:27:39, hayato wrote:
> I've taken a look at Event::path(). I think Event::path() never calls
> TreeScopeEventContext::ensureEventPath() when path.m_WindowEventContext is not
> set because it calls TreeScopeEventContext::ensureEventPath() only when an
event
> dispatch is finished (or while being dispatched).
> 
> I guess path.windowEventContext() is guaranteed to be set here. You can revert
> EventPath::hasWindowEventContext() if you can agree.

Reverted. I thought |LocalDOMWindow::dispatchLoadEvent()| will hit this, but
you're right that in that case |Event::path()| returns without calling this
function.

> You could also move this line into line 52. That's preferred.
> 
> e.g. 
> if (LocalDOMWindow* window = ....)
>   m_eventPath->append(window);

I need |window| in line 44 to determine the capacity. Swapped 42 and 43 to  make
it more obvious.

hayato

lgtm. Nit: > TEST=fast\dom\shadow\event-path-load.html We usually use slash, instead of back-slash, there.

5 years, 11 months ago (2015-01-19 04:54:33 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/849783002/140001

5 years, 11 months ago (2015-01-19 06:13:13 UTC) #11

commit-bot: I haz the power

Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188594

5 years, 11 months ago (2015-01-19 06:16:50 UTC) #12

brucedawson

https://codereview.chromium.org/849783002/diff/140001/Source/core/events/TreeScopeEventContext.cpp File Source/core/events/TreeScopeEventContext.cpp (right): https://codereview.chromium.org/849783002/diff/140001/Source/core/events/TreeScopeEventContext.cpp#newcode44 Source/core/events/TreeScopeEventContext.cpp:44: m_eventPath->reserveCapacity(path.size() + window ? 1 : 0); This does ...

5 years, 11 months ago (2015-01-21 19:20:48 UTC) #14

kojii

On 2015/01/21 19:20:48, brucedawson wrote: > https://codereview.chromium.org/849783002/diff/140001/Source/core/events/TreeScopeEventContext.cpp > File Source/core/events/TreeScopeEventContext.cpp (right): > > https://codereview.chromium.org/849783002/diff/140001/Source/core/events/TreeScopeEventContext.cpp#newcode44 > ...

5 years, 11 months ago (2015-01-22 00:57:25 UTC) #15

kojii

5 years, 11 months ago (2015-01-22 03:45:13 UTC) #16

Message was sent while issue was closed.

On 2015/01/22 00:57:25, koji wrote:
> On 2015/01/21 19:20:48, brucedawson wrote:
> >
>
https://codereview.chromium.org/849783002/diff/140001/Source/core/events/Tree...
> > File Source/core/events/TreeScopeEventContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/849783002/diff/140001/Source/core/events/Tree...
> > Source/core/events/TreeScopeEventContext.cpp:44:
> > m_eventPath->reserveCapacity(path.size() + window ? 1 : 0);
> > This does not do what you think it does.
> > 
> > The intent is, presumably, to reserve space for path.size() + 1/0 entries.
> What
> > this *actually* does is reserve space for 1/0 entries. This is a precedence
> > performance problem. What you want is:
> > 
> > m_eventPath->reserveCapacity(path.size() + (window ? 1 : 0));
> 
> Greatly appreciated for pointing this out, I feel ashamed. I'll work on fix.

CL is on its way: https://codereview.chromium.org/865753002/

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages