752548 - Use capturing phase for notification.xml handlers

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

Per bug 647875 comment 11.

See
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#1433
http://mxr.mozilla.org/comm-release/source/suite/common/bindings/notification.xml#1481

Comment 1

[[e-mail]]

Actually all the notification event handlers should probably use capturing.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Fix phase on notification.xml handlers [Checked in: See comments 5, 10, 15]

I checked all handlers in this file:
*Plugin*:
 as filed.
*DOMUpdatePageReport,
 npapi-carbon-event-model-failure,
 pageshow:
 SeaMonkey always had them reversed compared to what Firefox always had.
 I don't know why that should be, but please double-check.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #1)
> Actually all the notification event handlers should probably use capturing.

That's how I would understand bug 335586, but I was surprised it is not the case :-|


(In reply to Serge Gautherie (:sgautherie) from comment #2)
> *DOMUpdatePageReport,

Maybe we can keep capturing this one on SM ftb,
then I'll file FF bugs for this event and MozApplicationManifest?

Comment 4

[[e-mail]]

Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

>-      <handler event="DOMUpdatePageReport" phase="capturing">
>+      <handler event="DOMUpdatePageReport">
I don't think we should remove "capturing" unless there's a specific reason.
r=me with that fixed.
I'd also prefer to make MozApplicationManifest capturing, but you can do that as a followup if you want to fix Firefox first.

Note: npapi-carbon-event-model-failure is also actually a plugin event.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

http://hg.mozilla.org/comm-central/rev/a0df19ed338c
Av1, with comment 4 suggestion(s).


[Approval Request Comment]
No risk, resync' with FF, and fix bug 647875.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #4)
> Note: npapi-carbon-event-model-failure is also actually a plugin event.

Should we add a '#ifdef XP_MACOSX' as Firefox has?
Or at least a comment, fwiw?

Comment 7

[Philip Chee]

>> Note: npapi-carbon-event-model-failure is also actually a plugin event.
> Should we add a '#ifdef XP_MACOSX' as Firefox has?
> Or at least a comment, fwiw?
Well on non-OSX platforms this event will never be generated so it's a NOOP.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #7)
> Well on non-OSX platforms this event will never be generated so it's a NOOP.

Exactly: NOOP as in bloat/confusing.

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

On beta at least we don't have a ClickToPlay or pageshow event handler, so I am omitting them from my upcoming checkin

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

(In reply to Justin Wood (:Callek) from comment #9)
> On beta at least we don't have a ClickToPlay or pageshow event handler, so I
> am omitting them from my upcoming checkin

Right: bug 743312, which added both, didn't land on 2.10.

***

http://hg.mozilla.org/releases/comm-beta/rev/cd00c74eef12

It looks like you pushed the patch rather than the changeset :-<
Please, revert the DOMUpdatePageReport hunk.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Fix phase on (notification.xml) MozApplicationManifest handler [Checked in: Comments 12 and 16]

[Approval Request Comment]
Low risk, be consistent with other handlers.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 625815 [details] [diff] [review]
(Bv1) Fix phase on (notification.xml) MozApplicationManifest handler
[Checked in: Comments 12 and 16]

http://hg.mozilla.org/comm-central/rev/05ee1becb262

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Fix/Sync' phase on DOMMouseScroll handlers [Checked in: Comment 21]

These are some DOM* events outside notification.xml:
they succeeded on Firefox Try, but I'm not sure whether this is wanted too...

Comment 14

[[e-mail]]

Comment on attachment 626056 [details] [diff] [review]
(Cv1) Fix/Sync' phase on DOMMouseScroll handlers [Checked in: Comment 21]

The DOMMouseScroll one is reasonable but the other two are just chrome events.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

http://hg.mozilla.org/releases/comm-aurora/rev/1e676abebcc0

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 625815 [details] [diff] [review]
(Bv1) Fix phase on (notification.xml) MozApplicationManifest handler
[Checked in: Comments 12 and 16]

http://hg.mozilla.org/releases/comm-aurora/rev/eac2a85ceccd

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

Note: I intentionally didn't push the two code (!) changes to c-b here because I'm not sure about the risk assessment (don't want to be blamed for regressing Beta once again). I'll leave it to Callek to check with Neil and possibly push before tagging b3.

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

2.10 has been tagged, clearing requests to land on c-b since that contains the fixes now that we're past the migration. Sorry Serge.

Comment 19

[[e-mail]]

Comment on attachment 626056 [details] [diff] [review]
(Cv1) Fix/Sync' phase on DOMMouseScroll handlers [Checked in: Comment 21]

r=me on the DOMMouseScroll events ONLY, because there we're interested in content events (the DOMMenu* events are all chrome events).

Also it doesn't look as if you've filed a bug on Firefox's zoom manager.

Comment 20

[Philip Chee]

> r=me on the DOMMouseScroll events ONLY, because there we're interested in content
> events (the DOMMenu* events are all chrome events).
This patch has bit rotted. DOMMouseScroll events have been replaced by wheel events.

Comment 21

[Philip Chee]

> r=me on the DOMMouseScroll events ONLY, because there we're interested in content
> events (the DOMMenu* events are all chrome events).
Pushed only the DOMMouseScroll to comm-central:
http://hg.mozilla.org/comm-central/rev/57ba43a4ed08
Resolving as FIXED.

Comment 22

[Philip Chee]

> Pushed only the DOMMouseScroll part to comm-central:
> http://hg.mozilla.org/comm-central/rev/57ba43a4ed08
Actually I fixed the bit-rot (DOMMouseScroll -> wheel) before checking in.