Closed Bug 813445 Opened 12 years ago Closed 12 years ago

Sort out around flags of nsGUIEvent.h

Categories

(Core :: Widget, defect)

Product:
Core
Component:
Widget
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(14 files, 27 obsolete files)

part.1 Make widget::EventFlags and remove NS_EVENT_FLAG_TRUSTED (r=roc+smaug, feedback=emk)
54.03 KB, patch
Details | Diff | Splinter Review
part.2 Don't use NS_EVENT_FLAG_BUBBLE and NS_EVENT_FLAG_CAPTURE for nsEvent::flags (r=smaug)
7.03 KB, patch
Details | Diff | Splinter Review
part.3 Remove NS_EVENT_FLAG_CANT_CANCEL and NS_EVENT_FLAG_CANT_BUBBLE (r=smaug)
22.43 KB, patch
Details | Diff | Splinter Review
part.4 Remove NS_EVENT_FLAG_STOP_DISPATCH and NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY (r=smaug)
17.09 KB, patch
Details | Diff | Splinter Review
part.9 Remove NS_EVENT_FLAG_EXCEPTION_THROWN (r=smaug)
4.14 KB, patch
Details | Diff | Splinter Review
part.5 Remove NS_EVENT_FLAG_NO_DEFAULT and NS_EVENT_FLAG_NO_DEFAULT_CALLED_IN_CONTENT (r=smaug)
26.41 KB, patch
Details | Diff | Splinter Review
part.6 Remove NS_EVENT_FLAG_PREVENT_MULTIPLE_ACTIONS (r=smaug)
12.73 KB, patch
Details | Diff | Splinter Review
part.7 Remove NS_EVENT_DISPATCHED and NS_EVENT_FLAG_DISPATCHING (r=smaug)
8.34 KB, patch
Details | Diff | Splinter Review
part.8 Remove NS_EVENT_FLAG_SYNTHETIC_TEST_EVENT (r=smaug)
8.14 KB, patch
Details | Diff | Splinter Review
part.10 Remove NS_EVENT_FLAG_DONT_FORWARD_CROSS_PROCESS and NS_EVENT_RETARGET_TO_NON_NATIVE_ANONYMOUS (r=smaug)
5.81 KB, patch
Details | Diff | Splinter Review
part.11 Don't use NS_EVENT_FLAG_NO_CONTENT_DISPATCH for nsEvent::flags
11.27 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.12 Remove NS_EVENT_FLAG_ONLY_CHROME_DISPATCH and nsEvent::flags (r=smaug)
12.32 KB, patch
Details | Diff | Splinter Review
part.13 nsEventDispatcher shouldn't use the flags defined in nsGUIEvent.h (r=smaug)
18.09 KB, patch
Details | Diff | Splinter Review
part.14 Make dom::EventListenerFlags for nsEventListenerManager
56.49 KB, patch
smaug
: review+
Details | Diff | Splinter Review
I think that all flags in nsGUIEvent.h should be explained by comment. And nsEvent should have methods for checking or setting the flags since comparing flags directly may make developers take mistake.
I researched all flags. So, the comment might be wrong if I misunderstood.
Attachment #683466 - Flags: review?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #0) > And nsEvent should have methods for checking or setting the flags since > comparing flags directly may make developers take mistake. Consider using bit fields.
(In reply to Masatoshi Kimura [:emk] from comment #2) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #0) > > And nsEvent should have methods for checking or setting the flags since > > comparing flags directly may make developers take mistake. > Consider using bit fields. Sorry? I'm thinking something like these: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.h#668
Ah, did you mean that we should use |uint32_t foo : 1;|? Hmm, doesn't it make nsGUIEventIPC.h slow?
> Hmm, doesn't it make nsGUIEventIPC.h slow? I guess that WriteParm() and ReadParam() are needed to call every bit field.
Comment on attachment 683466 [details] [diff] [review] part.1 Explain the flags for events in nsGUIEvent.h > // These flags are sort of a mess. They're sort of shared between event > // listener flags and event flags, but only some of them. You've been > // warned! > #define NS_EVENT_FLAG_NONE 0x0000 >+ >+// If NS_EVENT_FLAG_TRUSTED is included in nsEvent::flags, the event is a >+// trusted event. Otherwise, it's an untrested event. > #define NS_EVENT_FLAG_TRUSTED 0x0001 Maybe s/is included in/flag is set in/ Same also elsewhere. >+// If NS_EVENT_FLAG_CANT_CANCEL is included in nsEvent::flags, the event is >+// not cancelable. I.e., nsDOMEvent::PreventDefault() won't work. Perhaps s/won't work/won't do anything/ >+// If NS_EVENT_FLAG_NO_CONTENT_DISPATCH is included in nsEvent::flags, >+// the event is never handled by event handler attributes in content. >+// But it can be handled by other event listeners such as added with >+// addEventListener(). Odd comment. NS_EVENT_FLAG_NO_CONTENT_DISPATCH has nothing to do with addEventListener() It is about handling events in content, Could you also add comment that we have unfortunately special cases for this http://mxr.mozilla.org/mozilla-central/source/content/events/public/nsEventDispatcher.h?rev=08187a7ea897&mark=127-132#126 >+// If NS_EVENT_FLAG_SYSTEM_EVENT is included in nsListenerStruct::mFlags, the >+// event listener is listening to the event in system group. listenening for the events in the system group Could re-read after those changes, so r-
Attachment #683466 - Flags: review?(bugs) → review-
Hmm, I still think that using bit fields is very elegant, but it makes additional cost in IPC. If we use bit field, developers who want to add new flags also need to change nsGUIEventIPC.h. I don't think it doesn't make sense.
I wonder if you could do something like union { uint32_t mFlags; struct { uint32_t mSomeFlag : 1; uint32_t mSomeOtherFlag : 1; ... } } IPC code would use mFlags.
Ah, union. It makes the convert safe.
I don't think we should try to optimize IPC this way. I don't see any reason to believe this cost is an issue. Also, use bool mFlag : 1 instead of uint32_t.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > I don't think we should try to optimize IPC this way. I don't see any reason > to believe this cost is an issue. Hmm... Okay.
nsEvent doesn't using m[A-Z][a-z]* for the members. So, I think that we should name the new bit fields flags like isTrusted, isBubblingPhase. Do you agree with that?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > > I don't think we should try to optimize IPC this way. I don't see any reason > > to believe this cost is an issue. > > Hmm... Okay. Er, does the ReadParam and WriteParam support bit field?
Hmm, I got it: m:\mc-a\fx-dbg\dist\include\IPC/nsGUIEventIPC.h(38) : error C2104: '&' on bit field ignored
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13) > nsEvent doesn't using m[A-Z][a-z]* for the members. So, I think that we > should name the new bit fields flags like isTrusted, isBubblingPhase. Do you > agree with that? No, I think the new code should follow our naming conventions. nsEvent is currently wrong. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15) > m:\mc-a\fx-dbg\dist\include\IPC/nsGUIEventIPC.h(38) : error C2104: '&' on > bit field ignored I think you can work around it by using a temporary bool when you read and write. But I guess this is a good reason that doing the union thing in comment #8 would be reasonable.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13) > > nsEvent doesn't using m[A-Z][a-z]* for the members. So, I think that we > > should name the new bit fields flags like isTrusted, isBubblingPhase. Do you > > agree with that? > > No, I think the new code should follow our naming conventions. nsEvent is > currently wrong. Okay. > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15) > > m:\mc-a\fx-dbg\dist\include\IPC/nsGUIEventIPC.h(38) : error C2104: '&' on > > bit field ignored > > I think you can work around it by using a temporary bool when you read and > write. > > But I guess this is a good reason that doing the union thing in comment #8 > would be reasonable. Yeah, I'm trying like this now: union { uint32_t rawFlags; struct { bool isTrusted : 1; ... }; };
GCC doesn't allow anonymous struct. So, we need to define the flags as nsEvent::mFlags::mIsFoo. Do you like this?
Attachment #683907 - Flags: review?(roc)
Comment on attachment 683907 [details] [diff] [review] part.2 Remove NS_EVENT_FLAG_TRUSTED Review of attachment 683907 [details] [diff] [review]: ----------------------------------------------------------------- Why are you converting just this one flag? If you're just converting one flag, you can make it a simple 'bool'
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19) > Why are you converting just this one flag? If you're just converting one > flag, you can make it a simple 'bool' I'll do for all flags if you grant for this. The flags are checked in a lot of places, so that I want your agreement, first. # Of course, I don't land the patches separately.
Thank you. I'll post patches for each flag since it makes reviews easy.
Comment on attachment 683864 [details] [diff] [review] part.1 Explain the flags for events in nsGUIEvent.h Hmm, well, if we get rid of using these flags, this patch won't be needed. Documentation should go to the struct.
Attachment #683864 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #22) > Comment on attachment 683864 [details] [diff] [review] > part.1 Explain the flags for events in nsGUIEvent.h > > Hmm, well, if we get rid of using these flags, this patch won't be needed. > Documentation should go to the struct. Ah, right. I'll request feedback for each patch later. Thank you.
If roc's review queue is long, I could review the patches.
This really screams for an accessor method
(In reply to :Ms2ger from comment #25) > This really screams for an accessor method Like comment #3? :)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18) > Created attachment 683907 [details] [diff] [review] > part.2 Remove NS_EVENT_FLAG_TRUSTED > > GCC doesn't allow anonymous struct. So, we need to define the flags as > nsEvent::mFlags::mIsFoo. Do you like this? Then the union hack will no longer be required because you can get an address of mFlags.
https://hg.mozilla.org/try/rev/1db8636f0395 It would be better to add template<> struct ParamTraits<mozilla::widget::EventFlags> than using a temporary local variable.
Thank you, Kimura-san. If you find some problems of the attached patches, let me know.
Attachment #683864 - Attachment is obsolete: true
Attachment #683907 - Attachment is obsolete: true
Attachment #686957 - Flags: review?(bugs)
The flag meaning is changed (true vs. false). But the new names are similar to DOM event's attribute names. So, this must be better.
Attachment #686961 - Flags: review?(bugs)
Attachment #686970 - Attachment description: Don't use NS_EVENT_FLAG_NO_CONTENT_DISPATCH for nsEvent::flags → part.11 Don't use NS_EVENT_FLAG_NO_CONTENT_DISPATCH for nsEvent::flags
This sorts out the second usage of the flags. nsEventDispatcher and nsEventListenerManager::HandleEvent*() uses the flags for checking the phase and group. The dispatching phase is already in nsEvent::mFlags which are set by nsEventTargetChainItem::HandleEventTargetChain(). So, if nsEvent::mFlags has if the event is being dispatch in system group, these methods don't need the additional flags. So, this adds a bit field to the widget::EventFlags, this is simpler and the dispatching state isn't managed by two places (nsEvent::mFlags and aFlags of the methods). And nsEventListenerManager::HandleEventInternal() should not be a public method :-)
Attachment #686973 - Flags: review?(bugs)
The last two usage of the flags are: 1. using flags of nsEventListenerManager::AddEventListener*() and nsEventListenerManager::RemoveEventListener*(). 2. using flags in nsListenerStruct. And they can share the same struct which has bit fields. I named it as dom::EventListenerFlags because nsEventListenerManager is too long prefix for callers. And protected nsEventListenerManager::AddEventListener() and nsEventListenerManager::RemoveEventListener() should not overload the public methods. I changed them too for making the change of this patch clear.
Attachment #686976 - Flags: review?(bugs)
Basically, I don't change any logic except here: https://bugzilla.mozilla.org/attachment.cgi?id=686961&action=diff#a/widget/nsGUIEvent.h_sec6 > flags |= (NS_EVENT_FLAG_CANT_CANCEL & NS_EVENT_FLAG_CANT_BUBBLE); This must be wrong. Should be: > (NS_EVENT_FLAG_CANT_CANCEL | NS_EVENT_FLAG_CANT_BUBBLE) ^^^
About part.14: I create a lot of inline methods for making dom::EventListenerFlags instances for the argument of the methods. This is easier to read the meaning than EventListenerFlags(true, true, false).
Comment on attachment 686957 [details] [diff] [review] part.1 Make widget::EventFlags and remove NS_EVENT_FLAG_TRUSTED + static void Write(Message* aMsg, const paramType& aParam) + { + uint32_t rawFlags = aParam.GetRawFlags(); + WriteParam(aMsg, rawFlags); + } + + static bool Read(const Message* aMsg, void** aIter, paramType* aResult) + { + uint32_t rawFlags = 0; + bool ret = ReadParam(aMsg, aIter, &rawFlags); + aResult->SetRawFlags(rawFlags); + return ret; + } +}; Use aMsg->ReadBytes and aMsg->WriteBytes. Then SetRawFlags and GetRawFlags will not be needed here.
Attachment #686957 - Flags: feedback+
Comment on attachment 686957 [details] [diff] [review] part.1 Make widget::EventFlags and remove NS_EVENT_FLAG_TRUSTED >+namespace mozilla { >+namespace widget { >+struct EventFlags >+{ >+ friend struct IPC::ParamTraits<EventFlags>; >+public: >+ // If mIsTrusted is true, the event is a trusted event. Otherwise, it's >+ // an untrested event. untrested?
Attachment #686957 - Flags: review?(bugs) → review+
Though, it is not clear to me why we need the memcpy stuff, and why union approach wouldn't work union { EventFlags mFlags; uin32_t mRawFlagData; };
...but I'll review the rest of the patches, since there can be a followup patch to make EventFlags simpler if that union approach works.
Comment on attachment 686959 [details] [diff] [review] part.2 Don't use NS_EVENT_FLAG_BUBBLE and NS_EVENT_FLAG_CAPTURE for nsEvent::flags This is not very pretty, because of the event retargeting in EventDispatcher. Not your fault. > struct EventFlags > { > friend struct IPC::ParamTraits<EventFlags>; > public: > // If mIsTrusted is true, the event is a trusted event. Otherwise, it's > // an untrested event. > bool mIsTrusted : 1; >+ // If mInBubblingPhase is true, the event is in bubbling phase or target >+ // pahse. pahse?
Attachment #686959 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #48) > Though, it is not clear to me why we need the memcpy stuff, and why union > approach wouldn't work > > union { > EventFlags mFlags; > uin32_t mRawFlagData; > }; I tried writing the patches using the raw flags. But my conclusion is that union hack isn't necessary. First of all, Windows's nsWindow::OnChar() and Element::DispatchClickEvent() take some event flags with their argument and they will OR the flags to dispatching event. Therefore I think that the struct should be named. I don't like to use raw flags for them instead of named stuct because if we do so, the callers need to make widget::EventFlags in its stack for setting each flag and cast it to uint32_t. I feel such code is ugly. And the named struct can avoid the union hack for ParamTraits. Second, if the size will become not enough to uint32_t in the future, such code may be missed at that time. So, I believe that the specific type widget::EventFlags helps maintaining the code. Finally, the hack shouldn't be used by other purpose. Therefore, I hide the raw flag related methods (into private). Current patch succeed capsuling the raw flags except ParamTrants. So, the flags users just set/refer as bool variants. This makes very simple access/code.
Comment on attachment 686961 [details] [diff] [review] part.3 Remove NS_EVENT_FLAG_CANT_CANCEL and NS_EVENT_FLAG_CANT_BUBBLE >@@ -520,16 +518,22 @@ public: > // If mIsTrusted is true, the event is a trusted event. Otherwise, it's > // an untrested event. > bool mIsTrusted : 1; > // If mInBubblingPhase is true, the event is in bubbling phase or target > // pahse. > bool mInBubblingPhase : 1; > // If mInCapturePhase is true, the event is in capture phase or target phase. > bool mInCapturePhase : 1; >+ // If mCancelable is true, the evnet can be consumed. I.e., calling event >@@ -580,16 +584,18 @@ protected: > refPoint(0, 0), > lastRefPoint(0, 0), > time(0), > flags(NS_EVENT_FLAG_NONE), > userType(0) > { > MOZ_COUNT_CTOR(nsEvent); > mFlags.mIsTrusted = isTrusted; >+ mFlags.mCancelable = true; >+ mFlags.mBubbles = true; Hmm, could we initialize mFlags member variables to their default values in the struct's ctor. Could be done in a followup patch in this bug. Also, I wonder if initializing everything one by one will be slower than the current flag based setup. Perhaps worth to profile event creation without any of these patches and then with all the patches.
Attachment #686961 - Flags: review?(bugs) → review+
Comment on attachment 686963 [details] [diff] [review] part.4 Remove NS_EVENT_FLAG_STOP_DISPATCH and NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY > bool mBubbles : 1; >+ // If mPropagationStopped is true, nsDOMEvent::StopPropagation() or >+ // nsDOMEvent::StopImmediatePropagation() has already been called. s/already// >+ bool mPropagationStopped : 1; >+ // If mRemaningCandidateListenersCanceled is true, >+ // nsDOMEvent::StopImmediatePropagation() has already been called. s/already// >+ // Note that mPropagationStopped must be true when this is true. >+ bool mRemaningCandidateListenersCanceled : 1; This is odd name. Use the same naming what is used in the spec. mImmediatePropagationStopped.
Attachment #686963 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #56) > >+ mFlags.mCancelable = true; > >+ mFlags.mBubbles = true; > Hmm, could we initialize mFlags member variables > to their default values in the struct's ctor. > Could be done in a followup patch in this bug. I was thinking so too. However, see https://bugzilla.mozilla.org/attachment.cgi?id=686964&action=diff#a/widget/windows/nsWindow.cpp_sec1 and https://bugzilla.mozilla.org/attachment.cgi?id=686965&action=diff#a/content/base/src/Element.cpp_sec1 When the EventFlags is used for additional flags for dispatching event, the caller of dispatcher needs to set them false. > Also, I wonder if initializing everything one by one will be slower than > the current flag based setup. Perhaps worth to profile event creation > without any of these > patches and then with all the patches. The main reason not initializing each bit is, when somebody adds new flags, they don't need to maintain the constructor.
And I guess that the initializing speed is optimized by compilers.
I don't trust compilers :) And profiling is easy.
Comment on attachment 686964 [details] [diff] [review] part.5 Remove NS_EVENT_FLAG_NO_DEFAULT and NS_EVENT_FLAG_NO_DEFAULT_CALLED_IN_CONTENT >+++ b/widget/android/nsWindow.cpp >@@ -1667,19 +1667,18 @@ nsWindow::OnKeyEvent(AndroidGeckoEvent * > > if (Destroyed()) > return; > if (!firePress) > return; > > nsKeyEvent pressEvent(true, NS_KEY_PRESS, this); > InitKeyEvent(pressEvent, *ae, &pluginEvent); >- if (status == nsEventStatus_eConsumeNoDefault) { >- pressEvent.flags |= NS_EVENT_FLAG_NO_DEFAULT; >- } >+ pressEvent.mFlags.mDefaultPrevented = >+ (status == nsEventStatus_eConsumeNoDefault); Nit, for some bizarre reason this code uses 4 space indentation >+ // If mDefaultPrevented is true, the event has already been consumed. >+ // E.g., nsDOMEvent::PreventDefault() has already been called or >+ // the default action has already been performed. >+ bool mDefaultPrevented : 1; >+ // If mDefaultPreventedByContent is true, the event has already been >+ // consumed by content. >+ // Note that mDefaultPrevented must be true when this is true. >+ bool mDefaultPreventedByContent : 1; s/already// Hmm, I need to go through this again to check if those cases when if (foobar) { event.flags |= SOME_FLAG } is replaced with event.mFlags.mSomeFlag = foobar; ends up overriding the old value of event.mFlags.mSomeFlag when foobar isn't set.
Hmm, do you worry about that the patch's initializer is slower than current flags(NS_EVENT_FLAG_NONE)? Or do you think that each members should be initialized one by one and it might be faster than the patch's approach?
(In reply to Olli Pettay [:smaug] from comment #61) > Hmm, I need to go through this again to check if those cases when > if (foobar) { > event.flags |= SOME_FLAG > } > is replaced with > event.mFlags.mSomeFlag = foobar; > ends up overriding the old value of event.mFlags.mSomeFlag when foobar isn't > set. Ah, yeah. I was careful about the issue. Basically, when I overwrite the flag without if statement, the code is initializing local ns*Event variable. Otherwise, I don't change the if statement. If there is some cases which are not so, they are mistake.
Comment on attachment 686964 [details] [diff] [review] part.5 Remove NS_EVENT_FLAG_NO_DEFAULT and NS_EVENT_FLAG_NO_DEFAULT_CALLED_IN_CONTENT >@@ -1912,19 +1911,17 @@ TextInputHandler::InsertText(NSAttribute > > // Don't set other modifiers from the current event, because here in > // -insertText: they've already been taken into account in creating > // the input string. > > if (currentKeyEvent) { > NSEvent* keyEvent = currentKeyEvent->mKeyEvent; > InitKeyEvent(keyEvent, keypressEvent, &str); >- if (currentKeyEvent->mKeyDownHandled) { >- keypressEvent.flags |= NS_EVENT_FLAG_NO_DEFAULT; >- } >+ keypressEvent.mFlags.mDefaultPrevented = currentKeyEvent->mKeyDownHandled; So all these changes are a bit risky. One needs to be sure that ctor of nsKeyEvent or InitKeyEvent doesn't set keypressEvent.mFlags.mDefaultPrevented to true. Could you just fix this in the simple way if (currentKeyEvent->mKeyDownHandled) { keypressEvent.mFlags.mDefaultPrevented = true; } Same also elsewhere in the patch.
Attachment #686964 - Flags: review?(bugs) → review-
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #62) > Hmm, do you worry about that the patch's initializer is slower than current > flags(NS_EVENT_FLAG_NONE)? This one. it is possibly faster to do flags = foobar | barfoo; than mFlags.foobar = true; mFlags.barfoo = false;
(In reply to Olli Pettay [:smaug] from comment #65) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #62) > > Hmm, do you worry about that the patch's initializer is slower than current > > flags(NS_EVENT_FLAG_NONE)? > This one. > > it is possibly faster to do flags = foobar | barfoo; > than > mFlags.foobar = true; > mFlags.barfoo = false; Yeah, I agree. But how do we test the performance? Even if your concern is true, the difference is probably very small difference. So, it could be hidden by other conditions such as background service of the environment.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #66) > Yeah, I agree. But how do we test the performance? Even if your concern is > true, the difference is probably very small difference. Very true, but since we don't have any data better to just make sure the difference is tiny. Just create millions of events in a loop or something and compare the time. And perhaps run also profiler. I don't expect perf problems, but I'd like to verify it. (In another bug I'm changing addref/release handling, which is way more performance sensitive of course, and there using bool mFoobar : 1; is certainly slower than using flags, at least on gcc 4.7.x)
Comment on attachment 686965 [details] [diff] [review] part.6 Remove NS_EVENT_FLAG_PREVENT_MULTIPLE_ACTIONS >+ // If mMultipleActionsPrevented is true, the event has already caused a >+ // default action without preventing default. However, the default event >+ // handler which set this true wants others don't handle the event for >+ // default action anymore. This is odd comment. At least s/wants others don't handle/wants others to not handle/ But perhaps the whole comment could be re-phrased.
Attachment #686965 - Flags: review?(bugs) → review+
Comment on attachment 686966 [details] [diff] [review] part.7 Remove NS_EVENT_DISPATCHED and NS_EVENT_FLAG_DISPATCHING >+ // If mDispatchedAndCompleted is true, the event has already been dispatched >+ // as a DOM event and the dispatch has already been completed. >+ bool mDispatchedAndCompleted : 1; This is not quite right name. We want to indicate that the event has been dispatched at least once. So, perhaps rename to mDispatchedAtLeastOnce
Attachment #686966 - Flags: review?(bugs) → review+
Attachment #686967 - Flags: review?(bugs) → review+
Attachment #686968 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #67) > Just create millions of events in a loop or something and compare the time. Okay. XPCOM's timestamp is enough for counting the time? http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h
Comment on attachment 686969 [details] [diff] [review] part.10 Remove NS_EVENT_FLAG_DONT_FORWARD_CROSS_PROCESS and NS_EVENT_RETARGET_TO_NON_NATIVE_ANONYMOUS >+ // If mRetargetToNativeAnonymous is true, the event target will be changed >+ // to a non chrome-only or native anonymous event target at being dispatched >+ // to DOM tree. >+ bool mRetargetToNativeAnonymous : 1; This is wrong name. Should be mRetargetToNonNativeAnonymous. Also, event target isn't changed, at least not the original target. It is mainly about retargeting, which in our case changes event target chain. So, fix the comment too. >+ // If mNotAllowedCrossProcessBoundary is true, the event is not allowed to >+ // cross process boundary. >+ bool mNotAllowedCrossProcessBoundary : 1; maybe mNoCrossProcessBoundaryForwarding A bit long, but used very rarely Would like to see those fixed.
Attachment #686969 - Flags: review?(bugs) → review-
Comment on attachment 686970 [details] [diff] [review] part.11 Don't use NS_EVENT_FLAG_NO_CONTENT_DISPATCH for nsEvent::flags mDontDispatchToHandlerAttr is really odd name. event handler means onfoo properties or attributes, but NS_EVENT_FLAG_NO_CONTENT_DISPATCH has nothing to do with those.
Attachment #686970 - Flags: review?(bugs) → review-
Comment on attachment 686971 [details] [diff] [review] part.12 Remove NS_EVENT_FLAG_ONLY_CHROME_DISPATCH and nsEvent::flags >+ // If mDispatchToOnlyChrome is true, the evnet is dispatched to only chrome. event >+ bool mDispatchToOnlyChrome : 1; I'd prefer keeping the old name mOnlyChromeDispatch
Attachment #686971 - Flags: review?(bugs) → review+
Comment on attachment 689032 [details] [diff] [review] part.13 nsEventDispatcher shouldn't use the flags defined in nsGUIEvent.h >+ bool IsListening(const nsEvent* aEvent) const MOZ_ALWAYS_INLINE since this is very performance critical. >+ if (aEvent->mFlags.InTargetPhase()) { >+ // FIXME Should check NS_EVENT_FLAG_CAPTURE because capture phase >+ // event listeners should not be fired. But it breaks at least >+ // <xul:dialog>'s buttons. Bug 235441. >+ return true; >+ } Do we need this if? Shouldn't the return below be enough?
Attachment #689032 - Flags: review?(bugs) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #70) > Okay. XPCOM's timestamp is enough for counting the time? > http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h I was thinking something as simple as data:text/html,<script>var s = (new Date()).getTime(); for (var i = 0; i < 1000000; ++i) new Event("foo"); document.write(((new Date()).getTime() - s) + "ms");</script> Again, I guess perf will be the same, but better to test and profile.
(In reply to Olli Pettay [:smaug] from comment #72) > Comment on attachment 686970 [details] [diff] [review] > part.11 Don't use NS_EVENT_FLAG_NO_CONTENT_DISPATCH for nsEvent::flags > > mDontDispatchToHandlerAttr is really odd name. > event handler means onfoo properties or attributes, > but NS_EVENT_FLAG_NO_CONTENT_DISPATCH has nothing to do with those. Oh? Then, I probably misunderstand the flag. I tested the behavior, then, onclick attribute's script was not called by middle or right click but a handler registered with addEventListener() is called. Therefore, I understand as the comment in the patch. Could you explain the meaning and suggest a better name? (In reply to Olli Pettay [:smaug] from comment #74) > Comment on attachment 689032 [details] [diff] [review] > part.13 nsEventDispatcher shouldn't use the flags defined in nsGUIEvent.h > >+ if (aEvent->mFlags.InTargetPhase()) { > >+ // FIXME Should check NS_EVENT_FLAG_CAPTURE because capture phase > >+ // event listeners should not be fired. But it breaks at least > >+ // <xul:dialog>'s buttons. Bug 235441. > >+ return true; > >+ } > Do we need this if? Shouldn't the return below be enough? Actually, no. It is there only for the FIXME comment. Do you think that it's enough the comment moved to the above of the last return statement? (In reply to Olli Pettay [:smaug] from comment #75) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #70) > > Okay. XPCOM's timestamp is enough for counting the time? > > http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h > I was thinking something as simple as > data:text/html,<script>var s = (new Date()).getTime(); for (var i = 0; i < > 1000000; ++i) new Event("foo"); document.write(((new Date()).getTime() - s) > + "ms");</script> > > Again, I guess perf will be the same, but better to test and profile. Thanks, I'll try it.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #76) > > Oh? Then, I probably misunderstand the flag. I tested the behavior, then, > onclick attribute's script was not called by middle or right click but a > handler registered with addEventListener() is called. Therefore, I > understand as the comment in the patch. Could you explain the meaning and > suggest a better name? How can a listener which is added using addEventListener be called, when the listener is added to some element. (document and window are possibly special cases) As far as I know onclick and addEventListener("click"...) should work the same way in this case - neither one should be called.
NS_EVENT_FLAG_NO_CONTENT_DISPATCH after all affects how event target chain is iterated, not how event gets handled in an event target.
You've r+ed this. But I rewrote the comment. I'd like you to review this again.
Attachment #686965 - Attachment is obsolete: true
Attachment #690039 - Flags: review?(bugs)
Attachment #689033 - Attachment is obsolete: true
Attachment #689033 - Flags: review?(bugs)
Comment on attachment 690038 [details] [diff] [review] part.5 Remove NS_EVENT_FLAG_NO_DEFAULT and NS_EVENT_FLAG_NO_DEFAULT_CALLED_IN_CONTENT >@@ -1966,17 +1966,17 @@ TextInputHandler::DoCommandBySelector(co > currentKeyEvent ? > TrueOrFalse(currentKeyEvent->mCausedOtherKeyEvents) : "N/A")); > > if (currentKeyEvent && !currentKeyEvent->mKeyPressDispatched) { > nsKeyEvent keypressEvent(true, NS_KEY_PRESS, mWidget); > InitKeyEvent(currentKeyEvent->mKeyEvent, keypressEvent); > if (currentKeyEvent->mKeyDownHandled || > currentKeyEvent->mCausedOtherKeyEvents) { >- keypressEvent.flags |= NS_EVENT_FLAG_NO_DEFAULT; >+ keypressEvent.mFlags.mDefaultPrevented = ture; I'm pretty sure you haven't pushed this to try ;) 'ture'
Attachment #690038 - Flags: review?(bugs) → review+
Comment on attachment 690039 [details] [diff] [review] part.6 Remove NS_EVENT_FLAG_PREVENT_MULTIPLE_ACTIONS >+ // If mMultipleActionsPrevented is true, the event has been handled by at >+ // least an default event handler one default handler > and other default actions won't be >+ // performed. For example, when an <a> element is in a <label> element and >+ // the <a> element is clicked, the <a> element's handler may set this true. >+ // Then, the <label> element won't handle the event. So, this prevents two >+ // or more default actions performed by an event. >+ bool mMultipleActionsPrevented : 1; But the comment is a bit wrong. Perhaps something like // mMultipleActionsPrevented may be used when default handling don't want to be prevented, // but only one of the event targets should handle the event. // For example, when a <label> element is in another <label> element and // the first <label> element is clicked, that one may set this true. // Then, the second <label> element won't handle the event.
Attachment #690039 - Flags: review?(bugs) → review+
Attachment #690043 - Flags: review?(bugs) → review+
Comment on attachment 690046 [details] [diff] [review] part.14 Make dom::EventListenerFlags for nsEventListenerManager > // XXX The (mFlags & aFlags) test here seems fragile. Shouldn't we > // specifically only test the capture/bubble flags. This comment could be removed. >+public: >+ // If mCapture is false, it means the listener captures the event. Otherwise, >+ // it's listening at bubbling phase. This comment is wrong. If mCapture is true ... >+ bool mCapture : 1; >+ // If mInSystemGroup is true, it's listening to the events in system group. .. the listener is listening to the events in the system group. >+ bool mInSystemGroup : 1; >+ // If mAllowUntrustedEvents is true, it's listening to untrusted events too. s/it's/the listener is ...the untrusted.. Something like that. >+ bool Equals(const EventListenerFlags& aOther, >+ bool aIgnoreUntrustedEventFlag = false) const I really wish there was another Equals method which explicitly says that it doesn't deal with untrusted flag. So, perhaps Equals and EqualsIgnoringTrustness >+ dom::TrustedSystemEventsAtBubbling()); I'm not quite happy with this naming. It emphasizes 'system' too much. Something like TrustedEventsAtSystemGroupBubbling perhaps? Same with other similar cases. I'd like to see a new patch, but in general this is great!
Attachment #690046 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #72) > Comment on attachment 686970 [details] [diff] [review] > part.11 Don't use NS_EVENT_FLAG_NO_CONTENT_DISPATCH for nsEvent::flags > > mDontDispatchToHandlerAttr is really odd name. > event handler means onfoo properties or attributes, > but NS_EVENT_FLAG_NO_CONTENT_DISPATCH has nothing to do with those. Oh, okay. I confirmed that: 1. onclick event handlers of any elements don't work for non-left click. 2. event listeners which are added to the elements don't work too. 3. event listeners which are added to the document and window do work.
I'll check the performance tomorrow. # I think that the patches should be landed on weekend.
Attachment #690046 - Attachment is obsolete: true
Attachment #691782 - Flags: review?(bugs)
Comment on attachment 691779 [details] [diff] [review] part.11 Don't use NS_EVENT_FLAG_NO_CONTENT_DISPATCH for nsEvent::flags >+ bool mNotDispatchToContents : 1; mNoContentDispatch sounds better to me
Attachment #691779 - Flags: review?(bugs) → review+
Comment on attachment 691782 [details] [diff] [review] part.14 Make dom::EventListenerFlags for nsEventListenerManager > manager->AddEventListenerByType(new nsXHRParseEndListener(this), > NS_LITERAL_STRING("DOMContentLoaded"), >- NS_EVENT_FLAG_BUBBLE | >- NS_EVENT_FLAG_SYSTEM_EVENT); >+ dom::TrustedEventsAtSystemGroupBubbling()); I think TrustedEventsAtSystemGroupBubble() is more correct and is closer to the capture version of the same method. >+nsEventListenerManager::RemoveEventListenerInternal( >+ nsIDOMEventListener *aListener, nsIDOMEventListener* aListener >+ bool Equals(const EventListenerFlags& aOther) const >+ { >+ return (mCapture == aOther.mCapture && >+ mInSystemGroup == aOther.mInSystemGroup && >+ mListenerIsJSListener == aOther.mListenerIsJSListener && >+ mAllowUntrustedEvents == aOther.mAllowUntrustedEvents); Align mAllowUntrustedEvents == aOther.mAllowUntrustedEvents
Attachment #691782 - Flags: review?(bugs) → review+
patched build unpatched build Win 5481ms 5364ms 5675ms 5456ms 5586ms 5350ms 5353ms 5248ms 5204ms 5244ms 5236ms 5310ms 5260ms 5479ms 5270ms 5336ms 5269ms 5342ms 5257ms 5263ms Mac 4090ms 4397ms 4045ms 4360ms 4073ms 4420ms 4025ms 4472ms 3981ms 4672ms 4026ms 4692ms 4032ms 4562ms 4042ms 4511ms 4049ms 4686ms On Windows, I don't see the difference. However, on Mac, the patched build is faster, that's strange... Anyway, I will land them on this Sunday.
(In reply to Olli Pettay [:smaug] from comment #106) > Do the patches affect to > http://mozilla.pettay.fi/moztests/events/event_speed_2.1_nolistener.html ? no big difference but: patched build unpatched build Win 145.0ms 138.7ms 137.0ms 139.1ms 137.1ms 138.8ms 135.8ms 138.5ms 133.2ms 136.9ms 133.6ms 135.7ms 134.8ms 138.0ms Mac 98.7ms 96.2ms 98.0ms 96.1ms 99.2ms 98.2ms 98.4ms 97.7ms 99.3ms 96.6ms 98.9ms 97.1ms 98.8ms 98.3ms On Windows, it's a little bit faster. However, on Mac, it's a little bit slower.
Depends on: 822088
This broke desktop B2G builds. See bug 822088.
Unfortunately this has run into a compiler bug in VC9. Compiling with -O1 instead of -O2 seems to avoid the issue.
Depends on: 822866
Oh BTW I am fine with the answer being that starting with version 20, VC9 is no longer a supported build platform. I plan to redo my builds to use VC10 over XMAS vacation form work.
(In reply to Bill Gianopoulos [:WG9s] from comment #113) > Oh BTW I am fine with the answer being that starting with version 20, VC9 is > no longer a supported build platform. I plan to redo my builds to use VC10 > over XMAS vacation form work. Sorry, posted this on the wrong bug.
Depends on: 823201
Depends on: 823369
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: