Last Comment Bug 711839 - --disable-ogg configure option breaks build after bug 689834
: --disable-ogg configure option breaks build after bug 689834
Status: VERIFIED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla12
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 689834
  Show dependency treegraph
 
Reported: 2011-12-18 08:23 PST by :aceman
Modified: 2012-02-23 08:16 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix compile (3.25 KB, patch)
2012-01-18 11:15 PST, :aceman
bzbarsky: review+
Details | Diff | Review

Description :aceman 2011-12-18 08:23:16 PST
Since 16th dec 2011 I get this compile error when using --disable-ogg in configure (I am building Thunderbird so ogg really isn't needed):

/usr/local/TB-hg/mozilla/content/xml/document/src/nsXMLContentSink.cpp: In member function 'nsresult nsXMLContentSink::HandleStartElement(const PRUnichar*, const PRUnichar**, PRUint32, PRInt
/usr/local/TB-hg/mozilla/content/xml/document/src/nsXMLContentSink.cpp:1081:33: error: 'audio' is not a member of 'nsGkAtoms'
/usr/local/TB-hg/mozilla/content/xml/document/src/nsXMLContentSink.cpp:1082:33: error: 'video' is not a member of 'nsGkAtoms'
gmake[8]: *** [nsXMLContentSink.o] Error 1
Comment 1 Jon Buckley 2012-01-17 19:37:54 PST
The problem is that nsGkAtoms::audio and nsGkAtoms::video are #ifdef MOZ_MEDIA. They need to be wrapped inside an #ifdef MOZ_MEDIA, like in http://mxr.mozilla.org/mozilla-central/source/content/xml/document/src/nsXMLContentSink.cpp#567
Comment 2 :aceman 2012-01-18 01:44:36 PST
Lines 1081 and 1082 should be wrapped in #ifdef MOZ_MEDIA ? OK, I can try that.
Comment 3 :aceman 2012-01-18 11:15:03 PST
Created attachment 589580 [details] [diff] [review]
fix compile

It needed some more places to fix, but thanks for the hint.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-18 19:00:09 PST
Comment on attachment 589580 [details] [diff] [review]
fix compile

r=me
Comment 5 Jon Buckley 2012-01-18 19:14:04 PST
Completely optional, but if you wanted to make the patch look a little cleaner, you could move the nsGkAtoms::audio and nsGkAtoms::video if statement checks into the middle of the if block, so that you don't have the || hanging out all by itself
Comment 6 :aceman 2012-01-19 02:24:20 PST
Yeah, I thought it is ugly :) But could it produce slower code? Is there shortcut evaluation of logical expression in some C++ compiler? If yes, then the least common nsGkAtoms values should be placed last (as it is now). So that the more common comparisons decide the result sooner and the execution can skip the less common ones.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-19 07:12:32 PST
Logical expressions are always shortcut-evaluated in C++.

A compiler could observe that the order doesn't matter here (due to lack of side-effects) and reorder the tests if it wanted, of course.  That's sort of dangerous for a compiler to do, though.
Comment 8 :aceman 2012-01-19 07:17:15 PST
Thanks. So you agree with me? Should I leave the order as is? I assume the 'audio' and 'video' values are the least common so the evaluation will usually finish before reaching them.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-19 07:43:00 PST
Yeah, just leave the order.  I don't think it matters for performance, but a lot of the code involved is on the chopping block anyway...  (and I think so should be the MOZ_MEDIA ifdefs).
Comment 11 Ed Morley [:emorley] 2012-01-21 05:59:37 PST
https://hg.mozilla.org/mozilla-central/rev/a8520279a1dc

Note You need to log in before you can comment on or make changes to this bug.