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

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

BUG=601514
Committed: https://crrev.com/413b72afce529a79146d60ad410a8600f8cd3098
Cr-Commit-Position: refs/heads/master@{#386767}

[dpapad, 2016-04-09 01:10:49]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-04-11 23:07:03]

Description was changed from

==========
MD Settings: Startup URLs, adding C++ support for editing.

BUG=601514
==========

to

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

BUG=601514
==========

[dpapad, 2016-04-11 23:08:03]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

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

lgtm

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:115: int
index;
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?

[Dan Beam, 2016-04-12 04:39:14]

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:136: }
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:55]

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:115: int
index;
On 2016/04/12 at 04:37:47, Dan Beam wrote:
> 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?

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.

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:136: }
On 2016/04/12 at 04:39:13, Dan Beam wrote:
> 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)

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]

still lgtm

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:115: int
index;
On 2016/04/12 17:52:54, dpapad wrote:
> On 2016/04/12 at 04:37:47, Dan Beam wrote:
> > 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?
> 
> 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.

Acknowledged.

https://codereview.chromium.org/1877633002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:136: }
On 2016/04/12 17:52:54, dpapad wrote:
> On 2016/04/12 at 04:39:13, Dan Beam wrote:
> > 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)
> 
> 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.

Acknowledged.

[dpapad, 2016-04-12 18:11:44]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-04-12 18:12:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877633002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877633002/60001

[commit-bot: I haz the power, 2016-04-12 19:40:09]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-04-12 19:42:47]

Description was changed from

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

BUG=601514
==========

to

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

BUG=601514

Committed: https://crrev.com/413b72afce529a79146d60ad410a8600f8cd3098
Cr-Commit-Position: refs/heads/master@{#386767}
==========

[commit-bot: I haz the power, 2016-04-12 19:42:48]

Patchset 3 (id:??) landed as
https://crrev.com/413b72afce529a79146d60ad410a8600f8cd3098
Cr-Commit-Position: refs/heads/master@{#386767}