Allow passing in a file path to a file that will be used for the new new tab...

Allow passing in a file path to a file that will be used for the new new tab
page. This allows faster prototyping (F5) of the NNTP.

TEST=Start Chrome with --new-new-tab-page=path/to/file.html. The NTP should
show the file. Make a visible change in the file and reload the tab page and
the change should be visible immidiately.

BUG=None


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

[Peter Kasting, 2009-05-27 23:50:47]

http://codereview.chromium.org/113927/diff/1004/5
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/113927/diff/1004/5#newcode206
Line 206: const std::string GetNewNewTabFromCommandLine();
This function can be static (and its return value should probably not be const).

http://codereview.chromium.org/113927/diff/1004/5#newcode310
Line 310: static const StringPiece new_tab_html_from_resource =
Why stuff this one in a static const but not the one above?  Seems like you
should do it to both?

http://codereview.chromium.org/113927/diff/1004/5#newcode327
Line 327: static const CommandLine* command_line =
CommandLine::ForCurrentProcess();
I wouldn't make either of these static (see below).

http://codereview.chromium.org/113927/diff/1004/5#newcode332
Line 332: if (file_util::ReadFileToString(FilePath(file_path), &file_contents))
{
Nit: no { }

http://codereview.chromium.org/113927/diff/1004/5#newcode1256
Line 1256: static const CommandLine* command_line =
CommandLine::ForCurrentProcess();
Don't make this static.  ForCurrentProcess() is fast, so you don't need to, and
I don't think it guarantees its return value is unchanging for the life of the
app (though I suspect it probably is).

[arv (Not doing code reviews), 2009-05-28 16:23:00]

PTAl

http://codereview.chromium.org/113927/diff/1004/5
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/113927/diff/1004/5#newcode206
Line 206: const std::string GetNewNewTabFromCommandLine();
On 2009/05/27 23:50:47, pkasting wrote:
> This function can be static (and its return value should probably not be
const).

Done.

http://codereview.chromium.org/113927/diff/1004/5#newcode310
Line 310: static const StringPiece new_tab_html_from_resource =
On 2009/05/27 23:50:47, pkasting wrote:
> Why stuff this one in a static const but not the one above?  Seems like you
> should do it to both?

This used to be a static const. I don't want the content read from the file to
be a static since I want F5 to reload the contents.

I'll just remove the statics.

http://codereview.chromium.org/113927/diff/1004/5#newcode327
Line 327: static const CommandLine* command_line =
CommandLine::ForCurrentProcess();
On 2009/05/27 23:50:47, pkasting wrote:
> I wouldn't make either of these static (see below).

Done.

http://codereview.chromium.org/113927/diff/1004/5#newcode332
Line 332: if (file_util::ReadFileToString(FilePath(file_path), &file_contents))
{
On 2009/05/27 23:50:47, pkasting wrote:
> Nit: no { }

Done.

http://codereview.chromium.org/113927/diff/1004/5#newcode1256
Line 1256: static const CommandLine* command_line =
CommandLine::ForCurrentProcess();
On 2009/05/27 23:50:47, pkasting wrote:
> Don't make this static.  ForCurrentProcess() is fast, so you don't need to,
and
> I don't think it guarantees its return value is unchanging for the life of the
> app (though I suspect it probably is).

Done.

[Peter Kasting, 2009-05-28 18:06:21]

LGTM