Closed Bug 191072 Opened 17 years ago Closed 17 years ago

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

Categories

(Core :: XUL, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: iamawalrus, Assigned: iamawalrus)

References

Details

Attachments

(1 file, 1 obsolete file)

10.08 KB, patch
Details | Diff | Splinter Review
To compatible with exsiting non-xembed plugins and make the focus of these
plugins work.
Attached patch patch (obsolete) — Splinter Review
Depends on: 188987
Blocks: 188987
No longer depends on: 188987
No longer depends on: gtk2
Blocks: gtk2
No longer blocks: 188987
Blocks: 188987
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-
Attached patch patchSplinter Review
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
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
Closed: 17 years ago
Resolution: --- → FIXED
Thanks!
No longer blocks: 188987
*** Bug 188987 has been marked as a duplicate of this bug. ***
*** Bug 187280 has been marked as a duplicate of this bug. ***
Attachment #114633 - Flags: review?(blizzard)
Depends on: 506254
You need to log in before you can comment on or make changes to this bug.