Chris Lawson (gone) Reporter
	Description • 19 years ago

Patch coming to fix more missing casts and similar warnings.

	Chris Lawson (gone) Reporter
	Comment 1 • 19 years ago

There are a few remaining warnings in the code that I (mostly) have no idea how to fix or whether they're even important. I'm posting them here:

mozilla/camino/src/embedding/CHDownloadProgressDisplay.mm:52: warning: '-release' not found in protocol(s)
mozilla/camino/src/embedding/CHDownloadProgressDisplay.mm:61: warning: '-retain' not found in protocol(s)

mozilla/camino/src/browser/RemoteDataProvider.mm:70: warning: '-retain' not found in protocol(s)
mozilla/camino/src/browser/RemoteDataProvider.mm:77: warning: '-release' not found in protocol(s)

mozilla/camino/src/embedding/CHBrowserListener.mm:352: warning: 'GetMainDevice' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Quickdraw.h:4873)
mozilla/camino/src/embedding/CHBrowserListener.mm:352: warning: 'GetMainDevice' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Quickdraw.h:4873)
mozilla/camino/src/embedding/CHBrowserListener.mm:403: warning: 'GetMainDevice' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Quickdraw.h:4873)
mozilla/camino/src/embedding/CHBrowserListener.mm:403: warning: 'GetMainDevice' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Quickdraw.h:4873)
(These are probably 10.4-only; we should keep it them mind for whatever version drops support for 10.3.)

mozilla/camino/src/extensions/ExtendedOutlineView.mm:613: warning: 'NSOutlineView' may not respond to '-_sendDelegateDidClickColumn:'
(This may be a 10.2-only workaround issue that we can drop after 1.0.)

mozilla/camino/src/browser/KeychainService.mm:225: warning: converting to non-pointer type 'char' from NULL
mozilla/camino/src/browser/KeychainService.mm:234: warning: converting to non-pointer type 'char' from NULL

mozilla/camino/src/extensions/ExtendedTableView.mm:120: warning: no '-tableView:contextMenuForRow:' method found

	Stuart Morgan Assignee
	Comment 3 • 19 years ago

Comment on attachment 215473 [details] [diff] [review]
fixes warnings and a spelling error

Stealing review from Mark in order to say:

 - (void)setupFontSampleOfType:(NSString*)fontType fromDict:(NSDictionary*)regionDict
 {
   NSFont *foundFont = [self getFontOfType:fontType fromDict:regionDict];
-  [self setFontSampleOfType:fontType withFont:foundFont andDict:regionDict];
+  [self setFontSampleOfType:fontType withFont:foundFont andDict:(NSMutableDictionary*)regionDict];
 }
 
 - (void)setFontSampleOfType:(NSString *)fontType withFont:(NSFont*)font andDict:(NSMutableDictionary*)regionDict

Dear lord no. Never, ever, ever cast a random NSFoo to NSMutableFoo.  It's an invitation to pain and suffering later. The cast is highlighting a bad assumption in the code.

setFontSampleOfType:withFont:andDict:
doesn't appear to ever change the dictionary (which may be the only reason this has never exploded), so the correct fix is to change the argument type.

	Stuart Morgan Assignee
	Comment 5 • 19 years ago

Comment on attachment 215522 [details] [diff] [review]
updated per Stuart's comment, fixes last two warnings in my comment 1

Sorry, I missed a few other things because the cast bothered me so much.

>-  buffer[actualSize] = NULL;
>+  buffer[actualSize] = 0;
...
>-  buffer[actualSize] = NULL;
>+  buffer[actualSize] = 0;

0 isn't really any better than NULL. It's a character array, so set it to a character explicitly:
    buffer[actualSize] = '\0';

>+-(NSMenu*)tableView:(NSTableView *)aTableView contextMenuForRow:(int)rowIndex;
...
>+#import "BookmarkViewController.h"
...
>     if ([delegate respondsToSelector:@selector(tableView:contextMenuForRow:)])
>-      return [delegate tableView:self contextMenuForRow:rowIndex];
>+      return [(BookmarkViewController*)delegate tableView:self contextMenuForRow:rowIndex];

This isn't okay. ExtendedTableView is a general-puprpose class, and you are making it directly dependent on a specific other class.  This is exactly what protocols are designed to avoid--if you really want to formalize the relationship the right way to do this is to make a protocol with that method and make BVC conform to the protocol.

However, since it is already using introspection to verify the presence of the method, and has a fallback return value, I don't think it needs to be changed at all.

> - (void) setURLs:(NSArray*)inUrls withTitles:(NSArray*)inTitles
> {
>   // Best format that we know about is Safari's URL + title arrays - build these up
>-  NSMutableArray* tmpTitleArray = inTitles;
>+  NSMutableArray* tmpTitleArray = (NSMutableArray*)inTitles;
>   if (!inTitles) {
>     tmpTitleArray = [NSMutableArray array];
>     for ( unsigned int i = 0; i < [inUrls count]; ++i )
>       [tmpTitleArray addObject:@""];
>   }

Oops, missed this too.  Once again, no dice. This will need to be restructured a tiny bit; the only reason it needs to be mutable is in the case where inTitles is nil, so the logic should be something like:
NSArray* tmpTitleArray = inTitles;
if (!tmpTitleArray) {
  //declare and create an NSMutableArray, and fill it with stuff
  //tmpTitleArray = the new NSMutableArray
}

	Stuart Morgan Assignee
	Comment 6 • 19 years ago

By the way, the deprecation warnings are because the entirety of quickdraw is officially deprecated in Tiger. There are presumably a metric ton of deprecation warnings about QD when building the entire project including core.

	Stuart Morgan Assignee
	Comment 7 • 19 years ago

(In reply to comment #1)
> There are a few remaining warnings in the code that I (mostly) have no idea how
> to fix or whether they're even important. I'm posting them here:
> 
> mozilla/camino/src/embedding/CHDownloadProgressDisplay.mm:52: warning:
> '-release' not found in protocol(s)
> mozilla/camino/src/embedding/CHDownloadProgressDisplay.mm:61: warning:
> '-retain' not found in protocol(s)
> 
> mozilla/camino/src/browser/RemoteDataProvider.mm:70: warning: '-retain' not
> found in protocol(s)
> mozilla/camino/src/browser/RemoteDataProvider.mm:77: warning: '-release' not
> found in protocol(s)

In both cases, this is an object that is being accessed entirely in terms of a protocol, but having methods sent to it (retain and release) that are not in fact part of the protocol.  You can fix it by adding retain and release to the protocols in question (CHDownloadDisplayFactory and RemoteLoadListener)

	Mark Mentovai
	Comment 8 • 19 years ago

Why do that when you can do this:

@protocol CHDownloadDisplayFactory <NSObject>

to bring -release and -retain in from NSObject, where they're supposed to come from.

	Stuart Morgan Assignee
	Comment 9 • 19 years ago

(In reply to comment #8)
> Why do that when you can do this:

Because I'm dumb, and forgot there's an NSObject protocol? ;)

	Chris Lawson (gone) Reporter
	Comment 10 • 19 years ago

(In reply to comment #6)
> By the way, the deprecation warnings are because the entirety of quickdraw is
> officially deprecated in Tiger. There are presumably a metric ton of
> deprecation warnings about QD when building the entire project including core.

Yep :)

Would it be appropriate to replace those calls in Camino with CGMainDisplayID (the Quartz equivalent of QD's GetMainDevice)? I can't seem to find anything on Apple's site about when CGMainDisplayID first appeared, though.

cl

	Chris Lawson (gone) Reporter
	Comment 11 • 19 years ago

Found it:

http://developer.apple.com/documentation/GraphicsImaging/Reference/Quartz_Services_Ref/Reference/reference.html

cl

	Stuart Morgan Assignee
	Comment 12 • 19 years ago

(In reply to comment #10)
> Would it be appropriate to replace those calls in Camino with CGMainDisplayID
> (the Quartz equivalent of QD's GetMainDevice)? I can't seem to find anything on
> Apple's site about when CGMainDisplayID first appeared, though.

Probably switching to CGMainDisplayID+CGDisplayBounds makes sense, but you'd have to tweak a bit of the rect-extracting code, and you'd have to be very sure that you still ended up with the same idea of what the coordinates were.  That would also mean that testing this would require a multi-monitor set-up and some run-time comparison of values to be sure you didn't regress anything, so I would suggest not doing it in this bug.

	Smokey Ardisson (offline for a while; not following bugs - do not email)
	Comment 13 • 18 years ago

This is the old patches police.  

What's the status of this bug?  Is there a fix for the last patch that will make it worthwhile (i.e., not involving the QD/Quartz stuff)?

	Stuart Morgan Assignee
	Comment 14 • 18 years ago

(In reply to comment #13)
> Is there a fix for the last patch that will
> make it worthwhile (i.e., not involving the QD/Quartz stuff)?

Making the changes from comment 5 and comment 8

	Stuart Morgan Assignee
	Comment 15 • 18 years ago

The last patch is largely obsolete; this covers most of the current warnings.

	Mark Mentovai
	Comment 16 • 18 years ago

Comment on attachment 264540 [details] [diff] [review]
updated fixes

>+// TODO: This code modifies sub-dictionaries of regionDict, which works only
>+// because they happen to have been constructed as mutableDictionaries. This
>+// API (and likely others in this class) should be re-worked to either remove or
>+// enforce that assumption.
>+- (void)setFontSampleOfType:(NSString *)fontType withFont:(NSFont*)font andDict:(NSDictionary*)regionDict

Oh my.  There has to be a better way!

	Stuart Morgan Assignee
	Comment 17 • 18 years ago

(In reply to comment #16)
> Oh my.  There has to be a better way!

Yeah, it's pretty skanky, but fixing it is out of the scope of this bug; I just wanted to make sure we don't forget.

	Mike Pinkerton (not reading bugmail)
	Comment 19 • 18 years ago

Comment on attachment 264540 [details] [diff] [review]
updated fixes

sr=pink

	Stuart Morgan Assignee
	Comment 20 • 18 years ago

Checked in on trunk and MOZILLA_1_8_BRANCH