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

RESOLVED FIXED in mozilla8

Status

()

Core
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 2 obsolete attachments)

980 bytes, patch
roc
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
1.31 KB, patch
roc
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
2.83 KB, patch
roc
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
18.96 KB, patch
dholbert
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
1.28 KB, patch
jwatt
: checkin+
Details | Diff | Splinter Review
1.02 KB, patch
roc
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
11.52 KB, patch
Robert Longson
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
6.47 KB, patch
roc
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
1.13 KB, patch
roc
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 539196 [details] [diff] [review]
Document what GetOpaqueRegion is useful for
Attachment #539196 - Flags: review?(roc)
Comment on attachment 539196 [details] [diff] [review]
Document what GetOpaqueRegion is useful for

Review of attachment 539196 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #539196 - Flags: review?(roc) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 539448 [details] [diff] [review]
Document what GetFrameBoundsForTransform is about
Attachment #539448 - Flags: review?(roc)
(Assignee)

Comment 4

6 years ago
Created attachment 539449 [details] [diff] [review]
Rename IsTransformed to HasLocalTransform
Attachment #539449 - Flags: review?(roc)
(Assignee)

Comment 5

6 years ago
Created attachment 539450 [details] [diff] [review]
Rename isComposited to isVisuallyAtomic
Attachment #539450 - Flags: review?(roc)
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.
Attachment #539448 - Flags: review?(roc) → review+
Let's leave the other two for now since they will rot mattwoodrow's 3D transforms patches.
(Assignee)

Comment 8

6 years ago
(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?
(Assignee)

Comment 9

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

Sure.
(Assignee)

Comment 10

6 years ago
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.
Attachment #539760 - Flags: review?(dholbert)
Attachment #539760 - Flags: review?(dholbert) → review+
(Assignee)

Comment 11

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

Pushed http://hg.mozilla.org/mozilla-central/rev/abde1fd28975
Attachment #539196 - Flags: checkin+
(Assignee)

Comment 12

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

Pushed http://hg.mozilla.org/mozilla-central/rev/dd1843b385b0
Attachment #539448 - Flags: checkin+
(Assignee)

Comment 13

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

Pushed http://hg.mozilla.org/mozilla-central/rev/e3efcfa1b14b
Attachment #539760 - Flags: checkin+
Version: unspecified → Trunk
(Assignee)

Comment 14

6 years ago
Created attachment 541351 [details] [diff] [review]
Add a documenting comment to nsROCSSPrimitiveValue
Attachment #541351 - Flags: review?(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.
Attachment #541351 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

6 years ago
Created attachment 542105 [details] [diff] [review]
Remove redundant 'isPositioned' check
Attachment #542105 - Flags: review?(roc)
(Assignee)

Comment 17

6 years ago
Created attachment 542106 [details] [diff] [review]
Clarify some display list comments and code
Attachment #542106 - Flags: review?(roc)
(Assignee)

Comment 18

6 years ago
Created attachment 542118 [details] [diff] [review]
Rename GetHittestMask to GetHitTestFlags to avoid confusion about masking
Attachment #542118 - Flags: review?(longsonr)

Updated

6 years ago
Attachment #542118 - Flags: review?(longsonr) → review+
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 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.
(Assignee)

Comment 21

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #542118 - Flags: checkin+
http://hg.mozilla.org/mozilla-central/rev/f53268a84b23
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 542105 [details] [diff] [review]
Remove redundant 'isPositioned' check

Review of attachment 542105 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #542105 - Flags: review?(roc) → review+
http://hg.mozilla.org/mozilla-central/rev/e703f4342489
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

6 years ago
Attachment #542105 - Flags: checkin+
(Assignee)

Comment 25

6 years ago
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?
Attachment #542106 - Attachment is obsolete: true
Attachment #542106 - Flags: review?(roc)
Attachment #552368 - Flags: review?(roc)
Attachment #552368 - Flags: review?(roc) → review+
It hasn't completely landed yet.
(Assignee)

Comment 27

6 years ago
Created attachment 558957 [details] [diff] [review]
Avoid creating empty nsAbsPosClipWrapper items
Attachment #558957 - Flags: review?(roc)
Remind me why you want to rename IsTransformed to HasLocalTransform?
Attachment #539450 - Flags: review?(roc) → review+
Attachment #558957 - Flags: review?(roc) → review+
(Assignee)

Comment 29

6 years ago
(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.
Hmm ... that hasn't bothered me before. How about HasTransform, is that better for you?

Matt, what do you think?
(Assignee)

Comment 31

6 years ago
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).
I'm not sure that HasLocalTransform is much better. Will it make people think there are "global" transforms?
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
(Assignee)

Updated

6 years ago
Attachment #539450 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #541351 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #552368 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #558957 - Flags: checkin+
(Assignee)

Updated

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

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.