Status

()

defect
RESOLVED FIXED
9 years ago
8 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)

Posted patch patch (obsolete) — Splinter Review
No description provided.
OS: Windows XP → All
Hardware: x86 → All
Posted patch patch (obsolete) — Splinter Review
Attachment #488805 - Attachment is obsolete: true
Attachment #489080 - Flags: review?(roc)
Attachment #489080 - Flags: review?(roc)
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)
ESM is not primary my code. It is mainly way older than my contribution to
Mozilla ;)
But I'll review.
So this can be done after --disable-libxul isn't supported anymore.
Posted patch Refreshed patch (obsolete) — Splinter Review
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 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+
Removed the virtual and fixed comments.
Attachment #521931 - Attachment is obsolete: true
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]
Posted patch Patch r=smaugSplinter Review
Hopefully this lands before it rots again
Attachment #527251 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
http://hg.mozilla.org/mozilla-central/rev/aaa99fe3ee29
Status: ASSIGNED → RESOLVED
Closed: 8 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?
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.
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.