Closed
Bug 588664
Opened 14 years ago
Closed 14 years ago
Speed up nsChildView::GetDPI
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta5+ |
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → beta5+
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".
Assignee | ||
Comment 4•14 years ago
|
||
(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.
Assignee | ||
Comment 5•14 years ago
|
||
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+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment 7•14 years ago
|
||
(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.
Assignee | ||
Comment 8•14 years ago
|
||
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.
Description
•