827961 - nsHTMLMediaElement.cpp:3576:24: error: suggest parentheses around ‘&&’ within ‘||’ [-Werror=parentheses]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Daniel Holbert [:dholbert]]

New build warning, treated as error in enable-warnings-as-error builds with newish GCC versions (I'm using ver 4.7 on Ubuntu 12.10):

{
content/html/content/src/nsHTMLMediaElement.cpp: In member function ‘void nsHTMLMediaElement::UpdateAudioChannelPlayingState()’:

content/html/content/src/nsHTMLMediaElement.cpp:3576:24: error: suggest parentheses around ‘&&’ within ‘||’ [-Werror=parentheses]
}

Comment 1

[Daniel Holbert [:dholbert]]

(encountered this while building w/ --enable-application=b2g to make B2G desktop client, FWIW)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix: add the already-implicit parenthesis

The obvious fix is to add the (already implicit) parenthesis that the warning suggests.

I haven't dug into the logic here to see if this grouping is what's really intended -- Marco, does this grouping match what you were going for in bug 819852?

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> I haven't dug into the logic here to see if this grouping is what's really
> intended

From staring at this slightly more, I think this makes sense.

It looks like this code says we're playing audio IF:
 (1) we're not paused, AND EITHER:
   (a) we're looping (in which case we're playing forever, I guess)
   ...OR...
   (b) we have data AND we haven't reached the end of it yet.

And this bug's patch just adds explicit parens around the conditions in line (b) there.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 699368 [details] [diff] [review]
fix: add the already-implicit parenthesis

RyanVM told me this manifested as a build error on B2G builds in the b2g18 branch when bug 819852's patch landed there.
https://tbpl.mozilla.org/php/getParsedLog.php?id=18606563&tree=Mozilla-B2g18
(Not sure why that didn't happen on m-c -- we must have a different build config there, or maybe the comparable build is hidden or something.)

I suspect mchen might be asleep -- roc, perhaps you could review this, since you reviewed bug 819852?

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> (Not sure why that didn't happen on m-c -- we must have a different build
> config there, or maybe the comparable build is hidden or something.)

(ah, looks like the failure is for a desktop b2g build, and I don't think we have any of those for m-c TBPL. That's why this didn't cause any issues on m-c.)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 699368 [details] [diff] [review]
fix: add the already-implicit parenthesis

Review of attachment 699368 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely correct

Comment 7

[Daniel Holbert [:dholbert]]

Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/f48ed62a1dee

Comment 8

[Daniel Holbert [:dholbert]]

Landed on mozilla-b2g18 with a=bustage (didn't want to wait on release drivers, or eat up any of their time, since this fixes a currently-broken build on TPBL and it's a near-NPOTB change anyway (just adding parens that are already implied):
  https://hg.mozilla.org/releases/mozilla-b2g18/rev/81910ad56238

Comment 9

[Marco Chen [:mchen]]

Dear Daniel and Robert,

Thanks for your help here.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f48ed62a1dee