Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
335583004:
Added a test that currently is able to print to pdf. (Closed)
|
Created:
6 years, 6 months ago by ivandavid Modified:
6 years, 5 months ago CC:
chromium-reviews, arv+watch_chromium.org, Dan Beam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded a test that currently is able to print to pdf. The test file
is print_preview_pdf_generated_browsertest.cc and it checks to see if
the print preview ui has been created. Then after doing so, it observes
the print preview web ui. Then it waits for the print preview settings
to be set, which is done in print_preview.js and then after the settings
have been set, print_preview.js tells test that it can proceed to save
the file to pdf.
The print_preview.js is not done very well however, it currently works
for the purposes of making a skeleton for the test. However, it will
have to be changed to make it more reliable and to make it
less hacky and obstrusive.
BUG=388517
Patch Set 1 #Patch Set 2 : Removed some unnecessary includes #Patch Set 3 : Added a directory for test files #
Total comments: 4
Patch Set 4 : Applied suggested changes #Patch Set 5 : Changed how the test saved the pdf #
Total comments: 13
Patch Set 6 : Renamed some variables #
Total comments: 4
Patch Set 7 : Added ability to set any print preview setting within a test. #
Total comments: 29
Patch Set 8 : Fixed style issues and made the testing functionality optional #
Total comments: 17
Patch Set 9 : Fixed style issues, handled the other cases for the checkboxes, got rid of chrome.send for margins #
Total comments: 7
Patch Set 10 : Refactored print_preview.js #
Total comments: 29
Patch Set 11 : Fixed bug where string was being used as input to a function rather than a bool. #Patch Set 12 : Fixed some whitespace issues and rewrote lines that were over 80 characters long. #
Total comments: 2
Patch Set 13 : Refactored onManipulateSettings_() #
Total comments: 65
Patch Set 14 : Changes properties from string to '.' form. Refactored test file. Fixed style issues. #
Total comments: 2
Patch Set 15 : Wrapped path string in FILE_PATH_LITERAL. #Patch Set 16 : Fixed 'if else' bracket style issues. #
Total comments: 66
Patch Set 17 : Refactored onManipulateSettings_() and the browsertest. Opened stdin for communication with layout test framework. #Patch Set 18 : Removed unnecessary comment. #
Total comments: 34
Patch Set 19 : Made browsertest assume layout test launched it. Fixed style and consistency issues in javascript files. #
Total comments: 14
Patch Set 20 : Removed unnecessary variables in browsertest. Fixed some style issues. #
Total comments: 1
Patch Set 21 : Layout test framework can now communicate with browsertest through stdin. #Patch Set 22 : Can now convert a pdf to a png and save it. #
Total comments: 34
Patch Set 23 : Refactored the browsertest. Fixed javascript bug where clicked was being used rather than checked. #
Total comments: 35
Patch Set 24 : Refactored print_preview.js and the browsertest. Made browsertest multiplatform compatible. #
Total comments: 105
Patch Set 25 : Refactored browser test and GetPDFPageSizeByIndex. Fixed style issues. #
Total comments: 22
Patch Set 26 : Browsertest modifies bitmap to fit in larger PNGs. #
Total comments: 11
Patch Set 27 : Removed IPC_MESSAGE_UNHANDLED. Fixed style and spacing issues. #
Total comments: 1
Patch Set 28 : Made observer_ in UIDoneLoadingMessageHandler point to a constant address. #Messages
Total messages: 56 (0 generated)
https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:204: ASSERT_TRUE(PathService::Get(base::DIR_TEST_DATA, &test_dir)); You probably want chrome::DIR_TEST_DATA and not base::DIR_TEST_DATA. https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:211: print_preview_observer_.get()->GetUI()->handler_->print_to_pdf_path_ = Maybe just call PrintPreviewHandler::FileSelected() ? https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:226: chrome::DuplicateTab(browser()); Why duplicate the tab? https://codereview.chromium.org/335583004/diff/40001/chrome/browser/resources... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/40001/chrome/browser/resources... chrome/browser/resources/print_preview/print_preview.js:1049: if (settings.offsetWidth > 10) { So instead of looping and look for this as the signal that the print preview is reading, what if you changed onInitialSettingsSet_() to do a chrome.send() to indicate it's ready. Then the C++ test can call a function in the JS file here to click() the DOM elements. Also add a chrome.send() call to onPreviewGenerationDone_() to send another message and listen for that on the C++ side?
On 2014/06/13 22:20:11, Lei Zhang wrote: > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... > File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... > chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:204: > ASSERT_TRUE(PathService::Get(base::DIR_TEST_DATA, &test_dir)); > You probably want chrome::DIR_TEST_DATA and not base::DIR_TEST_DATA. > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... > chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:211: > print_preview_observer_.get()->GetUI()->handler_->print_to_pdf_path_ = > Maybe just call PrintPreviewHandler::FileSelected() ? > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... > chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:226: > chrome::DuplicateTab(browser()); > Why duplicate the tab? > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/resources... > File chrome/browser/resources/print_preview/print_preview.js (right): > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/resources... > chrome/browser/resources/print_preview/print_preview.js:1049: if > (settings.offsetWidth > 10) { > So instead of looping and look for this as the signal that the print preview is > reading, what if you changed onInitialSettingsSet_() to do a chrome.send() to > indicate it's ready. Then the C++ test can call a function in the JS file here > to click() the DOM elements. Also add a chrome.send() call to > onPreviewGenerationDone_() to send another message and listen for that on the > C++ side? -I changed the save directory from base::DIR_TEST_DATA to chrome::DIR_TEST_DATA. Don't know why I did that initially. -Changed it to FileSelected, it works. -Got rid of the loop and took your suggestion but modified it slightly. For some reason I cannot use the onInitialSettingsSet_() to do the chrome.send() because the message is unhandled. However, I took your suggestion for onPreviewGenerationDone_() and had it fulfill the role of the former and latter suggestions. -I do the duplication for the same reason it is done in 'print_preview_dialog_controller_browsertest.cc' I want to start listening to messages right away and if I don't do this, I will be put in a state where I can't catch some messages, like the chrome.send() message from print_preview.js
On 2014/06/14 02:02:17, ivandavid wrote: > On 2014/06/13 22:20:11, Lei Zhang wrote: > > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... > > File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc > (right): > > > > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... > > chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:204: > > ASSERT_TRUE(PathService::Get(base::DIR_TEST_DATA, &test_dir)); > > You probably want chrome::DIR_TEST_DATA and not base::DIR_TEST_DATA. > > > > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... > > chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:211: > > print_preview_observer_.get()->GetUI()->handler_->print_to_pdf_path_ = > > Maybe just call PrintPreviewHandler::FileSelected() ? > > > > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/printing/... > > chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:226: > > chrome::DuplicateTab(browser()); > > Why duplicate the tab? > > > > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/resources... > > File chrome/browser/resources/print_preview/print_preview.js (right): > > > > > https://codereview.chromium.org/335583004/diff/40001/chrome/browser/resources... > > chrome/browser/resources/print_preview/print_preview.js:1049: if > > (settings.offsetWidth > 10) { > > So instead of looping and look for this as the signal that the print preview > is > > reading, what if you changed onInitialSettingsSet_() to do a chrome.send() to > > indicate it's ready. Then the C++ test can call a function in the JS file here > > to click() the DOM elements. Also add a chrome.send() call to > > onPreviewGenerationDone_() to send another message and listen for that on the > > C++ side? > > -I changed the save directory from base::DIR_TEST_DATA to chrome::DIR_TEST_DATA. > Don't know why I did that initially. > > -Changed it to FileSelected, it works. > > -Got rid of the loop and took your suggestion but modified it slightly. For some > reason I cannot use the onInitialSettingsSet_() to do > the chrome.send() because the message is unhandled. However, I took your > suggestion for onPreviewGenerationDone_() and had it fulfill > the role of the former and latter suggestions. > > -I do the duplication for the same reason it is done in > 'print_preview_dialog_controller_browsertest.cc' I want to start listening to > messages right > away and if I don't do this, I will be put in a state where I can't catch some > messages, like the chrome.send() message from print_preview.js Rather than replying at the bottom, click each of my comments in patch set 3 and hit reply. Then hit mail and your replies will have more context.
https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:88: enum state { FIRST_MESSAGE_WAIT, SECOND_MESSAGE_WAIT }; This can be private. Document what "first message wait" and "second message wait" means. https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:185: void PrintPreviewObserver::ClickStuff() { Please give this a better name. https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:213: print_preview_observer_.get()->set_quit_closure(loop.QuitClosure()); You don't need the .get(), here and below. https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:219: base::FilePath test_dir; This probably shouldn't be named |test_dir|. On line 228, the argument to FileSelcted() is a file path, not a directory path. https://codereview.chromium.org/335583004/diff/80001/chrome/browser/resources... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/print_preview.js:896: onClickStuff_: function() { Are you going to expand this function to have parameters, so its action is not hard coded to do just one thing, or are you going to have multiple functions to take different actions?
I renamed the variables and function names to better reflect their purposes and added some comments to the enum. I also removed the .get() statements. https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:88: enum state { FIRST_MESSAGE_WAIT, SECOND_MESSAGE_WAIT }; On 2014/06/16 23:12:40, Lei Zhang wrote: > This can be private. Document what "first message wait" and "second message > wait" means. I have documented what the enum represents and have renamed each enum. https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:185: void PrintPreviewObserver::ClickStuff() { On 2014/06/16 23:12:40, Lei Zhang wrote: > Please give this a better name. Renamed to 'onManipulateSettings' since it describes the action that will take place. https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:213: print_preview_observer_.get()->set_quit_closure(loop.QuitClosure()); On 2014/06/16 23:12:40, Lei Zhang wrote: > You don't need the .get(), here and below. Removed the .get()'s. https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:219: base::FilePath test_dir; On 2014/06/16 23:12:40, Lei Zhang wrote: > This probably shouldn't be named |test_dir|. On line 228, the argument to > FileSelcted() is a file path, not a directory path. Changed |test_dir| to |pdf_file_path| https://codereview.chromium.org/335583004/diff/80001/chrome/browser/resources... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/print_preview.js:896: onClickStuff_: function() { On 2014/06/16 23:12:40, Lei Zhang wrote: > Are you going to expand this function to have parameters, so its action is not > hard coded to do just one thing, or are you going to have multiple functions to > take different actions? I am going to expand this function to take arguments.
https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/80001/chrome/browser/printing/... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:219: base::FilePath test_dir; On 2014/06/17 00:38:56, ivandavid wrote: > On 2014/06/16 23:12:40, Lei Zhang wrote: > > This probably shouldn't be named |test_dir|. On line 228, the argument to > > FileSelcted() is a file path, not a directory path. > Changed |test_dir| to |pdf_file_path| How about |pdf_file_save_path| ? https://codereview.chromium.org/335583004/diff/80001/chrome/browser/resources... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/print_preview.js:896: onClickStuff_: function() { On 2014/06/17 00:38:56, ivandavid wrote: > On 2014/06/16 23:12:40, Lei Zhang wrote: > > Are you going to expand this function to have parameters, so its action is not > > hard coded to do just one thing, or are you going to have multiple functions > to > > take different actions? > I am going to expand this function to take arguments. If you can continue working on this CL and do this, that would be great. https://codereview.chromium.org/335583004/diff/100001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/100001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:63: void ManipulatePreviewSettings(); Add a comment to explain what this does. https://codereview.chromium.org/335583004/diff/100001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:88: explicit UIDoneLoadingMessageHandler(PrintPreviewObserver * observer); nit: (all over) "Foo* foo", not "Foo * foo" https://codereview.chromium.org/335583004/diff/100001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:93: enum state { WAITING_FOR_FIRST_MESSAGE, WAITING_FOR_SECOND_MESSAGE }; Let's format it this way: enum State { // Initial state, waiting for message X. WAITING_FOR_FIRST_MESSAGE, // Got message X, waiting for message Y. WAITING_FOR_SECOND_MESSAGE, }; https://codereview.chromium.org/335583004/diff/100001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:225: pdf_file_path = pdf_file_path.AppendASCII("dummy.pdf"); Actually, you don't want to litter DIR_TEST_DATA with files you created. You probably should output them in a way that's somewhat compatible with the layout test framework.
https://codereview.chromium.org/335583004/diff/80001/chrome/browser/resources... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/print_preview.js:896: onClickStuff_: function() { On 2014/06/17 00:53:26, Lei Zhang wrote: > On 2014/06/17 00:38:56, ivandavid wrote: > > On 2014/06/16 23:12:40, Lei Zhang wrote: > > > Are you going to expand this function to have parameters, so its action is > not > > > hard coded to do just one thing, or are you going to have multiple functions > > to > > > take different actions? > > I am going to expand this function to take arguments. > > If you can continue working on this CL and do this, that would be great. Done. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:124: I save all the settings in the class, because I want to fill them out at the beginning of the test, then later save. Therefore I need to save them ahead of time. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:162: state_ = static_cast<enum state>(static_cast<int>(state_) + 1); I wasn't sure if there was a cleaner way to do this, since I couldn't use the ++ operator on the enum. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:171: } I change most of the settings to show that it will work. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:312: base::FilePath pdf_file_save_path; Changed the variable to your suggestion. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:682: onManipulateSettings_: function(messageName, parameter) { 'messageName' is the property of print preview that I want to change and parameter is what I actually want to change it to.
https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:45: // Need to split declarations & definitions due to forward declaration problem You don't need to state this. It's fine to split the decl and definition. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:50: enum state { style: State, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer... https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:85: void ManipulatePreviewSettings(enum state); enum state -> State state https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:91: // For margins, 0 is default, 1 is none, 2 is minimum, and 3 is There's an enum MarginType in printing/print_job_constants.h. Use that rather than invent your own. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:93: void SetPrintPreviewSettings(std::string layout_settings = "", No default arguments please. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul... https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:232: void PrintPreviewObserver:: style: void PrintPreviewObserver::SetPrintPreviewSettings( std::string ...) { http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:233: SetPrintPreviewSettings(std::string layout_settings, non-POD types should be passed by const reference, not by value. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:238: layout_settings_.reset(new base::StringValue(layout_settings)); style: indentation https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:253: ui_->web_ui()->CallJavascriptFunction("onManipulateSettings", args); Since you are doing the same thing at the end of each one of these blocks, how about move it to the bottom? https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:682: onManipulateSettings_: function(messageName, parameter) { On 2014/06/17 21:38:18, ivandavid wrote: > 'messageName' is the property of print preview that I want to change and > parameter is what I actually want to change it to. You should update the JS doc, rather than mentioning it in a code review comment. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:697: chrome.send('UILoaded'); You may want to rename "UiLoaded" to "UiLoadedForTest". Since it's not normally needed. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:908: chrome.send('UILoaded'); Why not click the portrait button in this case? That is, go with the regular workflow and naturally send a UILoaded event, rather than having to remember to do it here again.
https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:45: // Need to split declarations & definitions due to forward declaration problem I removed the comment. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:50: enum state { Made the enums look like constants rather than macros. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:85: void ManipulatePreviewSettings(enum state); Changed everywhere. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:91: // For margins, 0 is default, 1 is none, 2 is minimum, and 3 is Change the type to printing::MarginType. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:93: void SetPrintPreviewSettings(std::string layout_settings = "", I removed them. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:233: SetPrintPreviewSettings(std::string layout_settings, I think I fixed it. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:253: ui_->web_ui()->CallJavascriptFunction("onManipulateSettings", args); Done. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:682: onManipulateSettings_: function(messageName, parameter) { Done. I think I correctly followed the @param section, however I am not sure, since |parameter| can be different types. So I listed all the possible types and the situations in which |parameter| would be of that type. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:697: chrome.send('UILoaded'); Done. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:908: chrome.send('UILoaded'); I didn't think it would work because its already selected by default. However, I implemented your change and it worked. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:49: global['onEnableManipulateSettings'] = Allows the native layer to send a message to print_preview that we are in a testing environment which needs to actually manipulate the print preview settings. That way, print preview should work with my changes in a non-testing environment because it will no longer automatically send an uncaught message (specifically the message sent by onPreviewGenerationDone). https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:685: onEnableManipulateSettings_: function() { See comment for line 49. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:697: if (global['onManipulateSettings']) { This makes it so that the chrome.send() is only used when doing this particular test.
https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:50: enum state { On 2014/06/17 22:59:16, ivandavid wrote: > Made the enums look like constants rather than macros. Oh, the previous MACRO_NAME style is actually preferred in Chromium. https://codereview.chromium.org/335583004/diff/120001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:85: void ManipulatePreviewSettings(enum state); On 2014/06/17 22:59:15, ivandavid wrote: > Changed everywhere. You didn't actually change this one. You still have "enum State" in places where you can just write "State". https://codereview.chromium.org/335583004/diff/140001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/140001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:235: const std::string& layout_settings, How about we use a bool here and rename it is_portrait or is_landscape, since the only 2 possible settings are portrait and landscape. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:236: const std::string& page_numbers, Mention this is a comma separated page range and give an example. If an empty string has a special meaning, mention that too. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:276: ui_->web_ui()->CallJavascriptFunction("onManipulateSettings", args); At least one of the if/if else cases should have modified |args|. Make sure that's the case by adding: DCHECK(!args.empty()); https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:683: * from the native layer, specifically during testing. specifically -> only? Maybe onEnableManipulateSettings can have test or testing in the name it's obvious. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:698: chrome.send('UILoadedForTest'); Although this is just 1 line, consider putting it in a function with a meaningful name and calling it everywhere, so you avoid typos in the message name. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:906: if (landscape) { So if your source page is a PDF and not HTML, then I believe there's no layout control. In that case, this will eval to false and ultimately no UILoadedForTest message will get sent. That would cause the state machine in the test to not advance. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:910: //chrome.send('UILoadedForTest'); remove https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:930: if (event.headersAndFooters == false) { There's 2x2 matrix of possibilities, where the axis are |event.headersAndFooters| and |headerFooterCheckbox|'s current value. You should handle all the cases. Same for background colors below. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:956: marginSettingsSelect.selectedIndex = index; Is this sufficient to cause the PDF to re-render? If so, you probably shouldn't need to do a chrome.send() on the next line? https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:957: chrome.send('UILoadedForTest'); nit: If you compare this to line 959, one of the two blocks is badly indented.
https://codereview.chromium.org/335583004/diff/140001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/140001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:235: const std::string& layout_settings, Using bool now. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:236: const std::string& page_numbers, Added comment. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:698: chrome.send('UILoadedForTest'); Done. Made a function in native_layer.js since that is where all the chrome.send() functions seem to be. Its called |doneManipulatingSettings()|. https://codereview.chromium.org/335583004/diff/140001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:956: marginSettingsSelect.selectedIndex = index; It wasn't. I 'fired' on event which now causes the PDF to re-render. https://codereview.chromium.org/335583004/diff/160001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/160001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:916: } else { I think this handles this situation. I will have to find a way to test this.
https://codereview.chromium.org/335583004/diff/160001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/160001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:89: // |page_numbers| is a comma separated page range. The lingering question is whether |page_numbers| can be empty or not. https://codereview.chromium.org/335583004/diff/160001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/160001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:916: } else { On 2014/06/18 00:26:00, ivandavid wrote: > I think this handles this situation. I will have to find a way to test this. The entire layout settings block here can be simplified a little: var elementName = event.isPortrait ? 'layout-settings-portrait-radio' : 'layout-settings-landscape-radio'; var element = document.getElementsByClassName(elementName)[0]; if ... https://codereview.chromium.org/335583004/diff/160001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:933: if (event.headersAndFooters == false) { The if and else cases here also have a lot of duplicate code. https://codereview.chromium.org/335583004/diff/160001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:937: if (headerFooterCheckbox.checked == true) { In general, I think in JS you can just do: if (foo) ...
https://codereview.chromium.org/335583004/diff/160001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/160001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:89: // |page_numbers| is a comma separated page range. On 2014/06/18 00:47:58, Lei Zhang wrote: > The lingering question is whether |page_numbers| can be empty or not. I think it can be. It seems to just revert back to the 'All' option. If the string is completely empty. However, if there is only whitespace, that will cause it to not work.
alekseys: can you look over the JS code? dbeam: FYI https://codereview.chromium.org/335583004/diff/160001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/160001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:89: // |page_numbers| is a comma separated page range. On 2014/06/18 02:29:37, ivandavid wrote: > On 2014/06/18 00:47:58, Lei Zhang wrote: > > The lingering question is whether |page_numbers| can be empty or not. > I think it can be. It seems to just revert back to the 'All' option. If the > string is completely empty. However, if there is only whitespace, that will > cause it to not work. You should mention this in the comment in the code, rather than in the code review.
I added a lot of comments but then the patchset got deleted... pity
Shoot, really sorry about that. i didn't mean to do that, because I didn't actually mean to upload that patch set. On Wed, Jun 18, 2014 at 2:16 PM, <dbeam@chromium.org> wrote: > I added a lot of comments but then the patchset got deleted... pity > > https://codereview.chromium.org/335583004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
JS part only: https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:722: else if (messageName == 'MARGINS') { Move all elses on the same line with } https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:724: } Why not pass a dictionary here (for example, similar to onUpdateWithPrinterCapabilities_) and relieve native_layer of this knowledge? https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:698: this.nativeLayer_.doneManipulatingSettings(); Call it this.nativeLayer_.previewReady(), call it unconditionally here and move if (global['onManipulateSettingsForTest']) check to native_layer. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:909: element.click(); What if it is already selected? https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:912: } So if element is not there, test assumes that everything is done and fine? Does not feel right to me. What about this: var element = document.querySelector(event.isPortrait ? '.layout-settings-portrait-radio' : '.layout-settings-landscape-radio'); if (element.checked) { this.nativeLayer_.previewReady(); } else { element.click(); } Same reasoning applies for other settings. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:919: pageSettingsCustomInput.value = event.pageSettings; What if it's the same, does it still trigger the preview? https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:922: this.nativeLayer_.doneManipulatingSettings(); But nothing was actually done, why do we notify here? https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:929: if ((event.headersAndFooters && checkbox.checked) || Why this condition is in brackets and other is not? Drop them here. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:930: !event.headersAndFooters && !checkbox.checked) { Why not just (event.headersAndFooters == checkbox.checked)? https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:933: this.otherOptionsSettings_.headerFooterCheckbox_.click(); You have your checkbox already, why refer to this.otherOptionsSettings_.headerFooterCheckbox_? https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:950: else if (event.messageName == 'MARGINS') { Move all elses to the same line with { https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:954: if (marginSettingsSelect && index != 0) { Zero index is still valid. Use isNaN to check for errors (but i'd say test should always pass a valid integer and it's ok to crash on the invalid one).
https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:722: else if (messageName == 'MARGINS') { On 2014/06/18 21:57:35, Aleksey Shlyapnikov wrote: > Move all elses on the same line with } Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:724: } On 2014/06/18 21:57:35, Aleksey Shlyapnikov wrote: > Why not pass a dictionary here (for example, similar to > onUpdateWithPrinterCapabilities_) and relieve native_layer of this knowledge? Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:698: this.nativeLayer_.doneManipulatingSettings(); On 2014/06/18 21:57:35, Aleksey Shlyapnikov wrote: > Call it this.nativeLayer_.previewReady(), call it unconditionally here and move > if (global['onManipulateSettingsForTest']) check to native_layer. Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:909: element.click(); On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > What if it is already selected? I rewrote it so that if it is already selected, it will just call previewReady(). I did a similar thing for the other settings where if the desired setting is already set, it will just advance without actually clicking anything, since it wouldn't be needed. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:909: element.click(); On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > What if it is already selected? Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:912: } On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > So if element is not there, test assumes that everything is done and fine? Does > not feel right to me. What about this: > > var element = document.querySelector(event.isPortrait ? > '.layout-settings-portrait-radio' : > '.layout-settings-landscape-radio'); > if (element.checked) { > this.nativeLayer_.previewReady(); > } else { > element.click(); > } > > Same reasoning applies for other settings. I rewrote this so that if the element == null, the instead of proceeding, it will actually send a message to the test notifying it that something has gone wrong. Is this a good change or is it maybe too harsh? https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:912: } On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > So if element is not there, test assumes that everything is done and fine? Does > not feel right to me. What about this: > > var element = document.querySelector(event.isPortrait ? > '.layout-settings-portrait-radio' : > '.layout-settings-landscape-radio'); > if (element.checked) { > this.nativeLayer_.previewReady(); > } else { > element.click(); > } > > Same reasoning applies for other settings. Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:919: pageSettingsCustomInput.value = event.pageSettings; On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > What if it's the same, does it still trigger the preview? Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:919: pageSettingsCustomInput.value = event.pageSettings; On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > What if it's the same, does it still trigger the preview? I am actually not sure. I rewrote this entire section since it wasn't actually working and now it works. See comment in patch set twelve. (Is there a way to link to other comments?) https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:922: this.nativeLayer_.doneManipulatingSettings(); On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > But nothing was actually done, why do we notify here? Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:929: if ((event.headersAndFooters && checkbox.checked) || On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > Why this condition is in brackets and other is not? Drop them here. Fixed it. It was a typo. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:929: if ((event.headersAndFooters && checkbox.checked) || On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > Why this condition is in brackets and other is not? Drop them here. Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:930: !event.headersAndFooters && !checkbox.checked) { On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > Why not just (event.headersAndFooters == checkbox.checked)? Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:930: !event.headersAndFooters && !checkbox.checked) { On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > Why not just (event.headersAndFooters == checkbox.checked)? Fixed in patch set 13. Same change made for the other checkbox. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:933: this.otherOptionsSettings_.headerFooterCheckbox_.click(); On 2014/06/18 21:57:35, Aleksey Shlyapnikov wrote: > You have your checkbox already, why refer to > this.otherOptionsSettings_.headerFooterCheckbox_? Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:950: else if (event.messageName == 'MARGINS') { On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > Move all elses to the same line with { Done. https://codereview.chromium.org/335583004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:954: if (marginSettingsSelect && index != 0) { On 2014/06/18 21:57:36, Aleksey Shlyapnikov wrote: > Zero index is still valid. Use isNaN to check for errors (but i'd say test > should always pass a valid integer and it's ok to crash on the invalid one). Rewrote this entire section to account for this and for the situation in which an index is not within a valid range (ie. negative or above the max index). https://codereview.chromium.org/335583004/diff/240001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/240001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:718: previewFailed: function() { Test will assert false when this happens, causing the test to fail. https://codereview.chromium.org/335583004/diff/240001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/240001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:918: } else if ('PAGE_NUMBERS' in event.settings) { Rewrote the whole section because it wasn't working when I actually tested it. Now it works correctly and can accept formatted strings.
didn't review the last js file thoroughly, work on this first https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:84: void ManipulatePreviewSettings(State state); just inline all the methods, i.e. void Method() { ... } instead of void Method(); https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:101: private: arguably doc comments for all method declarations that aren't OVERRIDE https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:186: ASSERT_TRUE(false); FAIL(); or ADD_FAILURE(); https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:217: IPC_MESSAGE_UNHANDLED(break;) IPC_MESSAGE_UNHANDLED(return false;) https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:237: IsPrintPreviewDialog(web_contents_)); split into different ASSERT_TRUE()'s, IMO https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:248: this->Observe(web_contents_); s/this->// everywhere https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:252: ui_->web_ui()->AddMessageHandler(handler); why does every other line have a \n? https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:268: //TODO: NEED TO MAKE THE DICTIONARY VALUE A "VALUE" I THINK. address or remove, also TODO(ldap) https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:304: public: 1 \s indent for {public,protected,private}: https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:305: PrintPreviewPdfGeneratedBrowserTest() {} 1 \s addl indent after {public,protected,private}: https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:309: base::FilePath directory("printing"); i think you need something along the lines of FILE_PATH_LITERAL() around this to work on windows https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:362: Print(); 2 \s indent https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:50: this.onEnableManipulateSettingsForTest_.bind(this); all of these would be better as global.property instead of global['property'] (are these quoted so they're not renamed when compiling JavaScript?) https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:84: 'print_preview.NativeLayer.MANIPULATE_SETTINGS_FOR_TEST' alphabetize list https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:683: * from the native layer. @private https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:693: * @param {Dictionary} settings Dictionary containing the value to be s/Dictionary/Object/g https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:694: * changed and that value should be set to. @private https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:709: if (global['onManipulateSettingsForTest']) { no curlies for conditionals and conditional bodies that are 1 line, e.g. // all 1 line -> no curlies if (...) ... else if (...) ... else ... or // conditional is 2 lines -> curlies if (... && ...) { ... } or // body is 2 lines -> curlies (enforced by language) if (...) { ... ... } https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:899: * be changed and what to set that option. @private
And please fix issue title. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:693: * @param {Dictionary} settings Dictionary containing the value to be !Object, since you do not handle settings to be null. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:708: previewReady: function() { previewReadyForTest https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:718: previewFailed: function() { previewFailedForTest https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:899: * be changed and what to set that option. * @param {Event} event Event object that contains the option to be * changed and what to set that option. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:902: if ('SAVE_AS_PDF' in event.settings) { ALL_CAPS property names look awkward and misleading. Can you name them in a more conventional way: selectSaveAsPdfDestination layoutSettings pageRange etc. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:903: this.destinationStore_.selectDefaultDestination_(); Since you added the failure handling logic, check that PDF destination is, in fact, selected. Some time soon 'default destination' will mean 'default printer for this computer' and won't be a PDF printer anymore. Or better yet, do not access private functions here, traverse this.destinationStore_.destinations() array, find the destination with print_preview.Destination.GooglePromotedId.SAVE_AS_PDF id and use this.destinationStore_.selectDestination() to select it. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:906: event.settings['LAYOUT_SETTINGS'] ? var element = document.querySelector(event.settings.layoutSettings ? and it begs for a event.settings.layoutSettings.portrait boolean property. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:910: if (element.clicked) { Crash here will also fail the test, right? Why add more code then? Same question for all other places checking for undefined controls. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:921: if (textbox.value == event.settings['PAGE_NUMBERS']) { event.settings.pageRange The same for all other settings. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:932: '.page-settings-custom-radio'); Fits one line. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:944: '.header-footer-checkbox'); It does fit into one line. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:956: '.css-background-checkbox'); Fits one line. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:969: document.querySelector('.margin-settings-select'); Fits one line.
https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:84: void ManipulatePreviewSettings(State state); On 2014/06/20 02:45:16, Dan Beam wrote: > just inline all the methods, i.e. > > void Method() { ... } > > instead of > > void Method(); Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:101: private: On 2014/06/20 02:45:16, Dan Beam wrote: > arguably doc comments for all method declarations that aren't OVERRIDE Not completely done. Will finish later since I think I'm going to rewrite and possible remove some of these. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:186: ASSERT_TRUE(false); On 2014/06/20 02:45:16, Dan Beam wrote: > FAIL(); or ADD_FAILURE(); Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:217: IPC_MESSAGE_UNHANDLED(break;) On 2014/06/20 02:45:16, Dan Beam wrote: > IPC_MESSAGE_UNHANDLED(return false;) Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:237: IsPrintPreviewDialog(web_contents_)); On 2014/06/20 02:45:16, Dan Beam wrote: > split into different ASSERT_TRUE()'s, IMO Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:248: this->Observe(web_contents_); On 2014/06/20 02:45:16, Dan Beam wrote: > s/this->// everywhere Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:252: ui_->web_ui()->AddMessageHandler(handler); On 2014/06/20 02:45:16, Dan Beam wrote: > why does every other line have a \n? Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:268: //TODO: NEED TO MAKE THE DICTIONARY VALUE A "VALUE" I THINK. On 2014/06/20 02:45:16, Dan Beam wrote: > address or remove, also TODO(ldap) Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:304: public: On 2014/06/20 02:45:16, Dan Beam wrote: > 1 \s indent for {public,protected,private}: Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:305: PrintPreviewPdfGeneratedBrowserTest() {} On 2014/06/20 02:45:16, Dan Beam wrote: > 1 \s addl indent after {public,protected,private}: Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:309: base::FilePath directory("printing"); On 2014/06/20 02:45:16, Dan Beam wrote: > i think you need something along the lines of FILE_PATH_LITERAL() around this to > work on windows Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:362: Print(); On 2014/06/20 02:45:16, Dan Beam wrote: > 2 \s indent Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:50: this.onEnableManipulateSettingsForTest_.bind(this); On 2014/06/20 02:45:16, Dan Beam wrote: > all of these would be better as global.property instead of global['property'] > (are these quoted so they're not renamed when compiling JavaScript?) Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:84: 'print_preview.NativeLayer.MANIPULATE_SETTINGS_FOR_TEST' On 2014/06/20 02:45:16, Dan Beam wrote: > alphabetize list Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:683: * from the native layer. On 2014/06/20 02:45:16, Dan Beam wrote: > @private Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:693: * @param {Dictionary} settings Dictionary containing the value to be On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > !Object, since you do not handle settings to be null. Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:694: * changed and that value should be set to. On 2014/06/20 02:45:17, Dan Beam wrote: > @private Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:708: previewReady: function() { On 2014/06/20 17:27:21, Aleksey Shlyapnikov wrote: > previewReadyForTest Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:709: if (global['onManipulateSettingsForTest']) { On 2014/06/20 02:45:17, Dan Beam wrote: > no curlies for conditionals and conditional bodies that are 1 line, e.g. > > // all 1 line -> no curlies > if (...) > ... > else if (...) > ... > else > ... > > or > > // conditional is 2 lines -> curlies > if (... && > ...) { > ... > } > > or > > // body is 2 lines -> curlies (enforced by language) > if (...) { > ... > ... > } Done and applied this everywhere else where it happened. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:709: if (global['onManipulateSettingsForTest']) { On 2014/06/20 02:45:17, Dan Beam wrote: > no curlies for conditionals and conditional bodies that are 1 line, e.g. > > // all 1 line -> no curlies > if (...) > ... > else if (...) > ... > else > ... > > or > > // conditional is 2 lines -> curlies > if (... && > ...) { > ... > } > > or > > // body is 2 lines -> curlies (enforced by language) > if (...) { > ... > ... > } Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:718: previewFailed: function() { On 2014/06/20 17:27:21, Aleksey Shlyapnikov wrote: > previewFailedForTest Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:899: * be changed and what to set that option. On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > * @param {Event} event Event object that contains the option to be > * changed and what to set that option. Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:899: * be changed and what to set that option. On 2014/06/20 02:45:17, Dan Beam wrote: > @private Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:902: if ('SAVE_AS_PDF' in event.settings) { On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > ALL_CAPS property names look awkward and misleading. Can you name them in a more > conventional way: > selectSaveAsPdfDestination > layoutSettings > pageRange > etc. Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:903: this.destinationStore_.selectDefaultDestination_(); On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > Since you added the failure handling logic, check that PDF destination is, in > fact, selected. Some time soon 'default destination' will mean 'default printer > for this computer' and won't be a PDF printer anymore. > > Or better yet, do not access private functions here, traverse > this.destinationStore_.destinations() array, find the destination with > print_preview.Destination.GooglePromotedId.SAVE_AS_PDF id and use > this.destinationStore_.selectDestination() to select it. Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:906: event.settings['LAYOUT_SETTINGS'] ? On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > var element = document.querySelector(event.settings.layoutSettings ? > > and it begs for a event.settings.layoutSettings.portrait boolean property. Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:910: if (element.clicked) { On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > Crash here will also fail the test, right? Why add more code then? > > Same question for all other places checking for undefined controls. I am not sure what you mean here. Is the problem that even if an element exists for a given query, that the clicked property might not actually be defined for that element, therefore I cannot assume that I can click it? https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:921: if (textbox.value == event.settings['PAGE_NUMBERS']) { On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > event.settings.pageRange > > The same for all other settings. Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:932: '.page-settings-custom-radio'); On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > Fits one line. Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:944: '.header-footer-checkbox'); On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > It does fit into one line. Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:956: '.css-background-checkbox'); On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > Fits one line. Done. https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:969: document.querySelector('.margin-settings-select'); On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > Fits one line. Done. https://codereview.chromium.org/335583004/diff/280001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/280001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:155: void OnDidGetPreviewPageCount( Needs to be separated from the definition because otherwise there is a forward declaration problem.
https://codereview.chromium.org/335583004/diff/280001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/280001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:155: void OnDidGetPreviewPageCount( On 2014/06/20 21:31:45, ivandavid wrote: > Needs to be separated from the definition because otherwise there is a forward > declaration problem. There's really no issue with separating the decl from the definition. It's just more verbose. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. It's 2014. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:63: kWaitingForFinalMessage = 6 Unlike JSON, you can put a comma after the 6, so if you need to add a new state in the future, you won't have to touch this line. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:68: //explicit PrintPreviewObserver(WebContents* dialog, Browser* browser); delete? https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:69: PrintPreviewObserver(WebContents* dialog, Browser* browser) : nit put the colon on the next line with 4 space indent https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:76: } Add a blank line after the end of a method. Ditto below. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:78: browser_ = NULL; Do we need this? https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:120: script_argument_->SetBoolean("backgroundColorsAndImages", The moment you have an if block that's multiple lines, the entire structure needs braces. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:161: Observe(new_web_contents); You'd actually only want to do this once right? That is, you should never have multiple clones in the same test. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:174: int count_; not used? https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:266: FILE_PATH_LITERAL("printing")); FILE_PATH_LITERAL is only used with Append(). AppendASCII() takes strings as is.
https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:71: SetPrintPreviewSettings(false, Rather than doing this here, you probably want to have Print() call it, so you can actually write tests that Print() with different settings. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:252: base::FilePath file(FILE_PATH_LITERAL(file_name.c_str())); FILE_PATH_LITERAL only works for literal strings. This won't compile on Windows. Pass in a base::FilePath by reference instead.
https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/260001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:910: if (element.clicked) { On 2014/06/20 21:31:45, ivandavid wrote: > On 2014/06/20 17:27:22, Aleksey Shlyapnikov wrote: > > Crash here will also fail the test, right? Why add more code then? > > > > Same question for all other places checking for undefined controls. > I am not sure what you mean here. Is the problem that even if an element exists > for a given query, that the clicked property might not actually be defined for > that element, therefore I cannot assume that I can click it? No, what I mean is the test will crash if you remove this "if (element)" check and just do "if (element.clicked)", thus removing one level of if statements from this already pretty deeply nested code. There are a few advantages: - you still get your test failure; - less code to maintain - it matches the module semantics better Here's why: your code creates an impression that there are valid conditions under which these elements might not exist (valid in terms of the module, not your test). It's not true, these elements are always there, they might not be visible (and you do not care about visibility here), but they are always accessible. So, your test should just use them and crash if someone renamed the element's class or changed the behavior is come other way. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:720: chrome.send('UIFailedLoadingForTest'); if (global.onManipulateSettingsForTest) chrome.send('UIFailedLoadingForTest'); https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:908: SAVE_AS_PDF) { if (destinations[i].id == print_preview.Destination.GooglePromotedId.SAVE_AS_PDF) { https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:915: this.nativeLayer_.previewFailedForTest(); var pdfDestination = null; ... pdfDestination = destinations[i]; break; ... if (pdfDestination) this.destinationStore_.selectDestination(pdfDestination); else this.nativeLayer_.previewFailedForTest(); is easier to understand. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:916: } else if ('layoutSettings' in event.settings) { I know that one liners do not require brackets, but don't you think reading this code with if and else from the different branches next to each other and with the same indent is confusing? I'd add brackets around one liner if here for the sake of readability anyway. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:920: '.layout-settings-landscape-radio'); 4 more spaces indent for these two lines. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:927: this.nativeLayer_.previewFailedForTest(); I think if one part of the statement has braces the other one should have them too, even if it's a one liner. Otherwise it's harder to grasp the structure of the code. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:937: // hang mysteriously if the string isn't correct. No indent is required. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:952: '.header-footer-checkbox'); Fits one line. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:975: event.settings.margins < element.length) { Fix indent.
https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:916: } else if ('layoutSettings' in event.settings) { On 2014/06/20 22:13:50, Aleksey Shlyapnikov wrote: > I know that one liners do not require brackets, but don't you think reading this > code with if and else from the different branches next to each other and with > the same indent is confusing? I'd add brackets around one liner if here for the > sake of readability anyway. an overwhelming majority of chrome/browser/resources disagrees with this https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:927: this.nativeLayer_.previewFailedForTest(); On 2014/06/20 22:13:49, Aleksey Shlyapnikov wrote: > I think if one part of the statement has braces the other one should have them > too, even if it's a one liner. Otherwise it's harder to grasp the structure of > the code. yes, should've mentioned this ^
https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:89: bool OnMessageReceived(const IPC::Message& message) OVERRIDE { fix indent https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:182: printing::MarginType margins_; please make sure all of these plain-old-data (e.g.bool, int, pointer) members are in the initializer list https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:215: } like Lei mentioned, this code would be easier to read with \n between each method https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:903: if ('selectSaveAsPdfDestination' in event.settings) { one strategy for splitting this up if ('selectSaveAsPdfDestination' in event.settings) this.selectSaveAsPdfDestination_(event); else if ('layoutSettings' in event.settings) this.clickLayoutSettings_(); else if ('pageRange' in event.settings) this.setPageRange_(event); https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:903: if ('selectSaveAsPdfDestination' in event.settings) { another strategy would be to simply make different methods for all this... is there any reason you *need* to go through onManipulateSettingsForTest_ for all this code? https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:911: break; return; https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:914: if (!didSet) ^ remove didSet https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:921: if (element) { if (!element) { this.nativeLayer_.previewFailedForTest(); return; } if (element.clicked) this.nativeLayer_.previewReadyForTest(); else element.click(); https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:931: if (textbox) { if (!textbox) { this.nativeLayer_.previewFailedForTest(); return; } ... less if indent ... https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:943: if (radio) if (!radio) { ... usual failure stuff + return ... } https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:982: } feel free to return early or break this method into smaller methods for readability (left some examples)
https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:903: if ('selectSaveAsPdfDestination' in event.settings) { On 2014/06/21 02:44:11, Dan Beam wrote: > another strategy would be to simply make different methods for all this... is > there any reason you *need* to go through onManipulateSettingsForTest_ for all > this code? I would support keeping native_layer.js involvement at minimum and do not have any logic in it. This is test code, it would be great to keep it as small and 'one piece' as possible. Removing unnecessary "!element" checks and returning early will help to improve readability of this code. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:931: if (textbox) { On 2014/06/21 02:44:11, Dan Beam wrote: > if (!textbox) { > this.nativeLayer_.previewFailedForTest(); > return; > } > > ... less if indent ... I don't see my extensive comment on the same matter in this revision, so just in case, these "!element" checks are not necessary, they can be removed altogether to save indentation and code. All the elements under test always exist, you should just query and use them.
https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/06/20 21:41:36, Lei Zhang wrote: > It's 2014. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:63: kWaitingForFinalMessage = 6 On 2014/06/20 21:41:37, Lei Zhang wrote: > Unlike JSON, you can put a comma after the 6, so if you need to add a new state > in the future, you won't have to touch this line. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:68: //explicit PrintPreviewObserver(WebContents* dialog, Browser* browser); On 2014/06/20 21:41:36, Lei Zhang wrote: > delete? Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:69: PrintPreviewObserver(WebContents* dialog, Browser* browser) : On 2014/06/20 21:41:36, Lei Zhang wrote: > nit put the colon on the next line with 4 space indent Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:71: SetPrintPreviewSettings(false, On 2014/06/20 21:57:57, Lei Zhang wrote: > Rather than doing this here, you probably want to have Print() call it, so you > can actually write tests that Print() with different settings. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:76: } On 2014/06/20 21:41:36, Lei Zhang wrote: > Add a blank line after the end of a method. Ditto below. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:78: browser_ = NULL; On 2014/06/20 21:41:36, Lei Zhang wrote: > Do we need this? Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:89: bool OnMessageReceived(const IPC::Message& message) OVERRIDE { On 2014/06/21 02:44:11, Dan Beam wrote: > fix indent Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:120: script_argument_->SetBoolean("backgroundColorsAndImages", On 2014/06/20 21:41:37, Lei Zhang wrote: > The moment you have an if block that's multiple lines, the entire structure > needs braces. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:174: int count_; On 2014/06/20 21:41:36, Lei Zhang wrote: > not used? Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:182: printing::MarginType margins_; On 2014/06/21 02:44:11, Dan Beam wrote: > please make sure all of these plain-old-data (e.g.bool, int, pointer) members > are in the initializer list Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:252: base::FilePath file(FILE_PATH_LITERAL(file_name.c_str())); On 2014/06/20 21:57:57, Lei Zhang wrote: > FILE_PATH_LITERAL only works for literal strings. This won't compile on Windows. > Pass in a base::FilePath by reference instead. Did you mean base::FilePath::StringType? Because that's what I changed file name to. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:266: FILE_PATH_LITERAL("printing")); On 2014/06/20 21:41:36, Lei Zhang wrote: > FILE_PATH_LITERAL is only used with Append(). AppendASCII() takes strings as is. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:720: chrome.send('UIFailedLoadingForTest'); On 2014/06/20 22:13:49, Aleksey Shlyapnikov wrote: > if (global.onManipulateSettingsForTest) > chrome.send('UIFailedLoadingForTest'); Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:903: if ('selectSaveAsPdfDestination' in event.settings) { On 2014/06/23 19:34:46, Aleksey Shlyapnikov wrote: > On 2014/06/21 02:44:11, Dan Beam wrote: > > another strategy would be to simply make different methods for all this... is > > there any reason you *need* to go through onManipulateSettingsForTest_ for all > > this code? > > I would support keeping native_layer.js involvement at minimum and do not have > any logic in it. This is test code, it would be great to keep it as small and > 'one piece' as possible. > Removing unnecessary "!element" checks and returning early will help to improve > readability of this code. I felt that keeping all the code in one function is better for the reasons Aleksey stated. Having it in one spot makes it difficult to read and makes it a huge function, at least its obvious where all the test related functionality is (save for the message passing parts, like the part in onPrintPreviewGenerationDone_). https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:908: SAVE_AS_PDF) { On 2014/06/20 22:13:49, Aleksey Shlyapnikov wrote: > if (destinations[i].id == > print_preview.Destination.GooglePromotedId.SAVE_AS_PDF) { Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:911: break; On 2014/06/21 02:44:11, Dan Beam wrote: > return; Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:914: if (!didSet) On 2014/06/21 02:44:11, Dan Beam wrote: > ^ remove didSet Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:915: this.nativeLayer_.previewFailedForTest(); On 2014/06/20 22:13:50, Aleksey Shlyapnikov wrote: > var pdfDestination = null; > ... > pdfDestination = destinations[i]; > break; > ... > if (pdfDestination) > this.destinationStore_.selectDestination(pdfDestination); > else > this.nativeLayer_.previewFailedForTest(); > > is easier to understand. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:916: } else if ('layoutSettings' in event.settings) { On 2014/06/21 02:27:51, Dan Beam wrote: > On 2014/06/20 22:13:50, Aleksey Shlyapnikov wrote: > > I know that one liners do not require brackets, but don't you think reading > this > > code with if and else from the different branches next to each other and with > > the same indent is confusing? I'd add brackets around one liner if here for > the > > sake of readability anyway. > > an overwhelming majority of chrome/browser/resources disagrees with this After removing all the unnecessary !element sort of statements (like 921 for example), I think I was able to get rid of this problem. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:927: this.nativeLayer_.previewFailedForTest(); On 2014/06/21 02:27:51, Dan Beam wrote: > On 2014/06/20 22:13:49, Aleksey Shlyapnikov wrote: > > I think if one part of the statement has braces the other one should have them > > too, even if it's a one liner. Otherwise it's harder to grasp the structure of > > the code. > > yes, should've mentioned this ^ Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:931: if (textbox) { On 2014/06/23 19:34:46, Aleksey Shlyapnikov wrote: > On 2014/06/21 02:44:11, Dan Beam wrote: > > if (!textbox) { > > this.nativeLayer_.previewFailedForTest(); > > return; > > } > > > > ... less if indent ... > > I don't see my extensive comment on the same matter in this revision, so just in > case, these "!element" checks are not necessary, they can be removed altogether > to save indentation and code. All the elements under test always exist, you > should just query and use them. I think I finally understand what you are saying and attempted to apply your suggestion through the code. I think I did it correctly. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:931: if (textbox) { On 2014/06/23 19:34:46, Aleksey Shlyapnikov wrote: > On 2014/06/21 02:44:11, Dan Beam wrote: > > if (!textbox) { > > this.nativeLayer_.previewFailedForTest(); > > return; > > } > > > > ... less if indent ... > > I don't see my extensive comment on the same matter in this revision, so just in > case, these "!element" checks are not necessary, they can be removed altogether > to save indentation and code. All the elements under test always exist, you > should just query and use them. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:937: // hang mysteriously if the string isn't correct. On 2014/06/20 22:13:49, Aleksey Shlyapnikov wrote: > No indent is required. I just got rid of the comment. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:952: '.header-footer-checkbox'); On 2014/06/20 22:13:49, Aleksey Shlyapnikov wrote: > Fits one line. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:952: '.header-footer-checkbox'); On 2014/06/20 22:13:49, Aleksey Shlyapnikov wrote: > Fits one line. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:952: '.header-footer-checkbox'); On 2014/06/20 22:13:49, Aleksey Shlyapnikov wrote: > Fits one line. Done. https://codereview.chromium.org/335583004/diff/310001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:975: event.settings.margins < element.length) { On 2014/06/20 22:13:50, Aleksey Shlyapnikov wrote: > Fix indent. Done.
lgtm for JS part. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:720: chrome.send('UIFailedLoadingForTest'); For symmetry: if (global.onManipulateSettingsForTest) chrome.send('UIFailedLoadingForTest'); https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:921: event.settings.layoutSettings.portrait ? 4 less spaces indent for this line. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:937: radio.click(); document.querySelector('.page-settings-custom-radio').click(); https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:955: I'd remove all these blank lines, they don't help reading the code. Blank lines before '} else if' lines would help some, but our style does not support it, so let's just remove them. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:971: @private * @private
Please explain what's happening with the C++ code for the benefit of those who weren't part of our in-person conversations. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. No (c), forgot to tell you last time. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:164: Observe(new_web_contents); In patch set 16, I asked if this should only be called once. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:177: scoped_ptr<PrintPreviewObserver> print_preview_observer_; Why does PrintPreviewObserver own another PrintPreviewObserver? https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:320: freopen("/tmp/pdf_generated_browsertest/stdin.txt", "r", stdin) Don't do this. It's hacky. I think this test should always be in layout test mode anyway. You'd want to change DummyTest to DISABLED_DummyTest, so it does not run as part of the normal browser_tests suite. Instead, you would only run it from the layout test runner, where you can pass --gtest_also_run_disabled_tests --gtest_filter=rintPreviewPdfGeneratedBrowserTest.DummyTest. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:333: getline(std::cin, cmd); std::getline, so you don't confuse this with getline from stdio.h. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:334: if (cmd.size() == 0) { std::string::size() == 0 -> std::string::empty() similarly, std::string::size() > 0 -> !std::string::empty() https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:348: // TODO(ivandavid): Read from cmd the name of the test html file. If you want to be content_shell compatible, you may need to read in multiple test file names and run multiple "layout tests" right?
https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:182: printing::MarginType margins_; On 2014/06/24 18:49:51, ivandavid wrote: > On 2014/06/21 02:44:11, Dan Beam wrote: > > please make sure all of these plain-old-data (e.g.bool, int, pointer) members > > are in the initializer list > > Done. not done https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:75: margins_(printing::DEFAULT_MARGINS) {} nit: each on a separate line https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:173: WebContents* web_contents_; not initialized, would crash if dereferenced https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:175: PrintPreviewUI* ui_; not initialized, would crash if deferenced
https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/06/24 21:24:58, Lei Zhang wrote: > No (c), forgot to tell you last time. Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:164: Observe(new_web_contents); On 2014/06/24 21:24:58, Lei Zhang wrote: > In patch set 16, I asked if this should only be called once. I think I'll just leave it like it is for now, but I will attempt to add a check later. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:177: scoped_ptr<PrintPreviewObserver> print_preview_observer_; On 2014/06/24 21:24:58, Lei Zhang wrote: > Why does PrintPreviewObserver own another PrintPreviewObserver? I think it was a copy paste accident. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:177: scoped_ptr<PrintPreviewObserver> print_preview_observer_; On 2014/06/24 21:24:58, Lei Zhang wrote: > Why does PrintPreviewObserver own another PrintPreviewObserver? Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:320: freopen("/tmp/pdf_generated_browsertest/stdin.txt", "r", stdin) On 2014/06/24 21:24:58, Lei Zhang wrote: > Don't do this. It's hacky. I think this test should always be in layout test > mode anyway. You'd want to change DummyTest to DISABLED_DummyTest, so it does > not run as part of the normal browser_tests suite. Instead, you would only run > it from the layout test runner, where you can pass > --gtest_also_run_disabled_tests > --gtest_filter=rintPreviewPdfGeneratedBrowserTest.DummyTest. I made it so that it automatically assumes that its in the layout test, however it doesn't currently cleanup itself correctly. It will once I've done more with the layout testing. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:320: freopen("/tmp/pdf_generated_browsertest/stdin.txt", "r", stdin) On 2014/06/24 21:24:58, Lei Zhang wrote: > Don't do this. It's hacky. I think this test should always be in layout test > mode anyway. You'd want to change DummyTest to DISABLED_DummyTest, so it does > not run as part of the normal browser_tests suite. Instead, you would only run > it from the layout test runner, where you can pass > --gtest_also_run_disabled_tests > --gtest_filter=rintPreviewPdfGeneratedBrowserTest.DummyTest. Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:333: getline(std::cin, cmd); On 2014/06/24 21:24:58, Lei Zhang wrote: > std::getline, so you don't confuse this with getline from stdio.h. Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:334: if (cmd.size() == 0) { On 2014/06/24 21:24:58, Lei Zhang wrote: > std::string::size() == 0 -> std::string::empty() > > similarly, > > std::string::size() > 0 -> !std::string::empty() Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:348: // TODO(ivandavid): Read from cmd the name of the test html file. On 2014/06/24 21:24:58, Lei Zhang wrote: > If you want to be content_shell compatible, you may need to read in multiple > test file names and run multiple "layout tests" right? I am not completely sure how it works when running multiple tests as I've only been running one at a time. However, I think for each test, it spawns a new thread. So if we wanted to test two different webpages, it would create two instances of this browsertest. So I think it only needs to read one test file. I'm going to have to do more tests to figure this out and will change it if need be. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:720: chrome.send('UIFailedLoadingForTest'); On 2014/06/24 19:10:00, Aleksey Shlyapnikov wrote: > For symmetry: > > if (global.onManipulateSettingsForTest) > chrome.send('UIFailedLoadingForTest'); Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:921: event.settings.layoutSettings.portrait ? On 2014/06/24 19:10:01, Aleksey Shlyapnikov wrote: > 4 less spaces indent for this line. Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:937: radio.click(); On 2014/06/24 19:10:00, Aleksey Shlyapnikov wrote: > document.querySelector('.page-settings-custom-radio').click(); Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:955: On 2014/06/24 19:10:01, Aleksey Shlyapnikov wrote: > I'd remove all these blank lines, they don't help reading the code. Blank lines > before '} else if' lines would help some, but our style does not support it, so > let's just remove them. Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:971: @private On 2014/06/24 19:10:01, Aleksey Shlyapnikov wrote: > * @private Don't know how that happened. Done.
https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:90: bool OnMessageReceived(const IPC::Message& message) OVERRIDE { most common way to do this: bool handled = true; IPC_BEGIN_MESSAGE_MAP(...) IPC_MESSAGE_HANDLER(...) IPC_MESSAGE_UNHANDLED(handled = false) IPC_END_MESSAGE_MAP() return handled; also note that there's no ; after these macros https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:248: ui_->web_ui()->AddMessageHandler(handler); nit: just inline |handler|, i.e. ui_->web_ui()->AddMessageHandler(new UIDoneLoadingMessageHandler(this)); https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:282: FileSelected(pdf_file_save_path_, 0, NULL); nit: print_preview_observer_->GetUI()->handler_->FileSelected( pdf_file_save_path_, 0, NULL); https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:295: WebContents* initiator_ = _ only go at the end of private members (so initiator_ -> initiator) https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:682: * Function that allows for onManipulateSettings to be called remove "Function that " everywhere https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:958: }, "much code, no newlines, ow" - my eyes
https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/310001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:182: printing::MarginType margins_; On 2014/06/24 22:55:15, Dan Beam wrote: > On 2014/06/24 18:49:51, ivandavid wrote: > > On 2014/06/21 02:44:11, Dan Beam wrote: > > > please make sure all of these plain-old-data (e.g.bool, int, pointer) > members > > > are in the initializer list > > > > Done. > > not done It was done in patch 17. Should have mentioned that. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:75: margins_(printing::DEFAULT_MARGINS) {} On 2014/06/24 22:55:16, Dan Beam wrote: > nit: each on a separate line Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:173: WebContents* web_contents_; On 2014/06/24 22:55:16, Dan Beam wrote: > not initialized, would crash if dereferenced Removed the variable. Turns out it was unnecessary. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:173: WebContents* web_contents_; On 2014/06/24 22:55:16, Dan Beam wrote: > not initialized, would crash if dereferenced Done. https://codereview.chromium.org/335583004/diff/350001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:175: PrintPreviewUI* ui_; On 2014/06/24 22:55:16, Dan Beam wrote: > not initialized, would crash if deferenced Same change as for web_contents_. Done. https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:90: bool OnMessageReceived(const IPC::Message& message) OVERRIDE { On 2014/06/24 23:13:01, Dan Beam wrote: > most common way to do this: > > bool handled = true; > > IPC_BEGIN_MESSAGE_MAP(...) > IPC_MESSAGE_HANDLER(...) > IPC_MESSAGE_UNHANDLED(handled = false) > IPC_END_MESSAGE_MAP() > > return handled; > > also note that there's no ; after these macros I am using the same format as the OnMessageReceived method in print_preview_dialog_controller_browsertest.cc. If I don't return false, it doesn't progress. The reason is because I am just watching the messages, but not actually progressing the print preview process. print_preview_message_handler.h is supposed to handle them and move everything forward. https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:248: ui_->web_ui()->AddMessageHandler(handler); On 2014/06/24 23:13:01, Dan Beam wrote: > nit: just inline |handler|, i.e. > > ui_->web_ui()->AddMessageHandler(new UIDoneLoadingMessageHandler(this)); > Done. https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:282: FileSelected(pdf_file_save_path_, 0, NULL); On 2014/06/24 23:13:01, Dan Beam wrote: > nit: > > print_preview_observer_->GetUI()->handler_->FileSelected( > pdf_file_save_path_, 0, NULL); Done. https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:295: WebContents* initiator_ = On 2014/06/24 23:13:01, Dan Beam wrote: > _ only go at the end of private members (so initiator_ -> initiator) Done. https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:682: * Function that allows for onManipulateSettings to be called On 2014/06/24 23:13:01, Dan Beam wrote: > remove "Function that " everywhere Done. https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:958: }, On 2014/06/24 23:13:01, Dan Beam wrote: > "much code, no newlines, ow" - my eyes Ha. I am not sure where to put new lines though. I tried putting them in, but it made the code look awkward. This whole file looks like this and Aleksey said the one nice place where some newlines would be good are before the 'else if' statements. Any suggestions for where I should put some newlines?
https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/370001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:958: }, On 2014/06/25 00:02:22, ivandavid wrote: > On 2014/06/24 23:13:01, Dan Beam wrote: > > "much code, no newlines, ow" - my eyes > Ha. I am not sure where to put new lines though. I tried putting them in, but it > made the code look awkward. This whole file looks like this and Aleksey said the > one nice place where some newlines would be good are before the 'else if' > statements. Any suggestions for where I should put some newlines? And those new lines don't conform to the style guide.
https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/350001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:955: On 2014/06/24 19:10:01, Aleksey Shlyapnikov wrote: > I'd remove all these blank lines, they don't help reading the code. Blank lines > before '} else if' lines would help some, but our style does not support it, so > let's just remove them. disagree https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/370001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:90: bool OnMessageReceived(const IPC::Message& message) OVERRIDE { On 2014/06/25 00:02:21, ivandavid wrote: > On 2014/06/24 23:13:01, Dan Beam wrote: > > most common way to do this: > > > > bool handled = true; > > > > IPC_BEGIN_MESSAGE_MAP(...) > > IPC_MESSAGE_HANDLER(...) > > IPC_MESSAGE_UNHANDLED(handled = false) > > IPC_END_MESSAGE_MAP() > > > > return handled; > > > > also note that there's no ; after these macros > > I am using the same format as the OnMessageReceived method in > print_preview_dialog_controller_browsertest.cc. If I don't return false, it > doesn't progress. The reason is because I am just watching the messages, but not > actually progressing the print preview process. print_preview_message_handler.h > is supposed to handle them and move everything forward. > the method I suggested does return false https://codereview.chromium.org/335583004/diff/390001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/390001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:918: } else if ('layoutSettings' in event.settings) { if ('blah' in bigObjectThing) { ... do blah stuff ... return; } if ('blee' in bigObjectThing) { ... do blee stuff ... return; }
fyi: looks pretty good other than my style nits
this needs a bug number in the BUG= line, though
On 2014/06/25 00:14:35, Dan Beam wrote: > this needs a bug number in the BUG= line, though I'll get around to filing bugs.
https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:53: class PrintPreviewObserver; The forward declarations are no longer needed. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:76: class PrintPreviewObserver : public WebContentsObserver { Please write comments to briefly describe what all the classes do. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:104: return false; Add a comment to explain why you return false. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:191: scoped_ptr<base::DictionaryValue> script_argument_; This is only used in 1 method. Make it a local variable? https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:207: observer_ = NULL; Why do we need to do this? https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:210: void HandleDone(const base::ListValue* /* args */) { Unless it's completely obvious, document methods, here and below. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:220: FAIL(); You should at least print out what state the class is in, so you'll have an idea of what failed. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:242: void PrintPreviewObserver::OnDidGetPreviewPageCount( So I missed the whole inline your method impls discussion, but if you have done that for all the other methods, don't leave this one out by itself. Consistency is good. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:258: // TODO (ivandavid): Provide arguments for SetPrintPreviewSettings here. I think you should just fix the TODO. It might be helpful to put all the options in a struct so you don't need to pass 5 arguments everywhere. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:278: pdf_file_save_path_ = pdf_file_save_path_.AppendASCII("printing"); You should create a temporary file and delete it afterwards, since the PDF is no longer the final output. There's functions to help you do that in base/ https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:308: int data_size = pdf_file.Read(0, &data[0], data.size()); Just use ReadFileToString(). It's a lot easier. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:312: gfx::Rect rect(400, 400); You should fix this... https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:315: new uint8_t[4 * settings.area().size().GetArea()]); You should check for integer overflow here. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:328: std::vector<gfx::PNGCodec::Comment> comments; You can name it empty_comments so it's obvious this is just a dummy variable. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:404: base::FilePath tmp_dir("/tmp"); There exists cross-platform code to get the temp directory. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:430: // TODO(ivandavid): Read from cmd the name of the test html file. So you need to make up your mind about whether the test can render multiple HTML files in one go, because that affects how some of the code should be structured.
The browsertest is almost completely functional and works with the layout tests with no problem (it runs and cleans itself up successfully, even if it crashes for some reason). It can run in fully-parallel mode or not and can already detect some bugs (had some disappearing vertical lines on cplusplus.com). I just need to give the layout test to send a list of parameters to the browsertest for the print preview settings and I need to remove some hardcoding when converting the pdf to a png. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:53: class PrintPreviewObserver; On 2014/06/26 21:36:43, Lei Zhang wrote: > The forward declarations are no longer needed. Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:76: class PrintPreviewObserver : public WebContentsObserver { On 2014/06/26 21:36:43, Lei Zhang wrote: > Please write comments to briefly describe what all the classes do. Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:104: return false; On 2014/06/26 21:36:43, Lei Zhang wrote: > Add a comment to explain why you return false. Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:191: scoped_ptr<base::DictionaryValue> script_argument_; On 2014/06/26 21:36:44, Lei Zhang wrote: > This is only used in 1 method. Make it a local variable? Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:207: observer_ = NULL; On 2014/06/26 21:36:43, Lei Zhang wrote: > Why do we need to do this? Don't know. It wasn't needed to I just removed it. Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:210: void HandleDone(const base::ListValue* /* args */) { On 2014/06/26 21:36:44, Lei Zhang wrote: > Unless it's completely obvious, document methods, here and below. Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:220: FAIL(); On 2014/06/26 21:36:43, Lei Zhang wrote: > You should at least print out what state the class is in, so you'll have an idea > of what failed. I'll do this after I do some more javascript debugging, since there might still be some problems. After that, it will print out the state of the print preview observer and I'll have the javascript send an error message (using the ListValue param). https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:242: void PrintPreviewObserver::OnDidGetPreviewPageCount( On 2014/06/26 21:36:43, Lei Zhang wrote: > So I missed the whole inline your method impls discussion, but if you have done > that for all the other methods, don't leave this one out by itself. Consistency > is good. This needs to be separated due to a forward declaration problem. I added a comment in the code. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:258: // TODO (ivandavid): Provide arguments for SetPrintPreviewSettings here. On 2014/06/26 21:36:44, Lei Zhang wrote: > I think you should just fix the TODO. It might be helpful to put all the options > in a struct so you don't need to pass 5 arguments everywhere. Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:278: pdf_file_save_path_ = pdf_file_save_path_.AppendASCII("printing"); On 2014/06/26 21:36:44, Lei Zhang wrote: > You should create a temporary file and delete it afterwards, since the PDF is no > longer the final output. There's functions to help you do that in base/ I changed how this works. I create a scoped temporary directory and store the named pipe and pdf in there. Once the program is done, it deletes everything in the directory and the directory itself. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:278: pdf_file_save_path_ = pdf_file_save_path_.AppendASCII("printing"); On 2014/06/26 21:36:44, Lei Zhang wrote: > You should create a temporary file and delete it afterwards, since the PDF is no > longer the final output. There's functions to help you do that in base/ Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:308: int data_size = pdf_file.Read(0, &data[0], data.size()); On 2014/06/26 21:36:43, Lei Zhang wrote: > Just use ReadFileToString(). It's a lot easier. Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:312: gfx::Rect rect(400, 400); On 2014/06/26 21:36:43, Lei Zhang wrote: > You should fix this... It turns out the functionality I need isn't exposed in the libpdf.so (FPDF_GetPageSizeByFunction()), so I can't seem to get the height and width of the pdf. Once I find a way to get these values, then I can fix it. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:315: new uint8_t[4 * settings.area().size().GetArea()]); On 2014/06/26 21:36:43, Lei Zhang wrote: > You should check for integer overflow here. I will fix this when I figure out the solution to the problem above, since it might require me to rewrite this part completely. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:328: std::vector<gfx::PNGCodec::Comment> comments; On 2014/06/26 21:36:43, Lei Zhang wrote: > You can name it empty_comments so it's obvious this is just a dummy variable. I actually need it to store an MD5 has in the png. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:404: base::FilePath tmp_dir("/tmp"); On 2014/06/26 21:36:44, Lei Zhang wrote: > There exists cross-platform code to get the temp directory. Done. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:430: // TODO(ivandavid): Read from cmd the name of the test html file. On 2014/06/26 21:36:44, Lei Zhang wrote: > So you need to make up your mind about whether the test can render multiple HTML > files in one go, because that affects how some of the code should be structured. The test can now accept multiple html files and print them one at a time. It doesn't receive them all at once however, it receives the next html after it has finished printing the current html file. https://codereview.chromium.org/335583004/diff/430001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:430: // TODO(ivandavid): Read from cmd the name of the test html file. On 2014/06/26 21:36:44, Lei Zhang wrote: > So you need to make up your mind about whether the test can render multiple HTML > files in one go, because that affects how some of the code should be structured. Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/450001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:930: if (element.checked) Was using clicked rather than checked, which wasn't working.
https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:78: PrintPreviewSettings() {} Do you need a default ctor? ... where the members are not initialized. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:91: bool is_portrait_; struct members don't have the trailing underscore https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:177: GetUI()->web_ui()->CallJavascriptFunction( Since the size of |args| is only ever 1, you can drop |args| altogether and just use CallJavascriptFunction(const std::string& function_name, const base::Value& arg); https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:190: void SetPrintPreviewSettings(PrintPreviewSettings settings) { pass by const ref https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:209: LOG(ERROR) << "DESTROYED"; Accidentally included? https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:302: gurl); fits on previous line https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:312: pdf_file_save_path_ = dir.AppendASCII("dummy.pdf"); Why not just set |pdf_file_save_path_| before you call Print() ? Then you don't even need to pass in |dir|. You can just ASSERT_FALSE(pdf_file_save_path_.empty()) here instead. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:320: // Converts the pdf to a a png file, so that the layout test can typo https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:330: LOG(ERROR) << pdf_module_path.value(); remove https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:377: hash_input.size(), this all fits on 1 line? https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:411: printf("Content-Length: %lu\n", output_.size()); You probably need the PRIuS macro from base/format_macros.h here. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:412: std::vector<unsigned char>::iterator it = output_.begin(); For vectors, if you write: for (size_t i = 0; i < output_.size(); ++i), then it's just 1 line. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:446: ASSERT_TRUE(browser()->tab_strip_model()->count() == 2); ASSERT_EQ https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:501: base::FilePath::StringType cmd; I don't think std::getline() takes a std::wstring on Windows. So I don't see how this can work. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:503: if (cmd.size() == 0) { .empty() https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:535: base::DeleteFile(tmp_path, false); You don't need this. Since |tmp_path| is within |tmp_dir|, it will get deleted when |tmp_dir| goes out of scope.
https://codereview.chromium.org/335583004/diff/450001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/450001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:965: } please break each if ('name' in event.settings) into its own method
I made a lot of changes to the CL however, I think its mostly fixed. The only problem now is that it doesn't seem to work for pdf files (it can print html files) on my computer. I am having a difficult time tracking down this problem, however I think I will be able to fix it soon. I had to expose extra pdf functions because I needed that functionality to actually print each page at the size its viewed at, rather than a constant size, which would introduce large errors. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:78: PrintPreviewSettings() {} On 2014/06/30 22:38:32, Lei Zhang wrote: > Do you need a default ctor? ... where the members are not initialized. At the time I did, however I rewrote the code so that is was no longer needed and instead kept the other constructor and added a copy constructor. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:78: PrintPreviewSettings() {} On 2014/06/30 22:38:32, Lei Zhang wrote: > Do you need a default ctor? ... where the members are not initialized. Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:91: bool is_portrait_; On 2014/06/30 22:38:32, Lei Zhang wrote: > struct members don't have the trailing underscore Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:177: GetUI()->web_ui()->CallJavascriptFunction( On 2014/06/30 22:38:31, Lei Zhang wrote: > Since the size of |args| is only ever 1, you can drop |args| altogether and just > use CallJavascriptFunction(const std::string& function_name, const base::Value& > arg); Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:190: void SetPrintPreviewSettings(PrintPreviewSettings settings) { On 2014/06/30 22:38:31, Lei Zhang wrote: > pass by const ref Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:209: LOG(ERROR) << "DESTROYED"; On 2014/06/30 22:38:32, Lei Zhang wrote: > Accidentally included? Yeah. Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:302: gurl); On 2014/06/30 22:38:31, Lei Zhang wrote: > fits on previous line Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:312: pdf_file_save_path_ = dir.AppendASCII("dummy.pdf"); On 2014/06/30 22:38:32, Lei Zhang wrote: > Why not just set |pdf_file_save_path_| before you call Print() ? Then you don't > even need to pass in |dir|. You can just > ASSERT_FALSE(pdf_file_save_path_.empty()) here instead. Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:320: // Converts the pdf to a a png file, so that the layout test can On 2014/06/30 22:38:32, Lei Zhang wrote: > typo Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:330: LOG(ERROR) << pdf_module_path.value(); On 2014/06/30 22:38:31, Lei Zhang wrote: > remove Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:377: hash_input.size(), On 2014/06/30 22:38:31, Lei Zhang wrote: > this all fits on 1 line? Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:411: printf("Content-Length: %lu\n", output_.size()); On 2014/06/30 22:38:32, Lei Zhang wrote: > You probably need the PRIuS macro from base/format_macros.h here. Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:412: std::vector<unsigned char>::iterator it = output_.begin(); On 2014/06/30 22:38:32, Lei Zhang wrote: > For vectors, if you write: for (size_t i = 0; i < output_.size(); ++i), then > it's just 1 line. Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:446: ASSERT_TRUE(browser()->tab_strip_model()->count() == 2); On 2014/06/30 22:38:31, Lei Zhang wrote: > ASSERT_EQ Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:501: base::FilePath::StringType cmd; On 2014/06/30 22:38:31, Lei Zhang wrote: > I don't think std::getline() takes a std::wstring on Windows. So I don't see how > this can work. I think I fixed it. I defined a macro STDIN_STREAM that is either std::cin if OS_POSIX is defined or OS_WIN is defined. I know getline works with std::wcin and std::wstring, so I think this should work. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:503: if (cmd.size() == 0) { On 2014/06/30 22:38:32, Lei Zhang wrote: > .empty() Done. https://codereview.chromium.org/335583004/diff/450001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:535: base::DeleteFile(tmp_path, false); On 2014/06/30 22:38:32, Lei Zhang wrote: > You don't need this. Since |tmp_path| is within |tmp_dir|, it will get deleted > when |tmp_dir| goes out of scope. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:57: #if defined(OS_POSIX) Is this an OK solution to the problem of getline needing different stream types depending on the platform? I think it is, however if there is a better way I will implement that one. Also, is there a specific header I have to include to make this work? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:416: size_t max_size = std::numeric_limits<size_t>::max(); I think I did this correctly. I feel like my reasoning is correct about having to do two overflow checks, however I am not sure. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:568: MANUAL_DummyTest) { Now a manual test since it should only be run with the layout test framework. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:599: // TODO(ivandavid): Get settings from file in resources. I will do this once I start actually writing tests and figure out what settings I might want to add, on top of these settings, such as DPI settings if the image is too large and eats up too much memory. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:904: this.saveAsPdfForTest_(); // No parameters needed. I split up this function and not native_layer.onManipulateSettingsForTest_(). This is because if I split that one up, I would have to add new event types to native layer, and then I would have to send dispatch at least 6 different different types of events.
https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:106: is_already_pdf(settings.is_already_pdf) {} do you need to explicitly declare these constructors? if you delete L87-106 does the code still work? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:232: OVERRIDE { indent off on OVERRIDE https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:255: explicit UIDoneLoadingMessageHandler(PrintPreviewObserver* observer) : : on next line https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:263: observer_->ManipulatePreviewSettings(); can |observer_| be NULL? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:282: base::Unretained(this))); indent off https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:287: base::Unretained(this))); indent off https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:296: // This function needs to be forward declared. I don't think you need this forward if you move UIDoneLoadingMessageHandler above PrintPreviewObserver https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:385: // This will do for now. i think you simply need to add PRINTING_EXPORT to the constants in printing/units.h https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:506: void Clean() { nit: Reset() https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:518: printf("#READY\n"); what is this doing? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:535: typedef bool (*PDFPageToBitmap) (const void * pdf_buffer, nit: ) ( -> )( https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:568: MANUAL_DummyTest) { On 2014/07/03 03:12:03, ivandavid wrote: > Now a manual test since it should only be run with the layout test framework. what's the point of a manual test like this? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:569: // What this code is supposed to do: Setup communication with the make into an ordered list: - much organized - so pretty - wow https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:573: remove \n https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:615: printf("#EOF\n"); what is this doing? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:944: this.nativeLayer_.previewFailedForTest(); indent off https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:950: * @param {boolean} portrait If true then the portrait settings is to be nit: @param {boolean} portrait Whether to use portrait page layout; if false: landscape. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:962: element.click(); indent off https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:968: * @param {string} pageRange Sets the page range to the desired value(s). nit * @param {string} pageRange A comma separated page range. Ex: "1-5,9" * means pages 1 through 5 and page 9. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:982: document.querySelector('.page-settings-custom-radio').click(); can you just do .click() event when the value's the same? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:989: * @param {boolean} headersAndFooters If true, the checkbox should be Whether the "Headers and footer" checkbox should be checked. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:1020: * a valid index or else the test fails. @private https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3403: ret_value = true; just always return true, ret_value is always overwritten here...
https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:106: is_already_pdf(settings.is_already_pdf) {} On 2014/07/07 23:08:11, Dan Beam wrote: > do you need to explicitly declare these constructors? if you delete L87-106 > does the code still work? The first one does, however the copy constructor doesn't need to be there. I forgot c++ provides one by default and I didn't need to override it because there are no pointers. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:106: is_already_pdf(settings.is_already_pdf) {} On 2014/07/07 23:08:11, Dan Beam wrote: > do you need to explicitly declare these constructors? if you delete L87-106 > does the code still work? Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:232: OVERRIDE { On 2014/07/07 23:08:11, Dan Beam wrote: > indent off on OVERRIDE Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:255: explicit UIDoneLoadingMessageHandler(PrintPreviewObserver* observer) : On 2014/07/07 23:08:11, Dan Beam wrote: > : on next line Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:263: observer_->ManipulatePreviewSettings(); On 2014/07/07 23:08:12, Dan Beam wrote: > can |observer_| be NULL? No it shouldn't. I added an ASSERT_TRUE to make sure its not. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:263: observer_->ManipulatePreviewSettings(); On 2014/07/07 23:08:12, Dan Beam wrote: > can |observer_| be NULL? Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:282: base::Unretained(this))); On 2014/07/07 23:08:11, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:287: base::Unretained(this))); On 2014/07/07 23:08:12, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:296: // This function needs to be forward declared. On 2014/07/07 23:08:11, Dan Beam wrote: > I don't think you need this forward if you move UIDoneLoadingMessageHandler > above PrintPreviewObserver If I did that, I would have had another forward declaration problem. However, I found a way around all this. I made UIDoneLoadingMessageHandler a nested class within PrintPreviewObserver, since its the only class that will use it. It fixes the forward declaration problem. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:296: // This function needs to be forward declared. On 2014/07/07 23:08:11, Dan Beam wrote: > I don't think you need this forward if you move UIDoneLoadingMessageHandler > above PrintPreviewObserver Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:385: // This will do for now. On 2014/07/07 23:08:11, Dan Beam wrote: > i think you simply need to add PRINTING_EXPORT to the constants in > printing/units.h I actually had problems accessing the function I needed, not the constants. I added PRINTING_EXPORT to the function and now it works. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:385: // This will do for now. On 2014/07/07 23:08:11, Dan Beam wrote: > i think you simply need to add PRINTING_EXPORT to the constants in > printing/units.h Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:506: void Clean() { On 2014/07/07 23:08:11, Dan Beam wrote: > nit: Reset() Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:518: printf("#READY\n"); On 2014/07/07 23:08:11, Dan Beam wrote: > what is this doing? See comment on line 615. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:568: MANUAL_DummyTest) { On 2014/07/07 23:08:11, Dan Beam wrote: > On 2014/07/03 03:12:03, ivandavid wrote: > > Now a manual test since it should only be run with the layout test framework. > > what's the point of a manual test like this? It was a suggestion by Dirk Pranke that he made in a design document for the layout test modifications that I am making. I shared the design document with you. If you click comments after opening the doc, you'll see it. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:569: // What this code is supposed to do: Setup communication with the On 2014/07/07 23:08:10, Dan Beam wrote: > make into an ordered list: > - much organized > - so pretty > - wow Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:573: On 2014/07/07 23:08:11, Dan Beam wrote: > remove \n Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:615: printf("#EOF\n"); On 2014/07/07 23:08:12, Dan Beam wrote: > what is this doing? All of the printf("#EOF\n") statements are for layout tests. I added a bunch of comments explaining why these are there in general and I also documented each instance of a call to printf("#EOF\n") to explain which specific content block has been sent. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:944: this.nativeLayer_.previewFailedForTest(); On 2014/07/07 23:08:12, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:950: * @param {boolean} portrait If true then the portrait settings is to be On 2014/07/07 23:08:12, Dan Beam wrote: > nit: @param {boolean} portrait Whether to use portrait page layout; if false: > landscape. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:962: element.click(); On 2014/07/07 23:08:12, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:968: * @param {string} pageRange Sets the page range to the desired value(s). On 2014/07/07 23:08:12, Dan Beam wrote: > nit > > * @param {string} pageRange A comma separated page range. Ex: "1-5,9" > * means pages 1 through 5 and page 9. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:982: document.querySelector('.page-settings-custom-radio').click(); On 2014/07/07 23:08:12, Dan Beam wrote: > can you just do .click() event when the value's the same? No, it doesn't cause the preview area to be generate again, therefore the test hangs since the preview area has to be generated again for a message to be sent to the test. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:989: * @param {boolean} headersAndFooters If true, the checkbox should be On 2014/07/07 23:08:12, Dan Beam wrote: > Whether the "Headers and footer" checkbox should be checked. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:1020: * a valid index or else the test fails. On 2014/07/07 23:08:12, Dan Beam wrote: > @private Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:1034: /** Sorry for the double indents. I was writing python code previously and was habitually hitting tab twice. https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3403: ret_value = true; On 2014/07/07 23:08:12, Dan Beam wrote: > just always return true, ret_value is always overwritten here... Oops. Didn't mean to assign it to true there. It actually shouldn't always return true. I now assign it correctly. https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3403: ret_value = true; On 2014/07/07 23:08:12, Dan Beam wrote: > just always return true, ret_value is always overwritten here... Done.
https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:57: #if defined(OS_POSIX) On 2014/07/03 03:12:03, ivandavid wrote: > Is this an OK solution to the problem of getline needing different stream types > depending on the platform? I think it is, however if there is a better way I > will implement that one. Also, is there a specific header I have to include to > make this work? Can you just use the same pattern as the blink layout test runner? I don't recall they having to do this. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:123: Browser* browser) Fits on the previous line. Not sure why you moved it. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:201: EndLoop(); Return after this? Your code should have failed the DCHECK on line 204 below. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:232: OVERRIDE { On 2014/07/07 23:08:11, Dan Beam wrote: > indent off on OVERRIDE Or just put it on the previous line... https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:282: base::Unretained(this))); On 2014/07/07 23:08:11, Dan Beam wrote: > indent off That is, indentation is off on line 280 and downwards. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:342: void PdfToPng() { PdfToPng() is over 100 lines long. Consider breaking it up into smaller pieces. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:373: &num_pages, ASSERT |num_pages| is positive just in case. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:380: double height, width; Since we generally think of sizes as WxH, maybe declare and access them that order when the ordering doesn't matter otherwise. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:390: height = height * kPixelsPerInch / kPointsPerInch; Use printing::ConvertUnit() ? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:392: bool autorotate = false; Can you explain why we need to rotate landscape images? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:395: double temp = height; use std::swap() https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:407: printing::PdfRenderSettings settings(rect, 300, true); What is 300? Is it the DPI? Make it a constant please. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:417: if (static_cast<size_t>(settings.area().size().width()) > Can't you just call area().width() and skip the size() call in the moddile? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:417: if (static_cast<size_t>(settings.area().size().width()) > Why do you need to cast this to a size_t? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:421: LOG(ERROR) << "OVERFLOW DETECTED: IMAGE WIDTH OR HEIGHT IS TOO LARGE!"; You can just FAIL() << "oh noes!"; https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:444: base::MD5Sum(static_cast<void*>(const_cast<char*>(hash_input.data())), Do you need the const_cast? base::MD5Sum() takes a const void* https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:448: gfx::Rect png_rect(min_width, total_height); If you start with a 2 page PDF file source, and the PDFs are 8x11 and 7x11. This would end up being 7x22. Doesn't that mean there's a 1x11 portion that'll get cut off? https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:495: new PrintPreviewObserver(tab, browser())); This fits on the previous line just fine before... https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:535: typedef bool (*PDFPageToBitmap) (const void * pdf_buffer, and void * -> void* https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:535: typedef bool (*PDFPageToBitmap) (const void * pdf_buffer, And name it PDFPageToBitmapProc to be consistent with the other two function pointer types. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:556: PDFPageToBitmap pdf_to_bitmap_func_; Document your member variables unless they are completely obvious. I'd group the three foo_func_ ones together with 1 comment. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.h:170: FRIEND_TEST_ALL_PREFIXES(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. https://codereview.chromium.org/335583004/diff/470001/pdf/libpdf.map File pdf/libpdf.map (right): https://codereview.chromium.org/335583004/diff/470001/pdf/libpdf.map#newcode8 pdf/libpdf.map:8: GetPDFPageSizeByIndex; This file no longer exists, please sync your checkout. https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3403: ret_value = true; I think what you want is: if (!doc) return false; bool retval == FPDF_GetPageSizeByIndex(doc, index, width, height) != 0; FPDF_CloseDocument(doc); return retval;
c/b/resources + c/b/ui/webui lgtm w/nits https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:982: document.querySelector('.page-settings-custom-radio').click(); On 2014/07/08 01:07:42, ivandavid wrote: > On 2014/07/07 23:08:12, Dan Beam wrote: > > can you just do .click() event when the value's the same? > No, it doesn't cause the preview area to be generate again, therefore the test > hangs since the preview area has to be generated again for a message to be sent > to the test. ok https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3403: ret_value = true; On 2014/07/08 01:11:37, Lei Zhang wrote: > I think what you want is: > > if (!doc) > return false; > bool retval == FPDF_GetPageSizeByIndex(doc, index, width, height) != 0; > FPDF_CloseDocument(doc); > return retval; if we're gonna keep this code, s/retval ==/success =/g https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:140: IPC_MESSAGE_UNHANDLED(break;) i still don't love that there's a break; here because it assumes the code generated by this macro is a switch or a loop, but i guess "return false;" assumes it's not in a ctor... https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:196: DCHECK(!script_argument.empty()); ^ wouldn't this fail in the case of kWaitingForFinalMessage? https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:218: observer_ = observer; nit: explicit UIDoneLoadingMessageHandler(PrintPreviewObserver* observer) : observer_(observer) {} https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:256: PrintPreviewObserver* observer_; nit: PrintPreviewObserver* observer_ const; https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:904: this.saveAsPdfForTest_(); // No parameters needed. nit: this.saveAsPdfForTest_(); // No parameters needed. ^^ 2 \s https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:951: * if false: landscape nit: . at end https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:958: '.layout-settings-landscape-radio'); nit: var mode = portrait ? 'portrait' : 'landscape'; var element = document.querySelector('.layout-settings-' + mode + '-radio'); https://codereview.chromium.org/335583004/diff/490001/pdf/pdf.cc File pdf/pdf.cc (right): https://codereview.chromium.org/335583004/diff/490001/pdf/pdf.cc#newcode218 pdf/pdf.cc:218: if(!g_sdk_initialized_via_pepper) if ( ^ \s https://codereview.chromium.org/335583004/diff/490001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/335583004/diff/490001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:315: what does the return value mean? that the call succeeded or failed?
https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:958: '.layout-settings-landscape-radio'); On 2014/07/08 04:25:13, Dan Beam wrote: > nit: > var mode = portrait ? 'portrait' : 'landscape'; > var element = document.querySelector('.layout-settings-' + mode + '-radio'); Please keep it as it is, code search won't find these classes if you follow the advice. You can save one line this way: var element = document.querySelector(portrait ? '.layout-settings-portrait-radio' : '.layout-settings-landscape-radio');
https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:958: '.layout-settings-landscape-radio'); On 2014/07/08 17:21:35, Aleksey Shlyapnikov wrote: > On 2014/07/08 04:25:13, Dan Beam wrote: > > nit: > > var mode = portrait ? 'portrait' : 'landscape'; > > var element = document.querySelector('.layout-settings-' + mode + '-radio'); > > Please keep it as it is, code search won't find these classes if you follow the > advice. You can save one line this way: > > var element = document.querySelector(portrait ? > '.layout-settings-portrait-radio' : > '.layout-settings-landscape-radio'); > that's fine too
After this patch set, I'm going to split off some code from this cl and make some new ones because its getting absurdly large. I probably should have done it a while ago. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:57: #if defined(OS_POSIX) On 2014/07/08 01:11:36, Lei Zhang wrote: > On 2014/07/03 03:12:03, ivandavid wrote: > > Is this an OK solution to the problem of getline needing different stream > types > > depending on the platform? I think it is, however if there is a better way I > > will implement that one. Also, is there a specific header I have to include to > > make this work? > > Can you just use the same pattern as the blink layout test runner? I don't > recall they having to do this. I'm going to keep this implementation for now until I run I start testing the program on windows. Then if it doesn't work, I'll change it, even though it should work. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:123: Browser* browser) On 2014/07/08 01:11:37, Lei Zhang wrote: > Fits on the previous line. Not sure why you moved it. I added an extra parameter, but then removed it, then forgot to move this back up. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:123: Browser* browser) On 2014/07/08 01:11:37, Lei Zhang wrote: > Fits on the previous line. Not sure why you moved it. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:201: EndLoop(); On 2014/07/08 01:11:36, Lei Zhang wrote: > Return after this? Your code should have failed the DCHECK on line 204 below. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:232: OVERRIDE { On 2014/07/08 01:11:36, Lei Zhang wrote: > On 2014/07/07 23:08:11, Dan Beam wrote: > > indent off on OVERRIDE > > Or just put it on the previous line... Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:282: base::Unretained(this))); On 2014/07/08 01:11:36, Lei Zhang wrote: > On 2014/07/07 23:08:11, Dan Beam wrote: > > indent off > > That is, indentation is off on line 280 and downwards. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:342: void PdfToPng() { On 2014/07/08 01:11:36, Lei Zhang wrote: > PdfToPng() is over 100 lines long. Consider breaking it up into smaller pieces. I turned lines 344-366 into a different setup function that is called at the beginning of the test. Then I made lines 443 to 466 into another function also. The function comes out to about 90 lines now. Unfortunately its still large, but a lot of the code can't really be split into different functions. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:342: void PdfToPng() { On 2014/07/08 01:11:36, Lei Zhang wrote: > PdfToPng() is over 100 lines long. Consider breaking it up into smaller pieces. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:373: &num_pages, On 2014/07/08 01:11:36, Lei Zhang wrote: > ASSERT |num_pages| is positive just in case. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:380: double height, width; On 2014/07/08 01:11:36, Lei Zhang wrote: > Since we generally think of sizes as WxH, maybe declare and access them that > order when the ordering doesn't matter otherwise. Done. I didn't realize how annoying this was until I decided to read it again. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:390: height = height * kPixelsPerInch / kPointsPerInch; On 2014/07/08 01:11:36, Lei Zhang wrote: > Use printing::ConvertUnit() ? Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:392: bool autorotate = false; On 2014/07/08 01:11:36, Lei Zhang wrote: > Can you explain why we need to rotate landscape images? Added a comment. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:395: double temp = height; On 2014/07/08 01:11:36, Lei Zhang wrote: > use std::swap() Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:407: printing::PdfRenderSettings settings(rect, 300, true); On 2014/07/08 01:11:36, Lei Zhang wrote: > What is 300? Is it the DPI? Make it a constant please. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:417: if (static_cast<size_t>(settings.area().size().width()) > On 2014/07/08 01:11:36, Lei Zhang wrote: > Can't you just call area().width() and skip the size() call in the moddile? Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:417: if (static_cast<size_t>(settings.area().size().width()) > On 2014/07/08 01:11:36, Lei Zhang wrote: > Why do you need to cast this to a size_t? settings.area().width() is a signed type, however we are comparing it to an unsigned type, size_t. Therefore it has to be cast so it can compile. However, now that I noticed this, I think comparing it to size_t would be incorrect, since if it overflowed, it could be negative, then when we cast it would no longer be causing some weird stuff to happen. I'll have to look into this more. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:421: LOG(ERROR) << "OVERFLOW DETECTED: IMAGE WIDTH OR HEIGHT IS TOO LARGE!"; On 2014/07/08 01:11:36, Lei Zhang wrote: > You can just FAIL() << "oh noes!"; Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:444: base::MD5Sum(static_cast<void*>(const_cast<char*>(hash_input.data())), On 2014/07/08 01:11:37, Lei Zhang wrote: > Do you need the const_cast? base::MD5Sum() takes a const void* Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:448: gfx::Rect png_rect(min_width, total_height); On 2014/07/08 01:11:36, Lei Zhang wrote: > If you start with a 2 page PDF file source, and the PDFs are 8x11 and 7x11. This > would end up being 7x22. Doesn't that mean there's a 1x11 portion that'll get > cut off? Done. I made it so that the bitmap gets filled in with a blank space so it keeps the proportions. So, in the example you gave, the 7x11 will be expanded to an 8x11 image. However, rather than stretching the image, in the 1x11 spot, there will just be white space, preserving the size of the image. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:495: new PrintPreviewObserver(tab, browser())); On 2014/07/08 01:11:36, Lei Zhang wrote: > This fits on the previous line just fine before... Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:535: typedef bool (*PDFPageToBitmap) (const void * pdf_buffer, On 2014/07/08 01:11:36, Lei Zhang wrote: > And name it PDFPageToBitmapProc to be consistent with the other two function > pointer types. Done. https://codereview.chromium.org/335583004/diff/470001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:556: PDFPageToBitmap pdf_to_bitmap_func_; On 2014/07/08 01:11:36, Lei Zhang wrote: > Document your member variables unless they are completely obvious. I'd group the > three foo_func_ ones together with 1 comment. Done. https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/335583004/diff/470001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3403: ret_value = true; On 2014/07/08 04:25:13, Dan Beam wrote: > On 2014/07/08 01:11:37, Lei Zhang wrote: > > I think what you want is: > > > > if (!doc) > > return false; > > bool retval == FPDF_GetPageSizeByIndex(doc, index, width, height) != 0; > > FPDF_CloseDocument(doc); > > return retval; > > if we're gonna keep this code, s/retval ==/success =/g Done. https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:140: IPC_MESSAGE_UNHANDLED(break;) On 2014/07/08 04:25:13, Dan Beam wrote: > i still don't love that there's a break; here because it assumes the code > generated by this macro is a switch or a loop, but i guess "return false;" > assumes it's not in a ctor... I decided to remove it. It doesn't affect my test, so it seems to be OK. I still just return false at the end. https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:196: DCHECK(!script_argument.empty()); On 2014/07/08 04:25:13, Dan Beam wrote: > ^ wouldn't this fail in the case of kWaitingForFinalMessage? It should have, but it wasn't. I changed it to ASSERT_TRUE and now it works. I have added a return after EndLoop() to account for this. https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:218: observer_ = observer; On 2014/07/08 04:25:13, Dan Beam wrote: > nit: > > explicit UIDoneLoadingMessageHandler(PrintPreviewObserver* observer) > : observer_(observer) {} Dunno why I changed it to that. Done. https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:256: PrintPreviewObserver* observer_; On 2014/07/08 04:25:13, Dan Beam wrote: > nit: > PrintPreviewObserver* observer_ const; I'm not sure I can make observer_ const because the message handler calls ManipulatePreviewSettings, which isn't const and can't be. So there is a problem with const correctness here. https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:904: this.saveAsPdfForTest_(); // No parameters needed. On 2014/07/08 04:25:14, Dan Beam wrote: > nit: > this.saveAsPdfForTest_(); // No parameters needed. > ^^ 2 \s Done. https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:951: * if false: landscape On 2014/07/08 04:25:14, Dan Beam wrote: > nit: . at end Done. https://codereview.chromium.org/335583004/diff/490001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:958: '.layout-settings-landscape-radio'); On 2014/07/08 17:21:35, Aleksey Shlyapnikov wrote: > On 2014/07/08 04:25:13, Dan Beam wrote: > > nit: > > var mode = portrait ? 'portrait' : 'landscape'; > > var element = document.querySelector('.layout-settings-' + mode + '-radio'); > > Please keep it as it is, code search won't find these classes if you follow the > advice. You can save one line this way: > > var element = document.querySelector(portrait ? > '.layout-settings-portrait-radio' : > '.layout-settings-landscape-radio'); > Done. https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:417: // TODO(ivandavid): Implement correct overflow detection. I got rid of the old overflow detection because it wasn't correct. I'll redo it later.
https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:256: PrintPreviewObserver* observer_; On 2014/07/08 23:24:21, ivandavid wrote: > On 2014/07/08 04:25:13, Dan Beam wrote: > > nit: > > PrintPreviewObserver* observer_ const; > I'm not sure I can make observer_ const because the message handler calls > ManipulatePreviewSettings, which isn't const and can't be. So there is a problem > with const correctness here. PrintPreviewObserver* observer_ const; means that the pointer *itself* doesn't change (e.g. observer_ = NULL; inside UIDoneLoadingMessageHandler wouldn't compile). const PrintPreviewObserver* observer_; means observer_->CanOnlyCallConstMethods().
https://codereview.chromium.org/335583004/diff/490001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/335583004/diff/490001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:315: On 2014/07/08 04:25:14, Dan Beam wrote: > what does the return value mean? that the call succeeded or failed? ping https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:144: bool OnMessageReceived(const IPC::Message& message) OVERRIDE { bool handled = true; https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:148: IPC_MESSAGE_UNHANDLED() IPC_MESSAGE_UNHANDLED(handled = false) https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:150: return false; return handled; https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:286: // the web ui is destroyed. wrap as close to 80 cols as possible (everywhere) https://codereview.chromium.org/335583004/diff/510001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/335583004/diff/510001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:904: this.saveAsPdfForTest_(); // No parameters . at end (just like everywhere else and what you had before and what I suggested in code review...) https://codereview.chromium.org/335583004/diff/510001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/335583004/diff/510001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3396: return success; if (!doc) return false; bool success = FPDF_GetPageSizeByIndex(doc, index, width, height) != 0;
While there were some notes about the javascript and pdf code, I will handle those notes in their respective issues. I will only handle notes about print_preview_pdf_generated_browsertest.cc here and will look into maybe splitting that one into a new CL, it will be difficult since it depends on so much, but I'll see what I can do. https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:256: PrintPreviewObserver* observer_; On 2014/07/09 01:56:32, Dan Beam wrote: > On 2014/07/08 23:24:21, ivandavid wrote: > > On 2014/07/08 04:25:13, Dan Beam wrote: > > > nit: > > > PrintPreviewObserver* observer_ const; > > I'm not sure I can make observer_ const because the message handler calls > > ManipulatePreviewSettings, which isn't const and can't be. So there is a > problem > > with const correctness here. > > PrintPreviewObserver* observer_ const; > > means that the pointer *itself* doesn't change (e.g. observer_ = NULL; inside > UIDoneLoadingMessageHandler wouldn't compile). > > const PrintPreviewObserver* observer_; > > means observer_->CanOnlyCallConstMethods(). Ahh. I see. I think what we want is this however, PrintPreviewObserver* const observer_; The one you posted doesn't seem to compile, however what I wrote has the behavior you described. https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:148: IPC_MESSAGE_UNHANDLED() On 2014/07/09 02:08:32, Dan Beam wrote: > IPC_MESSAGE_UNHANDLED(handled = false) I just removed this line as it was never needed in the first place. This function should always return false, no exceptions. This is because if it returns true when the page count is received, the OnMessageReceived function in print_preview_message_handler.cc never handles this message (I tested this to make sure). This then causes the test to not progress. Since it should always return false, there is no need to have that macro there, so I just removed it. https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:286: // the web ui is destroyed. On 2014/07/09 02:08:32, Dan Beam wrote: > wrap as close to 80 cols as possible (everywhere) I did my best to do this. Hopefully I caught most of it. https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:286: // the web ui is destroyed. On 2014/07/09 02:08:32, Dan Beam wrote: > wrap as close to 80 cols as possible (everywhere) Done. https://codereview.chromium.org/335583004/diff/530001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/530001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:410: // TODO(ivandavid): Implement correct overflow detection. Need to check I'll do this in the next patch set. I'll see if I can find something in the chromium code that can do it for me.
https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/490001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:256: PrintPreviewObserver* observer_; On 2014/07/09 19:58:21, ivandavid wrote: > On 2014/07/09 01:56:32, Dan Beam wrote: > > On 2014/07/08 23:24:21, ivandavid wrote: > > > On 2014/07/08 04:25:13, Dan Beam wrote: > > > > nit: > > > > PrintPreviewObserver* observer_ const; > > > I'm not sure I can make observer_ const because the message handler calls > > > ManipulatePreviewSettings, which isn't const and can't be. So there is a > > problem > > > with const correctness here. > > > > PrintPreviewObserver* observer_ const; > > > > means that the pointer *itself* doesn't change (e.g. observer_ = NULL; inside > > UIDoneLoadingMessageHandler wouldn't compile). > > > > const PrintPreviewObserver* observer_; > > > > means observer_->CanOnlyCallConstMethods(). > Ahh. I see. I think what we want is this however, > PrintPreviewObserver* const observer_; > > The one you posted doesn't seem to compile, however what I wrote has the > behavior you described. yeah, my bad, ^ that one (sorry if this took a while to find out yourself...) https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right): https://codereview.chromium.org/335583004/diff/510001/chrome/browser/printing... chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:148: IPC_MESSAGE_UNHANDLED() On 2014/07/09 19:58:22, ivandavid wrote: > On 2014/07/09 02:08:32, Dan Beam wrote: > > IPC_MESSAGE_UNHANDLED(handled = false) > I just removed this line as it was never needed in the first place. This > function should always return false, no exceptions. This is because if it > returns true when the page count is received, the OnMessageReceived function in > print_preview_message_handler.cc never handles this message (I tested this to > make sure). This then causes the test to not progress. Since it should always > return false, there is no need to have that macro there, so I just removed it. ah, ok, if we don't to handle -- great, this totally works
Closing this issue since this has been split into 3 pieces. |
