Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
383623002:
Add a browser test that can communicate with the layout test framework and print pdfs. (Closed)
DescriptionThe browser test can print webpages to PDFs, then convert those PDFs to PNGs and send them to the layout test framework for an image diff. This is for print preview end to end testing.
BUG=388517
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284219
Patch Set 1 #Patch Set 2 : Fixed typos, added overflow detection, rewrote poorly written comments. #
Total comments: 57
Patch Set 3 : Refactored FillPng(), rewrote comments, fixed typos, rewrote overflow checking. #Patch Set 4 : Browsertest uses strings that are compatible with Windows. #Patch Set 5 : Browsertest now works on Windows and Mac. #
Total comments: 42
Patch Set 6 : Rewrote FillPng, fixed typos and comments, and now check to see if PDF file exists. #Patch Set 7 : base::FilePath now forward declared and unnecessary parameters removed for SetSelectedFileForTesting. #
Total comments: 4
Patch Set 8 : Rewrote FillPng. Fixed typos and comments. Code checks to see if the PDF file exists before attempting to read. #
Total comments: 47
Patch Set 9 : Fixed variable typo. #Patch Set 10 : Rewrote comments. Renamed variables to better reflect what they represent. Removed excess headers . Removed 'for' loop when printing png. #
Total comments: 4
Patch Set 11 : Removed assert as it was unnecessary, added comments about |tmp_dir_| and |kColorByte|. #Patch Set 12 : Fixed grammar in a comment. #Patch Set 13 : Added the keyword 'virtual' to overriden functions for combatibility with clang. #
Messages
Total messages: 18 (0 generated)
I made another issue for all the browser test code because I realized that if I ever want to check it in, it needs to be in a separate spot. I rewrote many of the comments, added signed integer overflow detection (hopefully I did it right) and I fixed a bunch of typos. I think its mostly done, save for case where the source file is a PDF causing the test to be a bit flaky.
https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:57: #include "ipc/ipc_message_macros.h" this should be in alphabetical order https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:61: #define STRING_TYPE std::string Drop this and use base::FilePath::StringType. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:63: #define STDIN_STREAM std::wcin Have you actually tried this on Windows? I don't know if there is a version of std::getline() that takes std::wcin. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:77: enum State { Now that you added |is_already_pdf|, can you document which steps are only used for printing non-pdf source? https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:102: printing::MarginType margins, I'd put the entire file starting from line 70 in namespace printing, and drop the printing:: https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:124: PrintPreviewObserver(WebContents* dialog, Browser* browser) Can you make the parameters Browser* and then WebContents* ? https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:284: base::Closure closure_; quit_closure_ https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:287: // State of the observer. The state indicates what message it is to send s/it is// https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:303: PrintPreviewSettings settings) { const ref https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:327: // Library is closed when the browser test is killed. s/is killed/ends/ https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:365: &num_pages, Weren't we checking this is not negative? https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:368: max_width = printing::ConvertPointsToPixelDouble(max_width); I would make a new variable here named |max_width_in_pixels|. Having a variable take on multiple units over its lifetime is confusing. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:372: bool autorotate = false; Shouldn't this always be true? If width < height, then |pdf_to_bitmap_func_| will do the "right thing" whether this is true or false. So why bother with this variable at all? https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:376: width = printing::ConvertPointsToPixelDouble(width); ditto for new variables. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:400: if (settings.area().height() > max_int / settings.area().width() || The two comparisons in the if statement seem redundent. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:402: max_int / (kColorChannels * settings.area().width())) { The multiplication can also overflow here. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:403: FAIL() << "IMAGE SIZE IS TOO LARGE, DECREASE DIMENSIONS OR DPI."; No need to be all caps. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:419: // Must multiply width & max_width by number of color channels because Refer to variables in comments as |foo|. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:419: // Must multiply width & max_width by number of color channels because Shouldn't this be an internal detail to FillPng() ? Why should callers to FillPng() have to recalculate their sizes? https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:450: for (int i = 0; i < height; ++i, index = i * desired_width) { The complexity of this algorithm using std::vector::insert() is really bad. Please do this in O(N) time instead of O(N^2). https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:463: printf("ActualHash: %s\n", base::MD5DigestToBase16(hash_).data()); data() -> c_str(); http://www.cplusplus.com/reference/string/string/data/ https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:520: // communicate with this browsertest. browser test https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:607: // -Setup communication with the layout test framework Put a space after - https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:622: // There is now way to determine how many tests are to be run ahead of time now -> no https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:656: STRING_TYPE test_name(cmd_arguments[0]); Assert the |cmd_arguments| size >= 1 before accessing the first element, or assert it's 2 if that's always suppose to be 2. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/383623002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/print_preview/print_preview_ui.h:159: friend class PrintPreviewPdfGeneratedBrowserTest; Maybe just add a public SetSelectedFileForTesting method and use that instead of being a friend, since that's the one thing you need to be a friend for. You probably can also drop the friend in print_preview_handler.h. I had suggested this previously, but you didn't respond...
https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:57: #include "ipc/ipc_message_macros.h" On 2014/07/11 04:15:07, Lei Zhang wrote: > this should be in alphabetical order Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:61: #define STRING_TYPE std::string On 2014/07/11 04:15:07, Lei Zhang wrote: > Drop this and use base::FilePath::StringType. Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:63: #define STDIN_STREAM std::wcin On 2014/07/11 04:15:07, Lei Zhang wrote: > Have you actually tried this on Windows? I don't know if there is a version of > std::getline() that takes std::wcin. I removed this code and just use std::cin. Then I convert the the string with UTF8ToWide to put it in the correct format. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:77: enum State { On 2014/07/11 04:15:06, Lei Zhang wrote: > Now that you added |is_already_pdf|, can you document which steps are only used > for printing non-pdf source? Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:102: printing::MarginType margins, On 2014/07/11 04:15:06, Lei Zhang wrote: > I'd put the entire file starting from line 70 in namespace printing, and drop > the printing:: Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:124: PrintPreviewObserver(WebContents* dialog, Browser* browser) On 2014/07/11 04:15:07, Lei Zhang wrote: > Can you make the parameters Browser* and then WebContents* ? Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:284: base::Closure closure_; On 2014/07/11 04:15:07, Lei Zhang wrote: > quit_closure_ Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:287: // State of the observer. The state indicates what message it is to send On 2014/07/11 04:15:07, Lei Zhang wrote: > s/it is// Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:303: PrintPreviewSettings settings) { On 2014/07/11 04:15:07, Lei Zhang wrote: > const ref Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:327: // Library is closed when the browser test is killed. On 2014/07/11 04:15:06, Lei Zhang wrote: > s/is killed/ends/ Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:365: &num_pages, On 2014/07/11 04:15:06, Lei Zhang wrote: > Weren't we checking this is not negative? Accidentally deleted that. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:365: &num_pages, On 2014/07/11 04:15:06, Lei Zhang wrote: > Weren't we checking this is not negative? Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:368: max_width = printing::ConvertPointsToPixelDouble(max_width); On 2014/07/11 04:15:07, Lei Zhang wrote: > I would make a new variable here named |max_width_in_pixels|. Having a variable > take on multiple units over its lifetime is confusing. Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:372: bool autorotate = false; On 2014/07/11 04:15:07, Lei Zhang wrote: > Shouldn't this always be true? If width < height, then |pdf_to_bitmap_func_| > will do the "right thing" whether this is true or false. So why bother with this > variable at all? Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:372: bool autorotate = false; On 2014/07/11 04:15:07, Lei Zhang wrote: > Shouldn't this always be true? If width < height, then |pdf_to_bitmap_func_| > will do the "right thing" whether this is true or false. So why bother with this > variable at all? I didn't realize that the |autorotate| parameter doesn't rotate in the case where the page is already the correct dimensions. I removed this variable and just the argument to true now. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:376: width = printing::ConvertPointsToPixelDouble(width); On 2014/07/11 04:15:07, Lei Zhang wrote: > ditto for new variables. Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:400: if (settings.area().height() > max_int / settings.area().width() || On 2014/07/11 04:15:06, Lei Zhang wrote: > The two comparisons in the if statement seem redundent. I rewrote the overflow conditional. I first check to see if the width times the amount of color channels overflows and then I check to see if the amount of color channels times the area of the rectangle overflows. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:400: if (settings.area().height() > max_int / settings.area().width() || On 2014/07/11 04:15:06, Lei Zhang wrote: > The two comparisons in the if statement seem redundent. Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:402: max_int / (kColorChannels * settings.area().width())) { On 2014/07/11 04:15:07, Lei Zhang wrote: > The multiplication can also overflow here. Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:403: FAIL() << "IMAGE SIZE IS TOO LARGE, DECREASE DIMENSIONS OR DPI."; On 2014/07/11 04:15:06, Lei Zhang wrote: > No need to be all caps. Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:419: // Must multiply width & max_width by number of color channels because On 2014/07/11 04:15:07, Lei Zhang wrote: > Shouldn't this be an internal detail to FillPng() ? Why should callers to > FillPng() have to recalculate their sizes? Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:419: // Must multiply width & max_width by number of color channels because On 2014/07/11 04:15:06, Lei Zhang wrote: > Refer to variables in comments as |foo|. Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:450: for (int i = 0; i < height; ++i, index = i * desired_width) { On 2014/07/11 04:15:07, Lei Zhang wrote: > The complexity of this algorithm using std::vector::insert() is really bad. > Please do this in O(N) time instead of O(N^2). I think I came up with a better solution. I pre-allocated a vector with enough size to fit in the large PNG. At the same time I filled every element with '255' or rather the color white. I then went through the new vector and copied over the old vector's contents into the correct spot. Then I just copy the new vector data into the old one. I think this is O(n). https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:450: for (int i = 0; i < height; ++i, index = i * desired_width) { On 2014/07/11 04:15:07, Lei Zhang wrote: > The complexity of this algorithm using std::vector::insert() is really bad. > Please do this in O(N) time instead of O(N^2). Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:463: printf("ActualHash: %s\n", base::MD5DigestToBase16(hash_).data()); On 2014/07/11 04:15:07, Lei Zhang wrote: > data() -> c_str(); > > http://www.cplusplus.com/reference/string/string/data/ Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:520: // communicate with this browsertest. On 2014/07/11 04:15:07, Lei Zhang wrote: > browser test Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:607: // -Setup communication with the layout test framework On 2014/07/11 04:15:06, Lei Zhang wrote: > Put a space after - Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:622: // There is now way to determine how many tests are to be run ahead of time On 2014/07/11 04:15:07, Lei Zhang wrote: > now -> no Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:656: STRING_TYPE test_name(cmd_arguments[0]); On 2014/07/11 04:15:06, Lei Zhang wrote: > Assert the |cmd_arguments| size >= 1 before accessing the first element, or > assert it's 2 if that's always suppose to be 2. Done. https://codereview.chromium.org/383623002/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/383623002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/print_preview/print_preview_ui.h:159: friend class PrintPreviewPdfGeneratedBrowserTest; On 2014/07/11 04:15:07, Lei Zhang wrote: > Maybe just add a public SetSelectedFileForTesting method and use that instead of > being a friend, since that's the one thing you need to be a friend for. You > probably can also drop the friend in print_preview_handler.h. > > I had suggested this previously, but you didn't respond... Done.
https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/20001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:450: for (int i = 0; i < height; ++i, index = i * desired_width) { On 2014/07/16 20:06:32, ivandavid wrote: > On 2014/07/11 04:15:07, Lei Zhang wrote: > > The complexity of this algorithm using std::vector::insert() is really bad. > > Please do this in O(N) time instead of O(N^2). > I think I came up with a better solution. I pre-allocated a vector with enough > size to fit in the large PNG. At the same time I filled every element with '255' > or rather the color white. I then went through the new vector and copied over > the old vector's contents into the correct spot. Then I just copy the new vector > data into the old one. I think this is O(n). Sure, this uses 2*M memory, but you can get that down to 1*M later. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:14: #if defined(OS_WIN) Chromium style is to put platform-specific #includes after all the general #includes. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:219: std::string GetFailedSetting() { Why not just have a big if/else block here? if (state_ == foo) return "blah"; else if (...) return "..." and avoid needing |failed_setting_| altogether? https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:281: // The web ui deallocates the memory for the handler when |ui| is destroyed Just say |ui->web_ui()| owns the message handler. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:340: // isn't can't be read for a while, even though the path exists. This solves typo: "isn't can't" https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:343: while (true) { If |pdf_file_save_path_| doesn't actually exist, wouldn't this infinite loop until the test times out? https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:351: // Initializes the pdf libfunctions. Called once when browser test starts. "the pdf libfunctions" -> "function pointers from the PDF library" https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:385: std::string data; data -> pdf_data ? https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:422: FAIL() << "The dimensions of the image are too large." << the last << should be on the next line. FAIL() << "Foo" << "Bar"; https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:456: if (current_width < desired_width) { Also assert: current_width <= desired_width. In which case, if you change this to: if (current_width == desired_width) return; // Nothing to do ... buys you one extra level of indentation. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:457: ASSERT_TRUE(bitmap); Check all the pre-conditions before doing any real work. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:468: *filled_bitmap_it = *bitmap_it; Rather than slowly copying this byte by byte, just do a memcpy() for the entire row. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:485: std::cout << "Content-Type: image/png\n"; Can you explain why you switched from print() to std::cout? https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:545: std::ifstream* in = new std::ifstream(tmp_path.value().c_str()); Who frees |in| ? https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:649: // to send data to the browser test. Calling std::cout<<"EOF\n" indicates that Calling ... indicates -> Writing "EOF\n" to std::cout indicates https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:678: #if defined(OS_POSIX) You don't need the temp variables: base::FilePath::StringType file_extension = FILE_PATH_LITERAL(".pdf"); base::FilePath::StringType cmd; #if defined(OS_WIN) cmd = base::UTF8ToUTF16(input); #elif defined(OS_POSIX) cmd = input; #endif https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:704: // 1 Needs to be cast because ASSERT_GE can't compare unsigned and signed No, write "1" and "1U". https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:10: #include "base/files/file_path.h" you can forward declare instead. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:158: // Allows for tests to set a file path to print a PDF to. This lso initiates lso? https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:161: const base::FilePath& path, int index, void* params); |index| and |param| are ultimately unused. Omit them here so the caller doesn't have to deal with it?
https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:14: #if defined(OS_WIN) On 2014/07/17 00:30:37, Lei Zhang wrote: > Chromium style is to put platform-specific #includes after all the general > #includes. Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:219: std::string GetFailedSetting() { On 2014/07/17 00:30:36, Lei Zhang wrote: > Why not just have a big if/else block here? > > if (state_ == foo) > return "blah"; > else if (...) > return "..." > > and avoid needing |failed_setting_| altogether? I already tried that, but it ended up being too large. The problem is that there are multiple 'paths' in the state machine depending on the source. If the source is an html file and |state_| is |kWaitingForFinalMessage| then the setting that failed is the margins setting, however if the source is a PDF, the failed setting is the page range. This could get more and more complicated as settings are added or more source files or something like that. |failed_setting_| is easy to use because it doesn't matter what source file is being used or what settings can/can't be sent. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:281: // The web ui deallocates the memory for the handler when |ui| is destroyed On 2014/07/17 00:30:37, Lei Zhang wrote: > Just say |ui->web_ui()| owns the message handler. Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:340: // isn't can't be read for a while, even though the path exists. This solves On 2014/07/17 00:30:37, Lei Zhang wrote: > typo: "isn't can't" Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:343: while (true) { On 2014/07/17 00:30:36, Lei Zhang wrote: > If |pdf_file_save_path_| doesn't actually exist, wouldn't this infinite loop > until the test times out? Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:351: // Initializes the pdf libfunctions. Called once when browser test starts. On 2014/07/17 00:30:36, Lei Zhang wrote: > "the pdf libfunctions" -> "function pointers from the PDF library" Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:385: std::string data; On 2014/07/17 00:30:37, Lei Zhang wrote: > data -> pdf_data ? Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:422: FAIL() << "The dimensions of the image are too large." << On 2014/07/17 00:30:37, Lei Zhang wrote: > the last << should be on the next line. > > FAIL() << "Foo" > << "Bar"; Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:456: if (current_width < desired_width) { On 2014/07/17 00:30:37, Lei Zhang wrote: > Also assert: current_width <= desired_width. In which case, if you change this > to: > > if (current_width == desired_width) > return; // Nothing to do > > ... > > buys you one extra level of indentation. Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:457: ASSERT_TRUE(bitmap); On 2014/07/17 00:30:37, Lei Zhang wrote: > Check all the pre-conditions before doing any real work. Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:485: std::cout << "Content-Type: image/png\n"; On 2014/07/17 00:30:36, Lei Zhang wrote: > Can you explain why you switched from print() to std::cout? For compatibility reasons, I had to get rid of freopen() and instead allocate a new |rdbuf| for it. To keep everything consistent, I changed everything to std::cout and std::cerr. Also, on Windows when I tried using printf, the output was not correct. It would only display 'C' rather than 'C:\foo\...', ie. the path to the temporary file and directory. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:545: std::ifstream* in = new std::ifstream(tmp_path.value().c_str()); On 2014/07/17 00:30:36, Lei Zhang wrote: > Who frees |in| ? It should not be freed. If the |in| is ever freed, the buffer passed into |std::cin| will disappear, causing everything to not work. Unlike |std::cin|, |rdbuf| for |std::ifstream| cannot be set, if it could, I would set |in|'s |rdbuf| to NULL after passing the original buffer to |std::cin|, that way it won't get deallocated when |in| goes out if scope. I also tried using std::ios::rdbuf on |in| to set |rdbuf| to NULL and it didn't work. This isn't really a problem though, since |in| is only created once and it should exist for the entire duration of the test, so it never really needs to be explicitly freed. Also, since SIGKILL ends the test, there would never be an opportunity to free it. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:649: // to send data to the browser test. Calling std::cout<<"EOF\n" indicates that On 2014/07/17 00:30:36, Lei Zhang wrote: > Calling ... indicates -> Writing "EOF\n" to std::cout indicates Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:678: #if defined(OS_POSIX) On 2014/07/17 00:30:37, Lei Zhang wrote: > You don't need the temp variables: > > base::FilePath::StringType file_extension = FILE_PATH_LITERAL(".pdf"); > base::FilePath::StringType cmd; > #if defined(OS_WIN) > cmd = base::UTF8ToUTF16(input); > #elif defined(OS_POSIX) > cmd = input; > #endif Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:704: // 1 Needs to be cast because ASSERT_GE can't compare unsigned and signed On 2014/07/17 00:30:36, Lei Zhang wrote: > No, write "1" and "1U". Cool. I didn't know you could do that. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:704: // 1 Needs to be cast because ASSERT_GE can't compare unsigned and signed On 2014/07/17 00:30:36, Lei Zhang wrote: > No, write "1" and "1U". Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:10: #include "base/files/file_path.h" On 2014/07/17 00:30:37, Lei Zhang wrote: > you can forward declare instead. Done. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:158: // Allows for tests to set a file path to print a PDF to. This lso initiates On 2014/07/17 00:30:37, Lei Zhang wrote: > lso? Oops, should be 'also.' https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:161: const base::FilePath& path, int index, void* params); On 2014/07/17 00:30:37, Lei Zhang wrote: > |index| and |param| are ultimately unused. Omit them here so the caller doesn't > have to deal with it? Done.
https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:281: // The web ui deallocates the memory for the handler when |ui| is destroyed On 2014/07/17 20:49:20, ivandavid wrote: > On 2014/07/17 00:30:37, Lei Zhang wrote: > > Just say |ui->web_ui()| owns the message handler. > > Done. Not done. And neither is anything else below that you replied "done" to. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/383623002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:62: no random whitespace changes please https://codereview.chromium.org/383623002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/383623002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:9: #include "base/files/file_path.h" I don't think you need this since SetSelectedFileForTesting() just passes its |path| without actually using it. https://codereview.chromium.org/383623002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/383623002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:22: class FilePath; nit: Alphabetical order.
Sorry about that. I checked out another branch without committing my code, causing me to lose some of the code that I wrote. I probably should've checked the diff ahead of time. https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:468: *filled_bitmap_it = *bitmap_it; On 2014/07/17 00:30:36, Lei Zhang wrote: > Rather than slowly copying this byte by byte, just do a memcpy() for the entire > row. Done. https://codereview.chromium.org/383623002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/383623002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:9: #include "base/files/file_path.h" On 2014/07/17 21:08:16, Lei Zhang wrote: > I don't think you need this since SetSelectedFileForTesting() just passes its > |path| without actually using it. Done. https://codereview.chromium.org/383623002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/383623002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:22: class FilePath; On 2014/07/17 21:08:16, Lei Zhang wrote: > nit: Alphabetical order. Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:477: std::copy( I used std::copy rather than memcpy because I was using iterators and its more standard (I think).
https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:545: std::ifstream* in = new std::ifstream(tmp_path.value().c_str()); On 2014/07/17 20:49:19, ivandavid wrote: > On 2014/07/17 00:30:36, Lei Zhang wrote: > > Who frees |in| ? > It should not be freed. If the |in| is ever freed, the buffer passed into > |std::cin| will disappear, causing everything to not work. Unlike |std::cin|, > |rdbuf| for |std::ifstream| cannot be set, if it could, I would set |in|'s > |rdbuf| to NULL after passing the original buffer to |std::cin|, that way it > won't get deallocated when |in| goes out if scope. I also tried using > std::ios::rdbuf on |in| to set |rdbuf| to NULL and it didn't work. This isn't > really a problem though, since |in| is only created once and it should exist for > the entire duration of the test, so it never really needs to be explicitly > freed. Also, since SIGKILL ends the test, there would never be an opportunity to > free it. Can you add a (less verbose) comment to the code explain this? https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:5: #include <algorithm> I would make a pass through the #includes and remove the ones you are not using. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:105: bool is_already_pdf) How about we name this |source_is_pdf| ? https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:158: WebContents* web_contents = Just drop this variable and return instead. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:220: std::string GetFailedSetting() { return a const std::string&, and make this method const. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:266: PrintPreviewObserver * const observer_; PrintPreviewObserver* https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:276: PrintPreviewUI* ui = GetUI(); I'd move this down to line 282 where it's actually being used. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:280: ASSERT_TRUE(web_contents); I'd assert this before passing it to Observe(), not the other way around. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:349: break; indentation is off https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:354: // Called once when browser test starts. The ibrary is closed when the browser typo https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:431: std::vector<uint8_t> page_bitmap_data( Does it work of you make this a std::vector<uint32_t> and drop the |kColorChannels| multiplier? https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:493: for (size_t i = 0; i < output_.size(); ++i) Does this work? std::copy(output_.begin(), output_.end(), std::ostream_iterator<unsigned char>(std::cout, "")); https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:523: ASSERT_EQ(browser()->tab_strip_model()->count(), 2); ASSERT_EQ()'s signature is ASSERT_EQ(expected, actual), so the arguments should be reversed. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:547: ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir()); Didn't you mention in person that there exists an issue with this since the test gets killed? Can you add a comment and mention it here? https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:565: void CreatePng( CreatePng(arg1, arg2, arg3); https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:567: std::string hash_data(bitmap_data.begin(), bitmap_data.end()); Do you really need to make a copy of |bitmap_data| ? https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:626: // Used to open up the libpdf.so, which contains the functions above. s/libpdf.so/pdf plugin/ since it's not libpdf.so on Mac/Windows. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:630: std::vector<unsigned char> output_; png_output_ ? https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:630: std::vector<unsigned char> output_; Use uint8_t or uint32_t, whichever we decided to go with. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:643: MANUAL_DummyTest) { Can we give the test a better name than DummyTest? https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:651: // Throughout this code, there will be printf statements. The layout test there's no more printf statements. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:684: cmd = base::UTF8ToWide(temp); s/temp/input/ https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:702: // 1 Needs to be cast because ASSERT_GE can't compare unsigned and signed This comment is no longer relevant.
https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:105: bool is_already_pdf) On 2014/07/17 23:31:05, Lei Zhang wrote: > How about we name this |source_is_pdf| ? Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:158: WebContents* web_contents = On 2014/07/17 23:31:06, Lei Zhang wrote: > Just drop this variable and return instead. Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:220: std::string GetFailedSetting() { On 2014/07/17 23:31:05, Lei Zhang wrote: > return a const std::string&, and make this method const. Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:266: PrintPreviewObserver * const observer_; On 2014/07/17 23:31:07, Lei Zhang wrote: > PrintPreviewObserver* Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:276: PrintPreviewUI* ui = GetUI(); On 2014/07/17 23:31:05, Lei Zhang wrote: > I'd move this down to line 282 where it's actually being used. Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:280: ASSERT_TRUE(web_contents); On 2014/07/17 23:31:06, Lei Zhang wrote: > I'd assert this before passing it to Observe(), not the other way around. Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:349: break; On 2014/07/17 23:31:07, Lei Zhang wrote: > indentation is off Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:354: // Called once when browser test starts. The ibrary is closed when the browser On 2014/07/17 23:31:06, Lei Zhang wrote: > typo Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:431: std::vector<uint8_t> page_bitmap_data( On 2014/07/17 23:31:06, Lei Zhang wrote: > Does it work of you make this a std::vector<uint32_t> and drop the > |kColorChannels| multiplier? I will work on this later. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:493: for (size_t i = 0; i < output_.size(); ++i) On 2014/07/17 23:31:07, Lei Zhang wrote: > Does this work? > > std::copy(output_.begin(), output_.end(), std::ostream_iterator<unsigned > char>(std::cout, "")); Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:493: for (size_t i = 0; i < output_.size(); ++i) On 2014/07/17 23:31:07, Lei Zhang wrote: > Does this work? > > std::copy(output_.begin(), output_.end(), std::ostream_iterator<unsigned > char>(std::cout, "")); Wow. This is pretty cool, I never would've though to do a copy into the buffer. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:523: ASSERT_EQ(browser()->tab_strip_model()->count(), 2); On 2014/07/17 23:31:06, Lei Zhang wrote: > ASSERT_EQ()'s signature is ASSERT_EQ(expected, actual), so the arguments should > be reversed. Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:547: ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir()); On 2014/07/17 23:31:05, Lei Zhang wrote: > Didn't you mention in person that there exists an issue with this since the test > gets killed? Can you add a comment and mention it here? I actually already fixed it. The python code cleans it up. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:565: void CreatePng( On 2014/07/17 23:31:06, Lei Zhang wrote: > CreatePng(arg1, > arg2, > arg3); Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:567: std::string hash_data(bitmap_data.begin(), bitmap_data.end()); On 2014/07/17 23:31:07, Lei Zhang wrote: > Do you really need to make a copy of |bitmap_data| ? That was a holder over. Its gone. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:567: std::string hash_data(bitmap_data.begin(), bitmap_data.end()); On 2014/07/17 23:31:07, Lei Zhang wrote: > Do you really need to make a copy of |bitmap_data| ? Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:630: std::vector<unsigned char> output_; On 2014/07/17 23:31:06, Lei Zhang wrote: > png_output_ ? Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:643: MANUAL_DummyTest) { On 2014/07/17 23:31:06, Lei Zhang wrote: > Can we give the test a better name than DummyTest? Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:643: MANUAL_DummyTest) { On 2014/07/17 23:31:06, Lei Zhang wrote: > Can we give the test a better name than DummyTest? I renamed it to LayoutTestDriver, since that's what it is. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:651: // Throughout this code, there will be printf statements. The layout test On 2014/07/17 23:31:05, Lei Zhang wrote: > there's no more printf statements. Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:684: cmd = base::UTF8ToWide(temp); On 2014/07/17 23:31:06, Lei Zhang wrote: > s/temp/input/ Done. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:702: // 1 Needs to be cast because ASSERT_GE can't compare unsigned and signed On 2014/07/17 23:31:06, Lei Zhang wrote: > This comment is no longer relevant. Done.
lgtm with just a few more small details. https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:547: ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir()); On 2014/07/18 01:59:11, ivandavid wrote: > On 2014/07/17 23:31:05, Lei Zhang wrote: > > Didn't you mention in person that there exists an issue with this since the > test > > gets killed? Can you add a comment and mention it here? > I actually already fixed it. The python code cleans it up. Cleans up |stdin_path| or |tmp_dir_.path()|, which is the parent of |stdin_path| ? https://codereview.chromium.org/383623002/diff/280002/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/280002/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:238: ASSERT_TRUE(observer_); This is a weird place to put it. I'd just omit the assert. If the test crashes due to a NULL observer, it should be obvious what happened from a stack trace. https://codereview.chromium.org/383623002/diff/280002/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:457: const uint8_t kColorByte = 255; Can you add a comment to mention this works because all the RGBA?? (or is it ARGB or??) pixel values are 0xFFFFFFFF.
https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:547: ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir()); On 2014/07/18 02:11:01, Lei Zhang wrote: > On 2014/07/18 01:59:11, ivandavid wrote: > > On 2014/07/17 23:31:05, Lei Zhang wrote: > > > Didn't you mention in person that there exists an issue with this since the > > test > > > gets killed? Can you add a comment and mention it here? > > I actually already fixed it. The python code cleans it up. > > Cleans up |stdin_path| or |tmp_dir_.path()|, which is the parent of |stdin_path| > ? |tmp_dir_.path()| is cleaned up. I'll add a comment about it at line 629. https://codereview.chromium.org/383623002/diff/280002/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/383623002/diff/280002/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:238: ASSERT_TRUE(observer_); On 2014/07/18 02:11:01, Lei Zhang wrote: > This is a weird place to put it. I'd just omit the assert. If the test crashes > due to a NULL observer, it should be obvious what happened from a stack trace. Done. https://codereview.chromium.org/383623002/diff/280002/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:457: const uint8_t kColorByte = 255; On 2014/07/18 02:11:01, Lei Zhang wrote: > Can you add a comment to mention this works because all the RGBA?? (or is it > ARGB or??) pixel values are 0xFFFFFFFF. Done.
The CQ bit was checked by ivandavid@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ivandavid@chromium.org/383623002/360001
The CQ bit was unchecked by ivandavid@chromium.org
The CQ bit was checked by ivandavid@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ivandavid@chromium.org/383623002/380001
Message was sent while issue was closed.
Change committed as 284219
Message was sent while issue was closed.
was i supposed to review anything here? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
