Closed Bug 1040986 Opened 6 years ago Closed 5 years ago

nsIntRegion needs more useful APIs

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files)

ns[Int]Region is has very confusing APIs. This makes code hard to write and also hard to read it later. I'd like to add some simpler helpers that wrap the confusing APIs. This probably won't do much for writing code, but if you manage to find and use the simpler wrapper APIs, the code you write will be easier to read.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> Created attachment 8458987 [details] [diff] [review]
> Patch

Can you give some before and after examples?
Flags: needinfo?(bugmail.mozilla)
Having these functions return *this is redundant because the operation modifies the object the function is called on. For example the two lines at [1] do a redundant assignment. If this sort of code is mixed with code like [2] the reader ends up second-guessing themselves as to why some code does the assignment and other bits do not. Also note how at [2] whoever wrote the code decided to put in clarifying comments explaining what is happening - this shouldn't be necessary with self-documenting code.

As for providing single-argument versions of the functions, run this in your mozilla-central dir:

find . -name "*.cpp" | xargs grep "\.Or("

and see how many of the call sites do something of the from foo.Or(foo, bar). That's the overwhelming majority. This is a very common use-case and is simpler to read if you have fewer arguments: foo.Or(bar). Although maybe foo.OrWith(bar) might be more self-documenting, I'd be fine with that. The other problem I have with code of the form foo.Or(foo, bar) is that it makes it look like Or is a static function. There should really be no reason to pass foo to itself if the function is not static.

If you want me to go through the call sites and change them to use the new functions I added I can do that as well.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=2bcded4e3b4a#2955
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=2bcded4e3b4a#3112
Flags: needinfo?(bugmail.mozilla)
I think this change is an improvement, but I still find it counterintuitive that 'a.Or(b)' modifies 'a'.

If you mentally replace 'Or' with the operator '|', that's like having 'a | b' modify 'a'.

I think 'OrEquals' or 'OrWith' would be a better name for this function, while 'Or' would make sense for a function which returns the or'ed result without modifying either operand.
Comment on attachment 8458987 [details] [diff] [review]
Part 1 - Add some things to nsIntRegion

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

This seems reasonable to me. AndWith() etc. might be better but looks sort of ugly and I don't have a strong feeling either way.
Attachment #8458987 - Flags: review?(jmuizelaar) → review+
I'll change it to AndWith() etc. since I think that will improve the readability a little bit more.
Comment on attachment 8458987 [details] [diff] [review]
Part 1 - Add some things to nsIntRegion

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

Some more thoughts on this patch:
The api's should also probably be added nsRegion.
Contains(int x, int y) needs documentation. It should also probably call pixman_region_contains_point instead of constructing a rectangle.
Ok, will do. Let me know if you think of anything else.
Whiteboard: [leave open]
Keywords: leave-open
Whiteboard: [leave open]
Added contains to nsRegion, make it use the pixman function, document it, and add some tests.
Attachment #8460344 - Flags: review?(jmuizelaar)
Attachment #8460340 - Flags: review?(jmuizelaar) → review+
Attachment #8460344 - Flags: review?(jmuizelaar) → review+
Attachment #8458987 - Attachment description: Patch → Part 1 - Add some things to nsIntRegion
Attachment #8458987 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/24f3cfb76bf0
https://hg.mozilla.org/mozilla-central/rev/17e28bbaaac8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.