Status

()

Core
General
--
minor
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

(Depends on: 1 bug)

Trunk
PowerPC
Mac System 9.x
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [See comment 75], URL)

Attachments

(14 attachments, 9 obsolete attachments)

24.68 KB, patch
Details | Diff | Splinter Review
39.04 KB, patch
Details | Diff | Splinter Review
2.87 KB, patch
mano
: review+
Details | Diff | Splinter Review
13.59 KB, patch
Mark Mentovai
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
11.32 KB, patch
Darin Fisher
: review+
Details | Diff | Splinter Review
4.21 KB, patch
Details | Diff | Splinter Review
48.23 KB, patch
Details | Diff | Splinter Review
71.09 KB, patch
Details | Diff | Splinter Review
13.78 KB, patch
dveditz
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
7.97 KB, patch
Details | Diff | Splinter Review
13.72 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
2.91 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
22.48 KB, patch
cbarrett
: review+
Details | Diff | Splinter Review
577 bytes, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
It'll make it easier to read and get rid of some Mac OS X runtime code if we
clean up the XP_MAC stuff in XPCOM.
(Assignee)

Comment 1

13 years ago
Created attachment 174016 [details] [diff] [review]
cleanup v1.0
Assignee: general → joshmoz
Status: NEW → ASSIGNED
Attachment #174016 - Flags: review?(me)
(Assignee)

Updated

13 years ago
Summary: remove XP_MAC code from XPCOM → remove XP_MAC code from xpcom and js
(Assignee)

Updated

13 years ago
Attachment #174016 - Attachment is obsolete: true
Attachment #174016 - Flags: review?(me)
(Assignee)

Comment 2

13 years ago
Created attachment 174020 [details] [diff] [review]
xpcom cleanup v1.1
Attachment #174020 - Flags: review?(me)
(Assignee)

Comment 3

13 years ago
Created attachment 174021 [details] [diff] [review]
js cleanup v1.0
Attachment #174021 - Flags: review?(me)
(Assignee)

Updated

13 years ago
Summary: remove XP_MAC code from xpcom and js → remove XP_MAC code from xpcom, js, and layout
(Assignee)

Comment 4

13 years ago
Created attachment 174023 [details] [diff] [review]
layout cleanup v1.0
Attachment #174023 - Flags: review?(me)
Assignee: joshmoz → qa-mozilla
Status: ASSIGNED → NEW
xpcom is taken care in another bug.
Summary: remove XP_MAC code from xpcom, js, and layout → remove XP_MAC code from js, and layout
Comment on attachment 174020 [details] [diff] [review]
xpcom cleanup v1.1

Geoff no need to review this one. see bug 280247
Attachment #174020 - Flags: review?(me)
Comment on attachment 174023 [details] [diff] [review]
layout cleanup v1.0

asking review. We don't build Mac since 1.3.
Attachment #174023 - Flags: review?(me) → review?(bzbarsky)
Attachment #174021 - Flags: review?(me) → review?(brendan)
As per Irc discussion asking r to brendan fro the js part and ccing shaver.
So have drivers generally OK'ed this?
brendan I posted your question in <1gru8hk.j17n9af4h7awN%ludonews@nerim.net>.
(In reply to comment #9)
> So have drivers generally OK'ed this?

as of right now, no.
Ludovic, I couldn't find your post in
news://news.mozilla.org/netscape.public.mozilla.jseng -- can you give a google
groups link to it?

/be
(In reply to comment #12)
> Ludovic, I couldn't find your post in
> news://news.mozilla.org/netscape.public.mozilla.jseng -- can you give a google
> groups link to it?
> 
> /be

http://groups-beta.google.com/group/netscape.public.mozilla.jseng/msg/198d36139ddcad7a
So basically, I'd like to hear that this idea has driver approval before I spend
time reviewing any code changes...
Comment on attachment 174023 [details] [diff] [review]
layout cleanup v1.0

Please re-request review once it's clear that this is the policy we want to be
following...
Attachment #174023 - Flags: review?(bzbarsky)

Comment 16

13 years ago
It sounds like there may be some modules that still want to support Mac OS 9.
This will probably require individual module owner approvals from the affected
areas. 
Created attachment 180868 [details] [diff] [review]
js cleanup v2.0
Attachment #174021 - Attachment is obsolete: true
Attachment #180868 - Flags: review?(brendan)
(Assignee)

Comment 18

12 years ago
Created attachment 188164 [details] [diff] [review]
remove UNIVERSAL_INTERFACES_VERSION macros, some XP_MAC
[Checked in: Comment 21]

All versions of Mac OS X that we support define UNIVERSAL_INTERFACES_VERSION to
be 0x0400, so ifdefs using that can be removed. This patch also removes some
large XP_MAC section. Patch prompted by finding lots of clutter in lxr. I've
tested buildng Firefox with this patch and it works fine. Very safe.
Attachment #188164 - Flags: superreview?(bryner)
Attachment #188164 - Flags: review?(sfraser_bugs)

Updated

12 years ago
Attachment #188164 - Flags: review?(sfraser_bugs) → review+
Attachment #188164 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 19

12 years ago
Comment on attachment 188164 [details] [diff] [review]
remove UNIVERSAL_INTERFACES_VERSION macros, some XP_MAC
[Checked in: Comment 21]

asking for approval since this is a very safe patch - doesn't mess with code
that we actually compile
Attachment #188164 - Flags: approval1.8b3?

Comment 20

12 years ago
Comment on attachment 188164 [details] [diff] [review]
remove UNIVERSAL_INTERFACES_VERSION macros, some XP_MAC
[Checked in: Comment 21]

a=bsmedberg, please land expeditiously
Attachment #188164 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 21

12 years ago
Comment on attachment 188164 [details] [diff] [review]
remove UNIVERSAL_INTERFACES_VERSION macros, some XP_MAC
[Checked in: Comment 21]

this patch was landed, marking it obsolete to remove clutter
Attachment #188164 - Attachment is obsolete: true
(Assignee)

Comment 22

12 years ago
Created attachment 188600 [details] [diff] [review]
updated patch to remove XP_MAC from js
Attachment #180868 - Attachment is obsolete: true
(Assignee)

Comment 23

12 years ago
checked in patch to remove all XP_MAC from js
(Assignee)

Comment 24

12 years ago
All XP_MAC removal stuff should go in this bug. Closing 248039, 280247, 281889.
Please put component and patch version number in patch attachment names.
Summary: remove XP_MAC code from js, and layout → remove XP_MAC code
(Assignee)

Comment 25

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

Comment 26

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

Comment 27

12 years ago
*** Bug 196105 has been marked as a duplicate of this bug. ***
Attachment #174021 - Flags: review?(brendan)
Attachment #180868 - Flags: review?(brendan)
Attachment #188164 - Attachment description: remove UNIVERSAL_INTERFACES_VERSION macros, some XP_MAC → remove UNIVERSAL_INTERFACES_VERSION macros, some XP_MAC [Checked in: Comment 21]

Updated

12 years ago
Blocks: 283572
Created attachment 188836 [details] [diff] [review]
(Dv1a) <xpfe/*> (Sync./Moved from bug 196105)

I have no compiler: Could you compile/test/(super-)review/check in this patch ?

Thanks.

{{
Index: mozilla/xpfe/components/autocomplete/src/nsLDAPAutoCompleteSession.cpp
-// build system.  The MOZ_LDAP_XPCOM preprocessor symbol is only 
-// defined on Mac because noone else needs this weirdness; thus 
}}
I wonder if |MOZ_LDAP_XPCOM| could be removed too (other bug) then ?

{{ (from bug 196105)
 ------- Additional Comment #21 From Serge GAUTHERIE  2005-02-06 14:21 PDT 
[reply] -------

(In reply to comment #20)
> (From update of attachment 173538 [details] [diff] [review] [edit] [edit])
> Why is there no XP_MACOSX code in showOSAlert.cpp?

I would have no idea: I'm not a Mac user, and I assume that the current code
works as it is/was.
Should these XP_MAC be replaced by XP_MACOSX, instead of removed ?
Any hint are/will welcomed: "helpwanted".
}}
Attachment #188836 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #188836 - Flags: review?(sfraser_bugs)
Attachment #188836 - Attachment description: (Dv1a) <xpfe/*> (Updated/Moved from bug 281889) → (Dv1a) <xpfe/*> (Sync./Moved from bug 196105)

Comment 29

12 years ago
Comment on attachment 188836 [details] [diff] [review]
(Dv1a) <xpfe/*> (Sync./Moved from bug 196105)

Index: mozilla/xpfe/bootstrap/nsAppRunner.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/bootstrap/nsAppRunner.cpp,v
retrieving revision 1.441
diff -u -p -r1.441 nsAppRunner.cpp
--- mozilla/xpfe/bootstrap/nsAppRunner.cpp	30 Apr 2005 12:39:25 -0000     
1.441
+++ mozilla/xpfe/bootstrap/nsAppRunner.cpp	10 Jul 2005 11:42:18 -0000

 //	  nsISplashScreen will be removed.
 //

-#if !defined(XP_WIN) && !defined(XP_OS2)&& !defined( XP_BEOS ) &&
!defined(MOZ_WIDGET_GTK) && !defined(MOZ_WIDGET_GTK2) && !defined(XP_MAC) &&
(!defined(XP_MACOSX) || defined(MOZ_WIDGET_COCOA))
+#if !defined(XP_WIN) && !defined(XP_OS2) && !defined(XP_BEOS) &&
!defined(MOZ_WIDGET_GTK) && !defined(MOZ_WIDGET_GTK2) && (!defined(XP_MACOSX)
|| defined(MOZ_WIDGET_COCOA))

Can we turn this #ifdef around, and test for platforms that DO want this?

 nsresult NS_CreateNativeAppSupport(nsINativeAppSupport **aResult)
 {
@@ -395,10 +362,6 @@ static int TranslateReturnValue(nsresult
   return 1;
 }

Index: mozilla/xpfe/bootstrap/showOSAlert.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/bootstrap/showOSAlert.cpp,v
retrieving revision 1.10
diff -u -p -r1.10 showOSAlert.cpp
--- mozilla/xpfe/bootstrap/showOSAlert.cpp	20 Aug 2004 15:01:40 -0000     
1.10
+++ mozilla/xpfe/bootstrap/showOSAlert.cpp	10 Jul 2005 11:42:18 -0000
@@ -43,9 +43,6 @@
 //defines and includes for previous installation cleanup process
 #if defined (XP_WIN)
 #include <windows.h>
-#elif defined (XP_MAC)
-#include <Dialogs.h>
-#include <TextUtils.h>
 #elif defined (MOZ_WIDGET_GTK)
 #include <gtk/gtk.h>
 #elif defined (XP_OS2)
@@ -195,9 +192,6 @@ printf("\n****Inside ShowOSAlert ***\n")

 #if defined (XP_WIN)
     MessageBox(NULL, message_copy, NULL, MB_OK | MB_ICONERROR |
MB_SETFOREGROUND );
-#elif (XP_MAC)
-    short buttonClicked;
-    StandardAlert(kAlertStopAlert, c2pstr(message_copy), nil, nil,
&buttonClicked);
 #elif defined (MOZ_WIDGET_GTK)
     NS_gtk_alert(message_copy, NULL, "OK");
 #elif defined (XP_OS2)

Is there a Mac OS X block here at all? Not sure if this is used, but we might
want it.

Index: mozilla/xpfe/components/bookmarks/src/nsBookmarksService.h
===================================================================
RCS file: /cvsroot/mozilla/xpfe/components/bookmarks/src/nsBookmarksService.h,v
retrieving revision 1.39
diff -u -p -r1.39 nsBookmarksService.h
--- mozilla/xpfe/components/bookmarks/src/nsBookmarksService.h	17 Apr 2004
16:51:21 -0000	    1.39
+++ mozilla/xpfe/components/bookmarks/src/nsBookmarksService.h	10 Jul 2005
11:42:20 -0000
@@ -63,7 +63,7 @@
 class nsIOutputStream;

 #ifdef DEBUG
-#if defined(XP_MAC) || defined(XP_MACOSX)
+#if defined(XP_MACOSX)
 #include <Timer.h>
 #endif
 #endif

Probably don't need Timer.h any more.
(In reply to comment #29)
> (From update of attachment 188836 [details] [diff] [review] [edit])

Thanks for your comments.

Helpwanted: looking for someone with a MacOSX compiler to answer/test...
Keywords: helpwanted

Comment 31

12 years ago
Changes made to prmjtime.c, related to this bug, have broken Mac Date object,
try this on Win or Mac:
new Date().getTimezoneOffset()
On Win it returns 420, on Mac 0.
Filed bug 309392 on that (so we can request the right blocking flags).
Depends on: 309392
(Assignee)

Comment 33

12 years ago
Created attachment 200789 [details] [diff] [review]
browser component, also fix XP_MACSOX bug v1.0
Attachment #200789 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #200789 - Flags: review? → review?(mconnor)
Comment on attachment 200789 [details] [diff] [review]
browser component, also fix XP_MACSOX bug v1.0

>Index: components/bookmarks/src/nsBookmarksService.h
>===================================================================

>-#if defined(XP_MAC) || defined(XP_MACOSX)
>+#if defined(XP_MACOSX)

>Index: components/migration/src/nsDogbertProfileMigrator.h
>===================================================================

> 
>-#if defined(XP_MAC) || defined(XP_MACOSX)
>+#if defined(XP_MACOSX)

make those |#ifdef XP_MACOSX|.

r=mano.
Attachment #200789 - Flags: review?(mconnor) → review+
(Assignee)

Comment 35

12 years ago
"browser component, also fix XP_MACSOX bug v1.0" landed on trunk
(Assignee)

Comment 36

12 years ago
Created attachment 202051 [details] [diff] [review]
nsObjectFrame and some plugin cleanup v1.0
[Checked in: Comment 40]
Attachment #202051 - Flags: review?(mark)
(Assignee)

Updated

12 years ago
Attachment #202051 - Flags: superreview?(sfraser_bugs)

Comment 37

12 years ago
Comment on attachment 202051 [details] [diff] [review]
nsObjectFrame and some plugin cleanup v1.0
[Checked in: Comment 40]

--- layout/generic/nsObjectFrame.cpp	4 Nov 2005 02:38:32 -0000	1.531
+++ layout/generic/nsObjectFrame.cpp	6 Nov 2005 23:58:55 -0000

[...]
@@ -3090,7 +3086,7 @@
 
 nsresult nsPluginInstanceOwner::DispatchFocusToPlugin(nsIDOMEvent* aFocusEvent)
 {
-#if !(defined(XP_MAC) || defined(XP_MACOSX))
+#if !defined(XP_MACOSX)

Use #ifndef.

@@ -3209,7 +3205,7 @@
 
 nsresult nsPluginInstanceOwner::KeyPress(nsIDOMEvent* aKeyEvent)
 {
-#if defined(XP_MAC) || defined(XP_MACOSX) // send KeyPress events only on Mac
+#if defined(XP_MACOSX) // send KeyPress events only on Mac

Use #ifdef.

There are many other locations in this file that you're changing that should use #ifndef and #ifdef, too.

--- modules/plugin/base/src/nsPluginHostImpl.cpp	20 Oct 2005 00:28:35 -0000	1.537
+++ modules/plugin/base/src/nsPluginHostImpl.cpp	6 Nov 2005 23:59:01 -0000
@@ -4729,7 +4729,7 @@
 
   // On Windows, we also want to include the Quicktime plugin from the 4.x directory
   // But because it spans several DLL's, the best check for now is by filename
-  if (nsnull != PL_strcasestr(tag->mFileName,"npqtplugin"))
+  if (PL_strcasestr(tag->mFileName,"npqtplugin") != nsnull)

We've got constant-first comparisons elsewhere in that file, why are you only changing this one?  All or none, I think, unless you're working with a specific area of code.


--- modules/plugin/base/src/nsPluginsDirDarwin.cpp	7 Aug 2005 07:54:50 -0000	1.8
+++ modules/plugin/base/src/nsPluginsDirDarwin.cpp	6 Nov 2005 23:59:05 -0000

@@ -53,19 +53,16 @@
 #include "nsPluginsDirUtils.h"
 
 #include "nsILocalFileMac.h"
-#include <Carbon/Carbon.h>
+
 #include <string.h>
 #include <stdio.h>
 #include <unistd.h>
 #include <fcntl.h>
+
+#include <Carbon/Carbon.h>
 #include <mach-o/loader.h>
 #include <mach-o/fat.h>
 
-#include <CFURL.h>
-#include <CFBundle.h>
-#include <CFString.h>
-#include <CodeFragments.h>


Although Carbon/Carbon.h eventually brings in CoreFoundation headers, it feels wrong to me.  I'd be happier here if you added CoreServices/CoreServices.h to the mix.

These are trivial changes, I'd say you can make them on checkin.
Attachment #202051 - Flags: review?(mark) → review+
(Assignee)

Comment 38

12 years ago
For the constant-first check, that is just there because I did it on a whim. Not necessary, but no good reason to take it out of the patch.

As for #ifndef, I don't like using that because it looks too much like #ifdef. The equivalent sytax I used makes it more clear at a glance.

I'll add the corefoundation include on checkin.

Thanks for the review.

Updated

12 years ago
Attachment #202051 - Flags: superreview?(sfraser_bugs) → superreview+

Comment 39

12 years ago
Comment on attachment 202051 [details] [diff] [review]
nsObjectFrame and some plugin cleanup v1.0
[Checked in: Comment 40]

r+ is still valid after reading your comments upon my comments.  Does that make r++?
(Assignee)

Comment 40

12 years ago
landed "nsObjectFrame and some plugin cleanup v1.0" on the trunk with comments taken into account
Attachment #202051 - Attachment description: nsObjectFrame and some plugin cleanup v1.0 → nsObjectFrame and some plugin cleanup v1.0 [Checked in: Comment 40]
Attachment #202051 - Attachment is obsolete: true

Comment 41

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

Comment 42

11 years ago
Created attachment 229123 [details] [diff] [review]
plugin cleanup
Assignee: qa-mozilla → joshmoz
Status: NEW → ASSIGNED
Attachment #229123 - Flags: review?(mark)
(Assignee)

Comment 43

11 years ago
note - the plugin cleanup patch includes a comment change to indicate that nsplugin.h does not in fact supercede npapi.h.
(Assignee)

Updated

11 years ago
Attachment #188836 - Flags: superreview?(neil)
Attachment #188836 - Flags: review?(sfraser_bugs)
(Assignee)

Comment 44

11 years ago
Comment on attachment 229123 [details] [diff] [review]
plugin cleanup

there aren't any actual code changes here, just going straight for sr
Attachment #229123 - Flags: review?(mark) → superreview?(dbaron)
(Assignee)

Updated

11 years ago
Attachment #229123 - Flags: superreview?(mikepinkerton)
Attachment #229123 - Flags: superreview?(dbaron)
Attachment #229123 - Flags: review?(mark)

Comment 45

11 years ago
Comment on attachment 229123 [details] [diff] [review]
plugin cleanup

>Index: public/nsplugin.h
>- * <P>This superscedes the old plugin API (npapi.h, npupp.h), and 
>+ * This interface was an attempt to supercede npapi.h, npupp.h but it failed. It

Fix spelling: supersede.
Attachment #229123 - Flags: review?(mark) → review+
Comment on attachment 229123 [details] [diff] [review]
plugin cleanup

sr=pink
Attachment #229123 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 47

11 years ago
plugin cleanup landed on trunk
(Assignee)

Comment 48

11 years ago
Created attachment 245682 [details] [diff] [review]
fix v1.0

Going straight to sr since this is simple.
Attachment #245682 - Flags: superreview?
(Assignee)

Updated

11 years ago
Attachment #245682 - Flags: superreview? → superreview?(darin.moz)
(Assignee)

Comment 49

11 years ago
Created attachment 245684 [details] [diff] [review]
network fix v1.0.1

missed a file
Attachment #245682 - Attachment is obsolete: true
Attachment #245684 - Flags: review?(darin.moz)
Attachment #245682 - Flags: superreview?(darin.moz)

Updated

11 years ago
Attachment #245684 - Flags: review?(darin.moz) → review+
(Assignee)

Comment 50

11 years ago
network fix v1.0.1 checked in on trunk
(Assignee)

Comment 51

11 years ago
Created attachment 245696 [details] [diff] [review]
mail fix v1.0

has r from bienvenu, landed on trunk
(Assignee)

Comment 52

11 years ago
Created attachment 245703 [details] [diff] [review]
mailnews fix v1.0

r=bienvenu, landed on trunk
(Assignee)

Comment 53

11 years ago
Created attachment 245745 [details] [diff] [review]
xpcom fix v1.0

r=bsmedberg, landed on trunk
(Assignee)

Updated

11 years ago
Attachment #174020 - Attachment is obsolete: true
(Assignee)

Comment 54

11 years ago
Created attachment 245780 [details] [diff] [review]
xpinstall fix v1.0
Attachment #245780 - Flags: review?(dveditz)
Comment on attachment 245780 [details] [diff] [review]
xpinstall fix v1.0

r=dveditz
Attachment #245780 - Flags: review?(dveditz) → review+
(Assignee)

Updated

11 years ago
Attachment #245780 - Flags: superreview?(mikepinkerton)
Comment on attachment 245780 [details] [diff] [review]
xpinstall fix v1.0

rs=pink
Attachment #245780 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 57

11 years ago
"xpinstall fix v1.0" landed on trunk
(Assignee)

Comment 58

11 years ago
Created attachment 257382 [details] [diff] [review]
nsExternalHelperAppService fix v1.0
Attachment #257382 - Flags: review?(benjamin)
Comment on attachment 257382 [details] [diff] [review]
nsExternalHelperAppService fix v1.0

r+sr=bzbarsky
Attachment #257382 - Flags: superreview+
Attachment #257382 - Flags: review?(benjamin)
Attachment #257382 - Flags: review+
(Assignee)

Comment 60

11 years ago
"nsExternalHelperAppService fix v1.0" landed on trunk
Created attachment 272355 [details] [diff] [review]
more mailnews v1.0
Attachment #272355 - Flags: superreview?(bienvenu)
Attachment #272355 - Flags: review?(bienvenu)

Comment 62

10 years ago
Comment on attachment 272355 [details] [diff] [review]
more mailnews v1.0

thx, Phil
Attachment #272355 - Flags: superreview?(bienvenu)
Attachment #272355 - Flags: superreview+
Attachment #272355 - Flags: review?(bienvenu)
Attachment #272355 - Flags: review+
"more mailnews v1.0" landed on trunk

Updated

10 years ago
Component: General → General
Product: Mozilla Application Suite → Core
QA Contact: general → general
Created attachment 272437 [details] [diff] [review]
more mailnews, part deux v1.0

Having missed these would be less embarrassing if not for the fact that one of them is the file that made me notice them, by triggering a compiler warning about trigraphs not being enabled.
Attachment #272437 - Flags: superreview?(bienvenu)
Attachment #272437 - Flags: review?(bienvenu)

Comment 65

10 years ago
Comment on attachment 272437 [details] [diff] [review]
more mailnews, part deux v1.0

thx, Phil.
Attachment #272437 - Flags: superreview?(bienvenu)
Attachment #272437 - Flags: superreview+
Attachment #272437 - Flags: review?(bienvenu)
Attachment #272437 - Flags: review+
"more mailnews, part deux v1.0" landed on the trunk
(Assignee)

Comment 67

10 years ago
Created attachment 283027 [details] [diff] [review]
more layout,content v1.0
Attachment #283027 - Flags: review?(cbarrett)
Comment on attachment 283027 [details] [diff] [review]
more layout,content v1.0

Two nits:

1) if it were me, I would leave the |#pragma mark -|s in, because they add a horizontal separator to the list of functions in my editor, making it easier to find things. Up to you though. (Xcode also behaves this way fwiw)

2) File a bug about those comments that say "mac can't process events while dragging", since we can do that now (I know you asked steven in IRC, but just file a bug so it doesn't get completely lost)

Otherwise, r+. Good to get this out of the tree. (perhaps for moz2 we can move back to just "mac" instead of MAC_OS_X and "cocoa")
Attachment #283027 - Flags: review?(cbarrett) → review+
(Assignee)

Comment 69

10 years ago
As for the stuff about event processing, those are separate bugs I plan to file once I have time to test more.
(Assignee)

Updated

10 years ago
Attachment #283027 - Flags: superreview?(roc)
Attachment #283027 - Flags: approval1.9?
Attachment #283027 - Flags: superreview?(roc)
Attachment #283027 - Flags: superreview+
Attachment #283027 - Flags: approval1.9?
Attachment #283027 - Flags: approval1.9+
(Assignee)

Comment 70

10 years ago
"more layout,content v1.0" checked in

Comment 71

10 years ago
Josh - should this bug still be open?
The URL field says "yes, it should be open, 558 times," and the fact that cairo-platform.h is one of the things listed says it should probably be higher priority, to get the dead ones out so we can see newer mistakes - since cairo's vastly more recent than our System 9 support, that seems likely to have intended to be active for XP_MACOSX but isn't.
Depends on: 400188
(Assignee)

Comment 73

10 years ago
Mike - either we leave this open and keep using it as a catch-all, or we just have people file individual bugs for component-specific patches from here on out. I don't care either way, at this point I don't see a compelling reason to change what we're doing with this bug. If anyone feels strongly the other way, fine with me. Just comment here giving a reason and if it makes sense I'll close this out.

Comment 74

10 years ago
I think the general trend of smaller bugs for individual issues is more widely followed on BMO and functions more effectively overall.  If we're voting, I'd vote to close this one or evolve it to a meta-bug with smaller bugs depending on it.
(Assignee)

Comment 75

10 years ago
Fine with me. I don't want to do the meta bug thing though, there is really no point to keeping track of them collectively. The "problem" of remaining XP_MAC is small enough at this point that we hardly care any more anyway.

So - if your patch just removes XP_MAC from a component file a new bug from now on.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 76

10 years ago
Uh, but who's going to file all those bugs, let alone fix them? Who cares enough?
Depends on: 400605
Comment on attachment 188836 [details] [diff] [review]
(Dv1a) <xpfe/*> (Sync./Moved from bug 196105)

Moved to bug 400605.
Attachment #188836 - Attachment is obsolete: true
Created attachment 285660 [details] [diff] [review]
Fix/update a (/mailnews/) case mismatch (v1)

Fixes the one line which is not found by the URL (case sensitive) query.
Attachment #285660 - Flags: superreview?(bienvenu)
Attachment #285660 - Flags: review?(bienvenu)

Comment 79

10 years ago
Comment on attachment 285660 [details] [diff] [review]
Fix/update a (/mailnews/) case mismatch (v1)

the comment describes the change that was made - it was correct...if you want to remove the comment, that's OK, but changing it isn't right...
Attachment #285660 - Flags: superreview?(bienvenu)
Attachment #285660 - Flags: superreview-
Attachment #285660 - Flags: review?(bienvenu)
Attachment #285660 - Flags: review-
Attachment #285660 - Attachment description: Fix/update a (/mailnews/) case mismatch → Fix/update a (/mailnews/) case mismatch (v1)
Attachment #285660 - Attachment is obsolete: true
Created attachment 285747 [details] [diff] [review]
Remove a (/mailnews/) case mismatch (v2)
[Checkin: comment 83]

Per comment 79.
Attachment #285747 - Flags: superreview?(bienvenu)
Attachment #285747 - Flags: review?(bienvenu)

Comment 81

10 years ago
Comment on attachment 285747 [details] [diff] [review]
Remove a (/mailnews/) case mismatch (v2)
[Checkin: comment 83]

thx for the patch
Attachment #285747 - Flags: superreview?(bienvenu)
Attachment #285747 - Flags: superreview+
Attachment #285747 - Flags: review?(bienvenu)
Attachment #285747 - Flags: review+

Comment 82

10 years ago
adding checkin-needed keyword
Keywords: checkin-needed
Checking in mailnews/compose/src/nsMsgAppleDoubleEncode.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgAppleDoubleEncode.cpp,v  <--  nsMsgAppleDoubleEncode.cpp
new revision: 1.26; previous revision: 1.25
done
Keywords: checkin-needed
Attachment #285747 - Attachment description: Remove a (/mailnews/) case mismatch (v2) → Remove a (/mailnews/) case mismatch (v2) [Checkin: comment 83]
Depends on: 460310
Depends on: 461911
Depends on: 461912
Depends on: 461913
Depends on: 224012
Depends on: 479480
Depends on: 476996, 480745
Depends on: 495228
Depends on: 509533

Updated

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