Input box's context menu/language settings should set ...

Input box's context menu/language settings should set 
"Languages" tab as active tab
AND updated some utility functions in tabbed_pane.
BUG=5639
(http://crbug.com/5639)

[Mohamed Mansour (USE mhm), 2008-12-18 04:18:44]

I just made a public method to SelectTab for that window, I don't know if it is
the correct way, but there are two possibilities. Either rearrange the tabs so
Languages will be first then Fonts, or we can code a method to switch it.

I don't know if you guys like enums, I created the enum because I don't want to
refer to an index from a different view. Instead of referring it by number, an
enum is more readable. 

I know you are going to complain about the code style, or the way I did it, but
I am hear to listen and learn :x

[Peter Kasting, 2008-12-18 06:24:32]

Changed reviewers.

I sympathize with not wanting to use magic numbers from other views, but perhaps
this enum is overkill.  Maybe we should just write a "SelectLanguageTab()"
function.  sky's opinion requested.

LGTM otherwise.

http://codereview.chromium.org/15031/diff/12/207
File chrome/browser/render_view_context_menu_controller.cc (right):

http://codereview.chromium.org/15031/diff/12/207#newcode466
Line 466: source_web_contents_->GetContentHWND(), gfx::Rect(), window_)->Show();
Nit: I'd probably put the first arg on the same line as the call if it fits,
then indent the next line even:
foo(bar,
    baz)->Show();

[Mohamed Mansour (USE mhm), 2008-12-18 06:52:37]

Alright I refactored the code to SelectLanguageTab since it makes sense in my
opinion as well, that window only has 2 tabs, it wont have many in future. I
recommend making the Options window with enums since that could change in the
future.

Nevertheless it works now as well, tested.

[Peter Kasting, 2008-12-18 07:12:03]

LGTM

http://codereview.chromium.org/15031/diff/212/19
File chrome/browser/render_view_context_menu_controller.cc (right):

http://codereview.chromium.org/15031/diff/212/19#newcode467
Line 467: window_)->Show();
Nit: it's OK to combine this with the previous line.

[Mohamed Mansour (USE mhm), 2008-12-18 07:15:40]

ok! done!

[sky, 2008-12-18 16:52:58]

http://codereview.chromium.org/15031/diff/217/219
File chrome/browser/views/options/fonts_languages_window_view.cc (right):

http://codereview.chromium.org/15031/diff/217/219#newcode77
Line 77: tabs_->SelectTabAt(1);
nit: should be two space indented.

http://codereview.chromium.org/15031/diff/217/219#newcode100
Line 100: tabs_->AddTabAtIndex(1, l10n_util::GetString(
I would suggest using a static const defined in the file for this index. That
should make it harder for SelectLanguageTab to break if someone shuffles things
around.

[Mohamed Mansour (USE mhm), 2008-12-18 18:03:51]

done!

[Peter Kasting, 2008-12-18 19:01:38]

LGTM.

[sky, 2008-12-18 19:20:25]

http://codereview.chromium.org/15031/diff/233/32
File chrome/browser/views/options/fonts_languages_window_view.cc (right):

http://codereview.chromium.org/15031/diff/233/32#newcode103
Line 103: tabs_->AddTab(l10n_util::GetString(
The whole point of adding the constant is so that you use it in both places. If
you convert this back to AddTabAtIndex you can use the constant.

[sky, 2008-12-18 19:25:59]

Peter beat me up, and while I prefer using the index in both places I'm OK with
your change now. An even better change is to add a method to TabbedPane to tell
you the index of a child, that way we don't have to worry about magic numbers.
It's up to you if you want to do that change.

Sorry for the back and forth on this. I blame Peter though.

[Mohamed Mansour (USE mhm), 2008-12-18 19:56:43]

I can do that change by doing this method:
int SelectTabIndexFromTitle(const std::wstring& title);

That will return an index, and we can call it by doing:
tabs_->SelectTabAt(l10n_util::GetString(
    IDS_FONT_LANGUAGE_SETTING_LANGUAGES_TAB_TITLE));

I could do that, I just need to learn about CommCtrl.h

[Peter Kasting, 2008-12-18 20:53:24]

On 2008/12/18 19:25:59, sky wrote:
> An even better change is to add a method to TabbedPane to tell
> you the index of a child, that way we don't have to worry about magic numbers.

I agree with Scott that this would be more future-proof.  I tend to feel it's
overengineering for use in one caller (especially given the simplicity of this
file).  My preference would be to not bother, and do this later if we find more
cases that we could refactor to use it.  I won't stand in the way if you guys
feel strongly about doing it, I just think it's a waste of code.

[Mohamed Mansour (USE mhm), 2008-12-18 20:58:15]

Too late :x

I just did it :( Please criticism is allowed. It is my first time doing C++, and
... I was doing this at first, http://rafb.net/p/zwnfr878.html , then Scott told
me why not use View*.

It works great now and cleaner!

[Mohamed Mansour (USE mhm), 2008-12-18 20:59:03]

Now both of you are happy! that what counts!

[sky, 2008-12-18 21:03:31]

http://codereview.chromium.org/15031/diff/259/49
File chrome/views/tabbed_pane.cc (right):

http://codereview.chromium.org/15031/diff/259/49#newcode144
Line 144: std::vector<View*>::iterator it_tab_views_ = tab_views_.begin();
Use std::find here.

http://codereview.chromium.org/15031/diff/259/48
File chrome/views/tabbed_pane.h (right):

http://codereview.chromium.org/15031/diff/259/48#newcode55
Line 55: void SelectTabAt(const View* view);
nit: no point in passing in const here.

http://codereview.chromium.org/15031/diff/259/48#newcode58
Line 58: int GetIndexOfTab(const View* view);
nit: and here

[Peter Kasting, 2008-12-18 21:04:48]

http://codereview.chromium.org/15031/diff/40/250
File chrome/browser/views/options/fonts_languages_window_view.cc (right):

http://codereview.chromium.org/15031/diff/40/250#newcode24
Line 24: static const int kLanguagesTab = 1;
Remove this now-unneeded line.

http://codereview.chromium.org/15031/diff/40/250#newcode97
Line 97: // in the future, make sure kLanguagesTab gets set to the proper
position.
Remove this now-unneeded comment.

http://codereview.chromium.org/15031/diff/40/251
File chrome/browser/views/options/fonts_languages_window_view.h (right):

http://codereview.chromium.org/15031/diff/40/251#newcode41
Line 41: void SelectLanguageTab();
Nit: Since we say "Languages" everywhere else, change the function name to
match.

http://codereview.chromium.org/15031/diff/40/253
File chrome/views/tabbed_pane.cc (right):

http://codereview.chromium.org/15031/diff/40/253#newcode143
Line 143: int return_index = -1;
Nit: Don't use a temp, just return directly from inside the loop, and return -1
directly at the bottom of the function.

http://codereview.chromium.org/15031/diff/40/253#newcode144
Line 144: std::vector<View*>::iterator it_tab_views_ = tab_views_.begin();
Because you're trying to return an index below, just use an index here (size_t
i) instead of an iterator.

http://codereview.chromium.org/15031/diff/40/252
File chrome/views/tabbed_pane.h (right):

http://codereview.chromium.org/15031/diff/40/252#newcode55
Line 55: void SelectTabAt(const View* view);
Nit: I would prefer:
// Selects the tab with the specified contents.
void SelectTabForContents(const View* contents);

http://codereview.chromium.org/15031/diff/40/252#newcode57
Line 57: // Gets the tab index from the |view|.
No one calls this publicly, so it should be private.

[Peter Kasting, 2008-12-18 21:08:06]

http://codereview.chromium.org/15031/diff/259/49
File chrome/views/tabbed_pane.cc (right):

http://codereview.chromium.org/15031/diff/259/49#newcode144
Line 144: std::vector<View*>::iterator it_tab_views_ = tab_views_.begin();
On 2008/12/18 21:03:31, sky wrote:
> Use std::find here.

This is a better suggestion than mine, use it.

http://codereview.chromium.org/15031/diff/259/48
File chrome/views/tabbed_pane.h (right):

http://codereview.chromium.org/15031/diff/259/48#newcode55
Line 55: void SelectTabAt(const View* view);
On 2008/12/18 21:03:31, sky wrote:
> nit: no point in passing in const here.

Belay that, both these functions should indeed take a const View*, not a View*.

http://codereview.chromium.org/15031/diff/259/48#newcode58
Line 58: int GetIndexOfTab(const View* view);
Forgot: This member function should also be const

[Mohamed Mansour (USE mhm), 2008-12-18 21:38:22]

done!

[Peter Kasting, 2008-12-18 21:42:54]

http://codereview.chromium.org/15031/diff/271/72
File chrome/browser/views/options/fonts_languages_window_view.cc (right):

http://codereview.chromium.org/15031/diff/271/72#newcode96
Line 96: // in the future, make sure kLanguagesTab gets set to the proper
position.
Still didn't remove this now-unnecessary comment.

http://codereview.chromium.org/15031/diff/271/75
File chrome/views/tabbed_pane.cc (right):

http://codereview.chromium.org/15031/diff/271/75#newcode227
Line 227: std::vector<View*>::iterator i = std::find(tab_views_.begin(),
Nit: Use a const_iterator

http://codereview.chromium.org/15031/diff/271/75#newcode229
Line 229: if (i != tab_views_.end())
Nit: I would probably do:
return (i == tab_views_.end()) ?
    -1 : static_cast<int>(i - tab_views_.begin());

Though I bet Scott likes this way better :P

http://codereview.chromium.org/15031/diff/271/74
File chrome/views/tabbed_pane.h (right):

http://codereview.chromium.org/15031/diff/271/74#newcode55
Line 55: void SelectTabAt(const View* contents);
Still didn't call this SelectTabForContents().

http://codereview.chromium.org/15031/diff/271/74#newcode73
Line 73: // Gets the tab index from the specified |contents|.
Nit: Should mention that this returns -1 if no matching tab is found.

http://codereview.chromium.org/15031/diff/271/74#newcode74
Line 74: int GetIndexOfTab(const View* contents);
Still did not make this function const.

[Mohamed Mansour (USE mhm), 2008-12-18 22:00:34]

Alright, I promise no more nits. I hope. I think its more readable to return -1
:) Scott agrees I think. So now, we are all happy.

[Peter Kasting, 2008-12-18 22:27:16]

http://codereview.chromium.org/15031/diff/92/288
File chrome/views/tabbed_pane.h (right):

http://codereview.chromium.org/15031/diff/92/288#newcode73
Line 73: // Gets the tab index from the specified |contents|. If no |content|
exists,
This sounds like it returns -1 only for NULL.  Instead just say:
... Returns -1 when no tab contains |contents|.

http://codereview.chromium.org/15031/diff/92/288#newcode75
Line 75: int GetIndexOfTab(const View* contents);
_Still_ did not make this function const!

[Mohamed Mansour (USE mhm), 2008-12-18 22:34:34]

done

[Peter Kasting, 2008-12-18 22:40:55]

http://codereview.chromium.org/15031/diff/96/297
File chrome/views/tabbed_pane.h (right):

http://codereview.chromium.org/15031/diff/96/297#newcode73
Line 73: // Returns -1 when no tab contains |contents|.
Now you've deleted the first sentence.  This should not be this hard.  I feel
frustrated.

http://codereview.chromium.org/15031/diff/96/297#newcode74
Line 74: int const GetIndexOfTab(const View* contents);
This is not what it means to make a member function const.

[Mohamed Mansour (USE mhm), 2008-12-19 01:50:05]

I always have trouble with detailed comments. Okay nice, I just learned about
member function const.

Sorry for the troubles ...

[Peter Kasting, 2008-12-19 02:26:37]

Tweaked and landed in r7289.