Open GoogleCodeExporter opened 9 years ago
Tried applying the patches to 2171 but had this build error:
FAILED: ninja -t msvc -e environment.x86 -- "C:\Program Files (x86)\Microsoft Vi
sual Studio 12.0\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\cef\lib
cef\browser\libcef_static.browser_host_impl.obj.rsp /c ..\..\cef\libcef\browser\
browser_host_impl.cc /Foobj\cef\libcef\browser\libcef_static.browser_host_impl.o
bj /Fdobj\cef\libcef_static.cc.pdb
f:\cef-stuff\download\chromium\src\cef\libcef\browser\browser_host_impl.cc(478)
: error C2259: 'CefBrowserHostImpl' : cannot instantiate abstract class
due to following members:
'void CefBrowserHost::PrintToPDF(const CefString &,const CefPdfPrintSett
ings &,CefRefPtr<CefPdfPrintCallback>)' : is abstract
f:\cef-stuff\download\chromium\src\cef\include\cef_browser.h(404) : see
declaration of 'CefBrowserHost::PrintToPDF'
f:\cef-stuff\download\chromium\src\cef\libcef\browser\browser_host_impl.cc(835)
: error C2509: 'PrintToPDF' : member function not declared in 'CefBrowserHostImp
l'
f:\cef-stuff\download\chromium\src\cef\libcef\browser\browser_host_impl.
h(93) : see declaration of 'CefBrowserHostImpl'
f:\cef-stuff\download\chromium\src\cef\libcef\browser\browser_host_impl.cc(835)
Original comment by alexandr...@gmail.com
on 9 Jan 2015 at 9:07
The patch here was made for CEF trunk, not 2171 branch. You can use original
patch from http://www.magpcss.org/ceforum/viewtopic.php?f=8&t=12403
It was for 2171 branch
Original comment by alexe...@gmail.com
on 9 Jan 2015 at 11:10
My bad. Gonna try the other one. Does the 2171 patch implement the cefclient
test or just the interface? In the trunk version there are two separate
patches. Thanks for publishing this, I was looking for a way to tune some
printing options *and* export to pdf and your patch seems to do both.
Original comment by alexandr...@gmail.com
on 10 Jan 2015 at 11:37
I found a a compiler error in
0002-Run-PDF-printing-test-from-CefClient-menu.patch
I created new patch for 2171 and fixed the trunk patch
Original comment by alexe...@gmail.com
on 10 Jan 2015 at 3:22
Attachments:
@#4: Attaching the patches for trunk as separate files to make reviewing easier.
Original comment by magreenb...@gmail.com
on 12 Jan 2015 at 11:12
Attachments:
@#4: Thanks for the patch, some comments:
1. Generate patch files with `git diff --no-prefix` and don't include the
auto-generated translation files.
2. Wrap all lines to 80 characters (in browser_host_impl.cc, etc).
3. Line 131:
+ ///
+ // Method that will be executed. |path| is the output path, |ok| is true if
+ // printing was successfull otherwise it is false.
+ ///
+ /*--cef()--*/
+ virtual void OnPdfPrintFinished(const CefString& path, bool ok) =0;
Typo, should be "successful".
4. Line 144:
///
+ // Print the current browser contents.
+ ///
+ /*--cef()--*/
+ virtual void PrintToPDF(const CefString& path,
+ const CefPdfPrintSettings& settings,
+ CefRefPtr<CefPdfPrintCallback> callback) = 0;
Be more specific in your documentation. For example, "Print the current browser
contents to the PDF file specified by |path| and execute |callback| on
completion. The caller is responsible for deleting |path| when done."
5. Line 167:
+typedef struct _cef_pdf_print_settings_t {
Add comments for the members of this structure.
+ cef_string_t header_footer_title;
+ cef_string_t header_footer_url;
Where will these strings be placed on the printed page? Is there a way to
choose the location (header vs footer)?
+ int page_width;
+ int page_height;
+
+ double margin_top;
+ double margin_right;
+ double margin_bottom;
+ double margin_left;
What is the unit of measurement for these values?
+ int margin_type;
What does this member mean? What values are allowed?
+ int header_footer_enabled;
+ int selection_only;
+ int landscape;
+ int should_print_backgrounds;
Are these boolean values?
6. Line 312:
+void CefBrowserHostImpl::PrintToPdfImpl(const CefString& path,
It shouldn't be necessary to have a separate PrintToPdfImpl method since
CefPdfPrintSettings should be passable to base::Bind.
7. Line 393:
+struct PdfPrintSettings {
Why not just use the CefPdfPrintSettings type instead of defining another
equivalent type? Then you also don't need the toPdfPrintSettings function.
8. Line 440:
+void FillInDictionaryFromPdfPrintSettings(const PdfPrintSettings& pdfSettings,
int requestId, base::DictionaryValue& printSettings)
Where do the values populated in this function come from? Do they need to be
kept synchronized with values in some other source code file? Please answer
these questions in source code comments.
9. Line 609:
+void PrintViewManagerBase::PrintToPdfFinished(bool ok) {
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
+ VLOG(1) << "PrintToPDF: finished, ok = " << ok;
+
+ pdf_print_settings_.reset();
+ pdf_print_callback_.Run(ok);
+ pdf_print_callback_.Reset();
+}
|pdf_print_callback_| should probably be reset before Run() is called (on a
temporary copy of |pdf_print_callback_|). Otherwise, if the client calls
PrintToPDF again from inside the callback, bad things might happen. You should
also probably clear |pdf_output_path_|.
Original comment by magreenb...@gmail.com
on 12 Jan 2015 at 11:51
@#4: I notice your 0002-Run-PDF-printing-test-from-CefClient-menu.patch file
only adds tests for Windows. Have you tested the functionality on any other
platform?
Original comment by magreenb...@gmail.com
on 12 Jan 2015 at 11:53
I agree with your comments and will make corresponding changes in my code.
> Have you tested the functionality on any other platform?
No. Unfortunately I have no experience in writing GUI for OS X or Linux.
Original comment by alexe...@gmail.com
on 13 Jan 2015 at 9:16
New patches without defects described in comment #6
Original comment by alexe...@gmail.com
on 17 Jan 2015 at 5:46
Attachments:
[deleted comment]
The latest patches are against trunk, right?
Original comment by alexandr...@gmail.com
on 26 Jan 2015 at 8:41
> The latest patches are against trunk, right?
Yes
Original comment by alexe...@gmail.com
on 27 Jan 2015 at 9:10
CEF is transitioning from Google Code to Bitbucket project hosting. If you
would like to continue receiving notifications on this issue please add
yourself as a Watcher at the new location:
https://bitbucket.org/chromiumembedded/cef/issue/1478
Original comment by magreenb...@gmail.com
on 14 Mar 2015 at 3:36
Original issue reported on code.google.com by
alexe...@gmail.com
on 18 Dec 2014 at 11:57Attachments: