Move URLs out of *.pak files and put them into code.

Move URLs out of *.pak files and put them into code.

BUG=28174
TEST=trybots

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

[tfarina, 2010-10-30 01:16:04]

Hi Tony, could you review this to me?

With this, we left on the .grd file, only URLs that has GRITLANGCODE on it.

[tony, 2010-11-01 16:59:02]

http://codereview.chromium.org/4130012/diff/7001/8001
File chrome/app/resources/locale_settings.grd (left):

http://codereview.chromium.org/4130012/diff/7001/8001#oldcode598
chrome/app/resources/locale_settings.grd:598: </message>
This needs to stay because there are localized strings in the xtb files.  grep
for IDS_FLASH_STORAGE_URL to see how it's localized.  The languages that aren't
in the xtb files fallback to the English string.

http://codereview.chromium.org/4130012/diff/7001/8009
File chrome/browser/plugin_installer.cc (right):

http://codereview.chromium.org/4130012/diff/7001/8009#newcode19
chrome/browser/plugin_installer.cc:19:
"http://www.google.com/support/chrome/bin/answer.py?answer=95697&amp;topic=14687";
Nit: Indent the second line 4 spaces.

[tfarina, 2010-11-01 23:29:47]

PTAL!

http://codereview.chromium.org/4130012/diff/7001/8001
File chrome/app/resources/locale_settings.grd (left):

http://codereview.chromium.org/4130012/diff/7001/8001#oldcode598
chrome/app/resources/locale_settings.grd:598: </message>
On 2010/11/01 16:59:02, tony wrote:
> This needs to stay because there are localized strings in the xtb files.  grep
> for IDS_FLASH_STORAGE_URL to see how it's localized.  The languages that
aren't
> in the xtb files fallback to the English string.

OK, I left it as is.

http://codereview.chromium.org/4130012/diff/7001/8009
File chrome/browser/plugin_installer.cc (right):

http://codereview.chromium.org/4130012/diff/7001/8009#newcode19
chrome/browser/plugin_installer.cc:19:
"http://www.google.com/support/chrome/bin/answer.py?answer=95697&amp;topic=14687";
On 2010/11/01 16:59:02, tony wrote:
> Nit: Indent the second line 4 spaces.

Done.

[tony, 2010-11-01 23:36:27]

http://codereview.chromium.org/4130012/diff/14001/15006
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/4130012/diff/14001/15006#newcode101
chrome/common/chrome_constants.h:101: extern const char kPrivacyDashboardUrl[];
I think it would be a tiny bit better to make this a static variable of
PluginInstaller (in chrome/browser/plugin_installer.*).  What do you think?

[Evan Stade, 2010-11-01 23:36:54]

http://codereview.chromium.org/4130012/diff/14001/15006
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/4130012/diff/14001/15006#newcode1
chrome/common/chrome_constants.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
year

[Evan Stade, 2010-11-01 23:40:10]

On 2010/11/01 23:36:27, tony wrote:
> http://codereview.chromium.org/4130012/diff/14001/15006
> File chrome/common/chrome_constants.h (right):
> 
> http://codereview.chromium.org/4130012/diff/14001/15006#newcode101
> chrome/common/chrome_constants.h:101: extern const char
kPrivacyDashboardUrl[];
> I think it would be a tiny bit better to make this a static variable of
> PluginInstaller (in chrome/browser/plugin_installer.*).  What do you think?

wouldn't that would require including another header? If we did that a lot of
times (for fixing bug 28174 in general), compile time could be negatively
impacted.

[tony, 2010-11-01 23:48:16]

On 2010/11/01 23:40:10, Evan Stade wrote:
> On 2010/11/01 23:36:27, tony wrote:
> > http://codereview.chromium.org/4130012/diff/14001/15006
> > File chrome/common/chrome_constants.h (right):
> > 
> > http://codereview.chromium.org/4130012/diff/14001/15006#newcode101
> > chrome/common/chrome_constants.h:101: extern const char
> kPrivacyDashboardUrl[];
> > I think it would be a tiny bit better to make this a static variable of
> > PluginInstaller (in chrome/browser/plugin_installer.*).  What do you think?
> 
> wouldn't that would require including another header? If we did that a lot of
> times (for fixing bug 28174 in general), compile time could be negatively
> impacted.

It would be an extra include for browser.cc and a different include for
clear_server_data.cc.  This would make compile times faster for changes to
chrome_constants (since less needs to rebuild), but slower to compile
browser.cc.

I just realized putting the privacy dashboard URL in plugin_installer.h doesn't
make sense.  Actually, it looks like the use in clear_server_data.cc should just
call Browser::OpenPrivacyDashboardTabAndActivate and then we can inline the url
in browser.cc.

[tfarina, 2010-11-02 00:01:36]

Ready for another look.

[tony, 2010-11-02 00:04:01]

LGTM

http://codereview.chromium.org/4130012/diff/22001/23004
File chrome/browser/views/clear_server_data.cc (right):

http://codereview.chromium.org/4130012/diff/22001/23004#newcode254
chrome/browser/views/clear_server_data.cc:254: browser->window()->Show();
Do you still need to call Show() here?  It sounds like
OpenPrivacyDashboardTabAndActivate() does that for us.

[tfarina, 2010-11-02 00:11:25]

http://codereview.chromium.org/4130012/diff/22001/23004
File chrome/browser/views/clear_server_data.cc (right):

http://codereview.chromium.org/4130012/diff/22001/23004#newcode254
chrome/browser/views/clear_server_data.cc:254: browser->window()->Show();
On 2010/11/02 00:04:02, tony wrote:
> Do you still need to call Show() here?  It sounds like
> OpenPrivacyDashboardTabAndActivate() does that for us.

I'm not 100% sure, because at OpenPrivacyDashboardTabAndActivate it calls
Activate instead of Show. For me this sounds like calling Activate supposes
already exists a window (but as I said I'm not sure if calling Activate give us
the same effect of calling Show).

[tony, 2010-11-02 00:16:56]

Does manually testing the patch show a difference in behavior?

On 2010/11/02 00:11:25, tfarina wrote:
> http://codereview.chromium.org/4130012/diff/22001/23004
> File chrome/browser/views/clear_server_data.cc (right):
> 
> http://codereview.chromium.org/4130012/diff/22001/23004#newcode254
> chrome/browser/views/clear_server_data.cc:254: browser->window()->Show();
> On 2010/11/02 00:04:02, tony wrote:
> > Do you still need to call Show() here?  It sounds like
> > OpenPrivacyDashboardTabAndActivate() does that for us.
> 
> I'm not 100% sure, because at OpenPrivacyDashboardTabAndActivate it calls
> Activate instead of Show. For me this sounds like calling Activate supposes
> already exists a window (but as I said I'm not sure if calling Activate give
us
> the same effect of calling Show).

[Evan Stade, 2010-11-02 00:27:25]

indeed it is confusing that we have both Activate and Show. I wonder if we can
combine them.

[tfarina, 2010-11-02 00:29:24]

On 2010/11/02 00:16:56, tony wrote:
> Does manually testing the patch show a difference in behavior?
> 
I won't be able to test the difference. I'm making the patch on linux.

[tfarina, 2010-11-02 00:32:14]

On 2010/11/02 00:16:56, tony wrote:
> Does manually testing the patch show a difference in behavior?
> 
See views/window/window.h +96 for Activate description. Looks like we can get
different results if we call it instead of Show.

[tony, 2010-11-02 00:35:54]

On 2010/11/02 00:29:24, tfarina wrote:
> On 2010/11/02 00:16:56, tony wrote:
> > Does manually testing the patch show a difference in behavior?
> > 
> I won't be able to test the difference. I'm making the patch on linux.

Does the linux views build run this code?

[tfarina, 2010-11-02 00:41:09]

On 2010/11/02 00:35:54, tony wrote:
> On 2010/11/02 00:29:24, tfarina wrote:
> > On 2010/11/02 00:16:56, tony wrote:
> > > Does manually testing the patch show a difference in behavior?
> > > 
> > I won't be able to test the difference. I'm making the patch on linux.
> 
> Does the linux views build run this code?

I don't think so. It isn't include under the source list of os==linux and
toolkit_views=1

[tfarina, 2010-11-02 02:30:04]

http://codereview.chromium.org/4130012/diff/32001/33004
File chrome/browser/views/clear_server_data.cc (right):

http://codereview.chromium.org/4130012/diff/32001/33004#newcode253
chrome/browser/views/clear_server_data.cc:253:
browser->OpenPrivacyDashboardTabAndActivate();
Removed the call to Show. OK to commit?

[tony, 2010-11-02 18:40:27]

LG

http://codereview.chromium.org/4130012/diff/32001/33004
File chrome/browser/views/clear_server_data.cc (right):

http://codereview.chromium.org/4130012/diff/32001/33004#newcode253
chrome/browser/views/clear_server_data.cc:253:
browser->OpenPrivacyDashboardTabAndActivate();
On 2010/11/02 02:30:05, tfarina wrote:
> Removed the call to Show. OK to commit?

I can't even seem to find where this view is shown.  Seems fine w/o it.

It would be great if you could investigate whether we should be using Activate()
for the flash storage link too.  Maybe it should also be moved into a function
in browser.h/cc (or a more generic method that takes a URL to open and
activate).