991149 - Make image/decoders and image/encoders source files comply with Mozilla Coding Style

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Glenn Randers-Pehrson]

A number of long lines in the source files in image/decoders and image/encoders ought to be wrapped to 80 characters.  Also, whitespace should be removed from the end of some lines.

Comment 1

[Glenn Randers-Pehrson]

Attachment: v00 wrap long lines and remove trailing blanks

Comment 2

[Glenn Randers-Pehrson]

Attachment: v01 image wrap line remov trailing blanks

Also fix sources in image/decoders/icon; fix some misplaced "{" brackets.

Comment 3

[Glenn Randers-Pehrson]

Attachment: v02 wrap long lines remove trailing blanks and TABs in libimage

Expand TABs, fix previously overlooked decoders/icon files.

Comment 4

[Glenn Randers-Pehrson]

Attachment: v03 wrap long lines remove trailing blanks and TABs in libimage

Fixed bit rot in the v02 patch.  Removed PNG encoder/decoder patches which are now in bug #1072340.

Comment 5

[Glenn Randers-Pehrson]

Attachment: v04 clean up image codecs

Wrapped a couple of left-over long lines
Converted most "/* ... */" comments to "// ..."
Removed leading and trailing underscores from header guards

Comment 6

[Glenn Randers-Pehrson]

Comment on attachment 8500542 [details] [diff] [review]
v04 clean up image codecs

updates image codecs except for PNG which is taken care of in another bug.

Comment 7

[Glenn Randers-Pehrson]

Attachment: v05 clean up image codecs

Enclose one-line code branches in curley brackets; wrap a few remaining long lines and removed a few remaining trailing blanks.

Comment 8

[Glenn Randers-Pehrson]

Attachment: v06 clean up image codecs in accordance with Mozilla Coding Style

Cuddle some "else" statements in curly brackets, convert some /*-style comments to //-style

Comment 9

[Glenn Randers-Pehrson]

This is ready for a try run.  Locally on 64-bit Ubuntu I've verified that the object files are exactly the same size when built with and without the patch, and that Firefox runs properly.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Sorry for the delay.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=72858c047106

Comment 11

[Glenn Randers-Pehrson]

Comment on attachment 8502199 [details] [diff] [review]
v06 clean up image codecs in accordance with Mozilla Coding Style

Try runs passed.  r?

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8502199 [details] [diff] [review]
v06 clean up image codecs in accordance with Mozilla Coding Style

I'm going defer this review to seth as he has to look at this code more and so deserves to be the authority on aesthetics here.

Comment 13

[Glenn Randers-Pehrson]

Attachment: v07 clean up image codecs in accordance with Mozilla Coding Style

Merge with recent changes to image/public/imgIloader.idl and
image/decoders/icon/*/nsIconChannel.cpp

Comment 14

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8507318 [details] [diff] [review]
v07 clean up image codecs in accordance with Mozilla Coding Style

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

Thanks for writing this patch, Glenn! It'll be really nice to have this (formerly very inconsistent) code cleaned up!

I apologize for the sheer volume of comments, some of which are redundant; this is a truly huge patch. I'm definitely not trying to get you to make these files perfect in one patch, so hopefully it doesn't come across that way. I'm mainly just trying to adhere to two guidelines:

1. We should try to fix all of the coding style violations on lines that get changed.
2. We should try to avoid making anything less readable.

There are a few places where I suggested some additional tweaks on neighboring lines, but I tried not to go overboard with that. (Even though it's really tempting - there are so many style issues in this code!)

::: image/decoders/EXIF.h
@@ +23,5 @@
>  
>  struct EXIFData
>  {
> +  EXIFData() {
> +  }

Please don't make this change. An empty pair of braces should stay on one line.

@@ +25,5 @@
>  {
> +  EXIFData() {
> +  }
> +  explicit EXIFData(Orientation aOrientation) : orientation(aOrientation) {
> +  }

Same here.

@@ +48,5 @@
>      , mLength(0)
>      , mRemainingLength(0)
>      , mByteOrder(ByteOrder::Unknown)
> +  {
> +  }

Same here.

::: image/decoders/GIF2.h
@@ +52,1 @@
>  typedef struct gif_struct {

Can we get rid of this |typedef struct| business while we're at it? This is C++, not C, so this can just be |struct gif_struct|.

@@ +57,3 @@
>  
> +    // LZW decoder state machine
> +    uint8_t *stackp;            // Current stack pointer

As long as we're cleaning this stuff up, the style guide requires that the asterisk goes with the type here - in other words, this should be |uint8_t* stackp;|. The same goes for references; if it was a reference, it should be |uint8_t& stackp;|.

There are tons of instances of this problem in this code. I'm definitely not saying you should fix them all, though that would be quite a heroic act! But it would be nice to fix the issue in lines that you touch.

@@ +70,5 @@
> +    // Output state machine
> +    int ipass;                  // Interlace pass; Ranges 1-4 if interlaced.
> +    unsigned rows_remaining;    // Rows remaining to be output
> +    unsigned irow;              // Current output row, starting at zero
> +    uint8_t *rowp;              // Current output pointer

|uint8_t* rowp|

@@ +78,4 @@
>      unsigned height, width;
> +    int tpixel;                 // Index of transparent pixel
> +    int32_t disposal_method;    // Restore to background, leave in place, etc.
> +    uint32_t *local_colormap;   // Per-image colormap

|uint32_t* local_colormap|

I'm sure you get the idea. =) I won't point out this issue everywhere it appears in the rest of the patch.

::: image/decoders/icon/gtk/nsIconChannel.cpp
@@ +200,5 @@
> +                                                  "gnome_init_with_popt_table");
> +    _gnome_icon_theme_new = (_GnomeIconThemeNew_fn)PR_FindFunctionSymbol(
> +                                           gLibGnomeUI, "gnome_icon_theme_new");
> +    _gnome_icon_lookup = (_GnomeIconLookup_fn)PR_FindFunctionSymbol(gLibGnomeUI,
> +                                                           "gnome_icon_lookup");

I'd prefer to have the function arguments aligned, even if that means using more lines of code. For example, you could do:

_gnome_init =
  (_GnomeInit_fn)PR_FindFunctionSymbol(gLibGnomeUI,
                                       "gnome_init_with_popt_table");

(There are other situations like this later in this file.)

Bonus points if you replace these C-style casts with C++ casts. =)

::: image/decoders/icon/gtk/nsIconChannel.h
@@ +33,5 @@
>      nsresult Init(nsIURI* aURI);
>    private:
> +    ~nsIconChannel()
> +    {
> +    }

Please leave this as |~nsIconChannel () { }|.

::: image/decoders/icon/mac/nsIconChannelCocoa.mm
@@ +350,5 @@
>                    bufferCapacity, bufferCapacity, nonBlocking);
>  
>    if (NS_SUCCEEDED(rv)) {
>      uint32_t written;
> +    rv = outStream->Write((char*)iconBuffer.Elements(), bufferCapacity,

Bonus points for switching to a C++ cast here. =)

@@ +417,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsIconChannel::GetContentDispositionFilename(nsAString
> +                                             &aContentDispositionFilename)

I don't think you should make this change - this makes the code harder to understand. An argument and its type should stay on the same line. You could potentially fix the problem by making the argument name shorter, though. In this case, the method doesn't use the argument at all, so this can become just |nsIconChannel::GetContentDispositionFilename(nsAString&)|.

This happens several more times in this file.

::: image/decoders/icon/nsIconProtocolHandler.cpp
@@ +21,5 @@
>  }
>  
> +nsIconProtocolHandler::~nsIconProtocolHandler()
> +{
> +}

Again, please leave these on one line.

::: image/decoders/icon/nsIconURI.cpp
@@ +126,5 @@
>  // containing just the attribute value. i.e you could pass in this string with
>  // an attribute name of 'size=', this will return 32
>  // Assumption: attribute pairs in the string are separated by '&'.
> +void extractAttributeValue(const char * searchString,
> +                           const char * attributeName, nsCString& result)

Maybe just put each argument on a separate line. It'd be nice to name the arguments |aSearchString|, etc. And as usual |const char* aSearchString| would be ideal.

@@ +166,5 @@
>    mIconState = -1;
>  
>    nsAutoCString iconSpec(aSpec);
> +  if (!Substring(iconSpec, 0,
> +      MOZICON_SCHEME_LEN).EqualsLiteral(MOZICON_SCHEME)) {

Please align |MOZICON_SCHEME_LEN| with |iconSpec| or find another way to split this between lines. I find it really important for readability that the arguments to a function call are aligned. (One possible alternative approach: you could move |.EqualsLiteral(...)| to the second line, indented more than |Substring(...)|)

@@ +173,4 @@
>  
>    int32_t questionMarkPos = iconSpec.Find("?");
> +  if (questionMarkPos != -1 && static_cast<int32_t>(iconSpec.Length()) >
> +      (questionMarkPos + 1)) {

Both operands of |>| should be on the same line.

@@ +401,5 @@
>    return Equals(other, result);
>  }
>  
>  NS_IMETHODIMP
>  nsMozIconURI::SchemeIs(const char *i_Scheme, bool *o_Equals)

Wow, I don't think I've seen this particular argument naming scheme before in our code.

@@ +404,5 @@
>  NS_IMETHODIMP
>  nsMozIconURI::SchemeIs(const char *i_Scheme, bool *o_Equals)
>  {
>    NS_ENSURE_ARG_POINTER(o_Equals);
>    if (!i_Scheme) return NS_ERROR_INVALID_ARG;

Maybe add braces here?

::: image/decoders/icon/nsIIconURI.idl
@@ +14,5 @@
>     *
>     * What *is* a moz-icon URI you ask?  Well, it has the following syntax:
>     *
> +   * moz-icon:[<valid-url> | //<file-with-extension> | //stock/<stock-icon>]?
> +   *          ['?'[<parameter-value-pairs>]]

Maybe indent the second line here to make it a little bit clearer that it's a continuation?

::: image/decoders/icon/qt/gtkqticonsconverter.js
@@ +7,5 @@
>  
>  
> +function GtkQtIconsConverter()
> +{
> +};

Again, leave |GtkQtIconsConverter| all on one line.

@@ +16,5 @@
> +                                [Components.interfaces.nsIGtkQtIconsConverter]),
> +  convert: function(icon)
> +  {
> +    return this._gtk_qt_icons_table[icon];
> +  },

Unless it's more than 80 characters, I'd recommend leaving |convert| on one line too. It's explicitly allowed in our style guide to write short functions on one line.

::: image/decoders/icon/win/nsIconChannel.cpp
@@ +277,2 @@
>      infoFlag = SHGFI_SMALLICON;
> +  }

Just a grumble from me, not something you have to change, but this entire function is just |return aDesiredImageSize > 16 ? SHFI_SHELLICONSIZE : SHGFI_SMALLICON;|!

@@ +461,5 @@
>    }
>  
> +  if (colorTableSize < 0) {
> +    NS_WARNING(
> +           "Unable to figure out the color table size for this bitmap");

Please align "Unable" with the space after |NS_WARNING(|. I usually break things like this up by writing them something like this:

NS_WARNING("Unable to figure out the color table size "
           "for this bitmap");

@@ +483,5 @@
>    return bmi;
>  }
>  
> +nsresult nsIconChannel::MakeInputStream(nsIInputStream** _retval,
> +                                        bool nonBlocking)

Looks like this should have |nsresult| on the previous line and rename |nonBlocking| to |aNonBlocking|...

@@ +628,4 @@
>    return rv;
>  }
>  
> +NS_IMETHODIMP nsIconChannel::GetContentType(nsACString &aContentType)

Looks like NS_IMETHODIMP should be on the previous line. There are more cases in this file...

@@ +669,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsIconChannel::GetContentDispositionFilename(nsAString
> +                                             &aContentDispositionFilename)

As I mentioned elsewhere in the patch, we shouldn't separate an argument type from its name, but it so happens that you can just write |GetContentDispositionFilename(nsAString&)| here anyway.

As before, there are more situations later in this file where an argument type is separated from its name.

::: image/decoders/nsBMPDecoder.cpp
@@ +5,5 @@
>  
> +// I got the format description from http://www.daubnet.com/formats/BMP.html
> +
> +// This is a Cross-Platform BMP Decoder, which should work everywhere, including
> +// Big-Endian machines like the PowerPC.

Splinter failed to align the diff for most of this file, so I'm going to have to trust that nothing has changed other than style. =)

::: image/decoders/nsBMPDecoder.h
@@ +50,5 @@
>      // for 32BPP bitmaps.  Only use after the bitmap has been processed.
>      bool HasAlphaData() const;
>  
> +    virtual void WriteInternal(const char* aBuffer, uint32_t aCount,
> +                               DecodeStrategy aStrategy);

This needs a MOZ_OVERRIDE.

@@ +56,5 @@
>  
>  private:
>  
> +    // Calculates the red-, green- and blueshift in mBitFields using
> +    // the bitmasks from mBitFields

So the original comment here was a Doxygen comment (it started with |/**|), and it would be preferable to preserve that. You could do that by starting each line of the comment with |///|, but I actually think the |/**| style is fine for Doxygen comments. It's certainly not prohibited by our style guide. So it's up to you how you want to handle this, as long as this remains a Doxygen comment.

(Noticing this made me concerned that I may have missed other cases of this happening elsewhere in the patch. It'd be good to check it over and make sure that we didn't lose any more Doxygen comments...)

@@ +61,3 @@
>      NS_METHOD CalcBitShift();
>  
> +    uint32_t mPos; //< Number of bytes read from aBuffer in WriteInternal()

This is a Doxygen comment, too. It should stay as |///<|, or Doxygen won't pick it up.

Check here for more info:

http://www.stack.nl/~dimitri/doxygen/manual/docblocks.html

@@ +64,5 @@
>  
>      BMPFILEHEADER mBFH;
>      BITMAPV5HEADER mBIH;
> +    char mRawBuf[WIN_V3_INTERNAL_BIH_LENGTH]; //< If this is changed,
> +                                              // WriteInternal() MUST be updated

Again, this is a Doxygen comment. If you want to split it onto multiple lines, you need to start each line with |///<|. That will mean that it's not a "brief" comment anymore, though, which might not be desirable. So if you want to avoid going over 80 characters, you might be better off doing it like this:

/// If this is changed, WriteInternal() MUST be updated.
char mRawBuf[WIN_V3_INTERNAL_BIH_LENGTH];

That will preserve the "brief" nature of the comment.

These same issues about Doxygen comments occur many times in this file. Look for |///| and |/**| in the original code to find all the instances.

::: image/decoders/nsGIFDecoder2.cpp
@@ +135,4 @@
>      case 1:  // one pass on - need to handle bottom & top rects
>        FlushImageData(0, mCurrentRow + 1);
> +      FlushImageData(mLastFlushedRow + 1, mGIFStruct.height -
> +                     (mLastFlushedRow + 1));

I'd keep both operands of |-| on one line.

@@ +237,5 @@
>      if (mGIFStruct.rows_remaining && mGIFStruct.images_decoded) {
>        // Clear the remaining rows (only needed for the animation frames)
> +      uint8_t *rowp = mImageData +
> +                      ((mGIFStruct.height - mGIFStruct.rows_remaining) *
> +                       mGIFStruct.width);

It'd be ideal to keep both operands of |*| on one line but you might not be able to do it here.

@@ +572,5 @@
>    // Add what we have sofar to the block
> +  // If previous call to me left something in the hold first complete current
> +  // block, or if we are filling the colormaps, first complete the colormap
> +  uint8_t* p = (mGIFStruct.state == gif_global_colormap) ?
> +                                    (uint8_t*)mGIFStruct.global_colormap :

If you can avoid it, it'd be better not to split this onto a separate line, because right now this sequence of ternary operators has the pattern |condition ? value-if-true :| on each line, which makes this questionable construction somewhat readable. It'd be better to write this as:

uint8_t* p =
  (mGIFStruct.state == gif_global_colormap) ? (uint8_t*)mGIFStruct.global_colormap :

@@ +606,5 @@
>    //    to point to next buffer, 'len' is adjusted accordingly.
>    //    So that next round in for loop, q gets pointed to the next buffer.
>  
> +  for (;len >= mGIFStruct.bytes_to_consume; q=buf,
> +       mGIFStruct.bytes_in_hold = 0) {

I'd keep |q=buf, mGifStruct.bytes_in_hold = 0| on the same line. This is another construct that is very easy to misread, since that comma could easily be misread as a semicolon.

@@ +765,3 @@
>            mGIFStruct.state = gif_control_extension;
> +          mGIFStruct.bytes_to_consume = std::max(mGIFStruct.bytes_to_consume,
> +                                                 4u);

I'd be tempted to just add |using std::max| to the top of the file, which would let this fit on one line, but it's up to you.

@@ +932,2 @@
>          depth = (q[8]&0x07) + 1;
> +        }

This looks like it isn't aligned correctly in Splinter...

@@ +948,3 @@
>          if (size) {
> +          if (SetHold(q, mGIFStruct.bytes_to_consume +
> +                      mGIFStruct.bytes_in_hold, buf, len)) {

It'd be ideal to keep both operands of |+| on the same line.

@@ +1105,5 @@
>    // Copy the leftover into mGIFStruct.hold
>    if (len) {
>      // Add what we have sofar to the block
> +    if (mGIFStruct.state != gif_global_colormap && mGIFStruct.state !=
> +        gif_image_colormap) {

Put |mGIFStruct.state != gif_image_colormap| all on the same line.

::: image/decoders/nsGIFDecoder2.h
@@ +26,5 @@
>    explicit nsGIFDecoder2(RasterImage &aImage);
>    ~nsGIFDecoder2();
>  
> +  virtual void WriteInternal(const char* aBuffer, uint32_t aCount,
> +                             DecodeStrategy aStrategy);

This needs a MOZ_OVERRIDE.

::: image/decoders/nsICODecoder.cpp
@@ +234,5 @@
>    }
>  
>    if (mPos == ICONCOUNTOFFSET && aCount >= 2) {
> +    mNumIcons = LittleEndian::readUint16(reinterpret_cast<const
> +                              uint16_t*>(aBuffer));

It'd be better not to break up the type. I'd either write:

mNumIcons =
  LittleEndian::readUint16(reinterpret_cast<const uint16_t*>(aBuffer));

Or, if that won't fit:

mNumIcons = LittleEndian::readUint16(
              reinterpret_cast<const uint16_t*>(aBuffer));

@@ +489,5 @@
>      // Feed the actual image data (not including headers) into the BMP decoder
>      uint32_t bmpDataOffset = mDirEntry.mImageOffset + BITMAPINFOSIZE;
> +    uint32_t bmpDataEnd = mDirEntry.mImageOffset + BITMAPINFOSIZE +
> +                          static_cast<nsBMPDecoder*>(mContainedDecoder.get())->
> +                          GetCompressedImageSize() +

You should indent |GetCompressedImageSize| since it continues the previous expression.

@@ +521,5 @@
>        // The alpha mask should be checked in all other cases.
> +      if (static_cast<nsBMPDecoder*>(mContainedDecoder.get())->
> +          GetBitsPerPixel() != 32 ||
> +          !static_cast<nsBMPDecoder*>(mContainedDecoder.get())->
> +          HasAlphaData()) {

Please indent |GetBitsPerPixel| and |HasAlphaData|.

@@ +555,5 @@
>              mRowBytes = 0;
>  
> +            uint32_t* imageData =
> +              static_cast<nsBMPDecoder*>(mContainedDecoder.get())->
> +                                         GetImageData();

Could probably use an additional indent here too.

::: image/decoders/nsICODecoder.h
@@ +38,4 @@
>    }
>  
> +  virtual void WriteInternal(const char* aBuffer, uint32_t aCount,
> +                             DecodeStrategy aStrategy);

This needs a MOZ_OVERRIDE.

::: image/decoders/nsIconDecoder.cpp
@@ +26,5 @@
>  }
>  
>  nsIconDecoder::~nsIconDecoder()
> +{
> +}

Please leave this as one line.

::: image/decoders/nsIconDecoder.h
@@ +41,5 @@
>    explicit nsIconDecoder(RasterImage &aImage);
>    virtual ~nsIconDecoder();
>  
> +  virtual void WriteInternal(const char* aBuffer, uint32_t aCount,
> +                             DecodeStrategy aStrategy);

This should be marked MOZ_OVERRIDE.

::: image/decoders/nsJPEGDecoder.cpp
@@ +393,5 @@
>  
> +    // Don't allocate a giant and superfluous memory buffer
> +    // when not doing a progressive decode.
> +    mInfo.buffered_image = mDecodeStyle == PROGRESSIVE &&
> +                                           jpeg_has_multiple_scans(&mInfo);

Looks like |jpeg_has_multiple_scans| should be aligned to |mDecodeStyle|.

::: image/decoders/nsJPEGDecoder.h
@@ +29,5 @@
>  
>  namespace mozilla {
>  namespace image {
>  
>  typedef struct {

This is C++ code so this could be just |struct decoder_error_mgr|.

@@ +36,3 @@
>  } decoder_error_mgr;
>  
>  typedef enum {

And this could be |enum jstate|.

@@ +56,5 @@
>    virtual ~nsJPEGDecoder();
>  
>    virtual void InitInternal();
> +  virtual void WriteInternal(const char* aBuffer, uint32_t aCount,
> +                             DecodeStrategy aStrategy);

This needs a MOZ_OVERRIDE.

::: image/decoders/nsPNGDecoder.cpp
@@ +57,5 @@
>   : mDispose(FrameBlender::kDisposeKeep)
>   , mBlend(FrameBlender::kBlendOver)
>   , mTimeout(0)
> +{
> +}

Please leave this as one line.

::: image/decoders/nsPNGDecoder.h
@@ +28,5 @@
>    virtual ~nsPNGDecoder();
>  
>    virtual void InitInternal();
> +  virtual void WriteInternal(const char* aBuffer, uint32_t aCount,
> +                             DecodeStrategy aStrategy);

This needs a MOZ_OVERRIDE.

@@ +90,5 @@
>    bool mFrameHasNoAlpha;
>    bool mFrameIsHidden;
>    bool mDisablePremultipliedAlpha;
>  
> +  struct AnimFrameInfo {

I think we still want |struct AnimFrameInfo| and the opening brace on two separate lines.

::: image/encoders/bmp/nsBMPEncoder.cpp
@@ +59,5 @@
>    // Stride is the padded width of each row, so it better be longer
>    if ((aInputFormat == INPUT_FORMAT_RGB &&
>         aStride < aWidth * 3) ||
> +      ((aInputFormat == INPUT_FORMAT_RGBA ||
> +       aInputFormat == INPUT_FORMAT_HOSTARGB) &&

I'd align the two |aInputFormat|s here. (Maybe you did; Splinter is not always reliable about alignment.)

@@ +490,5 @@
>      mBMPFileHeader.filesize = mBMPFileHeader.dataoffset + aWidth * aHeight;
>    } else {
> +    mBMPFileHeader.filesize = mBMPFileHeader.dataoffset + (aWidth *
> +                              BytesPerPixel(aBPP) +
> +                              PaddingBytes(aBPP, aWidth)) *

Wow, this is very unreadable. Maybe:

mBMPFileHeader.filesize =
  mBMPFileHeader.dataoffset +
  (aWidth * BytesPerPixel(aBPP) + PaddingBytes(aBPP, aWidth)) *
  aHeight;

(I realize that it was already unreadable before you got to it!)

@@ +528,1 @@
>                                   PaddingBytes(aBPP, aWidth)) * aHeight;

Again, this is very unreadable.

@@ +656,5 @@
>  }
>  
>  // Sets a pixel in the image buffer that doesn't have alpha data
> +static inline void
> +  SetPixel24(uint8_t*& imageBufferCurr, uint8_t aRed, uint8_t aGreen,

|SetPixel24| should be in the first column...

::: image/encoders/bmp/nsBMPEncoder.h
@@ +65,5 @@
>    void EncodeImageDataRow24(const uint8_t* aData);
>    // Encodes a row of image data which does have alpha data
>    void EncodeImageDataRow32(const uint8_t* aData);
>    // Obtains the current offset filled up to for the image buffer
> +  inline int32_t GetCurrentImageBufferOffset() {

I think this was OK the way it was.

::: image/encoders/ico/nsICOEncoder.cpp
@@ +434,5 @@
>  nsICOEncoder::NotifyListener()
>  {
>    if (mCallback &&
> +      (GetCurrentImageBufferOffset() - mImageBufferReadPoint >=
> +       mNotifyThreshold || mFinished)) {

Please put both operands of |>=| on the same line.

::: image/encoders/ico/nsICOEncoder.h
@@ +63,5 @@
>    void EncodeFileHeader();
>    // Encodes the icon directory info header mICODirEntry
>    void EncodeInfoHeader();
>    // Obtains the current offset filled up to for the image buffer
> +  inline int32_t GetCurrentImageBufferOffset() {

I think this was OK as-is.

::: image/encoders/jpeg/nsJPEGEncoder.cpp
@@ +28,5 @@
>                                   mImageBufferUsed(0), mImageBufferReadPoint(0),
>                                   mCallback(nullptr),
>                                   mCallbackTarget(nullptr), mNotifyThreshold(0),
> +                                 mReentrantMonitor(
> +                                              "nsJPEGEncoder.mReentrantMonitor")

I'd maybe just move all of the initializers to the left:

nsJPEGEncoder::nsJPEGEncoder()
  : mFinished(false)
...

@@ +90,5 @@
>      if (aOutputOptions.Length() > qualityPrefix.Length()  &&
>          StringBeginsWith(aOutputOptions, qualityPrefix)) {
>        // have quality string
>        nsCString value = NS_ConvertUTF16toUTF8(Substring(aOutputOptions,
> +                                                       qualityPrefix.Length()));

I'd just put NS_ConvertUTF16toUTF8 on the next line. It's important that arguments are aligned.

@@ +281,3 @@
>    nsresult rv = aWriter(this, aClosure,
> +                        reinterpret_cast<const char*>
> +                        (mImageBuffer+mImageBufferReadPoint),

Indent this line since it continues the previous expression.

@@ +458,5 @@
>  void // static
>  nsJPEGEncoder::termDestination(jpeg_compress_struct* cinfo)
>  {
>    nsJPEGEncoder* that = static_cast<nsJPEGEncoder*>(cinfo->client_data);
> +  if (! that->mImageBuffer) {

There's an extra space here.

::: image/encoders/png/nsPNGEncoder.cpp
@@ +709,5 @@
>  nsPNGEncoder::WriteCallback(png_structp png, png_bytep data,
>                              png_size_t size)
>  {
>    nsPNGEncoder* that = static_cast<nsPNGEncoder*>(png_get_io_ptr(png));
> +  if (! that->mImageBuffer) {

There's an extra space here.

::: image/public/imgILoader.idl
@@ +47,5 @@
>     * @param aCacheKey cache key to use for a load if the original
>     *                  image came from a request that had post data
>  
>  
> +   * libimage does NOT keep a strong ref to the observer; this prevents

Thanks so much for removing the remaining libpr0n references! They have bugged me for a while but I never got around to getting rid of them.

Nit: I think we're generally calling this code "imagelib" or "ImageLib" these days. (If you change this, update the comment for loadImageWithChannelXPCOM too.)

::: image/test/unit/test_encoder_apng.js
@@ +1,2 @@
>  /*
> + * Test for APNG encoding in libimage

Same note about libimage vs imagelib/ImageLib.

::: image/test/unit/test_encoder_png.js
@@ +1,2 @@
>  /*
> + * Test for PNG encoding in libimage

Also here.

Comment 15

[Glenn Randers-Pehrson]

What do we want to do with "**" in things like (char **text) ?  I see all of these:
    function(char** text)
    function(char **text)
    function(char* *text)

Clearly the second is not conforming to the guide but I don't know which of
the first or third is preferable.

Among the empty bracketed things I see
    function_prototype { }
    function_prototype {}

I think the one with the space between the brackets is more in line with the guide.

Comment 16

[Glenn Randers-Pehrson]

seth wrote:

> ::: image/decoders/GIF2.h
> @@ +52,1 @@
> >  typedef struct gif_struct {
>
> Can we get rid of this |typedef struct| business while we're at it? This is C++, not C, so this can just be |struct gif_struct|.

Making this change caused a compiler error (using gcc 4.8.2).  I'd rather address this separately.  I see only two instances, one in GIF2.h and the other in nsJPEGDecoder.h

Comment 17

[Seth Fowler [:seth] [:s2h]]

(In reply to Glenn Randers-Pehrson from comment #15)
> What do we want to do with "**" in things like (char **text) ?  I see all of
> these:
>     function(char** text)
>     function(char **text)
>     function(char* *text)
> 
> Clearly the second is not conforming to the guide but I don't know which of
> the first or third is preferable.

I think |function(char** text)| is what the style guide intends.

> Among the empty bracketed things I see
>     function_prototype { }
>     function_prototype {}
> 
> I think the one with the space between the brackets is more in line with the
> guide.

I agree. I admit partly because the 'J' command in vim produces |function_prototype { }|, so it's easier for me to just go with that. =)

Comment 18

[Seth Fowler [:seth] [:s2h]]

(In reply to Glenn Randers-Pehrson from comment #16)
> Making this change caused a compiler error (using gcc 4.8.2).  I'd rather
> address this separately.  I see only two instances, one in GIF2.h and the
> other in nsJPEGDecoder.h

For sure. In general, if anything I suggested turns out to be tricky, we should handle it in another bug.

Comment 19

[Glenn Randers-Pehrson]

Attachment: v08 clean up image codecs in accordance with Mozilla Coding Style

Addresses issues raised in seth's review.

Comment 20

[Glenn Randers-Pehrson]

Attachment: v09 clean up image codecs in accordance with Mozilla Coding Style

Moved another bunch of "*" to the left.

Comment 21

[Glenn Randers-Pehrson]

Attachment: v10 clean up image codecs in accordance with Mozilla Coding Style

Merge with image/decoders/icon/nsIconProtocolHandler.cpp as changed in bug #1067471, and expand TAB characters in image/decoders/iccjpeg.c

Comment 22

[Glenn Randers-Pehrson]

The v10 patch works fine locally, but the changes from v06 are extensive so I suppose it wouldn't hurt to do another tryserver run before landing it.

Comment 23

[Glenn Randers-Pehrson]

Attachment: v11 clean up image codecs in accordance with Mozilla Coding Style

Fixed overnight bitrot in image/public/imgILoader.idl

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8511522 [details] [diff] [review]
v11 clean up image codecs in accordance with Mozilla Coding Style

Needs rebasing.

Comment 25

[Glenn Randers-Pehrson]

Attachment: v12 clean up image codecs in accordance with Mozilla Coding Style

Fix bitrot (relocation of content/base/src to dom/base)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

patching file image/decoders/EXIF.cpp
Hunk #2 FAILED at 78
1 out of 4 hunks FAILED -- saving rejects to file image/decoders/EXIF.cpp.rej
patching file image/encoders/png/nsPNGEncoder.cpp
Hunk #8 FAILED at 651
1 out of 9 hunks FAILED -- saving rejects to file image/encoders/png/nsPNGEncoder.cpp.rej

Comment 27

[Glenn Randers-Pehrson]

The v12 patch works for me, after a fresh "hg pull -u":
glenn.rp> hg sum
parent: 212384:8230834302c9 tip

Making a new clone now but that'll take most of the day and I don't expect it to change anything.

Comment 28

[Glenn Randers-Pehrson]

(In reply to Glenn Randers-Pehrson from comment #27)

> Making a new clone now but that'll take most of the day and I don't expect
> it to change anything.

I must have accidentally edited a couple of files in my "a" directory.

Comment 29

[Glenn Randers-Pehrson]

Attachment: v13 clean up image codecs in accordance with Mozilla Coding Style

v13 works locally against a fresh clone of mozilla-central.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=92a97b6aa2e2

Comment 31

[Glenn Randers-Pehrson]

Attachment: v14 clean up image codecs in accordance with Mozilla Coding Style

Fixed bustage on OS-X and WinXP builds.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=27d5365eb28d

Comment 33

[Glenn Randers-Pehrson]

Comment on attachment 8512793 [details] [diff] [review]
v14 clean up image codecs in accordance with Mozilla Coding Style

Please transfer r+ from the v07 patch to this (v14) patch.

Comment 34

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8512793 [details] [diff] [review]
v14 clean up image codecs in accordance with Mozilla Coding Style

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

Looks very good! Thanks so much for doing this, Glenn. This will be a huge improvement.

::: image/decoders/EXIF.h
@@ +44,5 @@
>      : mStart(nullptr)
>      , mCurrent(nullptr)
>      , mLength(0)
>      , mRemainingLength(0)
> +    , mByteOrder(ByteOrder::Unknown) { }

Please just leave the |{ }| on the next line. (In other words, revert this change.)

Comment 35

[Glenn Randers-Pehrson]

Attachment: v15 clean up image codecs in accordance with Mozilla Coding Style

Trivial changes from v14 patch recommended by Seth.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08434d415b5c

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/08434d415b5c