The spec more or less says we should. I don't see a reason not to. (The spec also says to treat application/octet-stream like no Content-Type. I'm less sure about doing that.)

See http://article.gmane.org/gmane.ietf.http-wg/4340

Just to explain my reasoning on the post there... Treating application/octet-stream that way would allow servers who just can't tell to just flag the file as "binary data" while still having us handle it sanely. For what it's worth, <object> falls back on certain kinds of sniffing (e.g. using its @type and the like) for application/octet-stream.

But I don't feel too strongly about the octet-stream part, fwiw.

I guess it makes sense. So we need to add sniffing logic to Necko, I guess. How do we avoid replicating the sniffing code between Necko and the video code (for the application/octet-stream case)?

The simplest approach is probably to create a service that implements nsIContentSniffer and register its contract in the "content-sniffing-services" category. Then nsUnknownDecoder will call it, and you can call it yourself too (either via the nsIContentSniffer API or whatever internal API you prefer).

If we won't implement content sniffing, what about naively mapping the file extension to mime type when we encounter a channel with content-type application/octet-stream in nsHTMLMediaElement::InitializeDecoderForChannel? That would make us more likely to be able to play something in our most most common (and probably our most unintuitive to debug) failure case.

I would much rather do magic byte sniffing than extension sniffing if we do sniffing at all, for what it's worth.

I have no problem implementing simple magic byte sniffing in preference to extension sniffing. I think we should do *something* here soon, as we're being bitten by this and it looks really bad when HTML5 media Just Works in browsers that sniff but not Firefox.

Boris, would the approach you outline in comment 5 mean that we always open an nsVideoDocument whenever we sniff that an application/octet stream has media content? Meaning the only possible way to download media is in an nsVideoDocument? I think I'd prefer to sniff only when loading a channel inside nsHTMLMediaElement, so users/developers still have the option to download media outside of our built in player. I note that Chrome takes this approach.

> Boris, would the approach you outline in comment 5 mean that we always open an > nsVideoDocument whenever we sniff that an application/octet stream has media content? It would mean that if the server sends no MIME type at all, or if you're dealing with a file:// channel or some other protocol that has no MIME type metadata your sniffer would be invoked and you would get to set a useful type on the data. Things actually sent as application/octet-stream would NOT be affected by this. > I think I'd prefer to sniff only when loading a channel inside nsHTMLMediaElement For cases when the type is actually application/octet-stream, yes. Comment 5 was about how to use the same code to do both application/octet-stream sniffing inside nsHTMLMediaElement and general sniffing on the necko level when no content type is sent.

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #9) > I think we should do *something* here soon, as we're being bitten by this > and it looks really bad when HTML5 media Just Works in browsers that sniff > but not Firefox. If we implement bug 513405 and have text for this condition, then at least when webdevs run across this they have some idea what's going on (and thus how to fix it). Almost Just Works. :-)

I'm in the process of starting working on that. Do we agree that we want to sniff for media when there is no Content-Type, AND when the Content-Type is "application/octet-stream"? The former would be done in the nsUnknowDecoder, the later in nsHTMLMediaElement (when creating the decoder). I'm planning to implement that feature the way comment 5 describes it, it still seem to be the way to go, looking at the code, but I wanted to make sure I was not missing anything.

I also plan to use the mimesniff spec [1] from the whatwg. It is incomplete (no mp3, for example), but provides what we need, I suppose. [1] : http://mimesniff.spec.whatwg.org

I've implemented what comment 5 mentions, that is a nsIContentSniffer, it get called by nsUnknownDecoder, succeeds to provide the Content-Type by sniffing the media, but then the load fails (Saying that the media is corrupted using a webm or a ogg video) or the media playback is bogus with tons of assertions firing (using an mp4 and the gstreamer backend), despite the right decoder being created in nsHTMLMediaElement. Investigating the error, I found that images (that are supposed to be sniffed as well when not served with a Content-Type, I used their code for example) don't load as well, they "cannot be displayed because they contain errors". Is that supposed to work? I suppose this has to do with the fact that nsUnknownDecoder read()s the data from the channel (that is, discards them after using them to determine the content type) instead of peeking the channel, thus providing an incomplete bitstream to the decoder. Reading nsBaseChannel and nsHttpChannel code, both classes can peek and sniff content type, but it is somehow not called. Do I miss anything, or is the behavior I observe a bug?

nsUnknown decoder can operate in two modes. It can be a stream converter, in which case it will read the data from the channel, buffer it up, detect the type, then send it on to the final consumer. Or it can be a content sniffer itself (but not registered with the content sniffer category, so it doesn't try to call itself). I'd be very interested in what testcase shows the image behavior you describe...

bz, I've modified [1] cpearce's web server [2] to prevent the Content-Type header to be set if we pass "?nomime" as parameter. Using curl, I checked that this works. I then load a png in Firefox (the urls looks like httpL//localhost:8080/image.pnblah?nomime, with a bizarre extension to prevent url sniffing). I see in gdb that sniffing occurs, using the first mode you mention, that is, 512 bytes are consumed from the stream by the nsUnknownDecoder and handed to the sniffing logic. The sniffer finds the right type by using these 512 bytes. Then, the channel is used by the image decoders, but the first 512 bytes are missing, so the image does not load. With my patch, the exact same scenario occurs for a media that is loaded. The gstreamer backend somehow succeeds to make sense of the remaining data, because it can play some frames, but a lot of assertion are failing, and we miss a lot of frames. [1] : https://github.com/padenot/HttpMediaServer [2] : https://github.com/cpearce/HttpMediaServer

> Using curl, I checked that this works. I just tried pulling from [1] above, building it on Linux, and testing it on a 174-byte png I have lying around, and it sent down 174 bytes alright... but the first two bytes were a CRLF and the last two bytes of the image data were missing. So in my case the result actually got sniffed as application/octet-stream, not as a PNG. > Then, the channel is used by the image decoders, but the first 512 bytes are missing The nsUnknownDecoder sends those 512 bytes on to the downstream consumer at http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsUnknownDecoder.cpp#606 and this works just fine last I tested. Are you hitting that code? Would you mind attaching your patch?

bz, the server was indeed faulty. My patch (and the image sniffing code of course) are working with the server fixed, thank you for the catch. Now the last bits to write/change/discuss : - how to propagate the sniffed content type when we drop and recreate a channel (because we can't really seek in the middle of a OGG packet anyway). I suppose saving the content type in the MediaResource and setting the content type before recreating the channel will do the trick (according to [1] and [2]). - where to put the code (would content/media/mediasniffer be acceptable ?), and fiddle with the build system to get it do what I want (the code is in sniffer/ as per now, it is obviously incorrect, but I copied image/). - exactly which types do we want to sniff for ? I have implemented the sniffing spec for ogg, webm, wave, mp4, and added detection for mp3 (with and without ID3v2 metadata container, ID3v1 files being at the end of the file, hence of no interest for us), but could add some more if needed. [1] : http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIChannel.idl#92 [2] : http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#887

> I suppose saving the content type in the MediaResource and setting the content type That will work if you set it in onStartRequest. It won't work if you set it before asyncOpen and the server is explicitly returning application/octet-stream, sadly.

I'm not sure about the location I put the code in. I picked it kind of randomly by searching stuff in mxr. The code is pretty straightforward, the sniffing patterns and algorithms are taken from the mime sniff spec [1] when available, and from the file(1) command otherwise (for the mp3 in particular). We might want to complete/finish that spec, that is incomplete for the media types (the pattern for mp3 is marked as TODO). [1]: http://mimesniff.spec.whatwg.org/

This is not necessary, but sniffing for a media type when seeking in the middle of a bitstream is kind of a lost cause.

This patch adds the tests, both in the "no mimetype" and "application/octet-stream" case. It also add mp3 (with and without id3 tags) and mp4 files to tests those when we have gstreamer.

Comment on attachment 635987 [details] [diff] [review] Patch v0: sniff to get the media type when we have no content-type or if the content-type is application/octet-stream Review of attachment 635987 [details] [diff] [review]: ----------------------------------------------------------------- Overall, it looks good to me! My comments are mostly trivial changes to make the code more readable, the approach I believe is fine. ::: toolkit/components/mediasniffer/nsMediaSniffer.cpp @@ +15,5 @@ > +NS_IMPL_CLASSINFO(nsMediaSniffer, NULL, 0, NS_MEDIA_SNIFFER_CID) > +NS_IMPL_ISUPPORTS1(nsMediaSniffer, nsIContentSniffer) > + > +nsMediaSniffer::nsMediaSnifferEntry nsMediaSniffer::sSnifferEntries[] = { > +// The string oggS, followed by the null byte. I'd indent the entries here, since they're inside {}. @@ +30,5 @@ > + > +PRUint32 nsMediaSniffer::sSnifferEntriesCount = > + sizeof(sSnifferEntries) / sizeof(nsMediaSnifferEntry); > + > +static bool MatchesMP4(const PRUint8* data, PRUint32 length) Add a comment explaining that you're following the algorithm at http://mimesniff.spec.whatwg.org/#signature-for-mp4 here. @@ +32,5 @@ > + sizeof(sSnifferEntries) / sizeof(nsMediaSnifferEntry); > + > +static bool MatchesMP4(const PRUint8* data, PRUint32 length) > +{ > + if (length <= 12) { It's not clear what 12 is here for. Presumably it's some sort of lower bound on the amount of data we need to successfully sniff. You should make this a #define or a "static const unsigned" value with a meaningful name to make its purpose clear. @@ +35,5 @@ > +{ > + if (length <= 12) { > + return false; > + } > + // Conversion to big endian. You're not converting *to* big endian here, you're converting *from* big endian *to* host byte order. Host byte order may or may not be big endian, depending on what machine the code is running on. Typically byte order won't matter; endianness is handled transparently by the hardware in bit shift operations, it only matters if you start messing the octets in the int itself. @@ +39,5 @@ > + // Conversion to big endian. > + PRUint32 boxSize = (PRUint32)(data[3] | data[2] << 8 | data[1] << 16 | data[0] << 24); > + > + // Boxsize should be evenly divisible by 4. > + if (boxSize % 4) { You also should return here if length<boxSize, otherwise in the loop i<=boxSize below, you could end up trying to read after data[length], which will either be reading garbage or cause a protection fault. @@ +53,5 @@ > + for (PRUint32 i = 2; i <= boxSize / 4 - 1 ; i++) { > + if (i == 3) { > + continue; > + } > + // The string mp4. s/mp4/"mp4"/ @@ +65,5 @@ > +} > + > +NS_IMETHODIMP > +nsMediaSniffer::GetMIMETypeFromContent(nsIRequest* request, > + const PRUint8* data, The 3 lines below the first should line up with the '(', i.e. they should be indented by one more space. Also your arguments here (and elsewhere) should follow the Mozilla standard aArgumentNamingConvention please. Also, please make as many of them const as possible, even the plain-old-data types; in general it reduces code clarity if you change the value of arguments to functions unless they're out params, so make the arguments const so that can't happen. @@ +73,5 @@ > + nsCOMPtr<nsIHttpChannel> channel(do_QueryInterface(request)); > + if (!channel) { > + return NS_ERROR_NO_INTERFACE; > + } > + The spec specifies that n (the length of data) should be capped at 512 bytes, so please enforce that. @@ +74,5 @@ > + if (!channel) { > + return NS_ERROR_NO_INTERFACE; > + } > + > + for (PRUint32 i = 0; i < sSnifferEntriesCount; ++i) { Use NS_ARRAY_LENGTH(sSnifferEntries) rather than storing nsMediaSniffer::sSnifferEntriesCount and making it public. @@ +75,5 @@ > + return NS_ERROR_NO_INTERFACE; > + } > + > + for (PRUint32 i = 0; i < sSnifferEntriesCount; ++i) { > + if (length < sSnifferEntries[i].mLength) { if (length < sSnifferEntries[i].mLength || sSnifferEntries[i].mLength == 0) Also, because you're nesting your loops here and they're not small, I think it would be clearer to either create a const nsMediaSnifferEntry* (or reference) to reduce the number of array indexes scattered around all over the place, or rename i and j so that their purpose is more obvious. @@ +79,5 @@ > + if (length < sSnifferEntries[i].mLength) { > + continue; > + } > + bool matched = false; > + for (PRUint32 j = 0; j < sSnifferEntries[i].mLength; ++j) { j < sSnifferEntries[i].mLength && j < length; @@ +84,5 @@ > + if ((sSnifferEntries[i].mMask[j] & data[j]) != sSnifferEntries[i].mPattern[j]) { > + matched = false; > + break; > + } else { > + matched = true; Initialize matched to true above, then you don't need this branch (provided you |continue| when sSnifferEntries[i].mLength==0 above). @@ +99,5 @@ > + return NS_OK; > + } > + > + // Could not sniff the media type. > + return NS_ERROR_NOT_AVAILABLE; Doesn't the spec require us to assign application/octet-stream to the mime type if we don't get a match? ::: toolkit/components/mediasniffer/nsMediaSniffer.h @@ +20,5 @@ > +#define NS_MEDIA_SNIFFER_CONTRACTID "@mozilla.org/media/sniffer;1" > + > +class nsMediaSniffer : public nsIContentSniffer > +{ > + Delete this unnecessary blank line please. @@ +33,5 @@ > + > + struct nsMediaSnifferEntry { > + const PRUint8* mMask; > + const PRUint8* mPattern; > + PRUint32 mLength; Can mLength be const? ::: toolkit/components/mediasniffer/nsMediaSnifferModule.cpp @@ +5,5 @@ > + > +#include "mozilla/ModuleUtils.h" > + > +#include "nsMediaSniffer.h" > + I'm not really qualified to review this contract/xpcom stuff.

Comment on attachment 635989 [details] [diff] [review] Patch v0: don't try to resniff when recreating a channel. Review of attachment 635989 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, again just needs trivial changes. ::: content/html/content/src/nsHTMLMediaElement.cpp @@ +2373,5 @@ > { > NS_ASSERTION(mLoadingSrc, "mLoadingSrc must already be set"); > NS_ASSERTION(mDecoder == nsnull, "Shouldn't have a decoder"); > > + aChannel->GetContentType(mMimeType); This sets mMimeType = mMimeType. Lets keep the old code using the GetContentType() accessor. Acessors are good, because they centralize access to the underlying data, allow the underlying structure to change without large changes to the code base. Also, does it make sense to assert that !contentType.IsEmpty() ? The sniffer should assign application/octect-stream if it doesn't find a match right? ::: content/media/MediaResource.cpp @@ +696,5 @@ > + nsnull, > + loadFlags); > + > + // We have cached the Content-Type, which should not change. Give a hint to > + // the channel to avoid a sniffing failure. Why would we expect sniffing to fail when we recreate the channel? Because we're usually requesting a byte range which doesn't start at offset 0? Your comment should make it clear why we expect failure. In general, add comments when the code isn't obvious enough by itself to make it clear what we're doing, and why we're doing it.

Comment on attachment 635991 [details] [diff] [review] Patch v0: add some tests for the sniffing. and files too. Review of attachment 635991 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/test/test_media_sniffer.html @@ +40,5 @@ > + ok(true, "The media loads when served without a Content-Type."); > + checkApplicationOctetStream(t); > +} > + > +function onerror(e) { Are you building with gstreamer enabled? The mp4/mp3 files should be failing to load on builds without gstreamer... Did you push this to Try?

(In reply to Chris Pearce (:cpearce) from comment #28) > Comment on attachment 635991 [details] [diff] [review] > Patch v0: add some tests for the sniffing. and files too. > > Review of attachment 635991 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/test/test_media_sniffer.html > @@ +40,5 @@ > > + ok(true, "The media loads when served without a Content-Type."); > > + checkApplicationOctetStream(t); > > +} > > + > > +function onerror(e) { > > Are you building with gstreamer enabled? The mp4/mp3 files should be failing > to load on builds without gstreamer... Did you push this to Try? I've tested both on a gstreamer enabled build, and on a default build, both works (i.e. the test is green). According to the logic in content/media/manifest.js, the media that we can't play are skipped (in the same fashion as if we were compiling without, say, OGG support), thus are not passed to the |startTest| function. Or maybe I don't understand you remark.

Comment on attachment 635991 [details] [diff] [review] Patch v0: add some tests for the sniffing. and files too. Review of attachment 635991 [details] [diff] [review]: ----------------------------------------------------------------- Ah yes, of course. Thanks!

(In reply to Chris Pearce (:cpearce) from comment #27) > Comment on attachment 635989 [details] [diff] [review] > Patch v0: don't try to resniff when recreating a channel. > > Review of attachment 635989 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, again just needs trivial changes. > > ::: content/html/content/src/nsHTMLMediaElement.cpp > @@ +2373,5 @@ > > { > > NS_ASSERTION(mLoadingSrc, "mLoadingSrc must already be set"); > > NS_ASSERTION(mDecoder == nsnull, "Shouldn't have a decoder"); > > > > + aChannel->GetContentType(mMimeType); > > This sets mMimeType = mMimeType. > > Lets keep the old code using the GetContentType() accessor. Acessors are > good, because they centralize access to the underlying data, allow the > underlying structure to change without large changes to the code base. I'm not too sure of what you are asking here. I'm still using |GetContentType()|, but merely caching the result for later use. I may be missing something, though. > Also, does it make sense to assert that !contentType.IsEmpty() ? The sniffer > should assign application/octect-stream if it doesn't find a match right? Yes.

(In reply to Paul ADENOT (:padenot) from comment #31) > (In reply to Chris Pearce (:cpearce) from comment #27) > > Comment on attachment 635989 [details] [diff] [review] > > Patch v0: don't try to resniff when recreating a channel. > > > > Review of attachment 635989 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Looks good, again just needs trivial changes. > > > > ::: content/html/content/src/nsHTMLMediaElement.cpp > > @@ +2373,5 @@ > > > { > > > NS_ASSERTION(mLoadingSrc, "mLoadingSrc must already be set"); > > > NS_ASSERTION(mDecoder == nsnull, "Shouldn't have a decoder"); > > > > > > + aChannel->GetContentType(mMimeType); > > > > This sets mMimeType = mMimeType. > > > > Lets keep the old code using the GetContentType() accessor. Acessors are > > good, because they centralize access to the underlying data, allow the > > underlying structure to change without large changes to the code base. > > I'm not too sure of what you are asking here. I'm still using > |GetContentType()|, but merely caching the result for later use. I may be > missing something, though. Oops, I misread your code and thought you were calling nsHTMLMediaElement::GetContentType() here. My mistake. Please ignore this comment; your change here is fine.

I found a cool header file with all the mime types #defined, so I replaced the hard coded strings by the defines, and addressed all the other remarks. This is green on try (with the patch for bug 767087 applied), https://tbpl.mozilla.org/?tree=Try&rev=af53f8131c39. I need someone to review the xpcom registration bits also, I think. And the build system bits, because I've no idea what I'm doing here.

Comment on attachment 636790 [details] [diff] [review] Addressed cpearce's comments. Review of attachment 636790 [details] [diff] [review]: ----------------------------------------------------------------- bsmedberg can review the XPCOM/toolkit stuff. ::: toolkit/components/mediasniffer/nsMediaSniffer.cpp @@ +78,5 @@ > + const PRUint32 clampedLength = NS_MIN(aLength, MAX_BYTES_SNIFFED); > + > + for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(sSnifferEntries); ++i) { > + const nsMediaSnifferEntry& currentEntry = sSnifferEntries[i]; > + if (clampedLength < sSnifferEntries[i].mLength) { s/sSnifferEntries[i]/currentEntry/ Same thing below as well too. @@ +83,5 @@ > + continue; > + } > + bool matched = false; > + for (PRUint32 j = 0; j < sSnifferEntries[i].mLength; ++j) { > + if ((currentEntry.mMask[j] & aData[j]) != currentEntry.mPattern[j]) { I had a comment about this branch and the "matched" variable, can you address it please?

Comment on attachment 637164 [details] [diff] [review] Patch v2: addressed last comments bsmedbergs, could you review the xpcom registration/build system bits of this patch ?

Comment on attachment 637164 [details] [diff] [review] Patch v2: addressed last comments Review of attachment 637164 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. But you forgot to remove sSnifferEntriesCount. r+ with sSnifferEntriesCount removed. ::: toolkit/components/mediasniffer/nsMediaSniffer.h @@ +37,5 @@ > + const char* mContentType; > + }; > + > + static nsMediaSnifferEntry sSnifferEntries[]; > + static PRUint32 sSnifferEntriesCount; Remove sSnifferEntriesCount...

Carrying forward r+.

Here is a better attempt at a comment.

Comment on attachment 640392 [details] [diff] [review] Patch v1: rephrased comment. Review of attachment 640392 [details] [diff] [review]: ----------------------------------------------------------------- This is identical to attachment 635989 [details] [diff] [review]. Did you forget to qref? It would also be handy if you could label your patches "Patch 1 v2: Description" to make it clear the order in which they're applied, and to make it clearer which patch you're updating and how many times it's been revised.

Comment on attachment 640638 [details] [diff] [review] Avoid sniffing when recreating a channel. r= Ah, I took the previous patch from my outdated gstreamer tree. Here is the right one.

(Adding r=cpearce to the commit message).

(Adding r=cpearce to the commit message).

(Adding r=cpearce to the commit message).

I was going to land this, but the following assertion is firing when running the content/media mochitests: ###!!! ASSERTION: When recreating a channel, we should know the Content-Type.: '!contentType.IsEmpty()', file /home/kinetik/mozilla-inbound/content/media/MediaResource.cpp, line 705 I saw two while test_streams_element_capture was running, but running that test individually doesn't cause the assertion to fire.

Thanks, I could reproduce, and this patch fixes it. The mistake was that I forgot to set the content-type when cloning the decoder instead of using a channel to initialize it a new decoder and getting the content-type at the same time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/67ea1314c777 https://hg.mozilla.org/integration/mozilla-inbound/rev/de811fbd27b0 https://hg.mozilla.org/integration/mozilla-inbound/rev/4a54f06efee8 https://hg.mozilla.org/integration/mozilla-inbound/rev/ec46cfa51bf8

Comment on attachment 649848 [details] [diff] [review] Sniff types of media files that are served with no Content-Type Review of attachment 649848 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/mediasniffer/nsMediaSniffer.cpp @@ +17,5 @@ > +static const unsigned MP4_MIN_BYTES_COUNT = 12; > +// The maximum number of bytes to consider when attempting to sniff a file. > +static const PRUint32 MAX_BYTES_SNIFFED = 512; > + > +NS_IMPL_CLASSINFO(nsMediaSniffer, NULL, 0, NS_MEDIA_SNIFFER_CID) This is incomplete, if you really want to implement classinfo you need a GetInterfacesHelper and it needs to be returned from your QI, probably by changing NS_IMPL_ISUPPORTS1 to NS_IMPL_ISUPPORTS1_CI. But given that the classinfo is not even hooked up you probably don't need it. As is this patch breaks my build with Undefined symbols for architecture x86_64: "nsMediaSniffer_GetInterfacesHelper(unsigned int*, nsID***)", referenced from: knsMediaSnifferClassInfoData in nsMediaSniffer.o

Sorry backed out for causing bug 781141, the first time the new tests ran (given that we've only just got the Android tests really green, so do not wish to regress that). (Also comment 50): https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea9a4023efa

I'm confused, the tests are passing pretty reliably on try, the only modification being that comment 50 has been addressed, and I added instrumentation, which does not want to show up because it's in a sjs file: https://tbpl.mozilla.org/?tree=Try&rev=1d1503e547d3

Sorry I didn't see your reply, since I don't CC on bugs when marking after merges/sheriffing/backouts, otherwise I get thousands of additional bugmails a month, 99.9% of which are not relevant and not easy to filter out. This was backed out since one of the tests it added failed on it's very first run (as explained in bug 781141, mentioned in my backout comment). This failure is likely only intermittent, but we are toughening up on adding new intermittent failures, otherwise our efforts to get on top of the massive backlog of existing [oranges] is somewhat pointless if we add them as fast as fixing :-)

I've rebased this and pushed to try [1], with an additional patch to address comment 50 remark. [1]: https://tbpl.mozilla.org/?tree=Try&rev=f54aef7bcadb

(rebased)

Comment on attachment 658294 [details] [diff] [review] Remove the classinfo stubs. r= Not sure if this fixes the build for you, it worked with and without this patch for me.

So I forgot to compile after rebasing and before pushing to try, [1] is the right try push. [1]: https://tbpl.mozilla.org/?tree=Try&rev=55171d7fece9

added r=peterv.

Alright, this is green on try (see comment 60), I guess we can land, in that order, top patch first.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8a5cb2d155 https://hg.mozilla.org/integration/mozilla-inbound/rev/32bfacfc4841 https://hg.mozilla.org/integration/mozilla-inbound/rev/efdaa596b3f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/aacd56bc4f44 https://hg.mozilla.org/integration/mozilla-inbound/rev/81b3c65bc17a

Forgot to upload this patch last time, that rebases over nsnull/nsCAutoString changes.

And backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/5c580e01ae0c

Let's try that again. https://hg.mozilla.org/integration/mozilla-inbound/rev/a027c9d63d20 https://hg.mozilla.org/integration/mozilla-inbound/rev/cb3dd01ba9be https://hg.mozilla.org/integration/mozilla-inbound/rev/14ac87e7546b https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4ba0fc1f8d https://hg.mozilla.org/integration/mozilla-inbound/rev/1cc49d5dcff4

https://hg.mozilla.org/mozilla-central/rev/a027c9d63d20 https://hg.mozilla.org/mozilla-central/rev/cb3dd01ba9be https://hg.mozilla.org/mozilla-central/rev/14ac87e7546b https://hg.mozilla.org/mozilla-central/rev/aa4ba0fc1f8d https://hg.mozilla.org/mozilla-central/rev/1cc49d5dcff4