Open Bug 1351190 Opened 3 years ago Updated 2 years ago

Label printing IPC actor

Categories

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

defect

Tracking

()

Tracking Status
firefox55 --- fixed
firefox56 --- fixed
firefox57 --- wontfix
firefox58 --- affected

People

(Reporter: fatseng, Unassigned)

References

Details

(Whiteboard: [QDL][TDC-MVP][LAYOUT])

User Story

https://wiki.mozilla.org/Quantum/DOM#IPC_Actors

Attachments

(2 files)

No description provided.
Assignee: nobody → fatseng
User Story: (updated)
User Story: (updated)
I would like to call the SetEventTargetForActor on the manager of the new printing actor.
Do you know how many printing actors need to be labeled here ?
Status: NEW → ASSIGNED
Whiteboard: [QDL][TDC-MVP][LAYOUT]
(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.
Priority: -- → P1
Attachment #8855387 - Flags: review?(btseng)
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!
Attachment #8855387 - Flags: review?(btseng) → review+
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.
Attachment #8855387 - Flags: review?(bugs)
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.
Attachment #8855387 - Flags: review?(bugs) → review+
(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.
Keywords: checkin-needed
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)
Pushed by fatseng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ec0ee339fff
Associate printing actor with SystemGroup r=bevistseng,smaug
https://hg.mozilla.org/mozilla-central/rev/3ec0ee339fff
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Flags: needinfo?(fatseng)
Today is my last day at Mozilla.
I think Astley will find someone to resolve it.
Flags: needinfo?(fatseng) → needinfo?(bmo)
CJ, could you help to revert this patch and uplift to beta afterward? Thanks.
Flags: needinfo?(bmo) → needinfo?(cku)
Assignee: fatseng → cku
Status: RESOLVED → REOPENED
Flags: needinfo?(cku)
Resolution: FIXED → ---
Attached patch reverted patchSplinter Review
inbound tree is close now.
Pushed by cku@mozilla.com:
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 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
Attachment #8915897 - Flags: approval-mozilla-beta?
Comment on attachment 8915897 [details] [diff] [review]
reverted patch

Backing out a patch that introduced a crash, beta57+
Attachment #8915897 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Target Milestone: mozilla55 → ---
Assignee: cku → nobody
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.