Last Comment Bug 306839 - events not properly consumed
: events not properly consumed
Status: RESOLVED FIXED
: fixed1.8
Product: Core Graveyard
Classification: Graveyard
Component: Embedding: GTK Widget (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Christian Persch (GNOME) (away; not receiving bug mail)
: Stuart Parmenter
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-02 07:00 PDT by Christian Persch (GNOME) (away; not receiving bug mail)
Modified: 2012-04-05 00:46 PDT (History)
0 users
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (2.17 KB, patch)
2005-09-02 07:01 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
mpgritti: review-
Details | Diff | Splinter Review
updated patch with review comments (7.25 KB, patch)
2005-09-06 05:19 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
mpgritti: review+
Details | Diff | Splinter Review
fix return value too (7.38 KB, patch)
2005-09-16 02:06 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
chpe: review+
roc: superreview+
mtschrep: approval1.8b5+
Details | Diff | Splinter Review

Description Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-02 07:00:43 PDT
The DOM focus listener methods don't mark events as consumed if the handler
returns TRUE.
Comment 1 Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-02 07:01:20 PDT
Created attachment 194654 [details] [diff] [review]
fix
Comment 2 Marco Pesenti Gritti 2005-09-06 01:44:19 PDT
Comment on attachment 194654 [details] [diff] [review]
fix

Patch looks fine. There are some inconsistencies in that file that would be
good to cleanup though, while we are at it.

 73 // All of the event listeners below return NS_OK to indicate that the
 74 // event should not be consumed in the default case.

I think we should kill this now, it's not true for all the events.

-  // return NS_OK to this function to mark this event as not
-  // consumed...

You are removing these for focus event but leaving them around for all other
events.
I'd say we can just keep this but s/NS_OK/FALSE
Comment 3 Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-06 05:19:11 PDT
Created attachment 195005 [details] [diff] [review]
updated patch with review comments

I changed the comments, as discussed on irc.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-15 18:10:13 PDT
Remind me why it makes sense to return PR_FALSE for an nsresult?
Comment 5 Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-16 01:55:25 PDT
(In reply to comment #4)
> Remind me why it makes sense to return PR_FALSE for an nsresult?

It does not make sense, we should fix that in a follow-up.
Comment 6 Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-16 02:06:55 PDT
Created attachment 196299 [details] [diff] [review]
fix return value too

Let's just put it in this patch too. I've looked in cvs history and it was
fixed for the other event listeners in the same steps as
PreventDefault/StopPropagation in bug 245529 so it makes sense to include this
here. Assuming r=marco, sr=roc still stands, asking for a1.8b5.
Comment 7 Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-16 02:08:35 PDT
Comment on attachment 196299 [details] [diff] [review]
fix return value too

Sorry I was confused with other bugs, asking for sr first :)
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-17 21:04:37 PDT
Make sure you get this on the branch!
Comment 9 Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-18 02:19:14 PDT
Comment on attachment 196299 [details] [diff] [review]
fix return value too

gtk embedding only fix
Comment 10 Mike Schroepfer 2005-09-19 15:05:29 PDT
Comment on attachment 196299 [details] [diff] [review]
fix return value too

Approved for 1.8b5 per bug meeting
Comment 11 Mike Schroepfer 2005-09-22 12:04:36 PDT
If you want this in for 1.5b2 you'll need to land before lockdown in 12 days.
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-23 10:58:44 PDT
trunk:
Checking in embedding/browser/gtk/src/EmbedEventListener.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedEventListener.cpp,v  <-- 
EmbedEventListener.cpp
new revision: 1.8; previous revision: 1.7
done

MOZILLA_1_8_BRANCH:
Checking in embedding/browser/gtk/src/EmbedEventListener.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedEventListener.cpp,v  <-- 
EmbedEventListener.cpp
new revision: 1.7.28.1; previous revision: 1.7
done

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