Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7497028:
Add SetUserPreference function to VirtualKeyboardSelector (Closed)
|
Created:
9 years, 5 months ago by Yusuke Sato Modified:
9 years, 4 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, paweł hajdan jr. penghuang_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd SetUserPreference function to VirtualKeyboardSelector
BUG=None
TEST=ran unit_tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95566
Patch Set 1 : review #
Total comments: 18
Patch Set 2 : review fix #Patch Set 3 : fix tests #
Total comments: 12
Patch Set 4 : review fix #
Total comments: 4
Patch Set 5 : review fix #
Messages
Total messages: 41 (0 generated)
LGTM with two comments http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc (right): http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: std::list<const VirtualKeyboard*>::const_iterator iter; How about moving this to the initializer expression of the following for-loop? http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h (right): http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool LayoutIsSupported(const std::string& layout) const; I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or IsSupportedLayout) because that makes it clear that this function return boolean value.
http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc (right): http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: current_ = iter->second; Before throwing away the value of current_, maybe we should check if the layout is still supported? I'm thinking of the case where the user has saved a preference, but then the extension is updated and no longer supports a layout that has the preference saved for it. In that case, we should not lose current_ I think. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: const VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? ByLayout doesn't really tell me that this function is skipping preferences. If you have an idea for a shorter name that conveys this that would be even better. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if (SameOrNull(url, &(keyboard->url())) && This could give bad results if somehow there is a keyboard with a NULL url. How about: if ((url == NULL || *url == *(keyboard->url()) && keyboard->LayoutIsSupported(layout)) I think that makes it more clear what the intent is as well. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h (right): http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool LayoutIsSupported(const std::string& layout) const; +1 for IsSupportedLayout On 2011/07/28 07:08:16, mazda wrote: > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or > IsSupportedLayout) because that makes it clear that this function return boolean > value. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: static const VirtualKeyboard* SelectVirtualKeyboardByUrl( This name is a bit misleading, as it is actually selecting by both Layout and Url. Also, please add to the comment that if url is NULL, then URL checking is ignored. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A map from a layout name to a virtual keyboard extension. would be easier to parse without "a", i.e. "A map from layout name to virtual keyboard extension". http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: std::map<std::string, const VirtualKeyboard*> user_preference_; How does this actually get saved between restarts of the browser?
http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc (right): http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: current_ = iter->second; On 2011/07/28 18:31:48, bryeung wrote: > Before throwing away the value of current_, maybe we should check if the layout > is still supported? > > I'm thinking of the case where the user has saved a preference, but then the > extension is updated and no longer supports a layout that has the preference > saved for it. In that case, we should not lose current_ I think. Done. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: const VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); On 2011/07/28 18:31:48, bryeung wrote: > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? > > ByLayout doesn't really tell me that this function is skipping preferences. If > you have an idea for a shorter name that conveys this that would be even better. Done. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: std::list<const VirtualKeyboard*>::const_iterator iter; On 2011/07/28 07:08:16, mazda wrote: > How about moving this to the initializer expression of the following for-loop? Done. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if (SameOrNull(url, &(keyboard->url())) && On 2011/07/28 18:31:48, bryeung wrote: > This could give bad results if somehow there is a keyboard with a NULL url. > > How about: > > if ((url == NULL || *url == *(keyboard->url()) && > keyboard->LayoutIsSupported(layout)) > > I think that makes it more clear what the intent is as well. > Done. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h (right): http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool LayoutIsSupported(const std::string& layout) const; On 2011/07/28 07:08:16, mazda wrote: > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or > IsSupportedLayout) because that makes it clear that this function return boolean > value. Done. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool LayoutIsSupported(const std::string& layout) const; On 2011/07/28 18:31:48, bryeung wrote: > +1 for IsSupportedLayout > > On 2011/07/28 07:08:16, mazda wrote: > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or > > IsSupportedLayout) because that makes it clear that this function return > boolean > > value. > Done. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: static const VirtualKeyboard* SelectVirtualKeyboardByUrl( On 2011/07/28 18:31:48, bryeung wrote: > This name is a bit misleading, as it is actually selecting by both Layout and > Url. I've moved the function to the anonymous namespace in virtual_keyboard_selector.cc, and renamed it. > Also, please add to the comment that if url is NULL, then URL checking is > ignored. Revised the function comment. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A map from a layout name to a virtual keyboard extension. On 2011/07/28 18:31:48, bryeung wrote: > would be easier to parse without "a", i.e. "A map from layout name to virtual > keyboard extension". Done. http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: std::map<std::string, const VirtualKeyboard*> user_preference_; Every time when the browser is restarted, NotifyPrefChanged function in chrome/browser/chromeos/preferences.cc is called. I'll add SetUserPreference calls to the function. On 2011/07/28 18:31:48, bryeung wrote: > How does this actually get saved between restarts of the browser?
Hi Bryan, Could you take another look? On 2011/07/29 05:15:35, Yusuke Sato wrote: > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc (right): > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: current_ = > iter->second; > On 2011/07/28 18:31:48, bryeung wrote: > > Before throwing away the value of current_, maybe we should check if the > layout > > is still supported? > > > > I'm thinking of the case where the user has saved a preference, but then the > > extension is updated and no longer supports a layout that has the preference > > saved for it. In that case, we should not lose current_ I think. > > Done. > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: const > VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); > On 2011/07/28 18:31:48, bryeung wrote: > > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? > > > > ByLayout doesn't really tell me that this function is skipping preferences. > If > > you have an idea for a shorter name that conveys this that would be even > better. > > Done. > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: > std::list<const VirtualKeyboard*>::const_iterator iter; > On 2011/07/28 07:08:16, mazda wrote: > > How about moving this to the initializer expression of the following for-loop? > > Done. > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if > (SameOrNull(url, &(keyboard->url())) && > On 2011/07/28 18:31:48, bryeung wrote: > > This could give bad results if somehow there is a keyboard with a NULL url. > > > > How about: > > > > if ((url == NULL || *url == *(keyboard->url()) && > > keyboard->LayoutIsSupported(layout)) > > > > I think that makes it more clear what the intent is as well. > > > > Done. > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h (right): > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool > LayoutIsSupported(const std::string& layout) const; > On 2011/07/28 07:08:16, mazda wrote: > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or > > IsSupportedLayout) because that makes it clear that this function return > boolean > > value. > > Done. > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool > LayoutIsSupported(const std::string& layout) const; > On 2011/07/28 18:31:48, bryeung wrote: > > +1 for IsSupportedLayout > > > > On 2011/07/28 07:08:16, mazda wrote: > > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or > > > IsSupportedLayout) because that makes it clear that this function return > > boolean > > > value. > > > > Done. > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: static > const VirtualKeyboard* SelectVirtualKeyboardByUrl( > On 2011/07/28 18:31:48, bryeung wrote: > > This name is a bit misleading, as it is actually selecting by both Layout and > > Url. > > I've moved the function to the anonymous namespace in > virtual_keyboard_selector.cc, and renamed it. > > > Also, please add to the comment that if url is NULL, then URL checking is > > ignored. > > Revised the function comment. > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A map > from a layout name to a virtual keyboard extension. > On 2011/07/28 18:31:48, bryeung wrote: > > would be easier to parse without "a", i.e. "A map from layout name to virtual > > keyboard extension". > > Done. > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... > chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: > std::map<std::string, const VirtualKeyboard*> user_preference_; > Every time when the browser is restarted, NotifyPrefChanged function in > chrome/browser/chromeos/preferences.cc is called. I'll add SetUserPreference > calls to the function. > > On 2011/07/28 18:31:48, bryeung wrote: > > How does this actually get saved between restarts of the browser?
http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc (right): http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:13: namespace { I don't think the anonymous namespace should be nested inside the others. http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:24: const GURL* url) { Is it worth adding a map from url to VirtualKeyboard in this class as well? The more I look at this, the more awkward I feel the URL check in this method is. It seems there are two use cases: 1) we want to search a list for a layout 2) we want to get the keyboard for a given URL and check if it supports a layout If you think that the lists are always short, and that it is not worth maintaining the map, I could be convinced to leave it like this. But I'd like to hear your feedback on this idea. http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:28: if (((!url) || (*url) == keyboard->url()) && Can you remove the extra parens? if ((!url || *url == keyboard->url()) && will be easier to read I think. http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:104: if (current_ && current_->IsLayoutSupported(layout)) { This should probably be an else if, or you'll be doing a redundant call to IsLayoutSupported Either that, or you should return current_ above. http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc (right): http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc:320: EXPECT_FALSE(selector.SetUserPreference("bad_layout", GURL("http://user"))); It would be valuable to ensure that the preference is not changed when this returns false http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc:322: EXPECT_TRUE(selector.SetUserPreference("a", GURL("http://user"))); it would be valuable to ensure that the preference is saved when this returns true
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc (right): http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:13: namespace { On 2011/08/03 22:04:46, bryeung wrote: > I don't think the anonymous namespace should be nested inside the others. Done. Added a namespace alias (which is explicitly allowed in the style guide) to follow the 80 col rule. http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:24: const GURL* url) { Sure, added the map to make SetUserPreference function simpler and faster. On 2011/08/03 22:04:46, bryeung wrote: > Is it worth adding a map from url to VirtualKeyboard in this class as well? > The more I look at this, the more awkward I feel the URL check in this method > is. > > It seems there are two use cases: > 1) we want to search a list for a layout > 2) we want to get the keyboard for a given URL and check if it supports a > layout > > If you think that the lists are always short, and that it is not worth > maintaining the map, I could be convinced to leave it like this. But I'd like > to hear your feedback on this idea. http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:28: if (((!url) || (*url) == keyboard->url()) && On 2011/08/03 22:04:46, bryeung wrote: > Can you remove the extra parens? > > if ((!url || *url == keyboard->url()) && > > will be easier to read I think. (I've removed the url parameter from the function.) http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:104: if (current_ && current_->IsLayoutSupported(layout)) { On 2011/08/03 22:04:46, bryeung wrote: > This should probably be an else if, or you'll be doing a redundant call to > IsLayoutSupported > > Either that, or you should return current_ above. Done. http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc (right): http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc:320: EXPECT_FALSE(selector.SetUserPreference("bad_layout", GURL("http://user"))); On 2011/08/03 22:04:46, bryeung wrote: > It would be valuable to ensure that the preference is not changed when this > returns false Done. http://codereview.chromium.org/7497028/diff/10004/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc:322: EXPECT_TRUE(selector.SetUserPreference("a", GURL("http://user"))); On 2011/08/03 22:04:46, bryeung wrote: > it would be valuable to ensure that the preference is saved when this returns > true Done.
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
LGTM with 2 nits http://codereview.chromium.org/7497028/diff/14005/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc (right): http://codereview.chromium.org/7497028/diff/14005/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:135: VLOG(1) << "Can't set user preference: unsupported layout"; nit: maybe "requested layout is not supported by requested URL" would be more clear? http://codereview.chromium.org/7497028/diff/14005/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc (right): http://codereview.chromium.org/7497028/diff/14005/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc:394: EXPECT_TRUE(selector.SetUserPreference("c", GURL("http://system"))); nit: this test should have the same size checks as the test above
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
On Mon, Aug 1, 2011 at 7:26 PM, <yusukes@chromium.org> wrote: > Hi Bryan, > Could you take another look? My apologies: I was on vacation and then we had a holiday (and then I traveled to MTV), but I forgot to set my Gmail auto-responder for any of that. Sorry for the delay, Bryan > > On 2011/07/29 05:15:35, Yusuke Sato wrote: > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc > > (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:88: >> current_ > > = >> >> iter->second; >> On 2011/07/28 18:31:48, bryeung wrote: >> > Before throwing away the value of current_, maybe we should check if the >> layout >> > is still supported? >> > >> > I'm thinking of the case where the user has saved a preference, but then >> > the >> > extension is updated and no longer supports a layout that has the >> > preference >> > saved for it. In that case, we should not lose current_ I think. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:96: >> const >> VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout); >> On 2011/07/28 18:31:48, bryeung wrote: >> > Maybe SelectVirtualKeyboardWithoutPreferences would be more clear? >> > >> > ByLayout doesn't really tell me that this function is skipping >> > preferences. >> If >> > you have an idea for a shorter name that conveys this that would be even >> better. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:140: >> std::list<const VirtualKeyboard*>::const_iterator iter; >> On 2011/07/28 07:08:16, mazda wrote: >> > How about moving this to the initializer expression of the following > > for-loop? > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:143: if >> (SameOrNull(url, &(keyboard->url())) && >> On 2011/07/28 18:31:48, bryeung wrote: >> > This could give bad results if somehow there is a keyboard with a NULL >> > url. >> > >> > How about: >> > >> > if ((url == NULL || *url == *(keyboard->url()) && >> > keyboard->LayoutIsSupported(layout)) >> > >> > I think that makes it more clear what the intent is as well. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> File chrome/browser/chromeos/input_method/virtual_keyboard_selector.h >> (right): > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 07:08:16, mazda wrote: >> > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > IsSupportedLayout) because that makes it clear that this function return >> boolean >> > value. > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:37: bool >> LayoutIsSupported(const std::string& layout) const; >> On 2011/07/28 18:31:48, bryeung wrote: >> > +1 for IsSupportedLayout >> > >> > On 2011/07/28 07:08:16, mazda wrote: >> > > I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or >> > > IsSupportedLayout) because that makes it clear that this function >> > > return >> > boolean >> > > value. >> > > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:105: >> static >> const VirtualKeyboard* SelectVirtualKeyboardByUrl( >> On 2011/07/28 18:31:48, bryeung wrote: >> > This name is a bit misleading, as it is actually selecting by both >> > Layout > > and >> >> > Url. > >> I've moved the function to the anonymous namespace in >> virtual_keyboard_selector.cc, and renamed it. > >> > Also, please add to the comment that if url is NULL, then URL checking >> > is >> > ignored. > >> Revised the function comment. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:115: // A >> map >> from a layout name to a virtual keyboard extension. >> On 2011/07/28 18:31:48, bryeung wrote: >> > would be easier to parse without "a", i.e. "A map from layout name to > > virtual >> >> > keyboard extension". > >> Done. > > > http://codereview.chromium.org/7497028/diff/3001/chrome/browser/chromeos/inpu... >> >> chrome/browser/chromeos/input_method/virtual_keyboard_selector.h:116: >> std::map<std::string, const VirtualKeyboard*> user_preference_; >> Every time when the browser is restarted, NotifyPrefChanged function in >> chrome/browser/chromeos/preferences.cc is called. I'll add >> SetUserPreference >> calls to the function. > >> On 2011/07/28 18:31:48, bryeung wrote: >> > How does this actually get saved between restarts of the browser? > > > > http://codereview.chromium.org/7497028/ >
http://codereview.chromium.org/7497028/diff/14005/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc (right): http://codereview.chromium.org/7497028/diff/14005/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc:135: VLOG(1) << "Can't set user preference: unsupported layout"; On 2011/08/04 17:41:41, bryeung wrote: > nit: maybe "requested layout is not supported by requested URL" would be more > clear? Done. http://codereview.chromium.org/7497028/diff/14005/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc (right): http://codereview.chromium.org/7497028/diff/14005/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/virtual_keyboard_selector_unittest.cc:394: EXPECT_TRUE(selector.SetUserPreference("c", GURL("http://system"))); On 2011/08/04 17:41:41, bryeung wrote: > nit: this test should have the same size checks as the test above Done.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
