Closed Bug 637111 Opened 9 years ago Closed 9 years ago

[OS/2] eliminate copy to clipboard at shutdown

Categories

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

x86
OS/2
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla5

People

(Reporter: dragtext, Assigned: dragtext)

Details

Attachments

(1 file, 1 obsolete file)

This patch eliminates a long-standing annoyance.  It also does some minor code maintenance by eliminating dependence on OS/2-specific types in nsClipboard.h.
Attached patch patch os2/nsClipboard.* (obsolete) — Splinter Review
Attachment #515429 - Flags: review?(wuno)
Any idea what the logic was behind this originally?
(In reply to comment #2)
> Any idea what the logic was behind this originally?

Not really, however the code was long ago in nsApprunner. bug239390 moved it into nsClipboard for OS/2 and Windows. The Windows code was removed later in bug402439 - our code stayed :-(
Comment on attachment 515429 [details] [diff] [review]
patch os2/nsClipboard.*

Rich, could you please get your .hgrc set up that your patches have 8 lines context?

>diff --git a/widget/src/os2/nsClipboard.cpp b/widget/src/os2/nsClipboard.cpp
>--- a/widget/src/os2/nsClipboard.cpp
>+++ b/widget/src/os2/nsClipboard.cpp
>@@ -44,13 +44,16 @@
> #include "nsPrimitiveHelpers.h"
> #include "nsXPIDLString.h"
> #include "prmem.h"
>-#include "nsIObserverService.h"

> #include "nsIServiceManager.h"
please remove this as well, not needed anymore

> #include "nsOS2Uni.h"
> #include "nsClipboard.h"

> #include "mozilla/Services.h"
this can also be removed
> 
>-inline ULONG RegisterClipboardFormat(PCSZ pcszFormat)
>+#define INCL_DOSERRORS
>+#define INCL_WIN
>+#include <os2.h>
Just that I want to learn something, why is it better to have these defines in the cpp file rather than in the h file?

>-NS_IMPL_ISUPPORTS_INHERITED1(nsClipboard, nsBaseClipboard, nsIObserver)

>+NS_IMPL_ISUPPORTS1(nsClipboard, nsBaseClipboard)
This is not needed, see https://bugzilla.mozilla.org/show_bug.cgi?id=402439#c6

>-  ULONG ulFormatID = GetFormatID( aFlavor );
>+  PRUint32 ulFormatID = GetFormatID( aFlavor );
Nit: Could you remove the space between the brackets and "aFlavor"
>-  PVOID pClipboardData = reinterpret_cast<PVOID>(WinQueryClipbrdData( 0, ulFormatID ));
>+  PVOID pClipboardData = reinterpret_cast<PVOID>(WinQueryClipbrdData( 0, aFormatID ));
Nit: again please remove the spaces between the brackets and arguments - we should do it every when we patch files with that old fashioned style
>-  ULONG ulFormatID = GetFormatID( aFlavor );
>+  PRUint32 ulFormatID = GetFormatID( aFlavor );
Nit: spaces ...

>diff --git a/widget/src/os2/nsClipboard.h b/widget/src/os2/nsClipboard.h
>--- a/widget/src/os2/nsClipboard.h
>+++ b/widget/src/os2/nsClipboard.h
>@@ -62,10 +55,7 @@ public:
>   virtual ~nsClipboard();
> 
>   // nsISupports
>-  NS_DECL_ISUPPORTS_INHERITED
>-
>-  // nsIObserver
>-  NS_DECL_NSIOBSERVER
>+  NS_DECL_ISUPPORTS
not needed, it's already defined, see https://bugzilla.mozilla.org/show_bug.cgi?id=402439#c6
(the bug that removed the stuff from windows)

r+ with the nits fixed
Attachment #515429 - Flags: review?(wuno) → review+
carrying over r+
Attachment #515429 - Attachment is obsolete: true
Attachment #517182 - Flags: review+
(In reply to comment #4)
>
> >+#define INCL_DOSERRORS
> >+#define INCL_WIN
> >+#include <os2.h>
>
> Just that I want to learn something, why is it better to have these defines
> in the cpp file rather than in the h file?

Say you #include multiple headers for a given module, and each one has its own |#define INCL_| and |#include os2.h|.  The first such header will determine which parts of os2.h are actually included.  Since this may not include those parts the current module needs, you get build errors.  This leads one to add #define INCL_'s to the offending header that aren't needed anywhere but the current module - which in turn leads to a big mess.

The solution is to accumulate all of the INCL_xxxx that each header needs in the c/cpp file, #include os2.h, then #include all of the headers that rely on OS/2-specific definitions.  Where possible, a better solution is to avoid the problem by eliminating all OS/2-specific typedefs from the header.  That's what I did with nsClipboard.h (e.g. substituting PRUint32 for ULONG).
Thanks for the explanation
Keywords: checkin-needed
Whiteboard: a=NPOTB, OS/2 files only
http://hg.mozilla.org/projects/cedar/rev/8bda9434ed31
Keywords: checkin-needed
Whiteboard: a=NPOTB, OS/2 files only → fixed-in-cedar
http://hg.mozilla.org/mozilla-central/rev/3ac1470a20bd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
Keywords: checkin-needed
Whiteboard: mozilla-2.0 a=NPOTB OS/2 files only
(In reply to comment #9)
> http://hg.mozilla.org/mozilla-central/rev/3ac1470a20bd

|Walter Meinl: Keywords: checkin-needed; Whiteboard: mozilla-2.0 a=NPOTB OS/2 files only|

Fwiw, m-2.0 will have no other tagged release. I suggest if you plan to create an OS/2 release off that, that you create a tracker for whatever bugs you need landed and try to convince someone to land for you. As there will be no tagged release that incorporates this. [Firedrill/Chemspill/etc. will be based off the relbranch, and even for NPOTB you need explicit approval to land on a relbranch]
(In reply to comment #10)
> (In reply to comment #9)
> > http://hg.mozilla.org/mozilla-central/rev/3ac1470a20bd
> 
> |Walter Meinl: Keywords: checkin-needed; Whiteboard: mozilla-2.0 a=NPOTB
> OS/2 files only|
> 
> Fwiw, m-2.0 will have no other tagged release. I suggest if you plan to
> create an OS/2 release off that, that you create a tracker for whatever bugs
> you need landed and try to convince someone to land for you. 
But if I got it right the upcoming Seamonkey-2.1 release pulls mozilla-2.0. I guess it will pull the tip, or are there plans that it will pull from a mozilla-2.0 release branch? If Seamonkey uses the tip the checkin would be still helpful for us even if no further firefox release will be tagged.
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > http://hg.mozilla.org/mozilla-central/rev/3ac1470a20bd
> > 
> > |Walter Meinl: Keywords: checkin-needed; Whiteboard: mozilla-2.0 a=NPOTB
> > OS/2 files only|
> > 
> > Fwiw, m-2.0 will have no other tagged release. I suggest if you plan to
> > create an OS/2 release off that, that you create a tracker for whatever bugs
> > you need landed and try to convince someone to land for you. 
> But if I got it right the upcoming Seamonkey-2.1 release pulls mozilla-2.0.
> I guess it will pull the tip, or are there plans that it will pull from a
> mozilla-2.0 release branch? If Seamonkey uses the tip the checkin would be
> still helpful for us even if no further firefox release will be tagged.
Answer to myself: SeaMonkey 2.1 will be released using the mozilla-2.0 relbranch. According to comment #10 I cleared all checkin requests. Asking approval for a checkin into the mozilla-2.0 relbranch is not possible.
Keywords: checkin-needed
Whiteboard: mozilla-2.0 a=NPOTB OS/2 files only
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.