Introduce nsEventState for manipulating event states

RESOLVED FIXED in mozilla2.0b7

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks 1 bug)

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

Posted patch WIP Patch (obsolete) — Splinter Review
At the moment event states are manipulated with PRInt32 but we should move to PRUint32 and in a very close future, PRUint64.

Introducing a typedef for this type might help.

This is a follow-up of bug 580575
Blocks: 580575
I'd almost rather have a struct, with an explicit ctor taking some integer type and the macros redefined to create instances of that struct.  That way we'll catch people trying to assign the event state to integers, etc.  The typedef does nothing for us in terms of catching someone using an int as an event state or vice versa, right?

There's the issue of how to handle the | we use right now in some places, I guess.
(In reply to comment #1)
> I'd almost rather have a struct, with an explicit ctor taking some integer type
> and the macros redefined to create instances of that struct.  That way we'll
> catch people trying to assign the event state to integers, etc.  The typedef
> does nothing for us in terms of catching someone using an int as an event state
> or vice versa, right?

It seems to be a nice idea indeed.

> There's the issue of how to handle the | we use right now in some places, I
> guess.

Can't we just override the | operator (or event the + operator if we think that's more convenient)? or there is a reason why we should not do that?
> Can't we just override the | operator

If we can, sure thing!
Status: ASSIGNED → NEW
operator| and operator& works fine. However, operator& is mostly used this way:
> if (eventStates & NS_EVENT_STATE_FOO)
and this will always return true if we properly override operator&. So, we should probably have a method returning if one state is present. (we can have operator& returning a boolean but that would be wrong)

In addition, creating an object with the macro seems a good idea to catch all places and make the change error proof but it would be better to use real integer to prevent creating a lot of objects for nothing. For example, this code:
> states |= NS_EVENT_STATE_1 | NS_EVENT_STATE_2 | NS_EVENT_STATE_3;
will probably create 3 temporary objects this probably has a cost.
So, shouldn't we have the macro "returning" real numbers as soon as the transition is done?
(In reply to comment #4)
> operator| and operator& works fine. However, operator& is mostly used this way:
> > if (eventStates & NS_EVENT_STATE_FOO)
> and this will always return true if we properly override operator&. So, we
> should probably have a method returning if one state is present. (we can have
> operator& returning a boolean but that would be wrong)

We can simply override operator== actually...
> We can simply override operator== actually...

I don't think making "==" mean "contains" is good.  We should just have a method.

> to prevent creating a lot of objects for nothing

I'd hope that compilers optimize that stuff away as long as your ctor/dtor are inline...
In layout/inspector/public/inIDOMUtils.idl, there is a getter and a setter for intrinsic states and there is a setter in content/xtf/public/nsIXTFElementWrapper.idl.

Unfortunately, the parameter in the setters are PRInt32. Do we have a type in our IDL management which is associated to PRUint64? unsigned long long?
For the getter, I can add a .GetValue() method returning a PRUint64 but that seems to be breach in the encapsulation for only one method :(
> unsigned long long?

Yes.  Also PRInt64 (the two are aliases for each other in xpidl).
The update is really simple: states getter and setter should use PRUint64 instead of PRInt32 now. The patch changes a few things to make sure it's working alone but Part2 is fixing all the cast uglinesses.
Attachment #482602 - Flags: review?(bzbarsky)
This is building but a lot of tests are failing. I probably did something stupid when updating some lines but in a 200K patch, it's hard to found which one :/
Attachment #473850 - Attachment is obsolete: true
Attachment #482607 - Flags: feedback?(bzbarsky)
Attachment #482607 - Attachment is patch: true
Attachment #482607 - Attachment mime type: application/octet-stream → text/plain
I believe this block is incorrect:

>-#define IMAGE_OK(_state, _loadingOK)                        \
>-   (((_state) & BAD_STATES) == 0 ||                         \
>-    (((_state) & BAD_STATES) == NS_EVENT_STATE_LOADING &&   \
>-     (_loadingOK)))
>+#define IMAGE_OK(_state, _loadingOK)                             \
>+   (!_state.HasStates(BAD_STATES) ||                             \
>+    (_state.HasStates(NS_EVENT_STATE_LOADING) && _loadingOK))

_state & BAD_STATES == NS_EVENT_STATE_LOADING is equivalent to _state.HasStates(NS_EVENT_STATE_LOADING) && !_state.HasStates(NS_EVENT_STATE_BROKEN | NS_EVENT_STATE_USERDISABLED)
This is very broken:

>-  if (inState & NS_EVENT_STATE_FOCUS && isActive)
>+  if (inState.HasStates(NS_EVENT_STATE_FOCUS && isActive))
Nevermind about comment 12, that's just me forgetting operator precedence.  It still looks gross to me, however.
Nevermind about comment 13, comment 12 is valid. & binds tighter than &&.  The fragment is quite wrong.
Nice catches Josh.
I've also seen that HasStates() is sometimes used for "has one of these states" and sometimes for "has all these states". I should fix that.
Attachment #482607 - Attachment is obsolete: true
Attachment #482607 - Flags: feedback?(bzbarsky)
Comment on attachment 482602 [details] [diff] [review]
Part 1 - Update nsIXTFElementWrapper and inIDOMUtils API

r=me
Attachment #482602 - Flags: review?(bzbarsky) → review+
Fwiw, if you want this to be a non-megapatch, then one way to do it is to create one patch for the actual class implementation, and at this stage give it an implicit ctor that takes PRInt32 (and perhaps an operator() that returns PRInt32).  Then have a series of patches working out from the style system code to consumers, replacing PRInt32 with the struct a bit at a time.  Then remove the conversion operator and constructor.  Might be too much work at this point, since you already have the megapatch...
This is still incorrect:

>+#define IMAGE_OK(_state, _loadingOK)                                           \
>+   (!_state.HasStates(BAD_STATES) ||                                           \
>+    (_loadingOK &&_state.HasStates(NS_EVENT_STATE_LOADING) &&                  \
>+     !_state.HasStates(NS_EVENT_STATE_BROKEN | NS_EVENT_STATE_USERDISABLED))

The original included an && _loadingOK condition which is missing here.
(In reply to comment #18)
> This is still incorrect:
> 
> >+#define IMAGE_OK(_state, _loadingOK)                                           \
> >+   (!_state.HasStates(BAD_STATES) ||                                           \
> >+    (_loadingOK &&_state.HasStates(NS_EVENT_STATE_LOADING) &&                  \
> >+     !_state.HasStates(NS_EVENT_STATE_BROKEN | NS_EVENT_STATE_USERDISABLED))
> 
> The original included an && _loadingOK condition which is missing here.

_loadingOK is now at the beginning of the condition.

(In reply to comment #17)
> Fwiw, if you want this to be a non-megapatch, then one way to do it is to
> create one patch for the actual class implementation, and at this stage give it
> an implicit ctor that takes PRInt32 (and perhaps an operator() that returns
> PRInt32).  Then have a series of patches working out from the style system code
> to consumers, replacing PRInt32 with the struct a bit at a time.  Then remove
> the conversion operator and constructor.  Might be too much work at this point,
> since you already have the megapatch...

I thought about that but given my knowledge of the style engine and operator& that wasn't usable, I thought it wouldn't really help.
I think I've found what was causing the failures so it should be fine, I hope.
Depends on: 604056
So it looks like with patch from bug 604056, tests are passing locally (at least those I tried to run). Will see if the try server is happy with that.

The issue was because of nsIDocumentObserver methods were explicitely declared by all classes inheriting from nsIDocumentObserver so, changing the parameters in nsIDocumentObserver methods wasn't making the compilation failing (IOW, the older methods were considered as a method of the child classes). I've introduced some macro like we do for nsIMutationObserver so changing signatures of the methods in nsIDocumentObserver will automatically change declarations and break compilation until definitions are changed too.

I'm going to clean a bit the patch, see if try server is happy then I will ask for another review.
This patch passes try server tests.

As I said in the comments, I would have prefer to not have a GetInternalValue() and only have a default constructor as a public constructor. But because of the two interfaces manipulating event states, we can't. BTW, these interfaces are using nsEventStates::InternalType instead of PRUint64 so build will fail when changing the internal type.

I'm also wondering if it wouldn't have been better to keep stuff like |(eventStates & NS_EVENT_STATES_FOO)| instead of changing that to |eventStates.HasState(NS_EVENT_STATES_FOO)|. I think the later is clearer but the former was doable by defining |operator&| and |operator bool|. Though, |operator bool| would have make |if (eventStates)| working which seems weird.
Anyway, I have no strong opinion, so Boris, feel free to ask me to change that.
Attachment #482732 - Attachment is obsolete: true
Attachment #482960 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Comment on attachment 482960 [details] [diff] [review]
Part 2 - Use nsEventStates class to manage event states.

>+++ b/content/base/public/nsIDocument.h
>+                                    const nsEventStates& aStateMask) = 0;

Imho, we should pass nsEventStates by value, not by reference.  It works better for a single call on both x86 and x86-64 (and _really_ better on that last, even with multiple calls), as far as I can tell from some simple g++ experimentation.

>+++ b/content/events/public/nsEventStates.h
>+  // NOTE: the ideal scenario would be to have the default constructor public
>+  // setting mStates to 0 and this constructor (without = 0) explicit and
>+  // private. In that case, we could be sure that only macros at the end were
>+  // creating nsEventStates instances with mStates set to something else than 0.
>+  // Unfortunately, this constructor is needed at at least two places now.
>+  nsEventStates(InternalType aStates = 0)

I don't understand.  We need a public |nsEventStates(void)| constructor, I agree.  We need a public |explicit nsEventStates(InternalType)| constructor (for the #defines and the DOMi thing).  But we should NOT have a non-explicit constructor taking an integer type.  Having that defeats half the purpose of using a struct, no?

>+    if ((aEventStates.mStates & (aEventStates.mStates - 1))) {
>+      NS_WARNING("When calling HasState, "
>+                 "nsEventStates object has to contain only one state!");

How about NS_ERROR?

>+  bool HasOneOfStates(const nsEventStates& aEventStates) const {

I'd call this HasAtLeastOneState.

>+#define NS_EVENT_STATE_ACTIVE        (nsEventStates(1 << 0))

For the 64-bit ones (N > 31), we'll want InternalType(1) << N for these defines.  Go ahead and do that here, so we'll get it right when we start adding those?

>+/**
>+ * NOTE: do not go over 1 << 64 without updating nsEventStates::InternalType!
>+ */

s/64/63/?

Please add AddStates and RemoveStates methods?  We can use |= and &=~ for now as we have, but longer-term something more readable might be nice.

>+++ b/content/events/src/nsEventStateManager.cpp
>@@ -4310,25 +4310,26 @@ nsEventStateManager::SetContentState(nsI
>-  if (aState & NS_EVENT_STATE_FOCUS) {
>-    aState |= NS_EVENT_STATE_FOCUSRING;
>+  nsEventStates simpleStates = aState;
>+  simpleStates &= ~NS_EVENT_STATE_ACTIVE|NS_EVENT_STATE_HOVER;
>+
>+  if (simpleStates.HasState(NS_EVENT_STATE_FOCUS)) {

This does change the behavior (different aState value once you're done).  On the other hand, aState is unused after this point in this function.... so I guess you're ok.

>+++ b/layout/generic/nsImageFrame.cpp
>-#define IMAGE_OK(_state, _loadingOK)                        \
>-   (((_state) & BAD_STATES) == 0 ||                         \
>-    (((_state) & BAD_STATES) == NS_EVENT_STATE_LOADING &&   \
>-     (_loadingOK)))
>+#define IMAGE_OK(_state, _loadingOK)                                           \
>+   (!_state.HasOneOfStates(BAD_STATES) ||                                      \

You lost the parens around _state.

>+    (_loadingOK &&_state.HasState(NS_EVENT_STATE_LOADING) &&                   \

And around both _loadingOK and _state here (and also, need space after the first &&.


>+     !_state.HasOneOfStates(NS_EVENT_STATE_BROKEN | NS_EVENT_STATE_USERDISABLED)))

And here.

This reordered things so that we evaluate _loadingOK more eagerly, while the stated point of the macro is to not evaluate it if at all possible.  Why the change?

>+++ b/layout/style/nsCSSRuleProcessor.cpp
>@@ -1582,17 +1581,17 @@ checkGenericEmptyMatches(RuleProcessorDa
>+static const nsEventStates sPseudoClassBits[] = {
> #define CSS_PSEUDO_CLASS(_name, _value)         \
>   0,
> #define CSS_STATE_PSEUDO_CLASS(_name, _value, _bit) \
>   _bit,

Should that be nsEventStates(0) and rename _bit to _states?

And maybe rename the array to sPseudoClassStates??

>+++ b/layout/style/nsRuleProcessorData.h
>@@ -150,20 +150,21 @@ private:
>+                                // mElement->IntrinsicState() if we have no ES$M

Don't sneak that '$' in there, please.  ;)

> struct StateRuleProcessorData : public RuleProcessorData {
>+  const nsEventStates mStateMask; // |HasStateDependentStyle| for which state(s)?
>+                                  //  Constants defined in nsIEventStateManager.h .

That comment is no longer true.  Fix the filename there?

>+++ b/widget/src/cocoa/nsNativeThemeCocoa.mm
>@@ -1283,17 +1285,17 @@ nsNativeThemeCocoa::GetScrollbarDrawInfo
>+    nsEventStates buttonStates[] = {0, 0, 0, 0};

s/0/nsEventStates()/ ?

The rest looks great.
Attachment #482960 - Flags: review?(bzbarsky) → review-
(In reply to comment #22)
> Comment on attachment 482960 [details] [diff] [review]
> Part 2 - Use nsEventStates class to manage event states.
> 
> >+++ b/content/base/public/nsIDocument.h
> >+                                    const nsEventStates& aStateMask) = 0;
> 
> Imho, we should pass nsEventStates by value, not by reference.  It works better
> for a single call on both x86 and x86-64 (and _really_ better on that last,
> even with multiple calls), as far as I can tell from some simple g++
> experimentation.

Should I always prefer |nsEventStates| instead of |const nsEventStates&|?

> >+  bool HasOneOfStates(const nsEventStates& aEventStates) const {
> 
> I'd call this HasAtLeastOneState.

I think this name might be confusing. I would prefer HasAtLeastOneOfStates() if AtLeast has to be added.

> Please add AddStates and RemoveStates methods?  We can use |= and &=~ for now
> as we have, but longer-term something more readable might be nice.

I'm going to open a follow-up for that.

> >+++ b/content/events/src/nsEventStateManager.cpp
> >@@ -4310,25 +4310,26 @@ nsEventStateManager::SetContentState(nsI
> >-  if (aState & NS_EVENT_STATE_FOCUS) {
> >-    aState |= NS_EVENT_STATE_FOCUSRING;
> >+  nsEventStates simpleStates = aState;
> >+  simpleStates &= ~NS_EVENT_STATE_ACTIVE|NS_EVENT_STATE_HOVER;
> >+
> >+  if (simpleStates.HasState(NS_EVENT_STATE_FOCUS)) {
> 
> This does change the behavior (different aState value once you're done).  On
> the other hand, aState is unused after this point in this function.... so I
> guess you're ok.

If SetContentState doesn't take a const nsEventStates& anymore, this change could be reverted.

> >+++ b/layout/generic/nsImageFrame.cpp
> >-#define IMAGE_OK(_state, _loadingOK)                        \
> >-   (((_state) & BAD_STATES) == 0 ||                         \
> >-    (((_state) & BAD_STATES) == NS_EVENT_STATE_LOADING &&   \
> >-     (_loadingOK)))
> >+#define IMAGE_OK(_state, _loadingOK)                                           \
> >+   (!_state.HasOneOfStates(BAD_STATES) ||                                      \
> This reordered things so that we evaluate _loadingOK more eagerly, while the
> stated point of the macro is to not evaluate it if at all possible.  Why the
> change?

It wasn't a choice even if I'm wondering if checking for _loadingOK is still slower than the other part of the condition.
To be safe, I put back _loadingOK to the end of the condition.
Blocks: 605117
Blocks: 605124
Blocks: 605125
> Should I always prefer |nsEventStates| instead of |const nsEventStates&|?

I think so, yes.

> I would prefer HasAtLeastOneOfStates()

Let's do that.

> If SetContentState doesn't take a const nsEventStates& anymore, this change
> could be reverted.

Ah, I was wondering why that change was made!  Yeah, please revert it.

> To be safe, I put back _loadingOK to the end of the condition.

Thanks.  In the perf-sensitive codepath, _loadingOK is just a call to HaveFixedSize plus a fetch of the position style struct.  I can assure you that this is much slower than the several masks-and-tests the event state code does here.
Posted patch Inter diff (obsolete) — Splinter Review
Attachment #484298 - Attachment description: Inner diff → Inter diff
Posted patch Inter diff updating IID's (obsolete) — Splinter Review
In addition of the first inter diff.
Changes done since the last patch are available in "Inter diff" and "Inter diff 2".
Attachment #482960 - Attachment is obsolete: true
Attachment #484301 - Flags: review?(bzbarsky)
Man...

Mounir, for future the right way to do this is to post one interdiff for the "change all the arguments by removing the '&'" bit and a separate interdiff for the substantive changes that you had to make because you got rid of the implicit "takes a number" constructor.  Reviewing what you posted will easily take 4-5x longer than reviewing those patches would have.  :(

Want to give me at least a list of the substantive changes to watch out for?
Attachment #484298 - Attachment is obsolete: true
Attachment #484300 - Attachment description: Inter diff 2 → Inter diff updating IID's
I hope it will be easier to review this way.

One or two lines in the inter diff for nsEventStates are related to HasAtLeastOneOfStates changes.
Comment on attachment 484301 [details] [diff] [review]
Part 2 - Use nsEventStates class to manage event states.

Yes, that's _much_ easier to review!

> nsObjectLoadingContent::NotifyStateChanged(ObjectType aOldType,

This should use %llx for the GetInternalValue() stuff, I would think.  Check whether that does the right thing?

> +  simpleStates &= ~NS_EVENT_STATE_ACTIVE|NS_EVENT_STATE_HOVER;

This is wrong.  You need parens after the ~ and before the ; just like the original code used to have.  I should have caught this in the first review...  I'm surprised this passed tests.

> nsCSSFrameConstructor::FindObjectData(nsIContent* aContent,

Fix the indent on the HasAtLeastOneOfStates arguments here.

r=me with those things fixed.
Attachment #484301 - Flags: review?(bzbarsky) → review+
Attachment #482602 - Flags: approval2.0?
Attachment #484301 - Flags: approval2.0?
Attachment #484300 - Attachment is obsolete: true
Attachment #484331 - Attachment is obsolete: true
Attachment #484332 - Attachment is obsolete: true
If someone approve the patches here, could you approve the patch in bug 604056 too?
Attachment #482602 - Flags: approval2.0? → approval2.0+
Attachment #484301 - Flags: approval2.0? → approval2.0+
(In reply to comment #32)
> > nsObjectLoadingContent::NotifyStateChanged(ObjectType aOldType,
> 
> This should use %llx for the GetInternalValue() stuff, I would think.  Check
> whether that does the right thing?

You meant %lx, don't you? PRUint64 is a long unsigned int, not a long long unsigned int.
Pushed:
http://hg.mozilla.org/mozilla-central/rev/f645206425b2
http://hg.mozilla.org/mozilla-central/rev/877a1f5be4f4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1287584222.1287586165.4915.gz

Possible falout? I'm seeing this build error:

In file included from /builds/slave/comm-central-trunk-linux-debug/build/mozilla/content/base/src/../../xul/content/src/nsXULElement.h:62:0,
                 from /builds/slave/comm-central-trunk-linux-debug/build/mozilla/content/base/src/nsGenericElement.cpp:87:
../../../dist/include/nsIDOMXULSelectCntrlEl.h:33:97: warning: ‘virtual nsresult nsIDOMXULSelectControlElement::GetSelectedItem(nsIDOMXULSelectControlItemElement**)’ was hidden
../../../dist/include/nsIDOMXULMultSelectCntrlEl.h:73:97: warning:   by ‘virtual nsresult nsIDOMXULMultiSelectControlElement::GetSelectedItem(PRInt32, nsIDOMXULSelectControlItemElement**)’
/builds/slave/comm-central-trunk-linux-debug/build/mozilla/content/base/src/nsGenericElement.cpp: In member function ‘void nsGenericElement::List(FILE*, PRInt32, const nsCString&) const’:
/builds/slave/comm-central-trunk-linux-debug/build/mozilla/content/base/src/nsGenericElement.cpp:5101:58: error: cannot pass objects of non-trivially-copyable type ‘class nsEventStates’ through ‘...’
/builds/slave/comm-central-trunk-linux-debug/build/mozilla/content/base/src/nsGenericElement.cpp:5101:58: warning: format ‘%08x’ expects type ‘unsigned int’, but argument 3 has type ‘nsEventStates’
NEXT ERROR make[7]: *** [nsGenericElement.o] Error 1
make[7]: Leaving directory `/builds/slave/comm-central-trunk-linux-debug/build/objdir/mozilla/content/base/src'
NEXT ERROR make[6]: *** [src_libs] Error 2
make[6]: Leaving directory `/builds/slave/comm-central-trunk-linux-debug/build/objdir/mozilla/content/base'
NEXT ERROR make[5]: *** [base_libs] Error 2
make[5]: *** Waiting for unfinished jobs....
> PRUint64 is a long unsigned int, not a long long unsigned int.

That depends on the OS.  On 32-bit systems it's a long long.  On 64-bit ones it's long, except on Windows, where it's __int64 and long is always 32-bit (even on 64-bit Windows).

So no, I meant what I said.  %llx, please.
(In reply to comment #36)
> /builds/slave/comm-central-trunk-linux-debug/build/mozilla/content/base/src/nsGenericElement.cpp:5101:58:
> warning: format ‘%08x’ expects type ‘unsigned int’, but argument 3 has type
> ‘nsEventStates’

Should I keep %08 or can I move %08x to %lxx. I don't think it's a good idea to have only 8 digits but I don't know if the output has to structured this way...

(In reply to comment #37)
> > PRUint64 is a long unsigned int, not a long long unsigned int.
> 
> That depends on the OS.  On 32-bit systems it's a long long.  On 64-bit ones
> it's long, except on Windows, where it's __int64 and long is always 32-bit
> (even on 64-bit Windows).
> 
> So no, I meant what I said.  %llx, please.

Sorry about that :(
I will fix both issues in the same patch.
Depends on: 605860
Using %llx in List looks good to me.
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Blocks: 611888
You need to log in before you can comment on or make changes to this bug.