Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2518993002:
Make printing work for PDF and Flash. (Closed)
DescriptionMake printing work for PDF and Flash.
BUG=666432
Committed: https://crrev.com/4665756d5cdece234b38a2c722f2489a5c53a647
Cr-Commit-Position: refs/heads/master@{#433657}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 22 (10 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: This issue passed the CQ dry run.
On 2016/11/21 12:14:58, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. This looks good. I was not sure if there were other types of |rfh| we want to address so I had only limited it to the PDF extensions. I will try to patch this in my chrome branded build and see if it also fixes the docs/ bug.
ekaramad@chromium.org changed reviewers: + ekaramad@chromium.org
This LGTM esp. given that it also fixes the sheets/ bug. I build chrome with is_chrome_branded and without this patch we still have the bug. I guess it would be great if we could test issues like this. I can work on a test in a follow-up patch to this if you think it is worth it. https://codereview.chromium.org/2518993002/diff/1/chrome/browser/printing/pri... File chrome/browser/printing/print_view_manager.cc (right): https://codereview.chromium.org/2518993002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/print_view_manager.cc:132: print_preview_rfh_ = rfh; I was not sure if we are accepting all types of frames here so I had narrowed it down to only PDF ones. But having it like this I suppose fixes all supported prints which initiate from the renderer. I was wondering if there could be cases that we should not support here (no idea just an uneducated guess).
On 2016/11/21 16:39:15, EhsanK wrote: > I guess it would be great if we could test issues like this. I can work on a > test in a follow-up patch to this if you think it is worth it. I have one test written for a different case: https://codereview.chromium.org/2496203003/ so I suppose we can go in that direction to write more full browser_tests test cases. Just add more TestDelegate methods as needed.
On 2016/11/21 20:03:56, Lei Zhang (Mostly OOO) wrote: > On 2016/11/21 16:39:15, EhsanK wrote: > > I guess it would be great if we could test issues like this. I can work on a > > test in a follow-up patch to this if you think it is worth it. > > I have one test written for a different case: > https://codereview.chromium.org/2496203003/ so I suppose we can go in that > direction to write more full browser_tests test cases. Just add more > TestDelegate methods as needed. Sounds good! Thanks!
https://codereview.chromium.org/2518993002/diff/1/chrome/browser/printing/pri... File chrome/browser/printing/print_view_manager.cc (right): https://codereview.chromium.org/2518993002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/print_view_manager.cc:132: print_preview_rfh_ = rfh; On 2016/11/21 16:39:15, EhsanK wrote: > I was not sure if we are accepting all types of frames here so I had narrowed it > down to only PDF ones. But having it like this I suppose fixes all supported > prints which initiate from the renderer. > > I was wondering if there could be cases that we should not support here (no idea > just an uneducated guess). AFAIK, there's only PDF and Flash. Both of which I have tested with. (I used --ppapi-flash-path=) I don't know what else can trigger this code.
On 2016/11/21 20:07:55, Lei Zhang (Mostly OOO) wrote: > https://codereview.chromium.org/2518993002/diff/1/chrome/browser/printing/pri... > File chrome/browser/printing/print_view_manager.cc (right): > > https://codereview.chromium.org/2518993002/diff/1/chrome/browser/printing/pri... > chrome/browser/printing/print_view_manager.cc:132: print_preview_rfh_ = rfh; > On 2016/11/21 16:39:15, EhsanK wrote: > > I was not sure if we are accepting all types of frames here so I had narrowed > it > > down to only PDF ones. But having it like this I suppose fixes all supported > > prints which initiate from the renderer. > > > > I was wondering if there could be cases that we should not support here (no > idea > > just an uneducated guess). > > AFAIK, there's only PDF and Flash. Both of which I have tested with. (I used > --ppapi-flash-path=) I don't know what else can trigger this code. Thanks. I still don't know how sheets/ was ending up here.
Description was changed from ========== Make printing work for PDF and Flash. BUG=666432 ========== to ========== Make printing work for PDF and Flash. BUG=666432 ==========
On 2016/11/21 20:10:23, EhsanK wrote: > Thanks. I still don't know how sheets/ was ending up here. [Flash/PDF plugin] -> [PPAPI layers and layers] -> ChromePDFPrintClient::Print() -> PrintNode().
On 2016/11/21 20:16:08, Lei Zhang (Mostly OOO) wrote: > On 2016/11/21 20:10:23, EhsanK wrote: > > Thanks. I still don't know how sheets/ was ending up here. > > [Flash/PDF plugin] -> [PPAPI layers and layers] -> ChromePDFPrintClient::Print() > -> PrintNode(). Yes. But I was wondering about the App how it ends up calling PDF plugin. I guess it downloads a PDF version of the doc and then tries to print that.
On 2016/11/21 20:22:29, EhsanK wrote: > On 2016/11/21 20:16:08, Lei Zhang (Mostly OOO) wrote: > > On 2016/11/21 20:10:23, EhsanK wrote: > > > Thanks. I still don't know how sheets/ was ending up here. > > > > [Flash/PDF plugin] -> [PPAPI layers and layers] -> > ChromePDFPrintClient::Print() > > -> PrintNode(). > > Yes. But I was wondering about the App how it ends up calling PDF plugin. I > guess it downloads a PDF version of the doc and then tries to print that. Yes, it generates a PDF for printing, loads it, and print it. In Firefox and Chromium, the same PDF gets downloaded instead.
The CQ bit was checked by thestig@chromium.org
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": 1, "attempt_start_ts": 1479763741139150, "parent_rev":
"94987593fcf1dfb11e39dbc679ab6ed9551c8bcd", "commit_rev":
"5a059f63806443f2722dc713ec8da3afe0b3e924"}
Message was sent while issue was closed.
Description was changed from ========== Make printing work for PDF and Flash. BUG=666432 ========== to ========== Make printing work for PDF and Flash. BUG=666432 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Make printing work for PDF and Flash. BUG=666432 ========== to ========== Make printing work for PDF and Flash. BUG=666432 Committed: https://crrev.com/4665756d5cdece234b38a2c722f2489a5c53a647 Cr-Commit-Position: refs/heads/master@{#433657} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4665756d5cdece234b38a2c722f2489a5c53a647 Cr-Commit-Position: refs/heads/master@{#433657} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
