decom nsEventStateManager

RESOLVED FIXED in mozilla6

Status

()

Core
General
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

(Blocks: 1 bug)

Trunk
mozilla6
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 488805 [details] [diff] [review]
patch
(Assignee)

Updated

7 years ago
OS: Windows XP → All
Hardware: x86 → All
Blocks: 105431
(Assignee)

Comment 1

7 years ago
Created attachment 489080 [details] [diff] [review]
patch
Attachment #488805 - Attachment is obsolete: true
Attachment #489080 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #489080 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #489080 - Flags: review?(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.
Attachment #489080 - Flags: review?(jst) → review?(Olli.Pettay)

Comment 3

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

Comment 4

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

Updated

7 years ago
Depends on: 589148
(Assignee)

Comment 5

6 years ago
Created attachment 521931 [details] [diff] [review]
Refreshed patch

This can be done now that we require libxul
Attachment #489080 - Attachment is obsolete: true
Attachment #521931 - Flags: review?(Olli.Pettay)
Attachment #489080 - Flags: review?(Olli.Pettay)

Comment 6

6 years ago
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.
Attachment #521931 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 527251 [details] [diff] [review]
Patch addressing review comments r=smaug

Removed the virtual and fixed comments.
Attachment #521931 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Fixed in cedar. Should be synced with mozilla-central in the next 24 hours.
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
Version: unspecified → Trunk
I had to backout from cedar. David, to you have an access to the try server?
Whiteboard: [fixed in cedar]
Here is a log: http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1303328920.1303330557.13094.gz
(Assignee)

Comment 11

6 years ago
Created attachment 527567 [details] [diff] [review]
Patch r=smaug

Hopefully this lands before it rots again
Attachment #527251 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
http://hg.mozilla.org/mozilla-central/rev/aaa99fe3ee29
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla6
This may have caused a minor Dromaeo regression.
(In reply to comment #13)
> This may have caused a minor Dromaeo regression.

Should it be backed out then?
I don't know.  Smaug?  Dzbarsky?  Is anyone looking at the regression?
(Assignee)

Comment 16

6 years ago
If it caused a regression we should probably back it out.  I don't know how to pinpoint the exact cause.
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.
are we sure this caused the regression?
And the code this touches isn't performance-vice too hot.
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#
http://hg.mozilla.org/mozilla-central/rev/6cc6b44735a7 looks more likely to me
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.
You need to log in before you can comment on or make changes to this bug.