506997 - OS/2 linking problem after mDeferredPositioner removal

Status: RESOLVED FIXED

(Core Graveyard :: Widget: OS/2, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.2a1pre) Gecko/20090725 Minefield/3.6a1pre
Build Identifier: 

E:\mozbuild1\staticlib\components\wdgtos2.lib(nsWindow.o) : error LNK2029: "nsWindow::DeferPosition(unsigned long, unsigned long, long, long, long, long, unsigned long)" : unresolved external
Seems that bug506103 either removed too much or too less
Those files still have references to DeferPosition
http://mxr.mozilla.org/mozilla-central/source/widget/src/os2/nsWindow.h#305
http://mxr.mozilla.org/mozilla-central/source/widget/src/os2/nsWindow.cpp#3355

Reproducible: Always

Comment 1

[Jim Mathies [:jimm]]

Uh oh, I guess I missed something in OS/2. Walter, do you have an OS/2 build you can use to dosome clean up or should I try and roll something myself?

Comment 2

[Peter Weilbacher]

The only use is in nsWindow::SetWindowPos and the if branch where DeferPosition is called can no longer occur, because mSWPs was only ever set in the now removed BeginResizingChildren.
So I guess we can get rid of SetWindowPos completely and use WinSetWindowPos directly. I'm trying to make a patch for that.

Comment 3

[Peter Weilbacher]

Attachment: try v1 [checkin comment 6]

This at least builds in widget again and links xul.dll and besides directly related stuff also removes an unused global variable.

On startup I get a crash, somewhere in Thebes, most likely because I only did a partial rebuild (don't have time tonight for full build). I don't think the crash is related to the code in question here.

Comment 4

[Peter Weilbacher]

Comment on attachment 391197 [details] [diff] [review]
try v1 [checkin comment 6]

Actually, I got a chance to complete the build after all. It runs and I don't see a difference compared to before.

Comment 5

[Peter Weilbacher]

Comment on attachment 391197 [details] [diff] [review]
try v1 [checkin comment 6]

This is a build break fix, so I don't know why I was asking for review at all...

Comment 6

[Peter Weilbacher]

http://hg.mozilla.org/mozilla-central/rev/5db7a9e88542

Comment 7

[Rich Walsh]

I'm sorry I didn't have the time to respond sooner because a couple of minor changes are needed:

1. there's a comment on lines 106-107 about deferred positioning that should be removed

2.
-      if( !SetWindowPos( 0, ptl.x, ptl.y, w, h, SWP_MOVE | SWP_SIZE))
-         if( aRepaint)
-            Invalidate(PR_FALSE);
+      WinSetWindowPos(GetMainWindow(), 0, ptl.x, ptl.y, w, h, SWP_MOVE | SWP_SIZE);
+      if (aRepaint)
+         Invalidate(PR_FALSE);

The original 'if (aRepaint)' was conditional on SetWindowPos() failing.  If that call succeeded it was unnecessary because the underlying WinSetWindowPos() invalidates the affected area by default.

Comment 8

[Peter Weilbacher]

No problem, I can still fix things. Comments are always welcome!

> 1. there's a comment on lines 106-107 about deferred positioning that should be
> removed

Will do.

> 2.
> -      if( !SetWindowPos( 0, ptl.x, ptl.y, w, h, SWP_MOVE | SWP_SIZE))
> -         if( aRepaint)
> -            Invalidate(PR_FALSE);
> +      WinSetWindowPos(GetMainWindow(), 0, ptl.x, ptl.y, w, h, SWP_MOVE |
> SWP_SIZE);
> +      if (aRepaint)
> +         Invalidate(PR_FALSE);
> 
> The original 'if (aRepaint)' was conditional on SetWindowPos() failing.  If
> that call succeeded it was unnecessary because the underlying WinSetWindowPos()
> invalidates the affected area by default.

But SetWindowPos() never failed, or at least never returned a possible failure of WinSetWindowPos(), but instead returned FALSE because it was not deferred. So I guess we were always doing more work than necessary...

Comment 9

[Peter Weilbacher]

Attachment: address Rich's comments [checkin comment 13]

I think this is what you meant. I didn't find any understandable info about the return codes of WinSetWindowPos() (what does FALSE really mean?), but I trust your expertise. And I didn't see any repaint problems with this patch.

Comment 10

[Peter Weilbacher]

Attachment: for 1.9.1? [checked in comment 15]

I wonder if we should then do a similar optimization on the 1.9.1 branch.

P.S.: Am I missing something or did the function GetSWPFlags() that
      returns its own argument never make sense? It's unchanged since 1999.

Comment 11

[Rich Walsh]

Comment on attachment 392067 [details] [diff] [review]
for 1.9.1? [checked in comment 15]

Don't change this patch, but what's the point of adding braces to single-line conditionals?  They strike me as visual clutter and invite mismatches during subsequent editing.

Comment 12

[Peter Weilbacher]

Without braces it looks so asymmetric and that always confuses me. I also like the braces because it allows to more easily add printfs for debugging. And when adding more code later I found that it indeed reduces the cases where I then forget to add braces which sometimes led to wrong logic.

Comment 13

[Peter Weilbacher]

http://hg.mozilla.org/mozilla-central/rev/4e10c020e0b6

Comment 14

[Peter Weilbacher]

http://hg.mozilla.org/mozilla-central/rev/4e10c020e0b6

Comment 15

[Peter Weilbacher]

Comment on attachment 392067 [details] [diff] [review]
for 1.9.1? [checked in comment 15]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b8231703062