Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2527913002:
Fix one more RenderFrame/RenderView ID mixup in printing code. (Closed)
DescriptionFix one more RenderFrame/RenderView ID mixup in printing code.
BUG=667208
Committed: https://crrev.com/26cd6767bd337d9a1bd4d71303669f4b371b0b90
Cr-Commit-Position: refs/heads/master@{#434853}
Patch Set 1 #
Total comments: 4
Patch Set 2 : nits #Patch Set 3 : put back PrintingMessageFilter::OnUpdatePrintSettings code #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
thestig@chromium.org changed reviewers: + creis@chromium.org, lfg@chromium.org, nasko@chromium.org
This will hopefully be the last ID mixup in printing code. This is a follow up to https://crrev.com/433857 The browser UI code sends the IDs to the print preview renderer, which then sends it back to non-UI printing code. They goes through quite a bit of plumbing and are ultimately used to grab a WebContents's NativeView for use with modal dialog code... except when we send the print preview to the printer, we don't even bother showing the dialog. That's why even if it was wrong before, we haven't noticed. But nevertheless, we should fix this mixup. I also dug through code history to figure out how this code got there. (I didn't write it) So I rewrote the comments too.
lgtm with nits https://codereview.chromium.org/2527913002/diff/1/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): https://codereview.chromium.org/2527913002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:261: int routing_id = -1; MSG_ROUTING_NONE https://codereview.chromium.org/2527913002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:264: printer_query = queue_->CreatePrinterQuery(host_id, routing_id); Do we still want to call this if the GetInteger above fails? If it can't fail, can you add a DCHECK?
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
If patch set 2 looks ok, feel free to put it in the CQ. https://codereview.chromium.org/2527913002/diff/1/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): https://codereview.chromium.org/2527913002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:261: int routing_id = -1; On 2016/11/24 18:09:14, lfg wrote: > MSG_ROUTING_NONE Done. https://codereview.chromium.org/2527913002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:264: printer_query = queue_->CreatePrinterQuery(host_id, routing_id); On 2016/11/24 18:09:14, lfg wrote: > Do we still want to call this if the GetInteger above fails? If it can't fail, > can you add a DCHECK? I'll just handle this case. The values are coming from the renderer.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! LGTM. Should this be associated with bug 667208? Does it need to be merged to M56?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Also, looks like some tests are failing in PS2: PrintPreviewDialogControllerBrowserTest.PrintPreviewPdfAccessibility PrintPreviewDialogControllerBrowserTest.PdfPluginDisabled
Description was changed from ========== Fix one more RenderFrame/RenderView ID mixup in printing code. ========== to ========== Fix one more RenderFrame/RenderView ID mixup in printing code. BUG=667208 ==========
On 2016/11/28 21:13:21, Charlie Reis (slow) wrote: > Also, looks like some tests are failing in PS2: > PrintPreviewDialogControllerBrowserTest.PrintPreviewPdfAccessibility > PrintPreviewDialogControllerBrowserTest.PdfPluginDisabled Ack. Will take a look in a bit. I'm not 100% sure if this is needed for M56, but I added the bug number.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> On 2016/11/24 18:09:14, lfg wrote: > https://codereview.chromium.org/2527913002/diff/1/chrome/browser/printing/pri... > chrome/browser/printing/printing_message_filter.cc:264: printer_query = > queue_->CreatePrinterQuery(host_id, routing_id); > On 2016/11/24 18:09:14, lfg wrote: > > Do we still want to call this if the GetInteger above fails? If it can't fail, > > can you add a DCHECK? > > I'll just handle this case. The values are coming from the renderer. Turns out the situation can be hit normally in print preview before this CL. In which case, the invalid values are ok since they end up not even being used. So I've mostly put back the code and will leave it as is. There's room for improvement to better document what's going on or simplify the amount of plumbing, but that will be for another day.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2527913002/#ps40001 (title: "put back PrintingMessageFilter::OnUpdatePrintSettings code")
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": 1480386865278920,
"parent_rev": "344bf615ff29efe44bd545ed6c0ff817a24b9dea", "commit_rev":
"39945cb0cae942d834b51920b3e5841e14258202"}
Message was sent while issue was closed.
Description was changed from ========== Fix one more RenderFrame/RenderView ID mixup in printing code. BUG=667208 ========== to ========== Fix one more RenderFrame/RenderView ID mixup in printing code. BUG=667208 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix one more RenderFrame/RenderView ID mixup in printing code. BUG=667208 ========== to ========== Fix one more RenderFrame/RenderView ID mixup in printing code. BUG=667208 Committed: https://crrev.com/26cd6767bd337d9a1bd4d71303669f4b371b0b90 Cr-Commit-Position: refs/heads/master@{#434853} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/26cd6767bd337d9a1bd4d71303669f4b371b0b90 Cr-Commit-Position: refs/heads/master@{#434853} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
