Closed Bug 1272349 Opened 8 years ago Closed 7 years ago

[e10s] Unable to check select "print selection only" dialog in mac when electrolysis is forced enabled

Categories

(Core :: Printing: Setup, defect, P2)

47 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
e10s + ---
firefox57 --- fixed

People

(Reporter: elfgoh, Assigned: mantaroh)

References

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160509171155

Steps to reproduce:

Print a webpage


Actual results:

On mac, the print dialog appears but i am unable to check select "print selection only" dialog in mac when electrolysis is forced enabled


Expected results:

I am able to check select select "print selection only"
Blocks: e10s
Component: Untriaged → Printing: Setup
Product: Firefox → Core
mconley - I don't have a Mac to investigate this one.
tracking-e10s: --- → ?
Flags: needinfo?(mconley)
Yep, this is a for-reals bug. Will triage today.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mconley)
Priority: -- → P2
Giving this a bump since I realise it still isn't fixed when I forced turned on electrolysis today
(In reply to elfgoh from comment #3)
> Giving this a bump since I realise it still isn't fixed when I forced turned
> on electrolysis today

Am on 49 beta 2
What happens here is that nsPrintEngine::DoCommonPrint sets kEnableSelectionRB on the print options object to true:

https://dxr.mozilla.org/mozilla-central/rev/0b77ed3f26c5335503bc16e85b8c067382e7bb1e/layout/printing/nsPrintEngine.cpp#557

then nsPrintOptionsX::SerializeToPrintData overwrites that value with the value it gets from for the key NSPrintSelectionOnly in the dict in NSPrintInfo.

https://dxr.mozilla.org/mozilla-central/rev/0b77ed3f26c5335503bc16e85b8c067382e7bb1e/widget/cocoa/nsPrintOptionsX.mm#147

The only other place in our code where we use the NSPrintSelectionOnly key is when we deserialize that data. So it appears we never set that value. What's the right fix here? Make setting kEnableSelectionRB affect the NSPrintSelectionOnly key for the OS X specific print settings? Or not touching the value of kEnableSelectionRB in nsPrintOptionsX::SerializeToPrintData so that the nsPrintEngine setting gets passed through?
Blocks: 1091112
Flags: needinfo?(mconley)
I'm afraid I've (conveniently!) swapped a lot of this OS X printing stuff out of my head. I could swap it in again, but before doing that, I'm going to redirect to haik to see if he has a better sense of this.

If not, feel free to toss back my way, and I'll try to sort it.
Flags: needinfo?(mconley) → needinfo?(haftandilian)
(In reply to Timothy Nikkel (:tnikkel) from comment #6)
> The only other place in our code where we use the NSPrintSelectionOnly key
> is when we deserialize that data. So it appears we never set that value.
> What's the right fix here? Make setting kEnableSelectionRB affect the
> NSPrintSelectionOnly key for the OS X specific print settings? Or not
> touching the value of kEnableSelectionRB in
> nsPrintOptionsX::SerializeToPrintData so that the nsPrintEngine setting gets
> passed through?

I'm not well versed in printing code, but here's my take.

I think we should change nsPrintOptionsX::SerializeToPrintData to use the value from the aSettings argument. We should not use the NSPrintSelectionOnly key and I don't see a reason to set it.

It doesn't affect this bug, but one thing to note is that the handling of print settings has some bugs in e10s. Mainly, the content process always fails to load a valid nsPrintInfo from preferences. https://bugzilla.mozilla.org/show_bug.cgi?id=1303051#c28

What I hope to do with bug 1328975 is to remove all the nsPrintInfo usage from content because, for sandboxing purposes, we don't want content calling into OS X print/print-settings API's. Having SerializeToPrintData not use nsPrintInfo is preferable from that perspective.
Flags: needinfo?(haftandilian)
Hi tnikkel,

Are you working on this bug?
If no, I'd like to working on this. May I take this bug?

As mentioned you on comment 6, Gecko override this option when serializing the print option data.[1]
This option which distinguish print selection area only was already serialize in 'nsPrintOptions::SerializeToPrintData' (i.e. Parent class).

[1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/widget/cocoa/nsPrintOptionsX.mm#150
[2] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/widget/cocoa/nsPrintOptionsX.mm#29

If we remove this overriding code from cocoa code, I confirmed that resolving this phenomenon:
 https://hg.mozilla.org/try/rev/f4f528a404654484dbe39a679189e4f6d992161e
Flags: needinfo?(tnikkel)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #9)
> Are you working on this bug?
> If no, I'd like to working on this. May I take this bug?

I'm not working on it. So please take it!
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #9)
> > Are you working on this bug?
> > If no, I'd like to working on this. May I take this bug?
> 
> I'm not working on it. So please take it!

Thanks.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Comment on attachment 8903407 [details]
Bug 1272349 - Use already set value of kEnableSelectionRB bit instead of NSPrintSelectionOnly.

https://reviewboard.mozilla.org/r/175254/#review181404

::: commit-message-d10c9:3
(Diff revision 1)
> +nsPrintOptionsX::SerializeToPrintData set NSPrintSelectionOnly value to option
> +bitfiled before serializing option bitfield as kEnableSelectionRB bit.
> +This bit represent that whether enable to the field of
> +'print the selected range only'.
> +
> +This bit is set by PrintEngine already, So we don't need to overwrite
> +this value when serializing.

Thanks for fixing this!

The commit message is bit difficult to understand. Please reword it to be more clear. Here's an example.

"The kEnableSelectionRB flag is already set in PrintEngine() code and the generic  nsPrintOptions::SerializeToPrintData() serializes the flags so we don't need to check NSPrintSelectionOnly and we don't need to override the kEnableSelectionRB setting in nsPrintOptionsX::SerializeToPrintData()."

I tested the fix and found that, in some cases, the printed text has the top cut off. That's a separate problem. I'll file a bug on it (unless that's something you want to do.)
Attachment #8903407 - Flags: review?(haftandilian) → review+
We should file a new bug to address how the top of the text is cut off.
Comment on attachment 8903407 [details]
Bug 1272349 - Use already set value of kEnableSelectionRB bit instead of NSPrintSelectionOnly.

https://reviewboard.mozilla.org/r/175254/#review181404

Thanks for the review!

I addressed the comments.
(In reply to Haik Aftandilian [:haik] from comment #14)
> Created attachment 8904647 [details]
> Screenshot showing text with top cut off using the fix and "Print Selection
> Only"
> 
> We should file a new bug to address how the top of the text is cut off.

Thanks.

I think that it's same as bug 1253639.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9a9286001f8a
Use already set value of kEnableSelectionRB bit instead of NSPrintSelectionOnly. r=haik
https://hg.mozilla.org/mozilla-central/rev/9a9286001f8a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → 1253639
You need to log in before you can comment on or make changes to this bug.