MD Settings: Adding C++ support for editing startup URLs.

chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc

Index: chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc
diff --git a/chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc b/chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc
index 17342b6645927c8ab392abe015bd099380acb116..53632f817b7fbc0930f4bc7b5b89a452cf25b84a 100644
--- a/chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc
+++ b/chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc
@@ -30,6 +30,9 @@ void StartupPagesHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback("addStartupPage",
       base::Bind(&StartupPagesHandler::HandleAddStartupPage,
                  base::Unretained(this)));
+  web_ui()->RegisterMessageCallback("editStartupPage",
+      base::Bind(&StartupPagesHandler::HandleEditStartupPage,
+                 base::Unretained(this)));
   web_ui()->RegisterMessageCallback("onStartupPrefsPageLoad",
       base::Bind(&StartupPagesHandler::HandleOnStartupPrefsPageLoad,
                  base::Unretained(this)));
@@ -105,6 +108,33 @@ void StartupPagesHandler::HandleAddStartupPage(const base::ListValue* args) {
   ResolveJavascriptCallback(*callback_id, base::FundamentalValue(true));
 }
 
+void StartupPagesHandler::HandleEditStartupPage(const base::ListValue* args) {
+  CHECK_EQ(args->GetSize(), 3U);
+  const base::Value* callback_id;
+  CHECK(args->Get(0, &callback_id));
+  int index;

[Dan Beam, 2016/04/12 04:37:47]

all numbers in javascript are doubles

i honestly don't really remember why we do this, but almost everywhere else we
do

double index_double;
CHECK(args->GetDouble(1, &index_double));
int index = static_cast<int>(index_double);

i think it only matters in the case that an argument is bigger than INT_MAX?

[dpapad, 2016/04/12 17:52:54]

This logic is mostly copied from the equivalent code in options/
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
which is using GetInteger() instead of GetDouble(). Looking at the GetInteger()
implementation
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&l=9...
and
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.ccl=203
it seems that the CHCEK() at line 116 should fail if there was any problem with
extracting an integer from whatever value was passed by JS.

I prefer sticking to this approach that already works for options/ than changing
this without having a full understanding of why GetDouble() + static_cast is
better.

[Dan Beam, 2016/04/12 18:08:53]

Acknowledged.

+  CHECK(args->GetInteger(1, &index));
+
+  if (index < 0 || index > startup_custom_pages_table_model_.RowCount()) {
+    RejectJavascriptCallback(*callback_id, *base::Value::CreateNullValue());
+    NOTREACHED();
+    return;
+  }
+
+  std::string url_string;
+  CHECK(args->GetString(2, &url_string));
+
+  GURL fixed_url;
+  if (settings_utils::FixupAndValidateStartupPage(url_string, &fixed_url)) {
+    std::vector<GURL> urls = startup_custom_pages_table_model_.GetURLs();
+    urls[index] = fixed_url;
+    startup_custom_pages_table_model_.SetURLs(urls);
+    ResolveJavascriptCallback(*callback_id, base::FundamentalValue(true));
+  } else {
+    ResolveJavascriptCallback(*callback_id, base::FundamentalValue(false));
+  }
+}

[Dan Beam, 2016/04/12 04:39:13]

it might be worthwhile to test any of these callbacks in C++ by injecting a test
version of web_ui() and scraping the CallJavascriptFunction() calls, but I
didn't do it so I wont make you (I'll just leave this here as a totally optional
suggestion)

[dpapad, 2016/04/12 17:52:54]

Agreed. C++ test coverage would be nice for this (and all other C++ handlers
too). After scanning for '*_unittest.cc' in this folder I realize we have an
imbalance between test coverage for C++ VS JS. A positive way to interpret this
imbalance is that our work to make writing JS tests easier has really worked.

I'll defer C++ tests for this handler for some time in the near future.

[Dan Beam, 2016/04/12 18:08:53]

Acknowledged.

+
 void StartupPagesHandler::HandleOnStartupPrefsPageLoad(
     const base::ListValue* args) {
   startup_custom_pages_table_model_.SetObserver(this);