Closed Bug 913603 Opened 6 years ago Closed 6 years ago

nsRect.h is a hub header

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: bjacob, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(3 files)

A hub header is one that is included by many other headers, and itself includes many other headers. See tracking bug 912735.
Attached patch Trim nsRect.hSplinter Review
Attachment #800892 - Flags: review?(jmuizelaar)
Attachment #800892 - Flags: review?(jmuizelaar) → review+
Attachment #800893 - Flags: review?(jmuizelaar) → review+
Looks like SaturatingInflate isn't used anymore, so it should probably be removed:
http://mxr.mozilla.org/mozilla-central/search?string=SaturatingInflate

SaturatingUnionEdges is used heavily on performance critical paths in layout though,
and I thought we intentionally made it inline for that reason.
(In reply to Mats Palmgren (:mats) from comment #8)
> Looks like SaturatingInflate isn't used anymore, so it should probably be
> removed:
> http://mxr.mozilla.org/mozilla-central/search?string=SaturatingInflate

Good point!

> 
> SaturatingUnionEdges is used heavily on performance critical paths in layout
> though,
> and I thought we intentionally made it inline for that reason.

According to DXR, it's not used anymore either:
http://dxr.mozilla.org/mozilla-central/search?q=SaturatingUnionEdges&redirect=false
Ah, no, I see, this is used by UnionEdges, which is indeed used outside of this file: http://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=UnionEdges&redirect=true
Attachment #805129 - Flags: review?(matspal)
Comment on attachment 805129 [details] [diff] [review]
Address Mats' comments

> +#include <algorithm>                    // for algorithm

s/for algorithm/for min, max/

and we should move back this include from .cpp to .h:
#include "nsDebug.h"                    // for NS_WARNING

and remove #include <algorithm> in .cpp
Attachment #805129 - Flags: review?(matspal) → review+
Thanks, integrated your comments, new try push https://tbpl.mozilla.org/?tree=Try&rev=242e7bf50719
Comment on attachment 805129 [details] [diff] [review]
Address Mats' comments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 913603
User impact if declined: possible performance regression
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): none (reverting a recent change)
String or IDL/UUID changes made by this patch: none
Attachment #805129 - Flags: approval-mozilla-aurora?
Attachment #805129 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone: mozilla26 → mozilla27
Target Milestone: mozilla27 → mozilla26
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.