610305 - decom nsEventStateManager

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: patch

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: patch

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 489080 [details] [diff] [review]
patch

That's one large patch :). smaug should review this one, as this is primarily his code.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

ESM is not primary my code. It is mainly way older than my contribution to
Mozilla ;)
But I'll review.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

So this can be done after --disable-libxul isn't supported anymore.

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: Refreshed patch

This can be done now that we require libxul

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 521931 [details] [diff] [review]
Refreshed patch

>+  /**
>+   * Notify that the given NS_EVENT_STATE_* bit has changed for this content.
>+   * @param aContent Content which has changed states
>+   * @param aState   Corresponding state flags such as NS_EVENT_STATE_FOCUS
>+   * @return  Whether the content was able to change all states. Returns PR_FALSE
>+   *                  if a resulting DOM event causes the content node passed in
>+   *                  to not change states. Note, the frame for the content may
>+   *                  change as a result of the content state change, because of
>+   *                  frame reconstructions that may occur, but this does not
>+   *                  affect the return value.
>+   */
>+  PRBool SetContentState(nsIContent *aContent, nsEventStates aState);
>+  void ContentRemoved(nsIDocument* aDocument, nsIContent* aContent);
>+  PRBool EventStatusOK(nsGUIEvent* aEvent);
> 
>-  NS_IMETHOD SetCursor(PRInt32 aCursor, imgIContainer* aContainer,
>-                       PRBool aHaveHotspot, float aHotspotX, float aHotspotY,
>-                       nsIWidget* aWidget, PRBool aLockCursor);
>+  /**
>+   * Register accesskey on the given element. When accesskey is activated then
>+   * the element will be notified via nsIContent::PerformAccesskey() method.
>+   *
>+   * @param  aContent  the given element
>+   * @param  aKey      accesskey
>+   */
>+  void RegisterAccessKey(nsIContent* aContent, PRUint32 aKey);
>+
>+  /**
>+   * Unregister accesskey for the given element.
>+   *
>+   * @param  aContent  the given element
>+   * @param  aKey      accesskey
>+   */
>+  void UnregisterAccessKey(nsIContent* aContent, PRUint32 aKey);
>+
>+  /**
>+   * Get accesskey registered on the given element or 0 if there is none.
>+   *
>+   * @param  aContent  the given element
>+   * @param  aKey      registered accesskey
>+   * @return           NS_OK
>+   */
>+  virtual PRUint32 GetRegisteredAccessKey(nsIContent* aContent);

Why is this virtual? So that external libraries can access it?
(Which library does that?)
@return is wrong, so is the latter @param.



I believe this patch is such that it should be taken to Fx6.
So it could land to trunk in a month or so.

r+ if the comments are fixed and the virtual is either
removed or explained.

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: Patch addressing review comments r=smaug

Removed the virtual and fixed comments.

Comment 8

[Mounir Lamouri (:mounir)]

Fixed in cedar. Should be synced with mozilla-central in the next 24 hours.

Comment 9

[Mounir Lamouri (:mounir)]

I had to backout from cedar. David, to you have an access to the try server?

Comment 10

[Mounir Lamouri (:mounir)]

Here is a log: http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1303328920.1303330557.13094.gz

Comment 11

[David Zbarsky (:dzbarsky)]

Attachment: Patch r=smaug

Hopefully this lands before it rots again

Comment 12

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/aaa99fe3ee29

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This may have caused a minor Dromaeo regression.

Comment 14

[(no longer active)]

(In reply to comment #13)
> This may have caused a minor Dromaeo regression.

Should it be backed out then?

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't know.  Smaug?  Dzbarsky?  Is anyone looking at the regression?

Comment 16

[David Zbarsky (:dzbarsky)]

If it caused a regression we should probably back it out.  I don't know how to pinpoint the exact cause.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Yeah, it is quite strange if a patch which make us execute less code, and that less should be even better in the caches, causes regression.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

are we sure this caused the regression?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

And the code this touches isn't performance-vice too hot.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, we're not sure.  But this was the only one in the range that looked likely.  

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/e8cacce1606670f2#

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/6cc6b44735a7 looks more likely to me

Comment 22

[Timothy Nikkel (:tnikkel)]

Dromaeo (jslib) seems to correspond to these pages http://mxr.mozilla.org/mozilla/source/testing/performance/talos/page_load_test/dromaeo/jslib.manifest

I had a quick look and there is only one application of border-radius, to b elements, and it does not seem to have overflow different from visible. If that is the case then http://hg.mozilla.org/mozilla-central/rev/6cc6b44735a7 should have no effect. It can still be backed out if people want.

Comment 23

[Pulsebot]

Pushed by ffxbld:
https://hg.mozilla.org/releases/mozilla-release/rev/c1a7c1bc1aeb
decom nsEventStateManager r=smaug