Closed Bug 1273079 Opened 4 years ago Closed 4 years ago

Use MOZ_MUST_USE in hal

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: wcpan, Assigned: wcpan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

It would be better to check return values for some functions, e.g. LockScreenOrientation().
Assignee: nobody → wpan
Blocks: MOZ_MUST_USE
Comment on attachment 8752743 [details]
MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto

https://reviewboard.mozilla.org/r/52762/#review49962

Looks good to me with only one small nit.

::: dom/base/ScreenOrientation.cpp:557
(Diff revision 1)
>  {
>    if (aOrientation == eScreenOrientation_None) {
>      hal::UnlockScreenOrientation();
>    } else {
> -    hal::LockScreenOrientation(aOrientation);
> +    bool rv = hal::LockScreenOrientation(aOrientation);
> +    NS_WARN_IF(!rv);

nit: You can use `NS_WARN_IF_FALSE(rv)` here
Attachment #8752743 - Flags: review?(gsvelto) → review+
Comment on attachment 8752743 [details]
MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52762/diff/1-2/
(In reply to Gabriele Svelto [:gsvelto] from comment #2)
> Comment on attachment 8752743 [details]
> MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto
> 
> https://reviewboard.mozilla.org/r/52762/#review49962
> 
> Looks good to me with only one small nit.
> 
> ::: dom/base/ScreenOrientation.cpp:557
> (Diff revision 1)
> >  {
> >    if (aOrientation == eScreenOrientation_None) {
> >      hal::UnlockScreenOrientation();
> >    } else {
> > -    hal::LockScreenOrientation(aOrientation);
> > +    bool rv = hal::LockScreenOrientation(aOrientation);
> > +    NS_WARN_IF(!rv);
> 
> nit: You can use `NS_WARN_IF_FALSE(rv)` here

Just a note that NS_WARN_IF_FALSE is noop in release build. Although it doesn't change the semantic here, beware the difference when you want to use it next time.

See also bug 1272010
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #4)
> Just a note that NS_WARN_IF_FALSE is noop in release build. Although it
> doesn't change the semantic here, beware the difference when you want to use
> it next time.
> 
> See also bug 1272010

Thanks for the heads up. In this case I think this behavior is fine as this is an error very unlikely to occur.
Comment on attachment 8752743 [details]
MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52762/diff/2-3/
https://reviewboard.mozilla.org/r/52762/#review49990

Silly me for not spotting the issue in the previous review :-/
I should compile it first anyway.
I thought NS_WARN_IF_FALSE was just a negative version of NS_WARN_IF.
Comment on attachment 8752743 [details]
MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52762/diff/3-4/
Ouch, I didn't know that release build will turn on warning-as-error.

:gsvelto, I guess maybe I need to use the first patch. At least it passed try-build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e175325ee2f4
https://treeherder.mozilla.org/#/jobs?repo=try&revision=950ad9b7ddf0
Flags: needinfo?(wpan) → needinfo?(gsvelto)
(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #14)
> :gsvelto, I guess maybe I need to use the first patch. At least it passed
> try-build.

Yes please, always run a try build on all architectures in both debug and release mode before landing. You never know what might break.
Flags: needinfo?(gsvelto)
Comment on attachment 8752743 [details]
MozReview Request: Bug 1273079 - Use MOZ_MUST_USE in hal. r?gsvelto

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52762/diff/4-5/
https://hg.mozilla.org/mozilla-central/rev/fbfd273902ac
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.