664130 - jwatt's random small patches that need somewhere to request review

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jonathan Watt [:jwatt]]

I'm forever finding myself wanting to make small improvements to pieces of the code that I'm reading. Given the overhead of opening and closing bugs just to get a quick review I often either roll these changes into a bigger patch that would be cleaner without them, or else don't bother to clean/clarify the code in question and just put up with the irritation. This bug is somewhere for me to attach such small patches for quick review.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: Document what GetOpaqueRegion is useful for

Comment 2

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

Comment on attachment 539196 [details] [diff] [review]
Document what GetOpaqueRegion is useful for

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

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: Document what GetFrameBoundsForTransform is about

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: Rename IsTransformed to HasLocalTransform

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: Rename isComposited to isVisuallyAtomic

Comment 6

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

Comment on attachment 539448 [details] [diff] [review]
Document what GetFrameBoundsForTransform is about

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

::: layout/base/nsDisplayList.h
@@ +2139,5 @@
>                                  const nsIFrame* aFrame,
>                                  const nsPoint &aOrigin);
>  
>    /**
> +   * Returns the bounds of a frame's context box as used to resolve

"Returns the bounds of a frame as used to resolve"

It's not the content-box or even the border-box really.

Comment 7

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

Let's leave the other two for now since they will rot mattwoodrow's 3D transforms patches.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to comment #6)
> > +   * Returns the bounds of a frame's context box as used to resolve
> 
> "Returns the bounds of a frame as used to resolve"
> 
> It's not the content-box or even the border-box really.

http://dev.w3.org/csswg/css3-2d-transforms/ says "refer to the size of the element's border box". Is that not what we do?

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to comment #7)
> Let's leave the other two for now since they will rot mattwoodrow's 3D
> transforms patches.

Sure.

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: const-ify some SVG element class methods

While working on bug 614732 I bumped into the problem of some new const code not being able to call other methods that should be const. This fixes those methods while trying not to go too far down the endless rabbit warren that can be const fixing. Stopping at an arbitrary point meant putting in four const_casts, which serve as markers for further fixup.

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 539196 [details] [diff] [review]
Document what GetOpaqueRegion is useful for

Pushed http://hg.mozilla.org/mozilla-central/rev/abde1fd28975

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 539448 [details] [diff] [review]
Document what GetFrameBoundsForTransform is about

Pushed http://hg.mozilla.org/mozilla-central/rev/dd1843b385b0

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 539760 [details] [diff] [review]
const-ify some SVG element class methods

Pushed http://hg.mozilla.org/mozilla-central/rev/e3efcfa1b14b

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: Add a documenting comment to nsROCSSPrimitiveValue

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 541351 [details] [diff] [review]
Add a documenting comment to nsROCSSPrimitiveValue

Leave the comment at the old location, so it shows up in mxr, and r=me.

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: Remove redundant 'isPositioned' check

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: Clarify some display list comments and code

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: Rename GetHittestMask to GetHitTestFlags to avoid confusion about masking

Comment 19

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

Comment on attachment 542105 [details] [diff] [review]
Remove redundant 'isPositioned' check

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

::: layout/generic/nsFrame.cpp
@@ +1800,5 @@
>        }
>      }
>      
>      if (NS_SUCCEEDED(rv)) {
> +      if (applyAbsPosClipping) {

Why is isPositioned redundant? We can reach here with isPositioned false, I think.

Comment 20

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

Comment on attachment 542106 [details] [diff] [review]
Clarify some display list comments and code

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

These transforms-related comment changes should wait for Matt's 3D transforms to be done.

Comment 21

[Jonathan Watt [:jwatt]]

(In reply to comment #19)
> Why is isPositioned redundant? We can reach here with isPositioned false, I
> think.

It's redundant because applyAbsPosClipping can only be true if nsStyleDisplay->IsAbsolutelyPositioned() returns true, in which case isPositioned *must* be true, since it's set to nsStyleDisplay->IsPositioned() which returns true if nsStyleDisplay->IsAbsolutelyPositioned() returns true.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/f53268a84b23

Comment 23

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

Comment on attachment 542105 [details] [diff] [review]
Remove redundant 'isPositioned' check

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

Comment 24

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e703f4342489

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: Clarify some display list comments and code

roc, can you take another look at the reviews here now that 3d-transforms has landed?

Comment 26

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

It hasn't completely landed yet.

Comment 27

[Jonathan Watt [:jwatt]]

Attachment: Avoid creating empty nsAbsPosClipWrapper items

Comment 28

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

Remind me why you want to rename IsTransformed to HasLocalTransform?

Comment 29

[Jonathan Watt [:jwatt]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> Remind me why you want to rename IsTransformed to HasLocalTransform?

Because the first time I was reading through code calling IsTransformed I assumed it meant "due to a transform on itself or any ancestor". The "Local" eliminates the potential for that misunderstanding.

Comment 30

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

Hmm ... that hasn't bothered me before. How about HasTransform, is that better for you?

Matt, what do you think?

Comment 31

[Jonathan Watt [:jwatt]]

I don't think IsTransformed is so confusing if you're familiar with the method and encounter with the code regularly enough to remember it. It's just one of those "make the code easier for new people, or people that don't work with it often" type of tweaks.

HasTransform would certainly be an improvement, but it seemed like in SVG code calls would look too much like a check for just the existence a 'transform' attribute. In fact even on HTML it could be misunderstood to be a check for the existence of just CSS transform. The "Local" was to hint that this might refer to more than just transform attributes or properties, since it will also be checking for implicit transforms such as from viewBox (anything that will create an nsDisplayTransform, basically).

Comment 32

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

I'm not sure that HasLocalTransform is much better. Will it make people think there are "global" transforms?

Comment 33

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/798802af9038
http://hg.mozilla.org/mozilla-central/rev/0126d20e120f
http://hg.mozilla.org/mozilla-central/rev/24ba516de8f3
http://hg.mozilla.org/mozilla-central/rev/e3d8ac2e6385

Comment 34

[Pulsebot]

Pushed by ffxbld:
https://hg.mozilla.org/releases/mozilla-release/rev/b16289a3fcbd
const-ify some SVG element class methods. r=dholbert.
https://hg.mozilla.org/releases/mozilla-release/rev/dbf773d916dd
Document what GetOpaqueRegion is useful for. r=roc.
https://hg.mozilla.org/releases/mozilla-release/rev/5b37ceb6897b
Document what GetFrameBoundsForTransform is about. r=roc.