Closed Bug 588664 Opened 14 years ago Closed 14 years ago

Speed up nsChildView::GetDPI

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 573890 (probably) caused a 5% Tp4 regression. Profiling shows that CGDisplayScreenSize accounts for 2.2% of a Tp3 run on my machine (we only call it once per document, basically). This is probably the cause of the regression.
blocking2.0: --- → beta5+
Attached patch fix (obsolete) — Splinter Review
This fixes it.
Attachment #467285 - Flags: review?(joshmoz)
Comment on attachment 467285 [details] [diff] [review]
fix

> float
> nsChildView::GetDPI()
> {
>   NSWindow* window = [mView window];
...
>+  if ([window isKindOfClass:[BaseWindow class]]) {
>+    return [(BaseWindow*)window getDPI];
>+  }

You might want to null check "window" here. It won't crash if it is null but the result from sending messages to it might be unexpected.

>+  CGFloat heightMM = CGDisplayScreenSize(displayID).height;
>+  size_t heightPx = CGDisplayPixelsHigh(displayID);

In all of our Mac OS X code we try to prefix Mac OS X framework C calls with "::" ("::CGDisplay..."). Makes it easier to tell quickly whether or not a function belongs to us or the OS.

>+  return (heightPx / scaleFactor) / (heightMM / MM_PER_INCH_FLOAT);

Are you sure "heightMM" will never be zero here? If it is we'll crash with divide by zero. Same for "scaleFactor".
(In reply to comment #3)
> You might want to null check "window" here. It won't crash if it is null but
> the result from sending messages to it might be unexpected.

OK. (How can a view have a null window?)

> >+  CGFloat heightMM = CGDisplayScreenSize(displayID).height;
> >+  size_t heightPx = CGDisplayPixelsHigh(displayID);
> 
> In all of our Mac OS X code we try to prefix Mac OS X framework C calls with
> "::" ("::CGDisplay..."). Makes it easier to tell quickly whether or not a
> function belongs to us or the OS.

OK.

> >+  return (heightPx / scaleFactor) / (heightMM / MM_PER_INCH_FLOAT);
> 
> Are you sure "heightMM" will never be zero here? If it is we'll crash with
> divide by zero. Same for "scaleFactor".

I think it would be extremely antisocial for Apple to return zero for those values, but I'll check them anyway.
Attached patch fix v2Splinter Review
Attachment #467285 - Attachment is obsolete: true
Attachment #467314 - Flags: review?(joshmoz)
Attachment #467285 - Flags: review?(joshmoz)
> OK. (How can a view have a null window?)

When it is detached, not a subview of any window.

> > Are you sure "heightMM" will never be zero here? If it is we'll crash with
> > divide by zero. Same for "scaleFactor".
> 
> I think it would be extremely antisocial for Apple to return zero for those
> values, but I'll check them anyway.

I agree it wouldn't be likely or nice, but the possibility is there and we've seen worse.
Attachment #467314 - Flags: review?(joshmoz) → review+
Keywords: checkin-needed
Whiteboard: [needs landing]
(In reply to comment #3)
> Comment on attachment 467285 [details] [diff] [review]
> fix
> 
> > float
> > nsChildView::GetDPI()
> > {
> >   NSWindow* window = [mView window];
> ...
> >+  if ([window isKindOfClass:[BaseWindow class]]) {
> >+    return [(BaseWindow*)window getDPI];
> >+  }
> 
> You might want to null check "window" here. It won't crash if it is null but
> the result from sending messages to it might be unexpected.

isKindOfClass returns a BOOL and as such is guaranteed to return NO when called on nil.
http://hg.mozilla.org/mozilla-central/rev/5a3b5dbd2929
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.