dom-ui settings: enable languages & spell checker settings for all platforms....

dom-ui settings: enable languages & spell checker settings for all platforms.

BUG=56415, 43716
TEST=Verify language & settings panel functionality on dom-ui settings window.

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

[csilv, 2011-01-20 03:03:03]

+jhawkins, satorux for review.

[James Hawkins, 2011-01-20 03:11:08]

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/language_options_handler.cc (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.cc:147:
chromeos::CrosLibrary::Get()->GetInputMethodLibrary()->GetSupportedInputMethods());
80cols.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/language_options_handler.h (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.h:13: #endif
#endif  // defined(OS_CHROMEOS)

here and everywhere else.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.h:18: // ChromeOS
language options page UI handler.
s/ChromeOS l/L/

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.h:39: #if
defined(OS_CHROMEOS)
I'm not going to make you change it now, but the number of ifdefs in this file
makes me think a refactoring is needed at some point.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/language_options_handler_unittest.cc (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler_unittest.cc:164:
TEST(LanguageOptionsHandlerTest, GetUiLanguageCodeSet) {
s/Ui/UI/

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
File chrome/browser/resources/options/language_add_language_overlay.html
(right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_add_language_overlay.html:13:
</select>
Fits on the line above.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_add_language_overlay.html:20:
</button>
Probably fits on the line above.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
File chrome/browser/resources/options/language_options.css (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_options.css:10: .language-options h3 {
h3 should be set already by options_page.css.

[satorux1, 2011-01-20 10:40:33]

Chris,

Thank you for implementing this. I'm guessing it was painful for you to modify
the existing code with #ifdefs.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/language_options_handler.h (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.h:39: #if
defined(OS_CHROMEOS)
On 2011/01/20 03:11:08, James Hawkins wrote:
> I'm not going to make you change it now, but the number of ifdefs in this file
> makes me think a refactoring is needed at some point.

I agree. Let's do it in a separate patch. One way to refactor this would be to
make the chrome os version to be a sub class. In retrospect, I should've written
this for all platforms from the beginning.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
File chrome/browser/resources/options/language_add_language_overlay.html
(right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_add_language_overlay.html:9: <if
expr="not pp_ifdef('chromeos')">
Why cannot we share the same UI? The Chrome OS version lists languages in two
columns. This makes it easy for users to choose a language, than from a list in
<select>. We implemented this way based on feedback from users and UX

[csilv, 2011-01-20 20:09:55]

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/language_options_handler.cc (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.cc:147:
chromeos::CrosLibrary::Get()->GetInputMethodLibrary()->GetSupportedInputMethods());
On 2011/01/20 03:11:08, James Hawkins wrote:
> 80cols.

Done.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/language_options_handler.h (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.h:13: #endif
On 2011/01/20 03:11:08, James Hawkins wrote:
> #endif  // defined(OS_CHROMEOS)
> 
> here and everywhere else.

Done.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.h:18: // ChromeOS
language options page UI handler.
On 2011/01/20 03:11:08, James Hawkins wrote:
> s/ChromeOS l/L/

Done.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.h:39: #if
defined(OS_CHROMEOS)
Agree to both of you.  I think a ChromeOS specific subclass makes perfect sense.
 Using conditionals is the more "low-impact" approach that I want to take right
now, but this will be high priority after all the M10 requirements are met.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/language_options_handler_unittest.cc (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler_unittest.cc:164:
TEST(LanguageOptionsHandlerTest, GetUiLanguageCodeSet) {
On 2011/01/20 03:11:08, James Hawkins wrote:
> s/Ui/UI/

Done.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
File chrome/browser/resources/options/language_add_language_overlay.html
(right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_add_language_overlay.html:9: <if
expr="not pp_ifdef('chromeos')">
On 2011/01/20 10:40:33, satorux1 wrote:
> Why cannot we share the same UI? The Chrome OS version lists languages in two
> columns. This makes it easy for users to choose a language, than from a list
in
> <select>. We implemented this way based on feedback from users and UX

The two column UI works well on ChromeOS, but on the browser it is problematic. 
This is because the list is populated with Accept Languages, which greatly
outnumbers input methods.  On ChromeOS, there are about 40 items, on the browser
there are 132.  This creates a very large overlay that is too large for even a
big screen.

While I think this solution is acceptable for now (it's essentially the same UI
as used on windows), this is probably worth revisiting for a UI review.  There
are other options such as a scrollable list that might be more friendly.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_add_language_overlay.html:13:
</select>
On 2011/01/20 03:11:08, James Hawkins wrote:
> Fits on the line above.

Done.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_add_language_overlay.html:20:
</button>
On 2011/01/20 03:11:08, James Hawkins wrote:
> Probably fits on the line above.

Done.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
File chrome/browser/resources/options/language_options.css (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_options.css:10: .language-options h3 {
On 2011/01/20 03:11:08, James Hawkins wrote:
> h3 should be set already by options_page.css.

This needs to be styled a bit differently for its use in this panel.  This will
go-away once we re-design the UI to use the common-list classes and UI.

[satorux1, 2011-01-21 01:38:46]

LGTM, with one request below:

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/language_options_handler.h (right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/language_options_handler.h:39: #if
defined(OS_CHROMEOS)
On 2011/01/20 20:09:55, csilv wrote:
> Agree to both of you.  I think a ChromeOS specific subclass makes perfect
sense.
>  Using conditionals is the more "low-impact" approach that I want to take
right
> now, but this will be high priority after all the M10 requirements are met.

We can take care of the refactoring from Tokyo, as we are more familiar with the
chrome os specific stuff. what do you think?

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
File chrome/browser/resources/options/language_add_language_overlay.html
(right):

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_add_language_overlay.html:2: <h1
i18n-content="add_language_title"></h1>
Please move this to inside of <if expr="not pp_ifdef('chromeos')">. We are
planning to rework the language selection UI. Until then, it'd be safer to leave
it as-is.

http://codereview.chromium.org/6296017/diff/38001/chrome/browser/resources/op...
chrome/browser/resources/options/language_add_language_overlay.html:9: <if
expr="not pp_ifdef('chromeos')">
On 2011/01/20 20:09:55, csilv wrote:
> On 2011/01/20 10:40:33, satorux1 wrote:
> > Why cannot we share the same UI? The Chrome OS version lists languages in
two
> > columns. This makes it easy for users to choose a language, than from a list
> in
> > <select>. We implemented this way based on feedback from users and UX
> 
> The two column UI works well on ChromeOS, but on the browser it is
problematic. 
> This is because the list is populated with Accept Languages, which greatly
> outnumbers input methods.  On ChromeOS, there are about 40 items, on the
browser
> there are 132.  This creates a very large overlay that is too large for even a
> big screen.
> 
> While I think this solution is acceptable for now (it's essentially the same
UI
> as used on windows), this is probably worth revisiting for a UI review.  There
> are other options such as a scrollable list that might be more friendly.

Thank you for the explanation. Didn't know that we have 132 languages in other
platforms.

I had a chat with ainslie@ about the language selection UI. His idea was that we
should stop showing the language list as overlay, but instead show the list on
the right hand side of the language options page:

http://code.google.com/p/chromium-os/issues/detail?id=10699

I think someone in Tokyo will be doing this.

[James Hawkins, 2011-01-21 01:39:35]

LGTM

[csilv, 2011-01-21 01:51:58]

On 2011/01/21 01:38:46, satorux1 wrote:
> We can take care of the refactoring from Tokyo, as we are more familiar with
the
> chrome os specific stuff. what do you think?

That would be fantastic!  I will help however I can.

> Please move this to inside of <if expr="not pp_ifdef('chromeos')">. We are
> planning to rework the language selection UI. Until then, it'd be safer to
leave
> it as-is.

Done.

> I had a chat with ainslie@ about the language selection UI. His idea was that
we
> should stop showing the language list as overlay, but instead show the list on
> the right hand side of the language options page:
> 
> http://code.google.com/p/chromium-os/issues/detail?id=10699
> 
> I think someone in Tokyo will be doing this.

That sounds like an interesting approach.  I think we need to ask Alex to
generate mocks for this new panel for both ChromeOS and browser so that we're on
the right track.

thanks!
Chris