Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890133002:
check whether the printing render frame host has been initialised before accepting a message in the… (Closed)
|
Created:
3 years, 7 months ago by Oleg Sushkov Modified:
3 years, 7 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncheck whether the printing render frame host has been initialised before accepting a message in the headless print manager
BUG=
Review-Url: https://codereview.chromium.org/2890133002
Cr-Commit-Position: refs/heads/master@{#473766}
Committed: https://chromium.googlesource.com/chromium/src/+/4df44c0cf2a2ea02bdc4f08a86d2827a5be2b487
Patch Set 1 #
Total comments: 2
Patch Set 2 : check whether the printing render frame host has been initialised before accepting a message in the… #
Total comments: 3
Patch Set 3 : check whether the printing render frame host has been initialised before accepting a message in the… #
Messages
Total messages: 21 (9 generated)
sushkov@chromium.org changed reviewers: + dvallet@chromium.org, jzfeng@chromium.org
lgtm https://codereview.chromium.org/2890133002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2890133002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:179: render_frame_host->Send(new IPC::Message()); use DLOG(ERROR), and add the message type into the error message, so that we know which IPC got wrong? Could you only send an empty reply when the IPC type is PrintHostMsg_GetDefaultPrintSettings, or PrintHostMsg_ScriptedPrint? Looks like other messages don't need a reply. https://codereview.chromium.org/2890133002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_print_manager.cc:247: } remove this block since now it is checked in OnMessageReceived.
On 2017/05/18 at 08:12:47, jzfeng wrote: > lgtm > > https://codereview.chromium.org/2890133002/diff/1/headless/lib/browser/headle... > File headless/lib/browser/headless_print_manager.cc (right): > > https://codereview.chromium.org/2890133002/diff/1/headless/lib/browser/headle... > headless/lib/browser/headless_print_manager.cc:179: render_frame_host->Send(new IPC::Message()); > use DLOG(ERROR), and add the message type into the error message, so that we know which IPC got wrong? > > Could you only send an empty reply when the IPC type is PrintHostMsg_GetDefaultPrintSettings, or PrintHostMsg_ScriptedPrint? Looks like other messages don't need a reply. > > https://codereview.chromium.org/2890133002/diff/1/headless/lib/browser/headle... > headless/lib/browser/headless_print_manager.cc:247: } > remove this block since now it is checked in OnMessageReceived. done
FYI. another example: http://publika.az/news_print.php?news_id=142674 is found. Looks like the window.print() js code make the renderer start to send unexpected IPC. https://codereview.chromium.org/2890133002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2890133002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:178: message.type() == PrintHostMsg_ScriptedPrint::ID)) { nit: check for PrintHostMsg_DidPrintPage here? Or you can leave the check in OnDidPrintPage as is.
The CQ bit was checked by sushkov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jzfeng@chromium.org Link to the patchset: https://codereview.chromium.org/2890133002/#ps20001 (title: "check whether the printing render frame host has been initialised before accepting a message in the…")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
sushkov@chromium.org changed reviewers: + skyostil@chromium.org
Nice, thanks! Mind adding a small test that calls window.print()? https://codereview.chromium.org/2890133002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_print_manager.cc (right): https://codereview.chromium.org/2890133002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:186: break; Please add a default case too. https://codereview.chromium.org/2890133002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_print_manager.cc:191: render_frame_host->Send(new IPC::Message()); Does this end up dropping the print request on the floor or what happens? I guess ideally we'd tell the embedder about it somehow? Maybe add a TODO to that effect?
On 2017/05/19 at 13:10:01, skyostil wrote: > Nice, thanks! Mind adding a small test that calls window.print()? > > https://codereview.chromium.org/2890133002/diff/20001/headless/lib/browser/he... > File headless/lib/browser/headless_print_manager.cc (right): > > https://codereview.chromium.org/2890133002/diff/20001/headless/lib/browser/he... > headless/lib/browser/headless_print_manager.cc:186: break; > Please add a default case too. > > https://codereview.chromium.org/2890133002/diff/20001/headless/lib/browser/he... > headless/lib/browser/headless_print_manager.cc:191: render_frame_host->Send(new IPC::Message()); > Does this end up dropping the print request on the floor or what happens? I guess ideally we'd tell the embedder about it somehow? Maybe add a TODO to that effect? done
lgtm, thanks!
The CQ bit was checked by sushkov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jzfeng@chromium.org Link to the patchset: https://codereview.chromium.org/2890133002/#ps40001 (title: "check whether the printing render frame host has been initialised before accepting a message in the…")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495497377819550,
"parent_rev": "267131adcedfb11f96f23c54cafb6cbfa03f6f1c", "commit_rev":
"4df44c0cf2a2ea02bdc4f08a86d2827a5be2b487"}
Message was sent while issue was closed.
Description was changed from ========== check whether the printing render frame host has been initialised before accepting a message in the headless print manager BUG= ========== to ========== check whether the printing render frame host has been initialised before accepting a message in the headless print manager BUG= Review-Url: https://codereview.chromium.org/2890133002 Cr-Commit-Position: refs/heads/master@{#473766} Committed: https://chromium.googlesource.com/chromium/src/+/4df44c0cf2a2ea02bdc4f08a86d2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4df44c0cf2a2ea02bdc4f08a86d2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
