Print dialog doesn't get focus automatically, if e10s is enabled

RESOLVED FIXED in Firefox 41

Status

()

defect
--
minor
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: alex, Assigned: mconley)

Tracking

36 Branch
mozilla41
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(e10sm7+, firefox41 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 927188
(Assignee)

Updated

5 years ago
tracking-e10s: --- → ?
(Assignee)

Updated

5 years ago
Assignee: nobody → mconley
Flags: firefox-backlog+
(Assignee)

Comment 1

4 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.
I am unable to reproduce it with Firefox Developer edition 40.0a2 (2015-05-17).
Maybe it got fixed?
I used Windows 8.1 to test it though.
(Reporter)

Comment 4

4 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 ;).
(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

4 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

4 years ago
Attachment #8608237 - Flags: review?(jmathies)
(Assignee)

Comment 7

4 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

4 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

4 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

4 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

4 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?
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.
Attachment #8608237 - Flags: review?(mstange)
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

4 years ago
Attachment #8608237 - Flags: review?(jmathies) → review+

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

4 years ago
Attachment #8608237 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Bug 1091112 - Serialize nsIPrintSettings options bitfield. r=jimm
(Assignee)

Comment 17

4 years ago
Bug 1091112 - Add OS X-specific members to PrintData IPDL struct. r=jimm
(Assignee)

Comment 18

4 years ago
Bug 1091112 - Proxy opening the print dialog on OS X to the parent. r=?
Attachment #8613250 - Flags: review?(mstange)
(Assignee)

Comment 19

4 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

4 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

4 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

4 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

4 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)
Attachment #8613250 - Flags: review?(mstange)
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

4 years ago
Attachment #8613250 - Flags: review?(mstange)
(Assignee)

Comment 25

4 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

4 years ago
https://reviewboard.mozilla.org/r/9135/#review8529

> Left-over comment

Ah, good catch!
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+
QA Whiteboard: [good first verify][verify in Nightly only]
Depends on: 1272349
You need to log in before you can comment on or make changes to this bug.