Closed Bug 390075 Opened 17 years ago Closed 17 years ago

OS/2 build break in nsMimeInfoOS2.cpp

Categories

(Core Graveyard :: File Handling, defect)

x86
OS/2
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: wuno, Assigned: mozilla)

References

Details

Attachments

(2 files)

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.
Blocks: 385065
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.  
No longer blocks: 385065
Depends on: 385065
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?
Hardware: Other → PC
Version: unspecified → Trunk
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.
Component: OS Integration → File Handling
Product: Firefox → Core
QA Contact: os.integration → file-handling
Target Milestone: --- → mozilla1.9 M11
Attached patch real fixSplinter Review
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?
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #293378 - Flags: superreview?(mozilla)
Attachment #293378 - Flags: review?
Attachment #293378 - Flags: review? → review?(wuno)
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?
Attachment #293378 - Flags: review?(wuno) → review+
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 :)
Attachment #293378 - Flags: superreview?(mozilla) → superreview+
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.]
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: