Don't mix nsresult with HRESULT

RESOLVED FIXED in mozilla19

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jacek Caban, Assigned: Jacek Caban)

Tracking

Trunk
mozilla19
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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).
(Assignee)

Comment 1

6 years ago
Created attachment 671373 [details] [diff] [review]
widget/windows part
Attachment #671373 - Flags: review?
(Assignee)

Comment 2

6 years ago
Created attachment 671375 [details] [diff] [review]
accessible/src/msaa part

The important part is s/NS_OK/S_OK/, but I also fixed other warnings around.
Attachment #671375 - Flags: review?(trev.saunders)
(Assignee)

Updated

6 years ago
Attachment #671373 - Flags: review? → review?(jmathies)

Comment 3

6 years ago
Does this match up with the changes in bug 801471?
(Assignee)

Comment 4

6 years ago
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.

Updated

6 years ago
No longer blocks: 779473
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+

Comment 6

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
Created attachment 671458 [details] [diff] [review]
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.
Attachment #671373 - Attachment is obsolete: true
Attachment #671373 - Flags: review?(jmathies)
Attachment #671458 - Flags: review?(jmathies)
(Assignee)

Comment 8

6 years ago
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 12

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.