Print Preview: Making margin selection sticky (part 2/2)

chrome/browser/ui/webui/print_preview_handler.cc

Index: chrome/browser/ui/webui/print_preview_handler.cc
diff --git a/chrome/browser/ui/webui/print_preview_handler.cc b/chrome/browser/ui/webui/print_preview_handler.cc
index 66e4aa3f243d49b59090e79b268394955acda618..c135a54781516258ae06f4b938b493ba937c961d 100644
--- a/chrome/browser/ui/webui/print_preview_handler.cc
+++ b/chrome/browser/ui/webui/print_preview_handler.cc
@@ -50,6 +50,7 @@
 #include "printing/metafile.h"
 #include "printing/metafile_impl.h"
 #include "printing/page_range.h"
+#include "printing/page_size_margins.h"
 #include "printing/print_settings.h"
 #include "unicode/ulocdata.h"
 
@@ -206,6 +207,8 @@ printing::ColorModels PrintPreviewHandler::last_used_color_model_ =
     printing::UNKNOWN_COLOR_MODEL;
 printing::MarginType PrintPreviewHandler::last_used_margins_type_ =
     printing::DEFAULT_MARGINS;
+printing::PageSizeMargins*
+    PrintPreviewHandler::last_used_page_size_margins_ = NULL;
 
 PrintPreviewHandler::PrintPreviewHandler()
     : print_backend_(printing::PrintBackend::CreateInstance(NULL)),
@@ -271,6 +274,9 @@ void PrintPreviewHandler::RegisterMessages() {
       base::Bind(
           &PrintPreviewHandler::HandleGetNumberFormatAndMeasurementSystem,
           base::Unretained(this)));
+  web_ui_->RegisterMessageCallback("getLastUsedMarginSettings",
+      base::Bind(&PrintPreviewHandler::HandleGetLastUsedMarginSettings,
+                 base::Unretained(this)));
 }
 
 TabContentsWrapper* PrintPreviewHandler::preview_tab_wrapper() const {
@@ -401,6 +407,11 @@ void PrintPreviewHandler::HandlePrint(const ListValue* args) {
   if (!settings->GetInteger(printing::kSettingMarginsType, &margin_type))
     margin_type = printing::DEFAULT_MARGINS;
   last_used_margins_type_ = static_cast<printing::MarginType>(margin_type);

[vandebo (ex-Chrome), 2011/11/03 18:07:27]

I think we don't want to do this if the source was a PDF...

+  if (last_used_margins_type_ == printing::CUSTOM_MARGINS) {
+    if (!last_used_page_size_margins_)
+      last_used_page_size_margins_ = new printing::PageSizeMargins();
+    getCustomMarginsFromJobSettings(*settings, last_used_page_size_margins_);
+  }
 
   bool print_to_pdf = false;
   settings->GetBoolean(printing::kSettingPrintToPDF, &print_to_pdf);
@@ -641,6 +652,29 @@ void PrintPreviewHandler::HandleGetNumberFormatAndMeasurementSystem(
       system);
 }
 
+void PrintPreviewHandler::HandleGetLastUsedMarginSettings(
+    const ListValue* /*args*/) {
+  base::FundamentalValue margins_type(
+      PrintPreviewHandler::last_used_margins_type_);
+  if (last_used_page_size_margins_) {

[kmadhusu, 2011/11/03 17:35:38]

You need to check the margins_type_ along with last_used_page_size_margins_.

Consider the scenario:

UserA initiates a print job with custom margins (3,4,5,6). When we come to
HandlePrint() we store these information in last_used_page_size_margins_ object.

After the print job is complete, UserA previews another webpage, since the last
used margin type is custom, we send the custom margin values to the UI. Lets say
the UserA reset the margins to Default and initiate the print job. 

In line 410, you don't override the last_used_page_size_margins_ object because
the current margin type is Default (and not custom).

UserA requests to preview another tab. Now, the user gets the custom margin
setting values (which is not the correct last used margin type and value).

Either delete the last_used_page_size_margins_ object whenever you see a
non-custom margin type or add a condition here to check whether the
last_used_margins_type_ is custom or not. 

[vandebo (ex-Chrome), 2011/11/03 18:07:27]

But the margin type is set correctly, if they select custom margins again, we
should give them the custom margins they used last, so we always have to send
it.

[kmadhusu, 2011/11/03 18:28:38]

Then we should change "last_used_page_size_margins_" =>
"last_used_custom_margin_values_". 

[dpapad, 2011/11/03 18:48:27]

Thanks for catching this. One correction to the scenario though, the user would
get the correct margin type value (Default) but would also get 4 extra unneeded
values.

Added a check here and also added a DCHECK for last_used_page_size_margins_ (it
has to be non-null if Custom).

+    base::DictionaryValue page_size_margins;
+    page_size_margins.SetDouble(printing::kSettingMarginTop,
+                                last_used_page_size_margins_->margin_top);
+    page_size_margins.SetDouble(printing::kSettingMarginBottom,
+                                last_used_page_size_margins_->margin_bottom);
+    page_size_margins.SetDouble(printing::kSettingMarginLeft,
+                                last_used_page_size_margins_->margin_left);
+    page_size_margins.SetDouble(printing::kSettingMarginRight,
+                                last_used_page_size_margins_->margin_right);
+    web_ui_->CallJavascriptFunction("print_preview.setLastUsedMargins",
+                                    margins_type,
+                                    page_size_margins);
+  } else {
+    web_ui_->CallJavascriptFunction("print_preview.setLastUsedMargins",
+                                    margins_type);
+  }
+}
+
 void PrintPreviewHandler::ActivateInitiatorTabAndClosePreviewTab() {
   TabContentsWrapper* initiator_tab = GetInitiatorTab();
   if (initiator_tab) {
@@ -660,12 +694,8 @@ void PrintPreviewHandler::SendPrinterCapabilities(
 void PrintPreviewHandler::SendDefaultPrinter(
     const StringValue& default_printer,
     const StringValue& cloud_print_data) {
-  base::FundamentalValue margins_type(
-      PrintPreviewHandler::last_used_margins_type_);
-  web_ui_->CallJavascriptFunction("setDefaultPrinter",
-                                  default_printer,
-                                  cloud_print_data,
-                                  margins_type);
+    web_ui_->CallJavascriptFunction(
+        "setDefaultPrinter", default_printer, cloud_print_data);
 }
 
 void PrintPreviewHandler::SetupPrinterList(const ListValue& printers) {