1351190 - Label printing IPC actor

Status: RESOLVED WONTFIX

(Core :: Printing: Output, defect, P3)

Comment 1

[Farmer Tseng[:fatseng](inactive)]

I would like to call the SetEventTargetForActor on the manager of the new printing actor.

Comment 2

[Astley Chen (inactive)]

Do you know how many printing actors need to be labeled here ?

Comment 3

[Farmer Tseng[:fatseng](inactive)]

(In reply to Astley Chen [:astley] (UTC+8) from comment #2)
> Do you know how many printing actors need to be labeled here ?

Just PPrinting need to be labeled because it is the manager of PPrintProgressDialog, PPrintSettingsDialog, and PRemotePrintJob.
PPrinting would be labeled by system group since it is a singleton in content process.
Unfortunately, it was initialized too early to use "SystemGroup::EventTargetFor".
I'm looking for another place to initialize it.

Comment 4

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1351190 - Associate printing actor with SystemGroup

Review commit: https://reviewboard.mozilla.org/r/127230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127230/

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8855387 [details]
Bug 1351190 - Associate printing actor with SystemGroup

https://reviewboard.mozilla.org/r/127230/#review130726

Looks good to me.
Please have printing peer to review this as well.

Thanks!

Comment 6

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8855387 [details]
Bug 1351190 - Associate printing actor with SystemGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127230/diff/1-2/

Comment 7

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8855387 [details]
Bug 1351190 - Associate printing actor with SystemGroup

Even through, I got r+ from :bevistseng.
We hope that DOM peer could help review it.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8855387 [details]
Bug 1351190 - Associate printing actor with SystemGroup

https://reviewboard.mozilla.org/r/127230/#review131472

r+, assuming you tested Bug 1189846 doesn't regress.

Comment 9

[Farmer Tseng[:fatseng](inactive)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8855387 [details]
> Bug 1351190 - Associate printing actor with SystemGroup
> 
> https://reviewboard.mozilla.org/r/127230/#review131472
> 
> r+, assuming you tested Bug 1189846 doesn't regress.

Thanks for reminding.
Bobowen already told me how to test printing via parent by PrintEdit addon.
It works fine.

Comment 10

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8855387 [details]
Bug 1351190 - Associate printing actor with SystemGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127230/diff/2-3/

Comment 11

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=114260d626a47ad5918b8f9e85f73e3220154bcd

Comment 12

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7dac8cb36065 -d abfece312d74: rebasing 389545:7dac8cb36065 "Bug 1351190 - Associate printing actor with SystemGroup r=bevistseng,smaug" (tip)
merging dom/ipc/ContentChild.cpp
warning: conflicts while merging dom/ipc/ContentChild.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 13

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8855387 [details]
Bug 1351190 - Associate printing actor with SystemGroup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127230/diff/3-4/

Comment 14

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9af6c7b438f447c122480eede7a07a5411788708

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ec0ee339fff
Associate printing actor with SystemGroup r=bevistseng,smaug

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3ec0ee339fff

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Farmer, this seems to be causing a crash in bug 1404681. Would you mind backing this out on trunk and beta? I don't think that it should have much impact since printing is a pretty rare operation. Thanks.

Comment 18

[Farmer Tseng[:fatseng](inactive)]

Today is my last day at Mozilla.
I think Astley will find someone to resolve it.

Comment 19

[Astley Chen (inactive)]

CJ, could you help to revert this patch and uplift to beta afterward? Thanks.

Comment 20

[u459114]

Attachment: reverted patch

inbound tree is close now.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7b8b5dd174
Revert the change we made in this bug, since it causes a crash in bug 1404681. r=me

Comment 22

[u459114]

Comment on attachment 8915897 [details] [diff] [review]
reverted patch

Approval Request Comment
[Feature/Bug causing the regression]:Bug 1351190
[User impact if declined]: the pathc we check in in this bug causes a crash in bug 1404681
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: this patch only
[Is the change risky?]: no
[Why is the change risky/not risky?]: recover to the original behavior
[String changes made/needed]:N/A

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged revert: https://hg.mozilla.org/mozilla-central/rev/2d7b8b5dd174

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915897 [details] [diff] [review]
reverted patch

Backing out a patch that introduced a crash, beta57+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/70802f5afa5d