Closed
Bug 1091112
Opened 10 years ago
Closed 9 years ago
Print dialog doesn't get focus automatically, if e10s is enabled
Categories
(Core :: Printing: Setup, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: alex, Assigned: mconley)
References
Details
Attachments
(4 files, 1 obsolete file)
Steps to reproduce:
Open the Print dialog with "File" => "Print..." or the "Print" button in the Australis menu. Repeat it one time with and one time without e10s enabled.
Expected result:
The Print dialog should automatically get focused in both cases.
Actual result:
With e10s enabled the focus remains on the main window.
For this test I used a MacBook Pro (Retina, 15', Mid 2014) with OS X Yosemite / 10.10.
Assignee | ||
Updated•10 years ago
|
tracking-e10s:
--- → ?
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mconley
Updated•10 years ago
|
Updated•10 years ago
|
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
Somehow, I think we should re-evaluate this bugs presence in the M6 list. I think this bug is annoying, but not a major uplift blocker.
Updated•10 years ago
|
Comment 2•10 years ago
|
||
I am unable to reproduce it with Firefox Developer edition 40.0a2 (2015-05-17).
Maybe it got fixed?
Comment 3•10 years ago
|
||
I used Windows 8.1 to test it though.
Reporter | ||
Comment 4•10 years ago
|
||
No, it's not fixed yet. I'm still able to reproduce it in the current (2015-05-18) Nightly and Aurora version on my Mac.
I've tested it in the current Windows Nightly also. Here you're right, the Windows version does not face this problem. But this bug is meant for OS X only ;).
Comment 5•10 years ago
|
||
(In reply to Alexander Ploner from comment #4)
> No, it's not fixed yet. I'm still able to reproduce it in the current
> (2015-05-18) Nightly and Aurora version on my Mac.
>
> I've tested it in the current Windows Nightly also. Here you're right, the
> Windows version does not face this problem. But this bug is meant for OS X
> only ;).
Thanks. And sorry for the confusion.
Assignee | ||
Comment 6•10 years ago
|
||
/r/9131 - Bug 1091112 - Serialize nsIPrintSettings options bitfield. r=?
/r/9133 - Bug 1091112 - Add OS X-specific members to PrintData IPDL struct. r=?
/r/9135 - Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r=?
Pull down these commits:
hg pull -r a81dfd2f1cb78cccfa24e1a29b3b7f068d6bc0f3 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Updated•10 years ago
|
Attachment #8608237 -
Flags: review?(jmathies)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8608237 [details]
MozReview Request: bz://1091112/mconley
/r/9131 - Bug 1091112 - Serialize nsIPrintSettings options bitfield. r=?
/r/9133 - Bug 1091112 - Add OS X-specific members to PrintData IPDL struct. r=?
/r/9135 - Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r=?
Pull down these commits:
hg pull -r 4f2bdcb119be56ba66dae80ed56f9eaef6074af9 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8608237 [details]
MozReview Request: bz://1091112/mconley
/r/9131 - Bug 1091112 - Serialize nsIPrintSettings options bitfield. r=?
/r/9133 - Bug 1091112 - Add OS X-specific members to PrintData IPDL struct. r=?
/r/9135 - Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r=?
Pull down these commits:
hg pull -r d89467d608c36fcb8ac36bc72c107a3ab9f11803 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8608237 [details]
MozReview Request: bz://1091112/mconley
/r/9131 - Bug 1091112 - Serialize nsIPrintSettings options bitfield. r=?
/r/9133 - Bug 1091112 - Add OS X-specific members to PrintData IPDL struct. r=?
/r/9135 - Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r=?
Pull down these commits:
hg pull -r 7582664c8a8b2ffcf4998d3cb65efdd973907762 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8608237 [details]
MozReview Request: bz://1091112/mconley
/r/9131 - Bug 1091112 - Serialize nsIPrintSettings options bitfield. r=?
/r/9133 - Bug 1091112 - Add OS X-specific members to PrintData IPDL struct. r=?
/r/9135 - Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r=?
Pull down these commits:
hg pull -r 7582664c8a8b2ffcf4998d3cb65efdd973907762 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8608237 -
Flags: review?(mstange)
Assignee | ||
Comment 11•10 years ago
|
||
https://reviewboard.mozilla.org/r/9129/#review7771
Note to Markus - I've asked you for review on this last patch, and it has a TON of TODOs in it, mostly for questions that I'm hoping you can answer. I'm particularly interested in knowing when I need to worry about deallocating things, because it appears as if sometimes Objective-C++ or Cocoa or some of these built-in classes seem to do their own reference counting or something. Is that correct?
Comment 12•10 years ago
|
||
https://reviewboard.mozilla.org/r/9135/#review8045
::: embedding/components/printingui/ipc/PrintDataUtils.cpp:143
(Diff revision 4)
> + char16_t** array = (char16_t**) moz_xmalloc(sizeof(char16_t*));
Oh god, is this how you return arrays through xpidl interfaces? I see that nsPrintEngine::EnumerateDocumentNames is doing the same, so this should be fine...
::: widget/cocoa/nsPrintOptionsX.mm:67
(Diff revision 4)
> + // TODO: Should we fail here if the printer name is not set?
> + // Is that even a condition we can be in?
I don't know the answer to these questions. I think what you're doing here is fine - use the information if it's there, keep our strings empty if it's not there. There's not really much else we could do here, is there?
::: widget/cocoa/nsPrintOptionsX.mm:107
(Diff revision 4)
> + // TODO: What if objectForKey returns nullptr?
In that case, the result of intValue / boolValue / etc will all be zero.
You can rely on this. See the note under "working with nil" here: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/WorkingwithObjects/WorkingwithObjects.html#//apple_ref/doc/uid/TP40011210-CH4-SW22
::: widget/cocoa/nsPrintOptionsX.mm:158
(Diff revision 4)
> + // I assume that this NSString doesn't need to be de-allocated, and that
> + // once the pointer goes out of scope the memory is freed. Is that a safe
> + // assumption?
The general rule of Objective C memory management is: If you didn't allocate it, you don't need to release it. See https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html
The way this is implemented for this particular case is interesting to look at. The "once the pointer goes out of scope" part of your comment is wrong - the pointer here really is just a raw pointer, nothing happens when it goes out of scope.
nsCocoaUtils::ToNSString calls [NSString string] or [NSString stringWithCharacters:... length:...].
These functions return a string that is "autoreleased" - it has reference count zero, but is kept alive by an autorelease pool. When the autorelease pool is destroyed, this string will be deleted.
See https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSAutoreleasePool_Class/ for more information on autorelease pools. The only thing you really need to know is that you can usually assume that there is an autorelease pool in place, and it's usually managed by Cocoa, and it usually survives at least until you're done handling the current event.
If you want to hold on to an autoreleased object, you need to increase its reference count, i.e. call retain on it - the autorelease pool will only destroy objects with reference count zero. But you don't need to worry about that here.
Here you're passing your string away to -[NSMutableDictionary setObject:forKey:], so it's really up to that function to do what it deems necessary to hold on to the object that you pass it. It might even choose to copy the string instead of keeping it alive.
In other words, everything you're doing here is correct.
::: widget/cocoa/nsPrintOptionsX.mm:227
(Diff revision 4)
> + // TODO: Deallocate things here?
I don't see anything that needs deallocating.
::: widget/cocoa/nsPrintOptionsX.mm:219
(Diff revision 4)
> + // ^-- TODO: Do I need to deallocate this thing?
No, since you didn't allocate it.
::: widget/cocoa/nsPrintOptionsX.mm:210
(Diff revision 4)
> + // TODO: What if rv is a failure?
I'd just store false in that case.
::: widget/cocoa/nsPrintOptionsX.mm:224
(Diff revision 4)
> + // TODO: Do I really need to allocate a formatter to do this?
Apparently? Some googling suggests that this is the way to do it.
::: widget/cocoa/nsPrintOptionsX.mm:239
(Diff revision 4)
> + // TODO: Do I need to release the newPrintInfoDict here?
You got newPrintInfoDict by calling [NSMutableDictionary dictionaryWithDictionary:...], so no, you don't need to release it. (There is no "alloc" in that line.)
::: widget/cocoa/nsPrintOptionsX.mm:245
(Diff revision 4)
> + // its NSPrintInfo on destruction, so it's up to us to release the old one
Uh, it should really release its old NSPrintInfo in SetCocoaPrintInfo. Leaving that to the caller is very bad form, and goes against all the "don't release what you didn't allocate" mantra.
Updated•10 years ago
|
Attachment #8608237 -
Flags: review?(mstange)
Comment 13•10 years ago
|
||
https://reviewboard.mozilla.org/r/9135/#review8121
> Apparently? Some googling suggests that this is the way to do it.
Actually, let me retract that statement. I think you should send over the NSDate as a double - you get an NSTimeInterval from [printTime timeIntervalSinceReferenceDate], and can convert that into an NSDate again by calling [NSDate dateWithTimeIntervalSinceReferenceDate:yourTimeInterval], and NSTimeInterval is just a typedef for double.
Updated•9 years ago
|
Attachment #8608237 -
Flags: review?(jmathies) → review+
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Updated•9 years ago
|
Attachment #8608237 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
Bug 1091112 - Serialize nsIPrintSettings options bitfield. r=jimm
Assignee | ||
Comment 17•9 years ago
|
||
Bug 1091112 - Add OS X-specific members to PrintData IPDL struct. r=jimm
Assignee | ||
Comment 18•9 years ago
|
||
Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r=?
Attachment #8613250 -
Flags: review?(mstange)
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8613250 [details]
MozReview Request: Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r=?
Attachment #8613250 -
Flags: review?(mstange)
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8613248 [details]
MozReview Request: Bug 1091112 - Serialize nsIPrintSettings options bitfield. r=jimm
Bug 1091112 - Serialize nsIPrintSettings options bitfield. r=jimm
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8613249 [details]
MozReview Request: Bug 1091112 - Add OS X-specific members to PrintData IPDL struct. r=jimm
Bug 1091112 - Add OS X-specific members to PrintData IPDL struct. r=jimm
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8613250 [details]
MozReview Request: Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
Attachment #8613250 -
Attachment description: MozReview Request: Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r=? → MozReview Request: Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8613250 [details]
MozReview Request: Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
Attachment #8613250 -
Flags: review?(mstange)
Updated•9 years ago
|
Attachment #8613250 -
Flags: review?(mstange)
Comment 24•9 years ago
|
||
Comment on attachment 8613250 [details]
MozReview Request: Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
https://reviewboard.mozilla.org/r/9135/#review8119
::: widget/cocoa/nsPrintOptionsX.mm:146
(Diff revision 6)
> + // I assume that this NSString doesn't need to be de-allocated, and that
> + // once the pointer goes out of scope the memory is freed. Is that a safe
> + // assumption?
Left-over comment
::: widget/cocoa/nsPrintOptionsX.mm:223
(Diff revision 6)
> + settingsX->SetCocoaPrintInfo(newPrintInfo);
You allocated newPrintInfo here, so you also need to release it after you've called SetCocoaPrintInfo. It's SetCocoaPrintInfo's job to retain it if it wants to hold onto it (which it does).
::: widget/cocoa/nsPrintSettingsX.mm:94
(Diff revision 6)
> void
> nsPrintSettingsX::SetCocoaPrintInfo(NSPrintInfo* aPrintInfo)
> {
> + if (mPrintInfo) {
> + [mPrintInfo release];
> + }
> +
> mPrintInfo = aPrintInfo;
> }
Yeah ok, I hadn't looked at this function earlier, but there's more work to do here.
Setters that take Objective C objects usually look like this:
if (mPrintInfo != aPrintInfo) {
[mPrintInfo release];
mPrintInfo = [aPrintInfo retain];
}
You'll also need to change the existing caller of SetCocoaPrintInfo to release the objects they pass to it. It looks like I wrote that code, so I'm sorry for not getting it right the first time.
So
settingsX->SetCocoaPrintInfo([[[NSPrintOperation currentOperation] printInfo] copy]);
will become
NSPrintInfo* copy = [[[NSPrintOperation currentOperation] printInfo] copy]
settingsX->SetCocoaPrintInfo(copy);
[copy release];
Assignee | ||
Updated•9 years ago
|
Attachment #8613250 -
Flags: review?(mstange)
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8613250 [details]
MozReview Request: Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
Assignee | ||
Comment 26•9 years ago
|
||
https://reviewboard.mozilla.org/r/9135/#review8529
> Left-over comment
Ah, good catch!
Comment 27•9 years ago
|
||
Comment on attachment 8613250 [details]
MozReview Request: Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r?mstange
https://reviewboard.mozilla.org/r/9135/#review8531
Ship It!
Attachment #8613250 -
Flags: review?(mstange) → review+
Comment 28•9 years ago
|
||
Comment 29•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c85e82f11795
https://hg.mozilla.org/mozilla-central/rev/9b2f9883b6a8
https://hg.mozilla.org/mozilla-central/rev/7380457b8ba0
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 30•9 years ago
|
||
bugnotes |
Updated•9 years ago
|
QA Whiteboard: [good first verify][verify in Nightly only]
You need to log in
before you can comment on or make changes to this bug.
Description
•