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

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/settings/settings_startup_pages_handler.h"
 
 #include <string>
 #include <vector>
 
 #include "chrome/browser/prefs/session_startup_pref.h"
@@ skipping 12 matching lines @@
 StartupPagesHandler::~StartupPagesHandler() {
 }
 
 void StartupPagesHandler::RegisterMessages() {
   if (Profile::FromWebUI(web_ui())->IsOffTheRecord())
     return;
 
   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)));
   web_ui()->RegisterMessageCallback("removeStartupPage",
       base::Bind(&StartupPagesHandler::HandleRemoveStartupPage,
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback("setStartupPagesToCurrentPages",
       base::Bind(&StartupPagesHandler::HandleSetStartupPagesToCurrentPages,
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback("validateStartupPage",
@@ skipping 55 matching lines @@
   int row_count = startup_custom_pages_table_model_.RowCount();
   int index;
   if (!args->GetInteger(1, &index) || index > row_count)
     index = row_count;
 
   startup_custom_pages_table_model_.Add(index, url);
   SaveStartupPagesPref();
   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);
 
   PrefService* prefService = Profile::FromWebUI(web_ui())->GetPrefs();
   SessionStartupPref pref = SessionStartupPref::GetStartupPref(
       prefService);
   startup_custom_pages_table_model_.SetURLs(pref.urls);
 
   if (pref.urls.empty())
@@ skipping 61 matching lines @@
 
 void StartupPagesHandler::UpdateStartupPages() {
   const SessionStartupPref startup_pref = SessionStartupPref::GetStartupPref(
       Profile::FromWebUI(web_ui())->GetPrefs());
   startup_custom_pages_table_model_.SetURLs(startup_pref.urls);
   // The change will go to the JS code in the
   // StartupPagesHandler::OnModelChanged() method.
 }
 
 }  // namespace settings