Make image/src source files comply with Mozilla Coding Style

RESOLVED FIXED in Firefox 40

Status

()

defect
--
trivial
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: glennrp+bmo, Assigned: glennrp+bmo)

Tracking

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

()

Attachments

(35 attachments, 307 obsolete attachments)

23.94 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
3.88 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
7.74 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
13.22 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
3.14 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
6.94 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
16.21 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
11.86 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
6.59 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
2.44 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
3.41 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
2.57 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
2.02 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
2.05 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
1.25 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
7.50 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
6.40 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
12.49 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
16.51 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
31.92 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
5.46 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
2.17 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
23.24 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
22.73 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
3.94 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
2.19 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
1.76 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
35.55 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
10.47 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
10.01 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
18.82 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
18.27 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
10.31 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
9.07 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
103.81 KB, patch
seth
: review+
seth
: checkin+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
Similar to bug #991149, this brings the source code in the image/src directory more closely in compliance with the Mozilla Coding Style Guide.  Long lines are wrapped, trailing blanks are removed, curly braces are used with all "if ()" statements, pointer stars are cuddled to the left,
and function type declarations are put on a separate line.
Assignee

Comment 1

5 years ago
Posted patch v00 image/src cleanup (obsolete) — Splinter Review
Assignee

Updated

5 years ago
Attachment #8525758 - Flags: review?(seth)
Glenn, could you please break this into a series of smaller patches? I think it'll be easier for both of us.

Also, you should rebase this against central, and be aware that you're probably going to have to rebase this fairly frequently, as ImageLib is undergoing major churn right now. I notice that some of the files you've touched in the current version of your patch (e.g. FrameSequence.cpp/h) no longer exist, and others are about to be removed (e.g. DiscardTracker.cpp/h).

(That's another reason to split it into many small patches; we can check in individual patches as soon as they are r+'d, which will help you avoid a lot of tedious rebasing.)
Flags: needinfo?(glennrp+bmo)
Attachment #8525758 - Flags: review?(seth)
(In reply to Seth Fowler [:seth] from comment #2)
> I notice that some of the files you've
> touched in the current version of your patch (e.g. FrameSequence.cpp/h)

Whoops! Actually I guess that patch hasn't landed yet either. That's another one that's about to go away, though.
Assignee

Comment 4

5 years ago
Flags: needinfo?(glennrp+bmo)
Assignee

Updated

5 years ago
Attachment #8528074 - Attachment filename: part03-1102048-v01-cleanup-image-src-bmp.dif → part03-1102048-v01-cleanup-image-src-bmp.diff
Attachment #8528074 - Attachment mime type: video/dv → text/plain
Assignee

Updated

5 years ago
Attachment #8528074 - Attachment is patch: true
Assignee

Comment 17

5 years ago
Attachment #8525758 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8528071 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528072 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528074 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528075 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528076 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528077 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528078 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528079 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528081 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528082 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528083 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528085 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528086 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528087 - Flags: review?(seth)
Assignee

Comment 18

5 years ago
Split from part14 and rebase
Attachment #8528667 - Flags: review?(seth)
Assignee

Comment 19

5 years ago
Move VectorImage patch to part14.
Attachment #8528087 - Attachment is obsolete: true
Attachment #8528087 - Flags: review?(seth)
Attachment #8528668 - Flags: review?(seth)
Assignee

Comment 20

5 years ago
Rebased
Attachment #8528076 - Attachment is obsolete: true
Attachment #8528076 - Flags: review?(seth)
Attachment #8528669 - Flags: review?(seth)
Assignee

Comment 21

5 years ago
Rebased.
Attachment #8528078 - Attachment is obsolete: true
Attachment #8528078 - Flags: review?(seth)
Attachment #8528671 - Flags: review?(seth)
Assignee

Comment 22

5 years ago
Rebased.
Attachment #8528086 - Attachment is obsolete: true
Attachment #8528086 - Flags: review?(seth)
Attachment #8528673 - Flags: review?(seth)
Assignee

Comment 23

5 years ago
Rebased.
Attachment #8528085 - Attachment is obsolete: true
Attachment #8528085 - Flags: review?(seth)
Attachment #8528678 - Flags: review?(seth)
Assignee

Comment 24

5 years ago
Rebased ImageWrapper.cpp
Attachment #8528081 - Attachment is obsolete: true
Attachment #8528082 - Attachment is obsolete: true
Attachment #8528081 - Flags: review?(seth)
Attachment #8528082 - Flags: review?(seth)
Attachment #8528684 - Flags: review?(seth)
Assignee

Comment 25

5 years ago
Rebased.
Attachment #8528703 - Flags: review?(seth)
Assignee

Comment 26

5 years ago
A "}" was missing.
Attachment #8528667 - Attachment is obsolete: true
Attachment #8528667 - Flags: review?(seth)
Attachment #8528741 - Flags: review?(seth)
Assignee

Comment 27

5 years ago
9 parts need rebasing today.
Attachment #8528079 - Attachment is obsolete: true
Attachment #8528083 - Attachment is obsolete: true
Attachment #8528668 - Attachment is obsolete: true
Attachment #8528669 - Attachment is obsolete: true
Attachment #8528671 - Attachment is obsolete: true
Attachment #8528673 - Attachment is obsolete: true
Attachment #8528684 - Attachment is obsolete: true
Attachment #8528703 - Attachment is obsolete: true
Attachment #8528741 - Attachment is obsolete: true
Attachment #8528079 - Flags: review?(seth)
Attachment #8528083 - Flags: review?(seth)
Attachment #8528668 - Flags: review?(seth)
Attachment #8528669 - Flags: review?(seth)
Attachment #8528671 - Flags: review?(seth)
Attachment #8528673 - Flags: review?(seth)
Attachment #8528684 - Flags: review?(seth)
Attachment #8528703 - Flags: review?(seth)
Attachment #8528741 - Flags: review?(seth)
Attachment #8529796 - Flags: review?(seth)
Assignee

Comment 28

5 years ago
Attachment #8529797 - Flags: review?(seth)
Assignee

Comment 29

5 years ago
Attachment #8529800 - Flags: review?(seth)
Assignee

Comment 30

5 years ago
Attachment #8529801 - Flags: review?(seth)
Assignee

Comment 31

5 years ago
Attachment #8529803 - Flags: review?(seth)
Assignee

Comment 32

5 years ago
Attachment #8529804 - Flags: review?(seth)
Assignee

Comment 33

5 years ago
Attachment #8529801 - Attachment is obsolete: true
Attachment #8529801 - Flags: review?(seth)
Attachment #8529806 - Flags: review?(seth)
Assignee

Comment 34

5 years ago
Attachment #8529807 - Flags: review?(seth)
Assignee

Comment 35

5 years ago
Attachment #8529810 - Flags: review?(seth)
Assignee

Comment 36

5 years ago
Attachment #8529815 - Flags: review?(seth)
Assignee

Comment 37

5 years ago
Fix botched merge (was missing one line)
Attachment #8529797 - Attachment is obsolete: true
Attachment #8529800 - Attachment is obsolete: true
Attachment #8529806 - Attachment is obsolete: true
Attachment #8529815 - Attachment is obsolete: true
Attachment #8529797 - Flags: review?(seth)
Attachment #8529800 - Flags: review?(seth)
Attachment #8529806 - Flags: review?(seth)
Attachment #8529815 - Flags: review?(seth)
Attachment #8529865 - Flags: review?(seth)
Assignee

Comment 38

5 years ago
A "{" was missing.
Attachment #8529912 - Flags: review?(seth)
Assignee

Comment 39

5 years ago
A space was missing.
Attachment #8529915 - Flags: review?(seth)
Assignee

Comment 40

5 years ago
Attachment #8529912 - Attachment is obsolete: true
Attachment #8529912 - Flags: review?(seth)
Attachment #8530025 - Flags: review?(seth)
Assignee

Comment 41

5 years ago
Attachment #8529803 - Attachment is obsolete: true
Attachment #8529803 - Flags: review?(seth)
Attachment #8530026 - Flags: review?(seth)
Assignee

Comment 42

5 years ago
Attachment #8530027 - Flags: review?(seth)
Assignee

Comment 43

5 years ago
Attachment #8529804 - Attachment is obsolete: true
Attachment #8529804 - Flags: review?(seth)
Attachment #8530028 - Flags: review?(seth)
Assignee

Comment 44

5 years ago
Attachment #8530029 - Flags: review?(seth)
Assignee

Comment 45

5 years ago
Attachment #8530031 - Flags: review?(seth)
Assignee

Comment 46

5 years ago
Attachment #8530032 - Flags: review?(seth)
Assignee

Comment 47

5 years ago
Subdivided several of the larger parts, adding parts 17-21.
Attachment #8530036 - Flags: review?(seth)
Assignee

Comment 48

5 years ago
Attachment #8530048 - Flags: review?(seth)
Assignee

Comment 49

5 years ago
Attachment #8530060 - Flags: review?(seth)
Assignee

Comment 50

5 years ago
None parts were broken overnight
Attachment #8528071 - Attachment is obsolete: true
Attachment #8528678 - Attachment is obsolete: true
Attachment #8529807 - Attachment is obsolete: true
Attachment #8529810 - Attachment is obsolete: true
Attachment #8529915 - Attachment is obsolete: true
Attachment #8530026 - Attachment is obsolete: true
Attachment #8530029 - Attachment is obsolete: true
Attachment #8530036 - Attachment is obsolete: true
Attachment #8530048 - Attachment is obsolete: true
Attachment #8528071 - Flags: review?(seth)
Attachment #8528678 - Flags: review?(seth)
Attachment #8529807 - Flags: review?(seth)
Attachment #8529810 - Flags: review?(seth)
Attachment #8529915 - Flags: review?(seth)
Attachment #8530026 - Flags: review?(seth)
Attachment #8530029 - Flags: review?(seth)
Attachment #8530036 - Flags: review?(seth)
Attachment #8530048 - Flags: review?(seth)
Attachment #8530334 - Flags: review?(seth)
Assignee

Comment 51

5 years ago
Nine (not "none") parts were broken overnight.
Sorry for all the bustage! Unfortunately I think you'll have to rebase one more time; a final set of big changes hit this morning. (DiscardTracker is now totally gone, for example.)

The good news is that next week I'll be focusing on regression hunting and won't be doing any big refactorings. So that'll be the perfect time to review these patches and get them landed.
Assignee

Comment 60

5 years ago
I took care of this morning's bustage.  The patches are good against changeset 218076.
Assignee

Comment 61

5 years ago
Attachment #8529865 - Attachment is obsolete: true
Attachment #8529865 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8530469 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8528077 - Attachment is obsolete: true
Attachment #8528077 - Flags: review?(seth)
Assignee

Comment 62

5 years ago
Fix long lines, trailing blanks, and pointer stars in image/public
Attachment #8530479 - Flags: review?(seth)
Comment on attachment 8530479 [details] [diff] [review]
part22-1102048-v01-cleanup-image-public

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

This patch looks great; thanks!

::: image/public/ImageLogging.h
@@ +23,1 @@
>      mLog(aLog), mFrom(from), mFunc(fn)

FWIW this initializer list is also against the style guide, though it's not stated very clearly. It should be:

LogScope(PRLogModuleInfo* aLog, void* aFrom, const char* aFunc)
  : mLog(aLog)
  , mFrom(aFrom)
  , mFunc(aFunc)
{
  ...
}

::: image/public/imgICache.idl
@@ +54,5 @@
>    nsIProperties findEntryProperties(in nsIURI uri);
>  
>    /**
> +   * Make this cache instance respect private browsing notifications. This
> +   * entails clearing * the chrome and content caches whenever the

Looks like there's an extra '*' in this line.
Attachment #8530479 - Flags: review?(seth) → review+
Comment on attachment 8530342 [details] [diff] [review]
part18-1102048-v02-cleanup-image-src-ops

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

Looks good!

::: image/src/ImageURL.h
@@ +36,5 @@
>    }
>  
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ImageURL)
>  
> +  nsresult GetSpec(nsACString& result) {

The brace should be on the next line. Same for GetScheme and other methods in this file.

Ideally we'd also change the argument names to be correct - i.e. |result| should be |aResult|. I know that'd result in you touching a lot of lines of code, though.

@@ +79,5 @@
>    nsAutoCString mScheme;
>    nsAutoCString mRef;
>  
> +  ~ImageURL()
> +  { }

Please don't make this change. It's not against the style guide to do it the other way. In fact, I've edited the style guide to make this more clear.

::: image/src/ImageWrapper.h
@@ +26,5 @@
>  
>    virtual already_AddRefed<ProgressTracker> GetProgressTracker() MOZ_OVERRIDE;
>    virtual nsIntRect FrameRect(uint32_t aWhichFrame) MOZ_OVERRIDE;
>  
> +  virtual size_t SizeOfSourceWithComputedFallback(

Putting |virtual size_t| on one line and |SizeOfSourceWithComputedFallback(MallocSizeOf...| on the second line might let us keep the method name and the first argument on the same line. It's preferable to do that if we can.

@@ +66,5 @@
>      NS_ABORT_IF_FALSE(aInnerImage, "Cannot wrap a null image");
>    }
>  
> +  virtual ~ImageWrapper()
> +  { }

Again, please don't make this change.
Attachment #8530342 - Flags: review+
Comment on attachment 8530032 [details] [diff] [review]
part21-1102048-v01-cleanup-image-src-blend

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

Nice!

::: image/src/FrameBlender.cpp
@@ +545,5 @@
> +                               aDstRect.width * 4);
> +
> +    pixman_image_composite32(
> +      aBlendMethod == FrameBlender::kBlendSource ?
> +          PIXMAN_OP_SRC : PIXMAN_OP_OVER,

Rather than shift all of the arguments to pixman_image_composite32 to the left because of this expression, I'd rather that we compute the compositing operation in a separate statement. Maybe:

auto op = aBlendMethod == FrameBlender::kBlendSource ? PIXMAN_OP_SRC
                                                     : PIXMAN_OP_OVER;

::: image/src/FrameBlender.h
@@ +141,5 @@
>      RawAccessFrameRef compositingPrevFrame;
>  
>      Anim() :
>        lastCompositedFrameIndex(-1)
> +    { }

The ':' should be on the same line as |lastCompositedFrameIndex(-1)|.
Attachment #8530032 - Flags: review?(seth) → review+
Comment on attachment 8530341 [details] [diff] [review]
part16-1102048-v02-cleanup-image-src-image

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

Looks good!

::: image/src/Image.h
@@ +159,5 @@
>  
>    virtual void IncrementAnimationConsumers() MOZ_OVERRIDE;
>    virtual void DecrementAnimationConsumers() MOZ_OVERRIDE;
>  #ifdef DEBUG
> +  virtual uint32_t GetAnimationConsumers() MOZ_OVERRIDE {

The brace should be on the next line here. (And for SetProgressTracker too.)
Attachment #8530341 - Flags: review+
Assignee

Comment 67

5 years ago
Rebased part05 (Decoder.h)
Attachment #8529796 - Attachment is obsolete: true
Attachment #8529796 - Flags: review?(seth)
Attachment #8532108 - Flags: review?(seth)
Comment on attachment 8530337 [details] [diff] [review]
part12-1102048-v03-cleanup-image-src-orient-progress.diff

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

Looks good!

One important note, though: in general in these patches I'm going to push back on any change that makes tiny functions/methods/constructors take up multiple lines. It's explicitly allowed in the style guide for them to be collapsed onto one line, and I think splitting them onto multiple lines decreases readability greatly by making the code more verbose.

::: image/src/OrientedImage.cpp
@@ +166,5 @@
>  struct MatrixBuilder
>  {
> +  explicit
> +  MatrixBuilder(bool aInvert) : mInvert(aInvert)
> +  { }

Please don't make this change. The original is not contrary to the style guide.

@@ +171,3 @@
>  
> +  gfxMatrix
> +  Build() { return mMatrix; }

Please don't make this change either.

@@ +174,3 @@
>  
> +  void
> +  Scale(gfxFloat aX, gfxFloat aY) {

Nor this one. This is inside a class body, and the rule about putting the type on a separate line is for method definitions outside of a class body.

The brace does belong on the next line, though.

@@ +181,5 @@
>      }
>    }
>  
> +  void
> +  Rotate(gfxFloat aPhi) {

Please don't make this change either.

@@ +190,5 @@
>      }
>    }
>  
> +  void
> +  Translate(gfxPoint aDelta) {

Nor this one.

::: image/src/OrientedImage.h
@@ +58,5 @@
>      , mOrientation(aOrientation)
>    { }
>  
> +  virtual ~OrientedImage()
> +  { }

Please don't make this change.

::: image/src/ProgressTracker.cpp
@@ +369,5 @@
>  
>  void
> +ProgressTracker::
> +    SyncNotifyProgress(Progress aProgress,
> +                       const nsIntRect& aInvalidRect /* = nsIntRect() */)

This formatting is quite strange... I'd say just move |/* = nsIntRect() */| to the following line and indent it.

::: image/src/ProgressTracker.h
@@ +70,5 @@
>   */
>  class ProgressTracker : public mozilla::SupportsWeakPtr<ProgressTracker>
>  {
> +  virtual ~ProgressTracker()
> +  { }

Please don't make this change.
Attachment #8530337 - Flags: review+
Attachment #8530335 - Flags: review?(seth)
Assignee

Comment 69

5 years ago
Rebased part21 (FrameBlender.h, .cpp)
Attachment #8530032 - Attachment is obsolete: true
Attachment #8532110 - Flags: review?(seth)
Attachment #8530336 - Flags: review?(seth)
Attachment #8530338 - Flags: review?(seth)
Attachment #8530339 - Flags: review?(seth)
Attachment #8530343 - Flags: review?(seth)
Assignee

Comment 70

5 years ago
Revised per seth's comments.  There were several more instances of the initialization style to be fixed.
Attachment #8530479 - Attachment is obsolete: true
Assignee

Comment 71

5 years ago
Revised per seth's review.  There were several other instances of "{" on the wrong line.  What about lines like the following, which I did not touch?

  virtual uint64_t InnerWindowID() const MOZ_OVERRIDE { return mInnerWindowId; }

  virtual bool HasError() MOZ_OVERRIDE    { return mError; }
  virtual void SetHasError() MOZ_OVERRIDE { mError = true; }

Should they be like this, instead?
  virtual void
  SetHasError() MOZ_OVERRIDE
  {
    mError = true;
  }
Attachment #8530341 - Attachment is obsolete: true
Attachment #8532135 - Flags: review?(seth)
Assignee

Comment 72

5 years ago
> Should they be like this, instead?
>   virtual void
>   SetHasError() MOZ_OVERRIDE
>   {
>     mError = true;
>   }

Never mind, I'll leave them alone per comment #68 (but should the
return type be put in a separate line anyhow, like

    virtual void
    SetHasError() MOZ_OVERRIDE { mError = true; }
Assignee

Comment 73

5 years ago
Revised part18 per review comments
Attachment #8530342 - Attachment is obsolete: true
Attachment #8532144 - Flags: review?(seth)
Assignee

Comment 74

5 years ago
Revised per review
Attachment #8530337 - Attachment is obsolete: true
Attachment #8532148 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8532128 - Flags: review?(seth)
Assignee

Comment 75

5 years ago
Comment on attachment 8528072 [details] [diff] [review]
part02-1102048-v01-cleanup-image-decoders

Marking v02 obsolete.  It only moves some "{ }" to separate lines, which we don't need to do per seth's review comments.
Attachment #8528072 - Attachment is obsolete: true
Attachment #8528072 - Flags: review?(seth)
Assignee

Comment 76

5 years ago
Attachment #8532108 - Attachment is obsolete: true
Attachment #8532108 - Flags: review?(seth)
Attachment #8532167 - Flags: review?(seth)
Assignee

Comment 77

5 years ago
Attachment #8530335 - Attachment is obsolete: true
Attachment #8530335 - Flags: review?(seth)
Attachment #8532169 - Flags: review?(seth)
Assignee

Comment 78

5 years ago
Attachment #8530025 - Attachment is obsolete: true
Attachment #8530025 - Flags: review?(seth)
Attachment #8532170 - Flags: review?(seth)
Assignee

Comment 79

5 years ago
Attachment #8530336 - Attachment is obsolete: true
Attachment #8530336 - Flags: review?(seth)
Attachment #8532175 - Flags: review?(seth)
Assignee

Comment 80

5 years ago
Attachment #8530338 - Attachment is obsolete: true
Attachment #8530338 - Flags: review?(seth)
Attachment #8532178 - Flags: review?(seth)
Assignee

Comment 81

5 years ago
Attachment #8530343 - Attachment is obsolete: true
Attachment #8530343 - Flags: review?(seth)
Attachment #8532180 - Flags: review?(seth)
(In reply to Glenn Randers-Pehrson from comment #72)
> Never mind, I'll leave them alone per comment #68 (but should the
> return type be put in a separate line anyhow, like
> 
>     virtual void
>     SetHasError() MOZ_OVERRIDE { mError = true; }

No, it's fine to leave the return type on the same line. It's only important that it be on a separate line for method definitions outside of the class body. We don't gain anything by doing it inside the class body because the utilities that care about that are confused by the indentation inside the class body anyway.

There are examples here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

Let me know if there's anything that's ambiguous and we'll get the style guide updated to make it more clear.
Assignee

Comment 83

5 years ago
Rebased
Attachment #8532178 - Attachment is obsolete: true
Attachment #8532178 - Flags: review?(seth)
Attachment #8532508 - Flags: review?(seth)
Assignee

Comment 84

5 years ago
Rebased
Attachment #8530339 - Attachment is obsolete: true
Attachment #8530339 - Flags: review?(seth)
Attachment #8532511 - Flags: review?(seth)
Assignee

Comment 85

5 years ago
Rebased SVGDocumentWrapper
Attachment #8532508 - Attachment is obsolete: true
Attachment #8532508 - Flags: review?(seth)
Attachment #8532515 - Flags: review?(seth)
Assignee

Comment 86

5 years ago
Rebased VectorImage
Attachment #8532511 - Attachment is obsolete: true
Attachment #8532511 - Flags: review?(seth)
Attachment #8532517 - Flags: review?(seth)
Comment on attachment 8532128 [details] [diff] [review]
part22-1102048-v02-cleanup-image-public

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

Looks good!
Attachment #8532128 - Flags: review?(seth) → review+
Comment on attachment 8532135 [details] [diff] [review]
part16-1102048-v03-cleanup-image-src-image

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

Nice!
Attachment #8532135 - Flags: review?(seth) → review+
Comment on attachment 8532144 [details] [diff] [review]
part18-1102048-v03-cleanup-image-src-ops

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

This one looks good as well!
Attachment #8532144 - Flags: review?(seth) → review+
Comment on attachment 8532148 [details] [diff] [review]
part12-1102048-v04-cleanup-image-src-orient-progress

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

::: image/src/ProgressTracker.cpp
@@ +370,5 @@
>  void
> +ProgressTracker::
> +    SyncNotifyProgress(Progress aProgress,
> +                       const nsIntRect& aInvalidRect)
> +                                        // = nsIntRect()

This formatting is still a little strange. =)

How about this:

ProgressTracker::SyncNotifyProgress(Progress aProgress,
                                    const nsIntRect& aInvalidRect
                                      /* = nsIntRect() */)
Attachment #8532148 - Flags: review?(seth) → review+
I'm going to check in patches that have no issues as we go, so that you don't have to rebase them. I'll mark the patches I check in as checkin+.
Keywords: leave-open
Assignee

Updated

5 years ago
Attachment #8532135 - Flags: checkin?(ryanvm)
Assignee

Updated

5 years ago
Attachment #8532144 - Flags: checkin?(ryanvm)
Assignee

Updated

5 years ago
Attachment #8532128 - Flags: checkin?(ryanvm)
Attachment #8532128 - Flags: checkin?(ryanvm) → checkin+
Attachment #8532135 - Flags: checkin?(ryanvm) → checkin+
Attachment #8532144 - Flags: checkin?(ryanvm) → checkin+
Assignee

Comment 93

5 years ago
(In reply to Seth Fowler [:seth] from comment #90)
> Comment on attachment 8532148 [details] [diff] [review]
> part12-1102048-v04-cleanup-image-src-orient-progress
> 
> Review of attachment 8532148 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/ProgressTracker.cpp
> @@ +370,5 @@
> >  void
> > +ProgressTracker::
> > +    SyncNotifyProgress(Progress aProgress,
> > +                       const nsIntRect& aInvalidRect)
> > +                                        // = nsIntRect()
> 
> This formatting is still a little strange. =)
> 
> How about this:
> 
> ProgressTracker::SyncNotifyProgress(Progress aProgress,
>                                     const nsIntRect& aInvalidRect
>                                       /* = nsIntRect() */)

How about just removing the comment?  It looks to me like a leftover
from a time when the "const nsIntRect&" part was something else.  It
does not now seem to be very helpful.
(In reply to Glenn Randers-Pehrson from comment #93)
> How about just removing the comment?  It looks to me like a leftover
> from a time when the "const nsIntRect&" part was something else.  It
> does not now seem to be very helpful.

It actually does serve a purpose - it lets the reader of the code know that aInvalidRect has a default argument, and what the value of that default argument is.
Comment on attachment 8532169 [details] [diff] [review]
part07-1102048-v06-cleanup-image-src-dynamic

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

Looks good!

::: image/src/DynamicImage.h
@@ +13,5 @@
>  namespace mozilla {
>  namespace image {
>  
>  /**
> +

This blank line seems like it shouldn't even be here, actually...
Attachment #8532169 - Flags: review?(seth) → review+
Comment on attachment 8532110 [details] [diff] [review]
part21-1102048-v02-cleanup-image-src-blend

For part 21, looks like you still need to address my review comments in comment 65.
Attachment #8532110 - Flags: review?(seth)
Comment on attachment 8532515 [details] [diff] [review]
part14-1102048-v07-cleanup-image-src-notify

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

Looks great!

::: image/src/SurfaceCache.cpp
@@ +704,5 @@
>  
>    nsTArray<CostEntry>                                       mCosts;
>    nsRefPtrHashtable<nsPtrHashKey<Image>, ImageSurfaceCache> mImageCaches;
>    SurfaceTracker                                            mExpirationTracker;
> +  nsRefPtr<MemoryPressureObserver>                     mMemoryPressureObserver;

Let's leave this as it is for now. The right fix is to make a typedef for the type of |mImageCaches|, but I think we should do that in a different bug.
Attachment #8532515 - Flags: review?(seth) → review+
Assignee

Comment 98

5 years ago
Revised part12 per seth's comment.
Attachment #8532148 - Attachment is obsolete: true
Attachment #8532707 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8532110 - Flags: review?(seth)
Comment on attachment 8530334 [details] [diff] [review]
part01-1102048-v02-cleanup-image-build

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

So that category entry list follows a specific format that we shouldn't change. Consider it more data than code - this is a very special case where we should worry about the style guide.

Since that's the only change in this patch, I'm r-'ing this one.
Attachment #8530334 - Flags: review?(seth) → review-
Comment on attachment 8530028 [details] [diff] [review]
part17-1102048-v01-cleanup-image-src-factory.diff

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

Looks good! A couple of tweaks:

::: image/src/ImageFactory.cpp
@@ +212,5 @@
>    // Pass anything usable on so that the RasterImage can preallocate
>    // its source buffer.
>    if (len > 0) {
> +    uint32_t sizeHint = std::min<uint32_t>(len, 20000000); // Bound by something
> +                                                           // reasonable

I'd say just move this comment to the line above.

@@ +238,5 @@
>        nsCOMPtr<nsIChannel> chan(do_QueryInterface(aRequest));
>        nsCOMPtr<nsIPrincipal> principal;
>        if (chan) {
>          nsContentUtils::GetSecurityManager()->GetChannelResultPrincipal(chan,
> +                                                     getter_AddRefs(principal));

This is kinda hard to read since the arguments aren't lined up. The formatting we usually use in a situation like this is:

nsContentUtils::GetSecurityManager()
  ->GetChannelResultPrincipal(chan, getter_AddRefs(principal));

Eyeballing it, I think that should work here.
Attachment #8530028 - Flags: review?(seth) → review+
Assignee

Comment 101

5 years ago
Revised part21 as seth suggested.
Attachment #8532110 - Attachment is obsolete: true
Attachment #8532110 - Flags: review?(seth)
Attachment #8532722 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8530334 - Attachment is obsolete: true
Assignee

Comment 102

5 years ago
Remove stray blank line from part07
Attachment #8532169 - Attachment is obsolete: true
Attachment #8532725 - Flags: review?(seth)
Assignee

Comment 103

5 years ago
Tweaked part17 as suggested.
Attachment #8530028 - Attachment is obsolete: true
Attachment #8532729 - Flags: review?(seth)
Assignee

Comment 104

5 years ago
Comment on attachment 8532180 [details] [diff] [review]
part20-1102048-v03-cleanup-image-src-imgloader

part20 needs rebase
Attachment #8532180 - Attachment is obsolete: true
Attachment #8532180 - Flags: review?(seth)
Assignee

Comment 105

5 years ago
Fixed some existing typos in FrameBlender.h
Attachment #8532722 - Attachment is obsolete: true
Attachment #8532722 - Flags: review?(seth)
Attachment #8532745 - Flags: review?(seth)
Assignee

Comment 106

5 years ago
Rebased part20.
Attachment #8532748 - Flags: review?(seth)
Assignee

Comment 107

5 years ago
Attachment #8532846 - Flags: review?(seth)
Assignee

Comment 108

5 years ago
Removed some timing and debugging stuff from part23
Attachment #8532846 - Attachment is obsolete: true
Attachment #8532846 - Flags: review?(seth)
Attachment #8532848 - Flags: review?(seth)
Assignee

Comment 109

5 years ago
Attachment #8532848 - Attachment is obsolete: true
Attachment #8532848 - Flags: review?(seth)
Attachment #8532849 - Flags: review?(seth)
Attachment #8532707 - Flags: review?(seth)
Attachment #8532707 - Flags: review+
Attachment #8532707 - Flags: checkin+
Attachment #8532725 - Flags: review?(seth)
Attachment #8532725 - Flags: review+
Attachment #8532725 - Flags: checkin+
Attachment #8532729 - Flags: review?(seth)
Attachment #8532729 - Flags: review+
Attachment #8532729 - Flags: checkin+
Attachment #8532745 - Flags: review?(seth)
Attachment #8532745 - Flags: review+
Attachment #8532745 - Flags: checkin+
Comment on attachment 8532170 [details] [diff] [review]
part08-1102048-v05-cleanup-image-src-animate

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

Looks good! Minor tweaks to indentation only:

::: image/src/FrameAnimator.cpp
@@ +53,5 @@
>  FrameAnimator::GetCurrentImgFrameEndTime() const
>  {
>    TimeStamp currentFrameTime = mCurrentAnimationFrameTime;
> +  int32_t timeout =
> +                  mFrameBlender.GetTimeoutForFrame(mCurrentAnimationFrameIndex);

I'd only indent two spaces for an assignment expression like this. Usually the rule of thumb is to indent by two spaces unless you're trying to line up with something, like function arguments or nested parens.

@@ +169,5 @@
>        // Explicitly use integer division to get the floor of the number of
>        // loops.
>        uint32_t loops = static_cast<uint32_t>(delay.ToMilliseconds()) / loopTime;
> +      mCurrentAnimationFrameTime +=
> +          TimeDuration::FromMilliseconds(loops * loopTime);

I'd also use a two-space indent here.
Attachment #8532170 - Flags: review?(seth) → review+
Comment on attachment 8532175 [details] [diff] [review]
part10-1102048-v06-cleanup-image-src-imgframe

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

Looks good!

::: image/src/imgFrame.cpp
@@ +188,5 @@
>      }
>  
>      // Use the fallible allocator here
> +    mPalettedImageData =
> +               (uint8_t*)moz_malloc(PaletteDataLength() + GetImageDataLength());

Please only indent by two spaces here.

@@ +367,5 @@
>  
>  #ifdef ANDROID
>    SurfaceFormat optFormat =
> +                gfxPlatform::GetPlatform()->Optimal2DFormatForContent(
> +                                                         gfxContentType::COLOR);

Please only indent by two spaces. Also, try to avoid the situation where you have an open paren for a function call on one line but the first argument on another line if at all possible. For this code, I'd prefer something like:

SurfaceFormat optFormat = gfxPlatform::GetPlatform()
  ->Optimal2DFormatForContent(gfxContentType::COLOR);

@@ +405,5 @@
>    }
>  #else
> +  mOptSurface = gfxPlatform::
> +     GetPlatform()->ScreenReferenceDrawTarget()->OptimizeSourceSurface(
> +                                                                 mImageSurface);

Same concerns as my previous comment. I'd prefer:

mOptSurface = gfxPlatform::GetPlatform()
  ->ScreenReferenceDrawTarget()->OptimizeSourceSurface(mImageSurface);

@@ +943,5 @@
>  imgFrame::SizeOfExcludingThis(gfxMemoryLocation aLocation,
>                                MallocSizeOf aMallocSizeOf) const
>  {
> +  // aMallocSizeOf is only used
> +  // if aLocation==gfxMemoryLocation::IN_PROCESS_HEAP.  It

This comment would have more sane word wrapping if we replaced |aLocation==gfxMemoryLocation::IN_PROCESS_HEAP| with |aLocation is gfxMemoryLocation::IN_PROCESS_HEAP|.
Attachment #8532175 - Flags: review?(seth) → review+
Comment on attachment 8532517 [details] [diff] [review]
part15-1102048-v06-cleanup-image-src-vector

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

Looks good. r+ with the MozSVGAsImageDocumentLoad issue below fixed.

::: image/src/VectorImage.cpp
@@ +226,5 @@
>    {
>      MOZ_ASSERT(mDocument, "Duplicate call to Cancel");
>      if (mDocument) {
> +      mDocument->RemoveEventListener(NS_LITERAL_STRING("MozSVGAsImage"
> +                                                       "DocumentLoad"),

Don't do this! It would be better to have the line go over 80 characters. If you do it this way, then searching for MozSVGAsImageDocumentLoad won't find this usage.

::: image/src/VectorImage.h
@@ +96,5 @@
>    nsRefPtr<SVGLoadEventListener>     mLoadEventListener;
>    nsRefPtr<SVGParseCompleteListener> mParseCompleteListener;
>  
>    bool           mIsInitialized;          // Have we been initalized?
> +  bool           mIsFullyLoaded;       // Has the SVG document finished loading?

Please just wrap the comment onto a second line so things can remain aligned.
Attachment #8532517 - Flags: review?(seth) → review+
Comment on attachment 8530031 [details] [diff] [review]
part19-1102048-v01-cleanup-image-src-meta

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

Looks good!

::: image/src/ImageMetadata.cpp
@@ +20,5 @@
>    if (mHotspotX != -1 && mHotspotY != -1) {
> +    nsCOMPtr<nsISupportsPRUint32> intwrapx =
> +                             do_CreateInstance(NS_SUPPORTS_PRUINT32_CONTRACTID);
> +    nsCOMPtr<nsISupportsPRUint32> intwrapy =
> +                             do_CreateInstance(NS_SUPPORTS_PRUINT32_CONTRACTID);

Please indent by two spaces only.
Attachment #8530031 - Flags: review?(seth) → review+
Assignee

Comment 116

5 years ago
Use 2-space indentation for wrapping long lines
Attachment #8532170 - Attachment is obsolete: true
Flags: needinfo?(seth)
Did you mean to request review?
Flags: needinfo?(seth)
Assignee

Comment 118

5 years ago
Indentation revised as suggested.
Flags: needinfo?(seth)
Flags: needinfo?(seth)
Assignee

Comment 119

5 years ago
Revised as suggested.  Also removed the trailing underscore from the guard in VectorImage.h, so it is now: #ifndef mozilla_imagelib_VectorImage_h
Question: should this really be #ifndef mozilla_image_src_VectorImage_h ?
or just #ifndef VectorImage_h ?
Attachment #8532517 - Attachment is obsolete: true
Flags: needinfo?(seth)
Attachment #8533371 - Flags: review?(seth)
(In reply to Glenn Randers-Pehrson from comment #119)
> Question: should this really be #ifndef mozilla_image_src_VectorImage_h ?
> or just #ifndef VectorImage_h ?

Probably the first one is better; we use long identifiers for include guards partly to make sure that we don't conflict with anything in a third-party library. (It happens!)
Flags: needinfo?(seth)
Assignee

Comment 121

5 years ago
Fixed indentation as requested and removed trailing double-underscore from guard in ImageMetadata.h
Attachment #8530031 - Attachment is obsolete: true
Attachment #8533378 - Flags: review?(seth)
Assignee

Comment 122

5 years ago
(In reply to Seth Fowler [:seth] from comment #120)
> (In reply to Glenn Randers-Pehrson from comment #119)
> > Question: should this really be #ifndef mozilla_image_src_VectorImage_h ?
> > or just #ifndef VectorImage_h ?
> 
> Probably the first one is better; we use long identifiers for include guards
> partly to make sure that we don't conflict with anything in a third-party
> library. (It happens!)

OK I'll fix all of them to be mozilla_image_src_*_h after the dust has settled
on the current set of patches.
Assignee

Updated

5 years ago
Attachment #8533362 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8533365 - Flags: review?(seth)
Assignee

Comment 124

5 years ago
Attachment #8533728 - Flags: review?(seth)
Assignee

Comment 125

5 years ago
EXIF_H -> EXIF_h and GIF2_H -> GIF2_h
Attachment #8533728 - Attachment is obsolete: true
Attachment #8533728 - Flags: review?(seth)
Attachment #8533734 - Flags: review?(seth)
Assignee

Comment 126

5 years ago
Removed indentation of namespace blocks
Attachment #8528074 - Attachment is obsolete: true
Attachment #8528074 - Flags: review?(seth)
Attachment #8533984 - Flags: review?(seth)
Assignee

Comment 127

5 years ago
Removed extra "#endif"; lined up some comments.
Attachment #8533984 - Attachment is obsolete: true
Attachment #8533984 - Flags: review?(seth)
Attachment #8533997 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8532175 - Attachment is obsolete: true
Assignee

Comment 128

5 years ago
Revised as suggested in review
Attachment #8532515 - Attachment is obsolete: true
Attachment #8534734 - Flags: review?(seth)
Assignee

Comment 129

5 years ago
Comment on attachment 8534734 [details] [diff] [review]
part14-1102048-v08-cleanup-image-src-notify

Needs rebase against new SurfaceCache.cpp
Attachment #8534734 - Attachment is obsolete: true
Attachment #8534734 - Flags: review?(seth)
Assignee

Comment 130

5 years ago
Rebased.
Attachment #8534998 - Flags: review?(seth)
Assignee

Comment 131

5 years ago
Fixed a spelling error
Attachment #8533997 - Attachment is obsolete: true
Attachment #8533997 - Flags: review?(seth)
Attachment #8535318 - Flags: review?(seth)
Assignee

Comment 132

5 years ago
Fixed an indendation error in a comment
Attachment #8532167 - Attachment is obsolete: true
Attachment #8532167 - Flags: review?(seth)
Attachment #8535319 - Flags: review?(seth)
Assignee

Comment 133

5 years ago
Rebase against new DecodePool.cpp
Attachment #8535319 - Attachment is obsolete: true
Attachment #8535319 - Flags: review?(seth)
Attachment #8535336 - Flags: review?(seth)
Assignee

Comment 134

5 years ago
Rebased against new RasterImage.cpp
Attachment #8530469 - Attachment is obsolete: true
Attachment #8530469 - Flags: review?(seth)
Attachment #8535337 - Flags: review?(seth)
Assignee

Comment 135

5 years ago
Moved many return types to a separate line
Attachment #8530027 - Attachment is obsolete: true
Attachment #8530027 - Flags: review?(seth)
Attachment #8535345 - Flags: review?(seth)
Assignee

Comment 136

5 years ago
Rebase DecodePool.cpp wasn't quite right
Attachment #8535336 - Attachment is obsolete: true
Attachment #8535336 - Flags: review?(seth)
Attachment #8535847 - Flags: review?(seth)
Assignee

Comment 137

5 years ago
Rebase of RasterImage.cpp was incorrect.
Attachment #8535337 - Attachment is obsolete: true
Attachment #8535337 - Flags: review?(seth)
Attachment #8535849 - Flags: review?(seth)
Assignee

Comment 138

5 years ago
Part 23 and part 24 were both updating decoders/nsICODecoder.cpp; combined these updates into part23.
Attachment #8532849 - Attachment is obsolete: true
Attachment #8532849 - Flags: review?(seth)
Attachment #8535851 - Flags: review?(seth)
Assignee

Comment 139

5 years ago
Moved update of decoders/nsICODecoder.h into part 23.
Attachment #8533734 - Attachment is obsolete: true
Attachment #8533734 - Flags: review?(seth)
Attachment #8535852 - Flags: review?(seth)
Assignee

Comment 140

5 years ago
Attachment #8536125 - Flags: review?(seth)
Assignee

Comment 141

5 years ago
Added checkin info at top of part25
Attachment #8536125 - Attachment is obsolete: true
Attachment #8536125 - Flags: review?(seth)
Attachment #8536127 - Flags: review?(seth)
Assignee

Comment 142

5 years ago
Attachment #8528075 - Attachment is obsolete: true
Attachment #8528075 - Flags: review?(seth)
Attachment #8536172 - Flags: review?(seth)
Assignee

Comment 143

5 years ago
Reduced indentation to 2 spaces when wrapping a long line.
Attachment #8533371 - Attachment is obsolete: true
Attachment #8533371 - Flags: review?(seth)
Attachment #8536173 - Flags: review?(seth)
Assignee

Comment 144

5 years ago
Fixed some indentation
Attachment #8536176 - Flags: review?(seth)
Assignee

Updated

5 years ago
Attachment #8532748 - Attachment is obsolete: true
Attachment #8532748 - Flags: review?(seth)
Assignee

Comment 145

5 years ago
Fixed indentation and a missing pair of curley brackets
Attachment #8533365 - Attachment is obsolete: true
Attachment #8533365 - Flags: review?(seth)
Attachment #8536181 - Flags: review?(seth)
Assignee

Comment 146

5 years ago
Updated header guard
Attachment #8533378 - Attachment is obsolete: true
Attachment #8533378 - Flags: review?(seth)
Attachment #8536239 - Flags: review?(seth)
Assignee

Comment 147

5 years ago
Updated header guard in FrameAnimator.h
Attachment #8533362 - Attachment is obsolete: true
Attachment #8533362 - Flags: review?(seth)
Attachment #8536240 - Flags: review?(seth)
Assignee

Comment 148

5 years ago
Added guards to image/encoders/ns{bmp|jpeg|ico}/*.h
Attachment #8535851 - Attachment is obsolete: true
Attachment #8535851 - Flags: review?(seth)
Attachment #8536613 - Flags: review?(seth)
Assignee

Comment 149

5 years ago
Attachment #8536633 - Flags: review?(seth)
Assignee

Comment 150

5 years ago
Rebased VectorImage.cpp
Attachment #8536173 - Attachment is obsolete: true
Attachment #8536173 - Flags: review?(seth)
Attachment #8537186 - Flags: review?(seth)
Assignee

Comment 151

5 years ago
Rebased image/src/VectorImage.cpp against changeset 219907
Attachment #8537186 - Attachment is obsolete: true
Attachment #8537186 - Flags: review?(seth)
Attachment #8537188 - Flags: review?(seth)
Assignee

Comment 152

5 years ago
Rebased SurfaceCache.cpp
Attachment #8534998 - Attachment is obsolete: true
Attachment #8534998 - Flags: review?(seth)
Attachment #8539504 - Flags: review?(seth)
Assignee

Comment 153

5 years ago
Rebased part20 against new imgLoader.cpp
Attachment #8536176 - Attachment is obsolete: true
Attachment #8536176 - Flags: review?(seth)
Attachment #8540937 - Flags: review?(seth)
Comment on attachment 8536240 [details] [diff] [review]
part08-1102048-v07-cleanup-image-src-animate

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

Looks good!
Attachment #8536240 - Flags: review?(seth) → review+
Comment on attachment 8536240 [details] [diff] [review]
part08-1102048-v07-cleanup-image-src-animate

Pushed part 8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/260f7fb1596a
Attachment #8536240 - Flags: checkin+
Glenn, I just wanted to let you know that I haven't forgotten about this bug, but due to some major deadlines I have looming over my head, I won't be able to review any more of these patches until the FF 37 merge. Sorry!
Assignee

Comment 158

4 years ago
Better long line wrapping, removed a stray space, fixed a typo in a comment.
Attachment #8543493 - Flags: review?(seth)
Assignee

Comment 159

4 years ago