Last Comment Bug 664130 - jwatt's random small patches that need somewhere to request review
: jwatt's random small patches that need somewhere to request review
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jonathan Watt [:jwatt]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-14 07:13 PDT by Jonathan Watt [:jwatt]
Modified: 2012-05-19 06:57 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Document what GetOpaqueRegion is useful for (980 bytes, patch)
2011-06-14 07:15 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
Document what GetFrameBoundsForTransform is about (1.31 KB, patch)
2011-06-15 00:57 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
Rename IsTransformed to HasLocalTransform (9.51 KB, patch)
2011-06-15 00:58 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
Rename isComposited to isVisuallyAtomic (2.83 KB, patch)
2011-06-15 00:58 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
const-ify some SVG element class methods (18.96 KB, patch)
2011-06-16 03:07 PDT, Jonathan Watt [:jwatt]
dholbert: review+
jwatt: checkin+
Details | Diff | Splinter Review
Add a documenting comment to nsROCSSPrimitiveValue (1.28 KB, patch)
2011-06-23 05:56 PDT, Jonathan Watt [:jwatt]
bzbarsky: review+
jwatt: checkin+
Details | Diff | Splinter Review
Remove redundant 'isPositioned' check (1.02 KB, patch)
2011-06-27 01:09 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
Clarify some display list comments and code (5.90 KB, patch)
2011-06-27 01:10 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
Rename GetHittestMask to GetHitTestFlags to avoid confusion about masking (11.52 KB, patch)
2011-06-27 03:35 PDT, Jonathan Watt [:jwatt]
longsonr: review+
jwatt: checkin+
Details | Diff | Splinter Review
Clarify some display list comments and code (6.47 KB, patch)
2011-08-11 07:29 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review
Avoid creating empty nsAbsPosClipWrapper items (1.13 KB, patch)
2011-09-07 14:24 PDT, Jonathan Watt [:jwatt]
roc: review+
jwatt: checkin+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] 2011-06-14 07:13:28 PDT
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] 2011-06-14 07:15:31 PDT
Created attachment 539196 [details] [diff] [review]
Document what GetOpaqueRegion is useful for
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-14 15:22:01 PDT
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] 2011-06-15 00:57:42 PDT
Created attachment 539448 [details] [diff] [review]
Document what GetFrameBoundsForTransform is about
Comment 4 Jonathan Watt [:jwatt] 2011-06-15 00:58:09 PDT
Created attachment 539449 [details] [diff] [review]
Rename IsTransformed to HasLocalTransform
Comment 5 Jonathan Watt [:jwatt] 2011-06-15 00:58:41 PDT
Created attachment 539450 [details] [diff] [review]
Rename isComposited to isVisuallyAtomic
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-15 03:02:45 PDT
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) 2011-06-15 03:04:30 PDT
Let's leave the other two for now since they will rot mattwoodrow's 3D transforms patches.
Comment 8 Jonathan Watt [:jwatt] 2011-06-15 03:59:13 PDT
(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] 2011-06-15 04:16:51 PDT
(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] 2011-06-16 03:07:18 PDT
Created attachment 539760 [details] [diff] [review]
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] 2011-06-16 07:57:55 PDT
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] 2011-06-16 07:59:01 PDT
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] 2011-06-16 07:59:33 PDT
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] 2011-06-23 05:56:38 PDT
Created attachment 541351 [details] [diff] [review]
Add a documenting comment to nsROCSSPrimitiveValue
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-06-23 15:27:38 PDT
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] 2011-06-27 01:09:40 PDT
Created attachment 542105 [details] [diff] [review]
Remove redundant 'isPositioned' check
Comment 17 Jonathan Watt [:jwatt] 2011-06-27 01:10:59 PDT
Created attachment 542106 [details] [diff] [review]
Clarify some display list comments and code
Comment 18 Jonathan Watt [:jwatt] 2011-06-27 03:35:01 PDT
Created attachment 542118 [details] [diff] [review]
Rename GetHittestMask to GetHitTestFlags to avoid confusion about masking
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-27 16:05:07 PDT
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) 2011-06-27 16:10:17 PDT
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] 2011-07-08 06:12:48 PDT
(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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-09 20:22:07 PDT
http://hg.mozilla.org/mozilla-central/rev/f53268a84b23
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-12 11:27:50 PDT
Comment on attachment 542105 [details] [diff] [review]
Remove redundant 'isPositioned' check

Review of attachment 542105 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 25 Jonathan Watt [:jwatt] 2011-08-11 07:29:14 PDT
Created attachment 552368 [details] [diff] [review]
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) 2011-08-11 16:06:17 PDT
It hasn't completely landed yet.
Comment 27 Jonathan Watt [:jwatt] 2011-09-07 14:24:34 PDT
Created attachment 558957 [details] [diff] [review]
Avoid creating empty nsAbsPosClipWrapper items
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-07 17:08:59 PDT
Remind me why you want to rename IsTransformed to HasLocalTransform?
Comment 29 Jonathan Watt [:jwatt] 2011-09-08 00:26:09 PDT
(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) 2011-09-08 04:17:58 PDT
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] 2011-09-08 04:34:33 PDT
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) 2011-09-08 04:40:27 PDT
I'm not sure that HasLocalTransform is much better. Will it make people think there are "global" transforms?

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