Option dialog DOM UI - ChromeOS System page.

Basic code structures for rewritten options dialogs as DOM UI. Also includes the initial implementation of System page in ChromeOS options.


BUG=chromium-os:
TEST=none yet, work in progress


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50117

[zel, 2010-06-16 17:31:59]

The first cut of the actual DOM UI implementation. HTML+JS still needs *tons* of
improvements on all levels and I expect to iterate on that one further. I also
want us to find the way to split its content similar to what I did to C++ part
(see below).

DOMMessageHandler instances are broken up per options 'page', so we can
develop/compile Chrome part separately from ChromeOS. The exception is
CoreOptionsHandler specialization that implements resource and JS bindings
common to all pages (i.e. preferences handling).

System page for ChromeOS is all wired up and functional (other than OS language
settings).

[Ben Goodger (Google), 2010-06-16 18:09:34]

One general comment I would like to make is that I would like to not have any
more dependencies on Browser from DOMUI objects... using browser pointers from
domuis is error and crash prone... I'd rAther we define a DOMUIDelegate
interface or some such... I will follow up later with thoughts.

[zel, 2010-06-16 18:29:27]

I have removed browser.h include from options_ui.cc since it really wasn't used.


Is there anything else that you were referring to as 'dependency on Browser'?

[James Hawkins, 2010-06-16 18:36:58]

http://codereview.chromium.org/2781005/diff/7001/8004
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/7001/8004#newcode28
chrome/browser/dom_ui/core_options_handler.cc:28: CHECK(localized_strings);
Do we have a need for CHECK over DCHECK?

http://codereview.chromium.org/2781005/diff/7001/8004#newcode75
chrome/browser/dom_ui/core_options_handler.cc:75: if (param_values->GetSize() <
3)
Can you add a constant for this value?  It's not obvious at first glance what it
represents.

http://codereview.chromium.org/2781005/diff/7001/8004#newcode92
chrome/browser/dom_ui/core_options_handler.cc:92: // Get the list of name for
prefs to build the response dicttionary.
s/dicttionary/dictionary/

http://codereview.chromium.org/2781005/diff/7001/8004#newcode103
chrome/browser/dom_ui/core_options_handler.cc:103: std::wstring pref_name;
string16 and GetAsUTF16(..), here and elsewhere.

http://codereview.chromium.org/2781005/diff/7001/8005
File chrome/browser/dom_ui/core_options_handler.h (right):

http://codereview.chromium.org/2781005/diff/7001/8005#newcode9
chrome/browser/dom_ui/core_options_handler.h:9: #include "base/values.h"
nit: base/values.h should be before chrome/browser/...

http://codereview.chromium.org/2781005/diff/7001/8005#newcode50
chrome/browser/dom_ui/core_options_handler.h:50: 
nit: Remove extra empty line.

http://codereview.chromium.org/2781005/diff/7001/8007
File chrome/browser/dom_ui/options_ui.cc (right):

http://codereview.chromium.org/2781005/diff/7001/8007#newcode66
chrome/browser/dom_ui/options_ui.cc:66: static const base::StringPiece
history_html(
history_html -> options_html?

http://codereview.chromium.org/2781005/diff/7001/8007#newcode113
chrome/browser/dom_ui/options_ui.cc:113: new
OptionsUIHTMLSource(localized_strings);
Maybe also add a comment here that OptionsUIHTMLSource takes ownership of
|localized_strings|.

http://codereview.chromium.org/2781005/diff/7001/8008
File chrome/browser/dom_ui/options_ui.h (right):

http://codereview.chromium.org/2781005/diff/7001/8008#newcode13
chrome/browser/dom_ui/options_ui.h:13: #include
"chrome/browser/cancelable_request.h"
cancelable_request.h should be above the first two headers.

http://codereview.chromium.org/2781005/diff/7001/8008#newcode23
chrome/browser/dom_ui/options_ui.h:23: explicit
OptionsUIHTMLSource(DictionaryValue* localized_strings);
You should add a comment that we take ownership of |localized_strings|.

http://codereview.chromium.org/2781005/diff/7001/8008#newcode37
chrome/browser/dom_ui/options_ui.h:37: DictionaryValue* localized_strings_;
scoped_ptr<DictionaryValue>, since I see we're deleting it in the constructor.

[Ben Goodger (Google), 2010-06-16 19:24:17]

Nope that's it...

If you find yourself needing to use Browser for some reason it'd be good to
understand what in particular you need it for. Let me know when this comes
up, Zel or James.

-Ben

On Wed, Jun 16, 2010 at 11:29 AM, <zelidrag@chromium.org> wrote:

> I have removed browser.h include from options_ui.cc since it really wasn't
> used.
>
>
> Is there anything else that you were referring to as 'dependency on
> Browser'?
>
>
> http://codereview.chromium.org/2781005/show
>

[Evan Stade, 2010-06-16 20:03:43]

mostly lgtm, just nits

http://codereview.chromium.org/2781005/diff/30001/29003
File chrome/browser/chromeos/dom_ui/system_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29003#newcode33
chrome/browser/chromeos/dom_ui/system_options_handler.cc:33:
icu::UnicodeString::fromUTF8("Pacific/Samoa")));
can you create a static struct that holds these literals and loop over it?

http://codereview.chromium.org/2781005/diff/30001/29003#newcode140
chrome/browser/chromeos/dom_ui/system_options_handler.cc:140: return
StringPrintf(hour_offset == 0 ? L"(GMT) " : (hour_offset > 0 ?
nit: can you spread this logic over a few more lines to make it more readable?

http://codereview.chromium.org/2781005/diff/30001/29004
File chrome/browser/chromeos/dom_ui/system_options_handler.h (right):

http://codereview.chromium.org/2781005/diff/30001/29004#newcode11
chrome/browser/chromeos/dom_ui/system_options_handler.h:11: #include
"unicode/timezone.h"
include fully specified paths: should this be
third_party/icu/public/i18n/unicode/timezone.h?

http://codereview.chromium.org/2781005/diff/30001/29004#newcode28
chrome/browser/chromeos/dom_ui/system_options_handler.h:28: std::wstring
GetTimezoneID(const icu::TimeZone* timezone);
these functions should probably return utf16 strings

http://codereview.chromium.org/2781005/diff/30001/29005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29005#newcode7
chrome/browser/dom_ui/core_options_handler.cc:7: #include <stdlib.h>
what is this used for?

http://codereview.chromium.org/2781005/diff/30001/29005#newcode16
chrome/browser/dom_ui/core_options_handler.cc:16: 
nit: remove this line.

http://codereview.chromium.org/2781005/diff/30001/29005#newcode102
chrome/browser/dom_ui/core_options_handler.cc:102: continue;
nit: can you put blank lines after each conditional block in this file? (as you
have done on lines 86 and 91, for example)

http://codereview.chromium.org/2781005/diff/30001/29005#newcode181
chrome/browser/dom_ui/core_options_handler.cc:181: wcstol(value_string.c_str(),
NULL, 10));
use StringToInt?

http://codereview.chromium.org/2781005/diff/30001/29006
File chrome/browser/dom_ui/core_options_handler.h (right):

http://codereview.chromium.org/2781005/diff/30001/29006#newcode33
chrome/browser/dom_ui/core_options_handler.h:33: void HandleFetchPrefs(const
Value* value);
nit: blank line after each function.

http://codereview.chromium.org/2781005/diff/30001/29008
File chrome/browser/dom_ui/options_ui.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29008#newcode83
chrome/browser/dom_ui/options_ui.cc:83:
////////////////////////////////////////////////////////////////////////////////
nit: insert blank line

http://codereview.chromium.org/2781005/diff/30001/29009
File chrome/browser/dom_ui/options_ui.h (right):

http://codereview.chromium.org/2781005/diff/30001/29009#newcode70
chrome/browser/dom_ui/options_ui.h:70: 
nit: extra line

http://codereview.chromium.org/2781005/diff/30001/29009#newcode74
chrome/browser/dom_ui/options_ui.h:74: 
even though you don't strictly need a virtual dtor here, I think we decided to
always put a virtual dtor when there is inheritance

http://codereview.chromium.org/2781005/diff/30001/29013
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/2781005/diff/30001/29013#newcode52
chrome/common/url_constants.h:52: extern const char kChromeUIAppLauncherURL[];
oi, these lists are so out of order

http://codereview.chromium.org/2781005/diff/30001/29013#newcode81
chrome/common/url_constants.h:81: extern const char kChromeUIOptionsHost[];
should this not be kept in sync with the above list?

[csilv, 2010-06-16 20:54:58]

http://codereview.chromium.org/2781005/diff/30001/29007
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29007#newcode28
chrome/browser/dom_ui/dom_ui_factory.cc:28: #include
"chrome/browser/dom_ui/options_ui.h"
options_ui should be pulled out of this OS_CHROMEOS section since it's going
into the browser as well.

[James Hawkins, 2010-06-16 20:56:22]

On 2010/06/16 20:54:58, Chris Silverberg wrote:
> http://codereview.chromium.org/2781005/diff/30001/29007
> File chrome/browser/dom_ui/dom_ui_factory.cc (right):
> 
> http://codereview.chromium.org/2781005/diff/30001/29007#newcode28
> chrome/browser/dom_ui/dom_ui_factory.cc:28: #include
> "chrome/browser/dom_ui/options_ui.h"
> options_ui should be pulled out of this OS_CHROMEOS section since it's going
> into the browser as well.

This should probably happen in your CL, since this code isn't behind a flag yet.

[csilv, 2010-06-16 20:58:36]

gotcha.  will do.

[zel, 2010-06-16 22:00:15]

http://codereview.chromium.org/2781005/diff/7001/8004
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/7001/8004#newcode28
chrome/browser/dom_ui/core_options_handler.cc:28: CHECK(localized_strings);
On 2010/06/16 18:36:58, James Hawkins wrote:
> Do we have a need for CHECK over DCHECK?

Done.

http://codereview.chromium.org/2781005/diff/7001/8004#newcode75
chrome/browser/dom_ui/core_options_handler.cc:75: if (param_values->GetSize() <
3)
On 2010/06/16 18:36:58, James Hawkins wrote:
> Can you add a constant for this value?  It's not obvious at first glance what
it
> represents.

Done.

http://codereview.chromium.org/2781005/diff/7001/8004#newcode92
chrome/browser/dom_ui/core_options_handler.cc:92: // Get the list of name for
prefs to build the response dicttionary.
On 2010/06/16 18:36:58, James Hawkins wrote:
> s/dicttionary/dictionary/

Done.

http://codereview.chromium.org/2781005/diff/7001/8004#newcode103
chrome/browser/dom_ui/core_options_handler.cc:103: std::wstring pref_name;
On 2010/06/16 18:36:58, James Hawkins wrote:
> string16 and GetAsUTF16(..), here and elsewhere.

I would be happy to do this but I am not sure I understand reasons why. Can you
please explain why would I need to convert to UTF16 here since both Value* and
Preference::FindPreference() deal with wchar_t* or std::wstrings?

http://codereview.chromium.org/2781005/diff/7001/8005
File chrome/browser/dom_ui/core_options_handler.h (right):

http://codereview.chromium.org/2781005/diff/7001/8005#newcode9
chrome/browser/dom_ui/core_options_handler.h:9: #include "base/values.h"
On 2010/06/16 18:36:58, James Hawkins wrote:
> nit: base/values.h should be before chrome/browser/...

Done.

http://codereview.chromium.org/2781005/diff/7001/8005#newcode50
chrome/browser/dom_ui/core_options_handler.h:50: 
On 2010/06/16 18:36:58, James Hawkins wrote:
> nit: Remove extra empty line.

Done.

http://codereview.chromium.org/2781005/diff/7001/8007
File chrome/browser/dom_ui/options_ui.cc (right):

http://codereview.chromium.org/2781005/diff/7001/8007#newcode66
chrome/browser/dom_ui/options_ui.cc:66: static const base::StringPiece
history_html(
On 2010/06/16 18:36:58, James Hawkins wrote:
> history_html -> options_html?

Done.

http://codereview.chromium.org/2781005/diff/7001/8007#newcode113
chrome/browser/dom_ui/options_ui.cc:113: new
OptionsUIHTMLSource(localized_strings);
On 2010/06/16 18:36:58, James Hawkins wrote:
> Maybe also add a comment here that OptionsUIHTMLSource takes ownership of
> |localized_strings|.

Done.

http://codereview.chromium.org/2781005/diff/7001/8008
File chrome/browser/dom_ui/options_ui.h (right):

http://codereview.chromium.org/2781005/diff/7001/8008#newcode13
chrome/browser/dom_ui/options_ui.h:13: #include
"chrome/browser/cancelable_request.h"
On 2010/06/16 18:36:58, James Hawkins wrote:
> cancelable_request.h should be above the first two headers.

Actually, it wasn't even needed here. Done.

http://codereview.chromium.org/2781005/diff/7001/8008#newcode23
chrome/browser/dom_ui/options_ui.h:23: explicit
OptionsUIHTMLSource(DictionaryValue* localized_strings);
On 2010/06/16 18:36:58, James Hawkins wrote:
> You should add a comment that we take ownership of |localized_strings|.

Done.

http://codereview.chromium.org/2781005/diff/7001/8008#newcode37
chrome/browser/dom_ui/options_ui.h:37: DictionaryValue* localized_strings_;
On 2010/06/16 18:36:58, James Hawkins wrote:
> scoped_ptr<DictionaryValue>, since I see we're deleting it in the constructor.

Done.

http://codereview.chromium.org/2781005/diff/30001/29003
File chrome/browser/chromeos/dom_ui/system_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29003#newcode33
chrome/browser/chromeos/dom_ui/system_options_handler.cc:33:
icu::UnicodeString::fromUTF8("Pacific/Samoa")));
On 2010/06/16 20:03:43, Evan Stade wrote:
> can you create a static struct that holds these literals and loop over it?

Sure, this was cut and paste from the old codebase. Done.

http://codereview.chromium.org/2781005/diff/30001/29003#newcode140
chrome/browser/chromeos/dom_ui/system_options_handler.cc:140: return
StringPrintf(hour_offset == 0 ? L"(GMT) " : (hour_offset > 0 ?
On 2010/06/16 20:03:43, Evan Stade wrote:
> nit: can you spread this logic over a few more lines to make it more readable?

Done.

http://codereview.chromium.org/2781005/diff/30001/29004
File chrome/browser/chromeos/dom_ui/system_options_handler.h (right):

http://codereview.chromium.org/2781005/diff/30001/29004#newcode11
chrome/browser/chromeos/dom_ui/system_options_handler.h:11: #include
"unicode/timezone.h"
On 2010/06/16 20:03:43, Evan Stade wrote:
> include fully specified paths: should this be
> third_party/icu/public/i18n/unicode/timezone.h?

Done.

http://codereview.chromium.org/2781005/diff/30001/29004#newcode28
chrome/browser/chromeos/dom_ui/system_options_handler.h:28: std::wstring
GetTimezoneID(const icu::TimeZone* timezone);
On 2010/06/16 20:03:43, Evan Stade wrote:
> these functions should probably return utf16 strings

I use them to create Value* elements, so wstring is really what I am looking
for.

http://codereview.chromium.org/2781005/diff/30001/29005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29005#newcode7
chrome/browser/dom_ui/core_options_handler.cc:7: #include <stdlib.h>
On 2010/06/16 20:03:43, Evan Stade wrote:
> what is this used for?

wcstol

http://codereview.chromium.org/2781005/diff/30001/29005#newcode16
chrome/browser/dom_ui/core_options_handler.cc:16: 
On 2010/06/16 20:03:43, Evan Stade wrote:
> nit: remove this line.

Done.

http://codereview.chromium.org/2781005/diff/30001/29005#newcode181
chrome/browser/dom_ui/core_options_handler.cc:181: wcstol(value_string.c_str(),
NULL, 10));
On 2010/06/16 20:03:43, Evan Stade wrote:
> use StringToInt?

There is no StringToInt(const std::wstring&) version of it. Do you feel I should
add it with this CL?

http://codereview.chromium.org/2781005/diff/30001/29006
File chrome/browser/dom_ui/core_options_handler.h (right):

http://codereview.chromium.org/2781005/diff/30001/29006#newcode33
chrome/browser/dom_ui/core_options_handler.h:33: void HandleFetchPrefs(const
Value* value);
On 2010/06/16 20:03:43, Evan Stade wrote:
> nit: blank line after each function.

Done where it made sense.

http://codereview.chromium.org/2781005/diff/30001/29008
File chrome/browser/dom_ui/options_ui.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29008#newcode83
chrome/browser/dom_ui/options_ui.cc:83:
////////////////////////////////////////////////////////////////////////////////
On 2010/06/16 20:03:43, Evan Stade wrote:
> nit: insert blank line

Done.

http://codereview.chromium.org/2781005/diff/30001/29009
File chrome/browser/dom_ui/options_ui.h (right):

http://codereview.chromium.org/2781005/diff/30001/29009#newcode70
chrome/browser/dom_ui/options_ui.h:70: 
On 2010/06/16 20:03:43, Evan Stade wrote:
> nit: extra line

Done.

http://codereview.chromium.org/2781005/diff/30001/29009#newcode74
chrome/browser/dom_ui/options_ui.h:74: 
On 2010/06/16 20:03:43, Evan Stade wrote:
> even though you don't strictly need a virtual dtor here, I think we decided to
> always put a virtual dtor when there is inheritance

Done. Are we doing this also for 'final' classes like this one?

http://codereview.chromium.org/2781005/diff/30001/29013
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/2781005/diff/30001/29013#newcode52
chrome/common/url_constants.h:52: extern const char kChromeUIAppLauncherURL[];
On 2010/06/16 20:03:43, Evan Stade wrote:
> oi, these lists are so out of order

Done.

http://codereview.chromium.org/2781005/diff/30001/29013#newcode81
chrome/common/url_constants.h:81: extern const char kChromeUIOptionsHost[];
On 2010/06/16 20:03:43, Evan Stade wrote:
> should this not be kept in sync with the above list?

I have reordered these, not sure what else you consider as 'in sync'

[James Hawkins, 2010-06-16 22:09:44]

Sorry I missed the ScopedVector the first time.

http://codereview.chromium.org/2781005/diff/7001/8004
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/7001/8004#newcode103
chrome/browser/dom_ui/core_options_handler.cc:103: std::wstring pref_name;
On 2010/06/16 22:00:15, zel wrote:
> On 2010/06/16 18:36:58, James Hawkins wrote:
> > string16 and GetAsUTF16(..), here and elsewhere.
> 
> I would be happy to do this but I am not sure I understand reasons why. Can
you
> please explain why would I need to convert to UTF16 here since both Value* and
> Preference::FindPreference() deal with wchar_t* or std::wstrings?

Ah yes, Preference::* should really be modified to take string16 instead of
wstring, but that's outside the scope of this change.  Unfortunate, but ignore
my comment.

http://codereview.chromium.org/2781005/diff/44001/45003
File chrome/browser/chromeos/dom_ui/system_options_handler.h (right):

http://codereview.chromium.org/2781005/diff/44001/45003#newcode34
chrome/browser/chromeos/dom_ui/system_options_handler.h:34:
std::vector<icu::TimeZone*> timezones_;
I see we're deleting the elements of |timezones_| in the destructor. We should
use a ScopedVector here.

[Evan Stade, 2010-06-16 22:30:59]

http://codereview.chromium.org/2781005/diff/30001/29004
File chrome/browser/chromeos/dom_ui/system_options_handler.h (right):

http://codereview.chromium.org/2781005/diff/30001/29004#newcode28
chrome/browser/chromeos/dom_ui/system_options_handler.h:28: std::wstring
GetTimezoneID(const icu::TimeZone* timezone);
There is almost never a good reason to use wstring in non-windows code (although
there are many examples of this in the codebase, most are wrong). That Value
paths are specified in wstring is regrettable. So GetTimezoneID "should" be
wstring, I guess, but GetTimezoneName should be utf16.

If you return a wide string, you have to go from utf16->wide->utf8 (the internal
format for StringValue). Why do we need this middle conversion? Returning UTF8
would also work, and might be easier since you need to use StringPrintf.

http://codereview.chromium.org/2781005/diff/30001/29005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29005#newcode181
chrome/browser/dom_ui/core_options_handler.cc:181: wcstol(value_string.c_str(),
NULL, 10));
On 2010/06/16 22:00:15, zel wrote:
> On 2010/06/16 20:03:43, Evan Stade wrote:
> > use StringToInt?
> 
> There is no StringToInt(const std::wstring&) version of it. Do you feel I
should
> add it with this CL?

value_string should be std::string.

http://codereview.chromium.org/2781005/diff/30001/29013
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/2781005/diff/30001/29013#newcode81
chrome/common/url_constants.h:81: extern const char kChromeUIOptionsHost[];
On 2010/06/16 22:00:15, zel wrote:
> On 2010/06/16 20:03:43, Evan Stade wrote:
> > should this not be kept in sync with the above list?
> 
> I have reordered these, not sure what else you consider as 'in sync'

I guess I thought the 'in sync' comment at the top of the list meant that for
each kChromeUIFooHost there should be a corresponding kChromeUIFooURL

http://codereview.chromium.org/2781005/diff/44001/45002
File chrome/browser/chromeos/dom_ui/system_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/44001/45002#newcode22
chrome/browser/chromeos/dom_ui/system_options_handler.cc:22: const char*
kTimeZonesUtf8[] = {
static?

also, perhaps the comment below belongs above this list now.

http://codereview.chromium.org/2781005/diff/44001/45002#newcode46
chrome/browser/chromeos/dom_ui/system_options_handler.cc:46:
"Pacific/Auckland"};
nit: space before }

http://codereview.chromium.org/2781005/diff/44001/45002#newcode47
chrome/browser/chromeos/dom_ui/system_options_handler.cc:47: 
nit: extra line

http://codereview.chromium.org/2781005/diff/44001/45002#newcode58
chrome/browser/chromeos/dom_ui/system_options_handler.cc:58: for (size_t i = 0;
i < sizeof(kTimeZonesUtf8)/sizeof(*kTimeZonesUtf8); i++) {
nit: use arraysize() from base/basictypes.h

http://codereview.chromium.org/2781005/diff/44001/45002#newcode127
chrome/browser/chromeos/dom_ui/system_options_handler.cc:127: format =
L"(GMT+%d) ";
nit: %+d instead of this block.

[dhg, 2010-06-16 23:24:00]

drive by

http://codereview.chromium.org/2781005/diff/44001/45009
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2781005/diff/44001/45009#newcode16
chrome/browser/resources/options.html:16: return eval("dict."+name)
semicolon?

http://codereview.chromium.org/2781005/diff/44001/45009#newcode52
chrome/browser/resources/options.html:52: nav.class = "navbar_item
navbar_item_selected"
Should we set the page's hash to the currently selected page, so if they do a
reload, they end up where they started?

http://codereview.chromium.org/2781005/diff/44001/45009#newcode80
chrome/browser/resources/options.html:80: console.log(templateData.timezoneMap);
all these console.logs, do we want them in the version that goes out?

http://codereview.chromium.org/2781005/diff/44001/45009#newcode111
chrome/browser/resources/options.html:111: if (control.id == "tapToClickChk") {
nit: use single quotes. all throughout this javascript.

http://codereview.chromium.org/2781005/diff/44001/45009#newcode181
chrome/browser/resources/options.html:181: padding-left:10px;
nit: space after colon

[zel, 2010-06-16 23:37:19]

http://codereview.chromium.org/2781005/diff/30001/29004
File chrome/browser/chromeos/dom_ui/system_options_handler.h (right):

http://codereview.chromium.org/2781005/diff/30001/29004#newcode28
chrome/browser/chromeos/dom_ui/system_options_handler.h:28: std::wstring
GetTimezoneID(const icu::TimeZone* timezone);
On 2010/06/16 22:30:59, Evan Stade wrote:
> There is almost never a good reason to use wstring in non-windows code
(although
> there are many examples of this in the codebase, most are wrong). That Value
> paths are specified in wstring is regrettable. So GetTimezoneID "should" be
> wstring, I guess, but GetTimezoneName should be utf16.
> 
> If you return a wide string, you have to go from utf16->wide->utf8 (the
internal
> format for StringValue). Why do we need this middle conversion? Returning UTF8
> would also work, and might be easier since you need to use StringPrintf.
> 

I return UTF8 from GetTimezoneName now. Done.

http://codereview.chromium.org/2781005/diff/30001/29005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29005#newcode181
chrome/browser/dom_ui/core_options_handler.cc:181: wcstol(value_string.c_str(),
NULL, 10));
On 2010/06/16 22:30:59, Evan Stade wrote:
> On 2010/06/16 22:00:15, zel wrote:
> > On 2010/06/16 20:03:43, Evan Stade wrote:
> > > use StringToInt?
> > 
> > There is no StringToInt(const std::wstring&) version of it. Do you feel I
> should
> > add it with this CL?
> 
> value_string should be std::string.

Done. Except that I still need to convert it to wstring to to call
PrefService::SetString(). So we will avoid conversion is most cases, but use
extra mem when we don't.

http://codereview.chromium.org/2781005/diff/30001/29013
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/2781005/diff/30001/29013#newcode81
chrome/common/url_constants.h:81: extern const char kChromeUIOptionsHost[];
On 2010/06/16 22:30:59, Evan Stade wrote:
> On 2010/06/16 22:00:15, zel wrote:
> > On 2010/06/16 20:03:43, Evan Stade wrote:
> > > should this not be kept in sync with the above list?
> > 
> > I have reordered these, not sure what else you consider as 'in sync'
> 
> I guess I thought the 'in sync' comment at the top of the list meant that for
> each kChromeUIFooHost there should be a corresponding kChromeUIFooURL

OK, I have added kChromeUIOptionsURL. I am sure we will use that one but I am
not sure if any other make sense to be added there as well.

http://codereview.chromium.org/2781005/diff/44001/45002
File chrome/browser/chromeos/dom_ui/system_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/44001/45002#newcode22
chrome/browser/chromeos/dom_ui/system_options_handler.cc:22: const char*
kTimeZonesUtf8[] = {
On 2010/06/16 22:30:59, Evan Stade wrote:
> static?
> 
> also, perhaps the comment below belongs above this list now.

Done.

http://codereview.chromium.org/2781005/diff/44001/45002#newcode46
chrome/browser/chromeos/dom_ui/system_options_handler.cc:46:
"Pacific/Auckland"};
On 2010/06/16 22:30:59, Evan Stade wrote:
> nit: space before }

Done.

http://codereview.chromium.org/2781005/diff/44001/45002#newcode47
chrome/browser/chromeos/dom_ui/system_options_handler.cc:47: 
On 2010/06/16 22:30:59, Evan Stade wrote:
> nit: extra line

Done.

http://codereview.chromium.org/2781005/diff/44001/45002#newcode58
chrome/browser/chromeos/dom_ui/system_options_handler.cc:58: for (size_t i = 0;
i < sizeof(kTimeZonesUtf8)/sizeof(*kTimeZonesUtf8); i++) {
On 2010/06/16 22:30:59, Evan Stade wrote:
> nit: use arraysize() from base/basictypes.h

Done.

http://codereview.chromium.org/2781005/diff/44001/45002#newcode127
chrome/browser/chromeos/dom_ui/system_options_handler.cc:127: format =
L"(GMT+%d) ";
On 2010/06/16 22:30:59, Evan Stade wrote:
> nit: %+d instead of this block.

Done.

[Evan Stade, 2010-06-16 23:44:59]

LGTM!

http://codereview.chromium.org/2781005/diff/30001/29005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2781005/diff/30001/29005#newcode181
chrome/browser/dom_ui/core_options_handler.cc:181: wcstol(value_string.c_str(),
NULL, 10));
On 2010/06/16 23:37:20, zel wrote:
> On 2010/06/16 22:30:59, Evan Stade wrote:
> > On 2010/06/16 22:00:15, zel wrote:
> > > On 2010/06/16 20:03:43, Evan Stade wrote:
> > > > use StringToInt?
> > > 
> > > There is no StringToInt(const std::wstring&) version of it. Do you feel I
> > should
> > > add it with this CL?
> > 
> > value_string should be std::string.
> 
> Done. Except that I still need to convert it to wstring to to call
> PrefService::SetString(). So we will avoid conversion is most cases, but use
> extra mem when we don't. 

thanks. I'm more concerned about clean code than memory usage or even
performance in the case of string types.

It's unfortunate that PrefService::SetString takes a wide string, it should be
std::string, but that is of course outsdie the scope of this change.

[zel, 2010-06-17 03:39:13]

http://codereview.chromium.org/2781005/diff/44001/45009
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2781005/diff/44001/45009#newcode16
chrome/browser/resources/options.html:16: return eval("dict."+name)
On 2010/06/16 23:24:01, dhg wrote:
> semicolon?

Done.

http://codereview.chromium.org/2781005/diff/44001/45009#newcode52
chrome/browser/resources/options.html:52: nav.class = "navbar_item
navbar_item_selected"
On 2010/06/16 23:24:01, dhg wrote:
> Should we set the page's hash to the currently selected page, so if they do a
> reload, they end up where they started?

Yes, we will do that in the next CL. This page was made just to test prefs.

http://codereview.chromium.org/2781005/diff/44001/45009#newcode80
chrome/browser/resources/options.html:80: console.log(templateData.timezoneMap);
On 2010/06/16 23:24:01, dhg wrote:
> all these console.logs, do we want them in the version that goes out?

All removed.

http://codereview.chromium.org/2781005/diff/44001/45009#newcode111
chrome/browser/resources/options.html:111: if (control.id == "tapToClickChk") {
On 2010/06/16 23:24:01, dhg wrote:
> nit: use single quotes. all throughout this javascript.

Done.

[dhg, 2010-06-17 04:44:11]

javascript LGTM

On Wed, Jun 16, 2010 at 8:39 PM,  <zelidrag@chromium.org> wrote:
>
> http://codereview.chromium.org/2781005/diff/44001/45009
> File chrome/browser/resources/options.html (right):
>
> http://codereview.chromium.org/2781005/diff/44001/45009#newcode16
> chrome/browser/resources/options.html:16: return eval("dict."+name)
> On 2010/06/16 23:24:01, dhg wrote:
>>
>> semicolon?
>
> Done.
>
> http://codereview.chromium.org/2781005/diff/44001/45009#newcode52
> chrome/browser/resources/options.html:52: nav.class = "navbar_item
> navbar_item_selected"
> On 2010/06/16 23:24:01, dhg wrote:
>>
>> Should we set the page's hash to the currently selected page, so if
>
> they do a
>>
>> reload, they end up where they started?
>
> Yes, we will do that in the next CL. This page was made just to test
> prefs.
>
> http://codereview.chromium.org/2781005/diff/44001/45009#newcode80
> chrome/browser/resources/options.html:80:
> console.log(templateData.timezoneMap);
> On 2010/06/16 23:24:01, dhg wrote:
>>
>> all these console.logs, do we want them in the version that goes out?
>
> All removed.
>
> http://codereview.chromium.org/2781005/diff/44001/45009#newcode111
> chrome/browser/resources/options.html:111: if (control.id ==
> "tapToClickChk") {
> On 2010/06/16 23:24:01, dhg wrote:
>>
>> nit: use single quotes. all throughout this javascript.
>
> Done.
>
> http://codereview.chromium.org/2781005/show
>

[arv (Not doing code reviews), 2010-06-17 22:53:17]

http://codereview.chromium.org/2781005/diff/39003/10010
File chrome/browser/dom_ui/options_ui.cc (right):

http://codereview.chromium.org/2781005/diff/39003/10010#newcode93
chrome/browser/dom_ui/options_ui.cc:93: // OptionsUIContents
OptionsUI

http://codereview.chromium.org/2781005/diff/39003/10012
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2781005/diff/39003/10012#newcode16
chrome/browser/resources/options.html:16: return eval('dict.'+name);
Do not use eval

return dict[name];

http://codereview.chromium.org/2781005/diff/39003/10012#newcode45
chrome/browser/resources/options.html:45: var page = $(name+'Page');
ws around binops

http://codereview.chromium.org/2781005/diff/39003/10012#newcode48
chrome/browser/resources/options.html:48: nav.class = 'navbar_item
navbar_item_selected';
use dashes (-) for css class names and ids

http://codereview.chromium.org/2781005/diff/39003/10012#newcode54
chrome/browser/resources/options.html:54: var kTimeZone =
'settings.datetime.timezone';
No k in the name. If you want const use const

const timeZone = 'settings.datatime.timezone';

http://codereview.chromium.org/2781005/diff/39003/10012#newcode65
chrome/browser/resources/options.html:65: chrome.send("fetchPrefs",
['systemPrefsFetched', '',
Use single quotes in JS for consistence.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode75
chrome/browser/resources/options.html:75: timezoneSel = $('timezoneSel');
missing var

http://codereview.chromium.org/2781005/diff/39003/10012#newcode79
chrome/browser/resources/options.html:79: new
Option(templateData.timezoneMap[key], key, false, selected);
or

timezoneSel.appendChild(new Option(...));

http://codereview.chromium.org/2781005/diff/39003/10012#newcode99
chrome/browser/resources/options.html:99: if (control.id == 'tapToClickChk') {
Use a switch

http://codereview.chromium.org/2781005/diff/39003/10012#newcode129
chrome/browser/resources/options.html:129: .navbar_container {
Put an empty line between each rule

http://codereview.chromium.org/2781005/diff/39003/10012#newcode132
chrome/browser/resources/options.html:132: cursor:pointer;
ws after :

http://codereview.chromium.org/2781005/diff/39003/10012#newcode134
chrome/browser/resources/options.html:134: float:left;
rtl?

http://codereview.chromium.org/2781005/diff/39003/10012#newcode178
chrome/browser/resources/options.html:178: <body onload="load();"
i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize">
Long line

http://codereview.chromium.org/2781005/diff/39003/10012#newcode184
chrome/browser/resources/options.html:184: <li class="navbar_item
navbar_item_normal" id="systemPageNav" i18n-values=".innerText:systemPage"></li>
Use i18n-content="systemPage"

http://codereview.chromium.org/2781005/diff/39003/10012#newcode184
chrome/browser/resources/options.html:184: <li class="navbar_item
navbar_item_normal" id="systemPageNav" i18n-values=".innerText:systemPage"></li>
Use dashes for word seperators

s/systemPageNav/system-page-nav/

http://codereview.chromium.org/2781005/diff/39003/10012#newcode216
chrome/browser/resources/options.html:216: <input id="sensitivityRng"
type="range" min="1" max="10" class="touchSlider"
style="-webkit-appearance:slider-horizontal;" onchange="rangeChanged(this)">
move css to the style element

I don't see why you need to set the appearance here.

[zel, 2010-06-18 02:07:05]

arv, the changes from your review ended up in my new CL at:

http://codereview.chromium.org/2835009

http://codereview.chromium.org/2781005/diff/39003/10012
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2781005/diff/39003/10012#newcode16
chrome/browser/resources/options.html:16: return eval('dict.'+name);
On 2010/06/17 22:53:18, arv wrote:
> Do not use eval
> 
> return dict[name];

dict[name] actually does not work. Something funny happens with dict while it is
constructed on C++ side, key values like "one.two.three" end up converted into
something like

dict = { one : { two: { three : 'some value' } } };

that's why I ended up using eval in the first place.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode45
chrome/browser/resources/options.html:45: var page = $(name+'Page');
On 2010/06/17 22:53:18, arv wrote:
> ws around binops

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode48
chrome/browser/resources/options.html:48: nav.class = 'navbar_item
navbar_item_selected';
On 2010/06/17 22:53:18, arv wrote:
> use dashes (-) for css class names and ids

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode65
chrome/browser/resources/options.html:65: chrome.send("fetchPrefs",
['systemPrefsFetched', '',
On 2010/06/17 22:53:18, arv wrote:
> Use single quotes in JS for consistence.

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode75
chrome/browser/resources/options.html:75: timezoneSel = $('timezoneSel');
On 2010/06/17 22:53:18, arv wrote:
> missing var

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode79
chrome/browser/resources/options.html:79: new
Option(templateData.timezoneMap[key], key, false, selected);
On 2010/06/17 22:53:18, arv wrote:
> or
> 
> timezoneSel.appendChild(new Option(...));

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode99
chrome/browser/resources/options.html:99: if (control.id == 'tapToClickChk') {
On 2010/06/17 22:53:18, arv wrote:
> Use a switch

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode129
chrome/browser/resources/options.html:129: .navbar_container {
On 2010/06/17 22:53:18, arv wrote:
> Put an empty line between each rule

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode132
chrome/browser/resources/options.html:132: cursor:pointer;
On 2010/06/17 22:53:18, arv wrote:
> ws after :

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode134
chrome/browser/resources/options.html:134: float:left;
On 2010/06/17 22:53:18, arv wrote:
> rtl?

Not sure I get this one.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode178
chrome/browser/resources/options.html:178: <body onload="load();"
i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize">
On 2010/06/17 22:53:18, arv wrote:
> Long line

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode184
chrome/browser/resources/options.html:184: <li class="navbar_item
navbar_item_normal" id="systemPageNav" i18n-values=".innerText:systemPage"></li>
On 2010/06/17 22:53:18, arv wrote:
> Use i18n-content="systemPage"

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode184
chrome/browser/resources/options.html:184: <li class="navbar_item
navbar_item_normal" id="systemPageNav" i18n-values=".innerText:systemPage"></li>
On 2010/06/17 22:53:18, arv wrote:
> Use i18n-content="systemPage"

Done.

http://codereview.chromium.org/2781005/diff/39003/10012#newcode216
chrome/browser/resources/options.html:216: <input id="sensitivityRng"
type="range" min="1" max="10" class="touchSlider"
style="-webkit-appearance:slider-horizontal;" onchange="rangeChanged(this)">
On 2010/06/17 22:53:18, arv wrote:
> move css to the style element
> 
> I don't see why you need to set the appearance here.

Done.