Last Comment Bug 610305 - decom nsEventStateManager
: decom nsEventStateManager
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
Depends on: require-libxul
Blocks: deCOM
  Show dependency treegraph
 
Reported: 2010-11-07 19:36 PST by David Zbarsky (:dzbarsky)
Modified: 2011-05-03 15:05 PDT (History)
9 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (118.11 KB, patch)
2010-11-07 19:36 PST, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
patch (119.40 KB, patch)
2010-11-08 19:08 PST, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Refreshed patch (118.97 KB, patch)
2011-03-25 13:40 PDT, David Zbarsky (:dzbarsky)
bugs: review+
Details | Diff | Splinter Review
Patch addressing review comments r=smaug (119.21 KB, patch)
2011-04-20 06:39 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch r=smaug (119.18 KB, patch)
2011-04-21 10:13 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2010-11-07 19:36:21 PST
Created attachment 488805 [details] [diff] [review]
patch
Comment 1 David Zbarsky (:dzbarsky) 2010-11-08 19:08:30 PST
Created attachment 489080 [details] [diff] [review]
patch
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-09 12:26:59 PST
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] (TPAC) 2010-11-09 12:30:51 PST
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] (TPAC) 2010-11-09 12:35:13 PST
So this can be done after --disable-libxul isn't supported anymore.
Comment 5 David Zbarsky (:dzbarsky) 2011-03-25 13:40:40 PDT
Created attachment 521931 [details] [diff] [review]
Refreshed patch

This can be done now that we require libxul
Comment 6 Olli Pettay [:smaug] (TPAC) 2011-03-28 05:54:58 PDT
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) 2011-04-20 06:39:48 PDT
Created attachment 527251 [details] [diff] [review]
Patch addressing review comments r=smaug

Removed the virtual and fixed comments.
Comment 8 Mounir Lamouri (:mounir) 2011-04-20 12:45:37 PDT
Fixed in cedar. Should be synced with mozilla-central in the next 24 hours.
Comment 9 Mounir Lamouri (:mounir) 2011-04-20 13:31:05 PDT
I had to backout from cedar. David, to you have an access to the try server?
Comment 10 Mounir Lamouri (:mounir) 2011-04-20 13:31:33 PDT
Here is a log: http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1303328920.1303330557.13094.gz
Comment 11 David Zbarsky (:dzbarsky) 2011-04-21 10:13:59 PDT
Created attachment 527567 [details] [diff] [review]
Patch r=smaug

Hopefully this lands before it rots again
Comment 12 Mounir Lamouri (:mounir) 2011-04-22 06:28:46 PDT
http://hg.mozilla.org/mozilla-central/rev/aaa99fe3ee29
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-22 13:34:57 PDT
This may have caused a minor Dromaeo regression.
Comment 14 :Ehsan Akhgari 2011-04-26 15:18:14 PDT
(In reply to comment #13)
> This may have caused a minor Dromaeo regression.

Should it be backed out then?
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-26 15:28:25 PDT
I don't know.  Smaug?  Dzbarsky?  Is anyone looking at the regression?
Comment 16 David Zbarsky (:dzbarsky) 2011-04-26 19:25:08 PDT
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] (TPAC) 2011-04-27 04:30:23 PDT
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] (TPAC) 2011-04-27 04:32:13 PDT
are we sure this caused the regression?
Comment 19 Olli Pettay [:smaug] (TPAC) 2011-04-27 04:39:20 PDT
And the code this touches isn't performance-vice too hot.
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-27 04:46:36 PDT
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] (TPAC) 2011-04-27 04:52:16 PDT
http://hg.mozilla.org/mozilla-central/rev/6cc6b44735a7 looks more likely to me
Comment 22 Timothy Nikkel (:tnikkel) 2011-05-03 15:05:42 PDT
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.

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