[gtk2] make focus of non-xembed plugin work for moz+gtk2

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
16 years ago
9 years ago

People

(Reporter: Robin Lu, Assigned: Robin Lu)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

10.08 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

16 years ago
To compatible with exsiting non-xembed plugins and make the focus of these
plugins work.
(Assignee)

Comment 1

16 years ago
Created attachment 112954 [details] [diff] [review]
patch
(Assignee)

Updated

16 years ago
Depends on: 188987
(Assignee)

Updated

16 years ago
Blocks: 188987
No longer depends on: 188987
(Assignee)

Updated

16 years ago
No longer depends on: 92033
(Assignee)

Updated

16 years ago
Blocks: 92033
No longer blocks: 188987
(Assignee)

Updated

16 years ago
Blocks: 188987
(Assignee)

Updated

16 years ago
Attachment #112954 - Flags: review?(blizzard)
Comment on attachment 112954 [details] [diff] [review]
patch

The patch here looks fine.  All my comments are regarding style.

>+    LOGFOCUS(("nsWindow::LoseNonXEmbedPluginFocus\n"));
>+    // This method is only for the nsWindow which contains a
>+    // Non-XEmbed plugin, for example, JAVA plugin.
>+    if(gPluginFocusWindow!=this) {
>+        return;
>+    }

This should be:

if (gPluginFocusWindow != this) {
    return;
}

note the use of whitespace.


>+    if(!curFocusWindow|| 
>+        curFocusWindow==GDK_WINDOW_XWINDOW(mDrawingarea->inner_window)){
>+        gdk_error_trap_push();

Also:

if (!curFocusWindow ||
     curFocusWindow == GDK_WINDOW_XWINDOW(mDrawingarea->inner_window)) {
     gdk_error_trap_push();
...

You should probably also add some veritcal whitespace here to try to break up
the lines.  Think of coding as sentences and paragraphs (there might be a
cultural difference here.)

I like this a lot more than the old code, fwiw. :)
Attachment #112954 - Flags: review?(blizzard) → review-
(Assignee)

Comment 3

16 years ago
Created attachment 114633 [details] [diff] [review]
patch

rearranged according to the comments and added some comments.
With this patch, we could seperate the treatments of xembed and non-xembed
plugins a little easier and cleaner in the future. It might be a long time for
that two kinds of plugins coexist. I know you are working at the xembed one so
I add nothing for it in my patch.
Attachment #112954 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #114633 - Flags: review?(blizzard)
There were still style problems in this patch, but I fixed them and checked it
in instead of wasting time and making us go around again.  Just FYI there were
still things like this:

if(foo) {

instead of:

if (foo) {

and some of this as well:

if (foo){

and even some of this:

if ( foo ) {
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

16 years ago
Thanks!
(Assignee)

Updated

16 years ago
No longer blocks: 188987
(Assignee)

Comment 6

16 years ago
*** Bug 188987 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

16 years ago
*** Bug 187280 has been marked as a duplicate of this bug. ***
Attachment #114633 - Flags: review?(blizzard)

Updated

9 years ago
Depends on: 506254
You need to log in before you can comment on or make changes to this bug.