OS/2 linking problem after mDeferredPositioner removal

RESOLVED FIXED

Status

--
major
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: wuno, Assigned: mozilla)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Updated

9 years ago
Blocks: 506103
Version: unspecified → Trunk

Comment 1

9 years ago
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?
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
Created attachment 391197 [details] [diff] [review]
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.
Assignee: mozilla → mozilla
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Attachment #391197 - Flags: review?(dragtext)
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
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...
Attachment #391197 - Flags: review?(dragtext) → review+
(Assignee)

Updated

9 years ago
Attachment #391197 - Flags: review+
(Assignee)

Comment 6

9 years ago
http://hg.mozilla.org/mozilla-central/rev/5db7a9e88542
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 7

9 years ago
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.
(Assignee)

Comment 8

9 years ago
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

9 years ago
Created attachment 391974 [details] [diff] [review]
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.
Attachment #391974 - Flags: review?(dragtext)
(Assignee)

Comment 10

9 years ago
Created attachment 392067 [details] [diff] [review]
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.
Attachment #392067 - Flags: review?(dragtext)

Updated

9 years ago
Attachment #391974 - Flags: review?(dragtext) → review+

Comment 11

9 years ago
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.
Attachment #392067 - Flags: review?(dragtext) → review+
(Assignee)

Comment 12

9 years ago
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.
(Assignee)

Comment 13

9 years ago
http://hg.mozilla.org/mozilla-central/rev/4e10c020e0b6
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

9 years ago
http://hg.mozilla.org/mozilla-central/rev/4e10c020e0b6
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Attachment #391974 - Attachment description: address Rich's comments → address Rich's comments [checkin comment 13]
(Assignee)

Updated

9 years ago
Attachment #391197 - Attachment description: try v1 → try v1 [checkin comment 6]
(Assignee)

Updated

9 years ago
Attachment #392067 - Attachment description: for 1.9.1? → for 1.9.1? [checked in comment 15]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.