Closed Bug 801579 Opened 8 years ago Closed 8 years ago

Don't mix nsresult with HRESULT

Categories

(Core :: XPCOM, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jacek, Assigned: jacek)

References

Details

Attachments

(2 files, 1 obsolete file)

This is follow-up of bug 779473, with changes for Windows-only code for fixing that were not found by MSVC build, but found by GCC (mingw).
Attached patch widget/windows part (obsolete) — Splinter Review
Attachment #671373 - Flags: review?
The important part is s/NS_OK/S_OK/, but I also fixed other warnings around.
Attachment #671375 - Flags: review?(trev.saunders)
Attachment #671373 - Flags: review? → review?(jmathies)
Does this match up with the changes in bug 801471?
Yes, I didn't find it because it wasn't in 779473 dependences. My patches fix a few more warnings in accessibility and make nsDataObj.cpp a bit cleaner by changing GetDownloadDetails to return HRESULT, but both bugs fix the same problem. I'm not sure if I should resolve it as duplicate in this case.
No longer blocks: nsresult-enum-class
Depends on: 801471
Comment on attachment 671375 [details] [diff] [review]
accessible/src/msaa part

As already noted this overlaps with other bugs, but I'm fine with this.

I'm actually sort of suprised all the msCOM goo actually builds on mingw, iirc we have open bug for a11y not building there, but I guess that's not the case.
Attachment #671375 - Flags: review?(trev.saunders) → review+
(In reply to Jacek Caban from comment #4)
> Yes, I didn't find it because it wasn't in 779473 dependences. My patches
> fix a few more warnings in accessibility and make nsDataObj.cpp a bit
> cleaner by changing GetDownloadDetails to return HRESULT, but both bugs fix
> the same problem. I'm not sure if I should resolve it as duplicate in this
> case.

If your patch to data obj makes additional improvements then merging with the changes on inbound and posting a new patch for review sounds like a good idea. I can do a review by applying this patch on top what just landed on inbound, I just want to make sure this patch is up to date and will apply cleanly.
This is a rebased version of previous patch. It changes GetDownloadDetails to return HRESULT directly, since each caller converts it anyway (nothing major, but nice to have clean up, IMO). It also contains one 'const' fix.
Attachment #671373 - Attachment is obsolete: true
Attachment #671373 - Flags: review?(jmathies)
Attachment #671458 - Flags: review?(jmathies)
Thanks for the review.

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> I'm actually sort of suprised all the msCOM goo actually builds on mingw,
> iirc we have open bug for a11y not building there, but I guess that's not
> the case.

Plain accessibility part of m-c can't compile with plain mingw-w64 part yet. I did some work to make it possible (among others integrating widl from Wine project with mingw-w64 to replace midl) and I have it compiling locally. I need to find some time to make a few more widl fixes amd once this is upstreamed, I will submit m-c patch. I have, however, builds working locally and since this particular patch wasn't mingw-specific, I decided to submit it already.
(In reply to Jacek Caban from comment #8)
> Thanks for the review.
> 
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > I'm actually sort of suprised all the msCOM goo actually builds on mingw,
> > iirc we have open bug for a11y not building there, but I guess that's not
> > the case.

thanks for fixing this stuff.

> Plain accessibility part of m-c can't compile with plain mingw-w64 part yet.
> I did some work to make it possible (among others integrating widl from Wine
> project with mingw-w64 to replace midl) and I have it compiling locally. I
> need to find some time to make a few more widl fixes amd once this is
> upstreamed, I will submit m-c patch. I have, however, builds working locally
> and since this particular patch wasn't mingw-specific, I decided to submit
> it already.

sounds great.  Personally I'm mostly interested because it would be convienent for me to be able to build windows a11y code on linux which I suspect will be close to doable with your patches.
Comment on attachment 671458 [details] [diff] [review]
widget/windows part, rebased

looks good to me.
Attachment #671458 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/496d1304440c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.