User-Agent: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a7pre) Gecko/2007072922 Minefield/3.0a7pre Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a7pre) Gecko/2007072922 Minefield/3.0a7pre After the check-in for bug 385065, where a big part was moved from nsOSHelperAppService.cpp to nsMIMEInfoOS2.cpp the OS/2 build breaks nsMIMEInfoOS2.cpp E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp: In member function `virtual nsresult nsMIMEInfoOS2::LoadUriInternal(nsIURI*)': E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:159: error: `LOG' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:159: error: (Each undeclared identifier is reported only once for each function it appears in.) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:160: error: ` nsIPref' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:160: error: template argument 1 is invalid E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:160: error: ` NS_PREF_CONTRACTID' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:160: error: ISO C++ forbids declaration of `thePrefsService' with no type E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:167: error: ` kStandardURLCID' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:186: error: base operand of `->' is not a pointer E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:189: error: ` MAXINIPARAMLENGTH' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:193: error: ` szParamsFromINI' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:193: error: ` GetApplicationAndParametersFromINI' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:212: error: ` NS_UnescapeURL' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:245: error: ` nsIPrefBranch' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:245: error: template argument 1 is invalid E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:245: error: ISO C++ forbids declaration of `prefBranch' with no type E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:246: error: base operand of `->' is not a pointer E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:246: error: no matching function for call to `getter_AddRefs(int&)' E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:248: error: base operand of `->' is not a pointer E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:265: error: base operand of `->' is not a pointer E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:273: error: base operand of `->' is not a pointer E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:281: error: base operand of `->' is not a pointer E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:289: error: base operand of `->' is not a pointer E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:380: error: ` NO_ERROR' undeclared (first use this function) make.exe[5]: *** [nsMIMEInfoOS2.o] Error 1 Reproducible: Always Steps to Reproduce: 1. 2. 3. Actual Results: I was able to add some missing includes, but not to resolve everything. However with the patch I'll attach, at least Minefield builds. I guess Peter will find a better solution, as he has written the code affected

Peter, I uncommented two functions as I saw mLog and GetApplicationAndParametersFromINI not declared after adding the includes I found in nsOSHelperAppService.cpp

Yeah, I did write the code that was moved around. But that was not designed to be moved around and still use private methods of the class that it was moved out from. I don't understand why reviewers let patches like that pass. Probably they see *os2* in the filename and switch to the next file in the patch. :-( Blocking bug 385065 as suggested there.

(In reply to comment #2) > But that was not designed to be moved around and still use private methods of > the class that it was moved out from. I was also surprised that there was a move, cause in other regards they did not take care about OS/2. Though I don't understand very much of the code it seemed to me very OS/2 specific and likely to break after the move. That's why I asked https://bugzilla.mozilla.org/show_bug.cgi?id=385065#c36. Anyway, when I move the code back from nsMimeInfoOS2.cpp to nsOSHelperAppService.cpp I see the same error in generating nsOSHelperAppService.o that I encounter (without moving back) in nsMimeInfoOS2: E:/usr/src/mozilla/uriloader/exthandler/os2/nsOSHelperAppService.cpp: In member function `virtual nsresult nsMIMEInfoOS2::LoadUriInternal(nsIURI*)': E:/usr/src/mozilla/uriloader/exthandler/os2/nsOSHelperAppService.cpp:1580: error : `mLog' undeclared (first use this function) E:/usr/src/mozilla/uriloader/exthandler/os2/nsOSHelperAppService.cpp:1580: error: (Each undeclared identifier is reported only once for each function it appears in.) E:/usr/src/mozilla/uriloader/exthandler/os2/nsOSHelperAppService.cpp:1614: error : `GetApplicationAndParametersFromINI' undeclared (first use this function) Just FYI

There was today another checkin concerning the files here, see bug 389969. Therefore the OS/2 patch suggested in bug 388701 https://bugzilla.mozilla.org/attachment.cgi?id=274392 needs to be adjusted again (only a few lines). See e.g. http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/uriloader/exthandler/win&command=DIFF_FRAMESET&file=nsOSHelperAppService.cpp&rev1=1.74&rev2=1.75&root=/cvsroot and for nsOSHelperAppService.h as well

The recent message by dmose in .planning shows that there is still a lot of work to be done on the extension , so any better fix that we come up with now will probably be broken again by subsequent work. So I tend to think we should just get in the something along the lines of attachment 274399 [details] [diff] [review], leave this bug open, and revisit this in two to three weeks from now.

Of course I also by mistake checked in the older patch for GetProtocolInfoFromOS() last night... Well, now I have checked in a build break fix that should at least get us through compiling uriloader\exthandler and linking in docshell\build.

(In reply to comment #5) > The recent message by dmose in .planning shows that there is still a lot of > work to be done on the extension , so any better fix that we come up with now > will probably be broken again by subsequent work. So I tend to think we should > just get in the something along the lines of attachment 274399 [details] [diff] [review], leave this bug > open, and revisit this in two to three weeks from now. This sounds like the best plan to me. Sorry for all the bustage, but unfortunately we need to iterate quickly here for a bit.

Is this the code where we will need to fix problems where when files have spaces the code is not sent to helper apps with quotes?

Andy, I don't really know the answer to your question, but it could well be. I have tried to revisit this specific problem again (to really fix it this time) but haven't actually found out where the current hack fails. Everything seems to work. Or am I blind?

Some apps like ezip work fine with zip files that have spaces in the name but others like lucide when there are spaces in pdf files tries to pass each word as the file so "This is my file.pdf" would give 4 errors "This", "is", "my", "file.pdf" as file not found. Downloading the file and clicking open works fine.

Andy, if some applications work fine then it could also be the fault of those that don't work. You should be able to check this in Theseus. If you double click on the program to get the general information window you can then see the parameters at the end of that window. If the filename is enclosed in quotes then everything is fine, if not you have to specify exactly how Lucide was called and compare with eZip. But please file a new bug on this.

To get back to the original problem that we just solved by crippling some functionality: I think the best way to proceed is to just move GetApplicationAndParametersFromINI() out of the classes. It doesn't access any class variables, so there is actually no complelling reason to have it implemented as class method. If I do that then at least everything then compiles and links.

This seems to work. It also removes the obsolete nsIPref stuff and replaces it with nsIPrefService/nsIPrefBranch (I hope I got that right). wuno, do you have time to compile/test this?

Built with this patch fine, updating from it now. I'll open another bug on the quotes. Turns out we are not quoting, its just that some apps like Adobe 5 handle not having the quotes just fine, while most others do not.

(In reply to comment #13) > Created an attachment (id=293378) [details] > real fix > > This seems to work. It also removes the obsolete nsIPref stuff and replaces it > with nsIPrefService/nsIPrefBranch (I hope I got that right). > > wuno, do you have time to compile/test this? > Minefield compiles and runs fine so far and from my limited knowledge the code is also ok. However, in the current build with the patch the live bookmarks are hosed, i.e. showing dates/times and not the headlines. With your latest PMW-Firefox-trunk of 2007/12/09 the livemarks are all ok as they are with todays nightlies on windows and linux. Unfortunately I deleted my more recent builds and therefore I want to build it again without the patch to confirm that not the patch here is the cause. Peter, do you think the patch here could interfere with livemarks at all?

Peter, I rebuilt w/o the patch and the feed problem is still there. I'll file a new bug about that. Unfortunately I was not allowed to change the review request to r+ (: So you'll have to do that ;-)

Comment on attachment 293378 [details] [diff] [review] real fix Walter and Andy are both happy with this patch. mkaply, do you still want to take a look at it?

Oops, Mike wasn't CCd before...

mkaply, I would appreciate a second set of eyes on this patch because it uses all the XPCOM methods that I rarely get a chance to use otherwise...

Comment on attachment 293378 [details] [diff] [review] real fix XPCOM looks good to me. As long as it works :)

OK, great. Attachment 293378 [details] [diff] checked into trunk. So this is really fixed now. :-) [There are still some functional problems with the exthandler code but let's file new bugs about those.]