418105 - Remove non-cairo Mac gfx code from the tree

Status: RESOLVED FIXED

Description

[Peter Weilbacher]

The whole tree in gfx/src/mac is still there and should be removed, together with references to it.

I found references to this directory only in toolkit/toolkit-makefiles.sh and xpfe/bootstrap/appleevents/Makefile.in.

Comment 2

[Peter Weilbacher]

Created attachment 304855 [details] [diff] [review]
remove it and the remaining references

I don't have a mac and so can't really test this but as I am in the mood for more cleanup/removal stuff, here is the patch. Only the first two hunks are changes to files that will stay, the rest is the content of the files to be CVS removed.

I found a reference to gfx_mac in embedding/tests/cocoaEmbed/CocoaEmbed.pbproj/project.pbxproj but as that seems to be a generated file I didn't want to touch it. Anything else that I might have missed?

Comment 3

[Josh Aas]

Comment on attachment 304855 [details] [diff] [review]
remove it and the remaining references

Why not also remove nsMacGFX.rsrc? And everything in the printerplugin directory?

The current patch doesn't compile because of the following error:

mozilla/xpfe/bootstrap/appleevents/nsAEWindowClass.cpp:55:24: error: nsGfxUtils.h: No such file or directory
mozilla/xpfe/bootstrap/appleevents/nsAEWindowClass.cpp: In member function ‘void AEWindowClass::SetWindowProperties(OpaqueWindowPtr*, const AEDesc*)’:
mozilla/xpfe/bootstrap/appleevents/nsAEWindowClass.cpp:776: error: ‘StPortSetter’ was not declared in this scope
mozilla/xpfe/bootstrap/appleevents/nsAEWindowClass.cpp:776: error: expected `;' before ‘portSetter’

Comment 4

[Peter Weilbacher]

(In reply to comment #3)
> (From update of attachment 304855 [details] [diff] [review])
> Why not also remove nsMacGFX.rsrc? And everything in the printerplugin
> directory?

Those are going to be removed. But as they are binary files, they only show up as e.g. "Files gfx/src/mac/nsMacGFX.rsrc and /dev/null differ" in the patch.

> The current patch doesn't compile because of the following error:
> 
> mozilla/xpfe/bootstrap/appleevents/nsAEWindowClass.cpp:55:24: error:
> nsGfxUtils.h: No such file or directory
> mozilla/xpfe/bootstrap/appleevents/nsAEWindowClass.cpp: In member function
> ‘void AEWindowClass::SetWindowProperties(OpaqueWindowPtr*, const AEDesc*)’:
> mozilla/xpfe/bootstrap/appleevents/nsAEWindowClass.cpp:776: error:
> ‘StPortSetter’ was not declared in this scope
> mozilla/xpfe/bootstrap/appleevents/nsAEWindowClass.cpp:776: error: expected `;'
> before ‘portSetter’

Josh, can you suggest as a fix for this? Moving nsGfxUtils.h to xpfe/bootstrap/appleevents/ doesn't seem like a good solution. Should I move the StPortSetter class definition from nsGfxUtils.h to nsAEWindowClass.h instead? As I don't have a Mac I can't verify that this will work.

Comment 5

[Josh Aas]

Just move stportsetter to nsAEWindowClass and remove the reference from the other file there, which doesn't seem to actually use it.

Comment 6

[Peter Weilbacher]

Created attachment 306616 [details] [diff] [review]
patch v2

OK, so this should be better.

Comment 7

[Josh Aas]

Peter - I get a lot of warnings about the patch, trailing CRs...

Comment 8

[Peter Weilbacher]

Ah, sorry. I created this one on OS/2 and the editor must have saved it with DOS linebreaks. (I wonder why they are not transformed to the native format when you download the patch from Bugzilla.) Is there a tool to convert such files to Unix linebreaks on Mac? Otherwise, if you are worried about that, I can upload a new version.

Comment 9

[Peter Weilbacher]

Created attachment 306744 [details] [diff] [review]
patch v3

OK, I remembered the try server and uploaded this patch (together with the one for bug418104). It took a few trials to get it right but this one now really builds and uses Unix-type linebreaks.
If someone wants to try the resulting try server Mac build, it's in
https://build.mozilla.org/tryserver-builds/2008-03-01_07:[e-mail]-macwingfxremoval3/

Comment 10

[Josh Aas]

+// the following was moved here from old Mac GFX headers

This comment is useless, please remove it.

+inline PRBool CurrentPortIsWMPort()
+{
+	return PR_FALSE;
+}

What is the point of this? Just remove that and fix up the logic that uses it. You can remove that function and this code that uses it since it'll never be true:

+	// if we have a window, but we're set to the WM port, things are bad
+	if (CurrentPortIsWMPort() && (FrontWindow() != nil))
+		return false;

Otherwise this compiles and works fine, thanks. Please post a new patch with that stuff taken care of.

Comment 11

[Peter Weilbacher]

(In reply to comment #10)
> +// the following was moved here from old Mac GFX headers
> 
> This comment is useless, please remove it.

Well, it would tell people where the code that I'm simply moving from somewhere else comes from, so that they could do more CVS archeology. In my experience this is not easily apparent otherwise. But OK, I personally will never again look at this, so I'll do whatever you want...

Comment 12

[Peter Weilbacher]

Created attachment 306925 [details] [diff] [review]
v4

Comments addressed.

Comment 13

[Josh Aas]

Comment on attachment 306925 [details] [diff] [review]
v4

Thanks, it is nice to have this cleaned up. Makes things less confusing for potential contributors and cleans up mxr results.

Comment 14

[Peter Weilbacher]

Vladimir, will you have time to sr this? If not who else should? This blocks a wanted-1.9 bug, so I would like to get it in before 1.9b5.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306925 [details] [diff] [review]
v4

a1.9=beltzner

Comment 16

[Peter Weilbacher]

Changes checked into trunk and all files under gfx/src/mac cvs removed.