801579 - Don't mix nsresult with HRESULT

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jacek Caban]

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).

Comment 1

[Jacek Caban]

Attachment: widget/windows part

Comment 2

[Jacek Caban]

Attachment: accessible/src/msaa part

The important part is s/NS_OK/S_OK/, but I also fixed other warnings around.

Comment 3

[Jim Mathies [:jimm]]

Does this match up with the changes in bug 801471?

Comment 4

[Jacek Caban]

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.

Comment 5

[Trevor Saunders (:tbsaunde)]

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.

Comment 6

[Jim Mathies [:jimm]]

(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.

Comment 7

[Jacek Caban]

Attachment: widget/windows part, rebased

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.

Comment 8

[Jacek Caban]

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.

Comment 9

[Trevor Saunders (:tbsaunde)]

(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 10

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19132b752db1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19132b752db1

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 671458 [details] [diff] [review]
widget/windows part, rebased

looks good to me.

Comment 13

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/496d1304440c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/496d1304440c