Closed Bug 813445 Opened 11 years ago Closed 10 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)
The flags will be removed by part.13.
Attachment #686970 - 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.

        Attachment #686976 -
        Attachment is obsolete: true

        Attachment #686976 -
        Flags: review?(bugs)

        Attachment #689033 -
        Flags: review?(bugs)

    
    

    
  
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)

    
    

    
  


  
I'll check the part.11 later.

        Attachment #690046 -
        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
Depends on: 1270649

          You need to log in
          before you can comment on or make changes to this bug.