Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
821703005:
Remove calls to deprecated SkPDFDevice and SkPDFDocuemnt. (Closed)
|
Created:
6 years ago by hal.canary Modified:
5 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove calls to deprecated SkPDFDevice and SkPDFDocuemnt.
This relands http://crrev.com/694213002
In the PdfMetafileSkia class, Instead of storing a
SkPDFDocument, store a vector of pages as
SkPictures. This allows access to individual at any
time. When FinishDocument() is called, use the
SkDocument API to print all pages to PDF.
In PrintWebViewHelper::RenderPageContent, skip clipping
content to content area, since Windows printing prints
content just outside of the content area, as noted in
http://crbug.com/434079 .
BUG=278148
Committed: https://crrev.com/18387e7ebb0eae6e4944e841d63ea058adab6e11
Cr-Commit-Position: refs/heads/master@{#310032}
Committed: https://crrev.com/ff21476a27b22e7819fc9874456e31aa516a9c1c
Cr-Commit-Position: refs/heads/master@{#312672}
Patch Set 1 #
Total comments: 8
Patch Set 2 : comments from reed@ #
Total comments: 2
Patch Set 3 : comments from danakj #
Total comments: 1
Patch Set 4 : rebase #Patch Set 5 : braces #
Total comments: 6
Patch Set 6 : comment style #Patch Set 7 : Fix issue 446729. #Patch Set 8 : update for new skstream semantics #
Messages
Total messages: 47 (19 generated)
halcanary@google.com changed reviewers: + reed@google.com, vitalybuka@chromium.org
please take a look. I have tested this on Windows.
https://codereview.chromium.org/821703005/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/821703005/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:536: if (content_area.origin() != canvas_area.origin()) { If you like, you can skip this test and always call translate. translate() is smart about skipping zeros. https://codereview.chromium.org/821703005/diff/1/printing/pdf_metafile_skia.cc File printing/pdf_metafile_skia.cc (right): https://codereview.chromium.org/821703005/diff/1/printing/pdf_metafile_skia.c... printing/pdf_metafile_skia.cc:53: static SkRect ToSkRect(const gfx::Rect& rect) { This exists in ui/gfx/skia_util.h, and perhaps some of the other helpers do too. https://codereview.chromium.org/821703005/diff/1/printing/pdf_metafile_skia.c... printing/pdf_metafile_skia.cc:82: skia::RefPtr<SkStreamAsset> pdf_data_; Can pdf_data_ just be SkData? https://codereview.chromium.org/821703005/diff/1/printing/pdf_metafile_skia.c... printing/pdf_metafile_skia.cc:102: SkDynamicMemoryWStream dynamic_memory; Use SkMemoryStream(src_buffer, src_buffer_size, true) instead?
https://codereview.chromium.org/821703005/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/821703005/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:536: if (content_area.origin() != canvas_area.origin()) { On 2014/12/22 20:41:24, reed1 wrote: > If you like, you can skip this test and always call translate. translate() is > smart about skipping zeros. Done. https://codereview.chromium.org/821703005/diff/1/printing/pdf_metafile_skia.cc File printing/pdf_metafile_skia.cc (right): https://codereview.chromium.org/821703005/diff/1/printing/pdf_metafile_skia.c... printing/pdf_metafile_skia.cc:53: static SkRect ToSkRect(const gfx::Rect& rect) { On 2014/12/22 20:41:25, reed1 wrote: > This exists in ui/gfx/skia_util.h, and perhaps some of the other helpers do too. Done. https://codereview.chromium.org/821703005/diff/1/printing/pdf_metafile_skia.c... printing/pdf_metafile_skia.cc:82: skia::RefPtr<SkStreamAsset> pdf_data_; On 2014/12/22 20:41:25, reed1 wrote: > Can pdf_data_ just be SkData? That would not be free like SkDynamicMemoryWStream::detachAsStream is. https://codereview.chromium.org/821703005/diff/1/printing/pdf_metafile_skia.c... printing/pdf_metafile_skia.cc:102: SkDynamicMemoryWStream dynamic_memory; On 2014/12/22 20:41:25, reed1 wrote: > Use SkMemoryStream(src_buffer, src_buffer_size, true) instead? Done.
halcanary@google.com changed reviewers: + danakj@chromium.org
adding danakj@ to take a look at changes to ui/gfx/skia_util
https://codereview.chromium.org/821703005/diff/20001/ui/gfx/skia_util.h File ui/gfx/skia_util.h (right): https://codereview.chromium.org/821703005/diff/20001/ui/gfx/skia_util.h#newco... ui/gfx/skia_util.h:36: GFX_EXPORT Size SkSizeToSize(const SkSize& size); This should be SkSizeToSizeF. Then use the size_conversions to make it into a Size. https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/si...
https://codereview.chromium.org/821703005/diff/20001/ui/gfx/skia_util.h File ui/gfx/skia_util.h (right): https://codereview.chromium.org/821703005/diff/20001/ui/gfx/skia_util.h#newco... ui/gfx/skia_util.h:36: GFX_EXPORT Size SkSizeToSize(const SkSize& size); On 2014/12/23 15:57:18, danakj wrote: > This should be SkSizeToSizeF. Then use the size_conversions to make it into a > Size. > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/si... Done.
ui/gfx LGTM https://codereview.chromium.org/821703005/diff/40001/printing/pdf_metafile_sk... File printing/pdf_metafile_skia.cc (right): https://codereview.chromium.org/821703005/diff/40001/printing/pdf_metafile_sk... printing/pdf_metafile_skia.cc:165: if (page_number < data_->pages_.size()) {}
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Ping (Vitaly)
lgtm https://codereview.chromium.org/821703005/diff/120001/printing/pdf_metafile_s... File printing/pdf_metafile_skia.cc (right): https://codereview.chromium.org/821703005/diff/120001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:54: // keeps original asset state unchanged. put comment before code and start text with capital. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio... https://codereview.chromium.org/821703005/diff/120001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:225: // keeps original asset state unchanged. same https://codereview.chromium.org/821703005/diff/120001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:278: // Should increment refcnt on page->content_. same. maybe this comment unnecessary here
The CQ bit was checked by halcanary@google.com
The CQ bit was unchecked by halcanary@google.com
https://codereview.chromium.org/821703005/diff/120001/printing/pdf_metafile_s... File printing/pdf_metafile_skia.cc (right): https://codereview.chromium.org/821703005/diff/120001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:54: // keeps original asset state unchanged. On 2015/01/05 21:38:57, Vitaly Buka wrote: > put comment before code and start text with capital. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio... Done. https://codereview.chromium.org/821703005/diff/120001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:225: // keeps original asset state unchanged. On 2015/01/05 21:38:57, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/821703005/diff/120001/printing/pdf_metafile_s... printing/pdf_metafile_skia.cc:278: // Should increment refcnt on page->content_. On 2015/01/05 21:38:58, Vitaly Buka wrote: > same. > maybe this comment unnecessary here Done.
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821703005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821703005/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/18387e7ebb0eae6e4944e841d63ea058adab6e11 Cr-Commit-Position: refs/heads/master@{#310032}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/788053006/ by halcanary@google.com. The reason for reverting is: A bisect proved that this change caused https://code.google.com/p/chromium/issues/detail?id=446729.
Message was sent while issue was closed.
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821703005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...) Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821703005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821703005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...) Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821703005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821703005/200001
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ff21476a27b22e7819fc9874456e31aa516a9c1c Cr-Commit-Position: refs/heads/master@{#312672} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
