[Hotword] Add new base URLs for the additional languages so it works on their respective local sear…

[Hotword] Add new base URLs for the additional languages so it works on their respective local search urls.

The function has been tested manually inside Chrome since it will not work as part of the larger system until the extension and some server side pieces are in place.

BUG=345806
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269950

[saurya1, 2014-05-09 01:23:34]

lgtm

[rpetterson, 2014-05-09 01:24:13]

Adding James for OWNERS approval.

[James Hawkins, 2014-05-09 02:54:27]

https://codereview.chromium.org/277523005/diff/20001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/277523005/diff/20001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:128: * Helper function to
test urls.
Can you update the method name and documentation to clarify what this method
does?

[rpetterson, 2014-05-09 06:07:20]

https://codereview.chromium.org/277523005/diff/20001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/277523005/diff/20001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:128: * Helper function to
test urls.
On 2014/05/09 02:54:27, James Hawkins wrote:
> Can you update the method name and documentation to clarify what this method
> does?

Done.

[James Hawkins, 2014-05-12 18:13:43]

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:128: * Helper function to
test urls as being valid for running the
nit: URLs.

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:177: if
(this.checkEligibleUrl(url, baseUrls[0]))
Can you clarify why we need to check baseUrls[0] separately from the loop below?
 Or maybe document what that loop is doing.

[rpetterson, 2014-05-12 19:42:38]

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:177: if
(this.checkEligibleUrl(url, baseUrls[0]))
On 2014/05/12 18:13:43, James Hawkins wrote:
> Can you clarify why we need to check baseUrls[0] separately from the loop
below?
>  Or maybe document what that loop is doing.

chrome://newtab doesn't have the local tlds (.com, .de, .fr, etc.). I left it as
an array for when we add additional ones, but if you think it would be clearer,
I can just hardcode a check for
if (this.checkEligibleUrl(url, 'chrome://newtab')

Let me know what you think.

[James Hawkins, 2014-05-12 19:44:33]

LGTM with nit and comment update.

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:177: if
(this.checkEligibleUrl(url, baseUrls[0]))
On 2014/05/12 19:42:39, rpetterson wrote:
> On 2014/05/12 18:13:43, James Hawkins wrote:
> > Can you clarify why we need to check baseUrls[0] separately from the loop
> below?
> >  Or maybe document what that loop is doing.
> 
> chrome://newtab doesn't have the local tlds (.com, .de, .fr, etc.). I left it
as
> an array for when we add additional ones, but if you think it would be
clearer,
> I can just hardcode a check for
> if (this.checkEligibleUrl(url, 'chrome://newtab')
> 
> Let me know what you think.

No, that's fine.  Just add a comment to the code to clarify the intent.

[rpetterson, 2014-05-12 21:09:04]

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:128: * Helper function to
test urls as being valid for running the
On 2014/05/12 18:13:43, James Hawkins wrote:
> nit: URLs.

Done.

https://codereview.chromium.org/277523005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:177: if
(this.checkEligibleUrl(url, baseUrls[0]))
On 2014/05/12 19:44:39, James Hawkins wrote:
> On 2014/05/12 19:42:39, rpetterson wrote:
> > On 2014/05/12 18:13:43, James Hawkins wrote:
> > > Can you clarify why we need to check baseUrls[0] separately from the loop
> > below?
> > >  Or maybe document what that loop is doing.
> > 
> > chrome://newtab doesn't have the local tlds (.com, .de, .fr, etc.). I left
it
> as
> > an array for when we add additional ones, but if you think it would be
> clearer,
> > I can just hardcode a check for
> > if (this.checkEligibleUrl(url, 'chrome://newtab')
> > 
> > Let me know what you think.
> 
> No, that's fine.  Just add a comment to the code to clarify the intent.

Done.

[rpetterson, 2014-05-12 21:17:36]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-05-12 21:19:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlp@chromium.org/277523005/60001

[commit-bot: I haz the power, 2014-05-13 01:10:08]

Change committed as 269950