Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(41)

Issue 1241613004: Rework dispatchEvent so it is consistent for isTrusted support. (Closed)

Created:
5 years, 5 months ago by dtapuska
Modified:
5 years, 5 months ago
Reviewers:
tkent, grt (UTC plus 2)
CC:
blink-reviews, shans, tzik, eae+blinkwatch, fs, eric.carlson_apple.com, dgrogan, rwlbuis, jsbell+serviceworker_chromium.org, blink-reviews-html_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, Eric Willigers, nessy, rjwright, sof, nhiroki, darktears, jsbell+idb_chromium.org, vcarbune.chromium, philipj_slow, michaeln, blink-reviews-animation_chromium.org, serviceworker-reviews, gasubic, falken, kinuko+serviceworker, cmumford, horo+watch_chromium.org, peter+watch_chromium.org, Rick Byers
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Rework dispatchEvent so it is consistent for isTrusted support. Make dispatchEvent a non-virtual method on EventTarget; make the current dispatchEvent a protected method and rename it to dispatchEventInternal. The goal of this is that there are two main entry points to dispatching events. 1) dispatchEventForBindings (from JS) 2) dispatchEvent() (from C++) The trusted event code will take advantage of this to assign the trusted value in these implementations; so this change will make more sense in the context of that change. This CL has no behavior changes. BUG=334015 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198996

Patch Set 1 #

Total comments: 4

Patch Set 2 : Work around MSVC optimization bug #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -37 lines) Patch
M Source/core/animation/Animation.h View 2 chunks +3 lines, -1 line 0 comments Download
M Source/core/animation/Animation.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/dom/Node.h View 1 2 chunks +2 lines, -3 lines 0 comments Download
M Source/core/dom/Node.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/events/EventTarget.h View 2 chunks +4 lines, -1 line 0 comments Download
M Source/core/events/EventTarget.cpp View 1 chunk +6 lines, -1 line 0 comments Download
M Source/core/html/track/TextTrackCue.h View 2 chunks +1 line, -3 lines 1 comment Download
M Source/core/html/track/TextTrackCue.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/workers/WorkerEventQueue.cpp View 1 1 chunk +6 lines, -1 line 1 comment Download
M Source/modules/indexeddb/IDBDatabase.h View 1 2 chunks +4 lines, -3 lines 0 comments Download
M Source/modules/indexeddb/IDBDatabase.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/modules/indexeddb/IDBOpenDBRequest.h View 1 2 chunks +3 lines, -1 line 0 comments Download
M Source/modules/indexeddb/IDBOpenDBRequest.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/modules/indexeddb/IDBRequest.h View 1 2 chunks +3 lines, -3 lines 0 comments Download
M Source/modules/indexeddb/IDBRequest.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/modules/indexeddb/IDBTransaction.h View 1 1 chunk +4 lines, -3 lines 0 comments Download
M Source/modules/indexeddb/IDBTransaction.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/modules/notifications/Notification.h View 1 2 chunks +4 lines, -1 line 0 comments Download
M Source/modules/notifications/Notification.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/modules/serviceworkers/ServiceWorkerGlobalScope.h View 1 2 chunks +4 lines, -1 line 0 comments Download
M Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp View 1 chunk +2 lines, -2 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 29 (9 generated)
dtapuska
5 years, 5 months ago (2015-07-14 14:19:19 UTC) #2
dtapuska
On 2015/07/14 14:19:19, Dave Tapuska wrote: I'm investigating windows build bot failures.
5 years, 5 months ago (2015-07-14 21:39:09 UTC) #3
tkent
lgtm. If the fix for Windows bot failures is trivial, please go ahead landing.
5 years, 5 months ago (2015-07-14 23:57:01 UTC) #4
tkent
https://codereview.chromium.org/1241613004/diff/1/Source/core/dom/Node.h File Source/core/dom/Node.h (right): https://codereview.chromium.org/1241613004/diff/1/Source/core/dom/Node.h#newcode628 Source/core/dom/Node.h:628: void dispatchEventDispatchMediator(PassRefPtrWillBeRawPtr<EventDispatchMediator>); This looks unnecessary. https://codereview.chromium.org/1241613004/diff/1/Source/modules/indexeddb/IDBDatabase.h File Source/modules/indexeddb/IDBDatabase.h (right): ...
5 years, 5 months ago (2015-07-15 00:00:54 UTC) #5
dtapuska
On 2015/07/15 00:00:54, tkent wrote: > https://codereview.chromium.org/1241613004/diff/1/Source/core/dom/Node.h > File Source/core/dom/Node.h (right): > > https://codereview.chromium.org/1241613004/diff/1/Source/core/dom/Node.h#newcode628 > ...
5 years, 5 months ago (2015-07-15 19:33:41 UTC) #6
dtapuska
https://codereview.chromium.org/1241613004/diff/1/Source/core/dom/Node.h File Source/core/dom/Node.h (right): https://codereview.chromium.org/1241613004/diff/1/Source/core/dom/Node.h#newcode628 Source/core/dom/Node.h:628: void dispatchEventDispatchMediator(PassRefPtrWillBeRawPtr<EventDispatchMediator>); On 2015/07/15 00:00:54, tkent wrote: > This ...
5 years, 5 months ago (2015-07-15 22:34:42 UTC) #7
tkent
lgtm https://codereview.chromium.org/1241613004/diff/20001/Source/core/workers/WorkerEventQueue.cpp File Source/core/workers/WorkerEventQueue.cpp (right): https://codereview.chromium.org/1241613004/diff/20001/Source/core/workers/WorkerEventQueue.cpp#newcode80 Source/core/workers/WorkerEventQueue.cpp:80: // before the target is queried. Wow, nice ...
5 years, 5 months ago (2015-07-16 00:22:03 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1241613004/20001
5 years, 5 months ago (2015-07-16 00:22:45 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/builds/41509)
5 years, 5 months ago (2015-07-16 00:31:11 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1241613004/20001
5 years, 5 months ago (2015-07-16 00:37:49 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/builds/41511)
5 years, 5 months ago (2015-07-16 00:45:30 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1241613004/20001
5 years, 5 months ago (2015-07-16 01:05:39 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/builds/41516)
5 years, 5 months ago (2015-07-16 01:13:30 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1241613004/20001
5 years, 5 months ago (2015-07-16 01:36:12 UTC) #22
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198996
5 years, 5 months ago (2015-07-16 01:55:06 UTC) #23
grt (UTC plus 2)
https://codereview.chromium.org/1241613004/diff/20001/Source/core/html/track/TextTrackCue.h File Source/core/html/track/TextTrackCue.h (right): https://codereview.chromium.org/1241613004/diff/20001/Source/core/html/track/TextTrackCue.h#newcode112 Source/core/html/track/TextTrackCue.h:112: bool dispatchEventInternal(PassRefPtrWillBeRawPtr<Event>) override; "override" here is causing a compile ...
5 years, 5 months ago (2015-07-16 13:40:40 UTC) #25
dtapuska
On 2015/07/16 13:40:40, grt wrote: > https://codereview.chromium.org/1241613004/diff/20001/Source/core/html/track/TextTrackCue.h > File Source/core/html/track/TextTrackCue.h (right): > > https://codereview.chromium.org/1241613004/diff/20001/Source/core/html/track/TextTrackCue.h#newcode112 > ...
5 years, 5 months ago (2015-07-16 13:51:27 UTC) #26
grt (UTC plus 2)
On 2015/07/16 13:51:27, Dave Tapuska wrote: > On 2015/07/16 13:40:40, grt wrote: > > > ...
5 years, 5 months ago (2015-07-16 14:15:59 UTC) #27
Daniel Bratell
On 2015/07/16 14:15:59, grt wrote: > On 2015/07/16 13:51:27, Dave Tapuska wrote: > > On ...
5 years, 5 months ago (2015-07-16 14:46:39 UTC) #28
Daniel Bratell
5 years, 5 months ago (2015-07-16 14:46:44 UTC) #29
Message was sent while issue was closed.
On 2015/07/16 14:15:59, grt wrote:
> On 2015/07/16 13:51:27, Dave Tapuska wrote:
> > On 2015/07/16 13:40:40, grt wrote:
> > >
> >
>
https://codereview.chromium.org/1241613004/diff/20001/Source/core/html/track/...
> > > File Source/core/html/track/TextTrackCue.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1241613004/diff/20001/Source/core/html/track/...
> > > Source/core/html/track/TextTrackCue.h:112: bool
> > > dispatchEventInternal(PassRefPtrWillBeRawPtr<Event>) override;
> > > "override" here is causing a compile error on chromium.fyi Chromium
Builder:
> > > 
> > >
> >
>
c:\b\build\slave\chromium-rel-builder\build\src\third_party\webkit\source\core\html\track\texttrackcue.h(112):error
> > > C3668: 'blink::TextTrackCue::dispatchEventInternal': method with override
> > > specifier 'override' did not override any base class methods
> > > 
> > > please fix.
> > 
> > You sure something isn't wrong with this builder? (Caching of precompiled
> > headers?)
> > 
> > There is another failure as well...
> >
>
c:\b\build\slave\chromium-rel-builder\build\src\third_party\webkit\source\core\fetch\scriptresource.h(60):
> > error C3668:
'blink::ScriptResource::destroyDecodedDataForFailedRevalidation':
> > method with override specifier 'override' did not override any base class
> > methods
> > 
> > The error makes no logical sense... TextTrackCue.h derives from
> > EventTargetWithInlineData which derives from EventTarget which has the
virtual
> > method.
> 
> Ah, I see that now. Freaky and sad. Thanks for taking a look.

Is that builder not using goma? Goma would have disabled precompiled headers
completely.

Powered by Google App Engine
This is Rietveld 408576698