Implemented a "Last Updated" field for the about page of Chrome OS.

Implemented a "Last Updated" field for the about page of Chrome OS.

BUG=117517
TEST=


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

[James Hawkins, 2012-04-13 03:36:17]

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h
File base/sys_info.h (right):

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h#newcode19
base/sys_info.h:19: class BASE_EXPORT SysInfo {
Boo, this should really be a namespace :(

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h#newcode81
base/sys_info.h:81: // numbers of the release. We return the time at which that
file was last
nit: Don't use pronouns, e.g. 'we', in comments; pronouns are ambiguous.

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h#newcode84
base/sys_info.h:84: static bool GetTimeLastUpdated(base::Time* time);
nit: Document |time|.

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h#newcode85
base/sys_info.h:85: #endif
#endif  // defined(OS_CHROMEOS)

https://chromiumcodereview.appspot.com/10038034/diff/7/chrome/browser/ui/webu...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/10038034/diff/7/chrome/browser/ui/webu...
chrome/browser/ui/webui/help/help_handler.cc:95: string16
CreateLastUpdatedString() {
nit: Document method.

https://chromiumcodereview.appspot.com/10038034/diff/7/chrome/browser/ui/webu...
chrome/browser/ui/webui/help/help_handler.cc:99: // If the time of the last
update cannot be retrieved, we punt and just
nit: nix 'we'.

[Kyle Horimoto, 2012-04-13 18:12:38]

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h
File base/sys_info.h (right):

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h#newcode19
base/sys_info.h:19: class BASE_EXPORT SysInfo {
On 2012/04/13 03:36:17, James Hawkins wrote:
> Boo, this should really be a namespace :(

I'll leave it as a class for this CL and redo it as a namespace in another CL
because it'll be a widespread change.

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h#newcode81
base/sys_info.h:81: // numbers of the release. We return the time at which that
file was last
On 2012/04/13 03:36:17, James Hawkins wrote:
> nit: Don't use pronouns, e.g. 'we', in comments; pronouns are ambiguous.

Done.

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h#newcode84
base/sys_info.h:84: static bool GetTimeLastUpdated(base::Time* time);
On 2012/04/13 03:36:17, James Hawkins wrote:
> nit: Document |time|.

Done.

https://chromiumcodereview.appspot.com/10038034/diff/7/base/sys_info.h#newcode85
base/sys_info.h:85: #endif
On 2012/04/13 03:36:17, James Hawkins wrote:
> #endif  // defined(OS_CHROMEOS)

Done.

https://chromiumcodereview.appspot.com/10038034/diff/7/chrome/browser/ui/webu...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/10038034/diff/7/chrome/browser/ui/webu...
chrome/browser/ui/webui/help/help_handler.cc:95: string16
CreateLastUpdatedString() {
On 2012/04/13 03:36:17, James Hawkins wrote:
> nit: Document method.

Done.

https://chromiumcodereview.appspot.com/10038034/diff/7/chrome/browser/ui/webu...
chrome/browser/ui/webui/help/help_handler.cc:99: // If the time of the last
update cannot be retrieved, we punt and just
On 2012/04/13 03:36:17, James Hawkins wrote:
> nit: nix 'we'.

Done.

[Evan Stade, 2012-04-14 00:37:46]

https://chromiumcodereview.appspot.com/10038034/diff/8001/base/sys_info_chrom...
File base/sys_info_chromeos.cc (right):

https://chromiumcodereview.appspot.com/10038034/diff/8001/base/sys_info_chrom...
base/sys_info_chromeos.cc:116: //static
// static

https://chromiumcodereview.appspot.com/10038034/diff/8001/chrome/browser/ui/w...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/10038034/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/help/help_handler.cc:101: if
(!base::SysInfo::GetTimeLastUpdated(&time))
curlies

https://chromiumcodereview.appspot.com/10038034/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/help/help_handler.cc:185:
localized_strings->SetString("lastUpdatedDate", CreateLastUpdatedString());
you can't read a file on the UI thread. You must proxy this call to the file
thread.

[Kyle Horimoto, 2012-04-16 18:32:54]

https://chromiumcodereview.appspot.com/10038034/diff/8001/base/sys_info_chrom...
File base/sys_info_chromeos.cc (right):

https://chromiumcodereview.appspot.com/10038034/diff/8001/base/sys_info_chrom...
base/sys_info_chromeos.cc:116: //static
On 2012/04/14 00:37:46, Evan Stade wrote:
> // static

Done.

https://chromiumcodereview.appspot.com/10038034/diff/8001/chrome/browser/ui/w...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/10038034/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/help/help_handler.cc:101: if
(!base::SysInfo::GetTimeLastUpdated(&time))
On 2012/04/14 00:37:46, Evan Stade wrote:
> curlies

Done.

https://chromiumcodereview.appspot.com/10038034/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/help/help_handler.cc:185:
localized_strings->SetString("lastUpdatedDate", CreateLastUpdatedString());
On 2012/04/14 00:37:46, Evan Stade wrote:
> you can't read a file on the UI thread. You must proxy this call to the file
> thread.

Done.

[Mark Mentovai, 2012-04-16 18:46:56]

LGTM for base OWNERS approval only.

I don’t personally think that this feature is particularly useful but as it’s
Chrome OS-only, I won’t argue against it.

[Evan Stade, 2012-04-16 19:42:50]

http://codereview.chromium.org/10038034/diff/15001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/15001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:196:
base::Bind(&AddLastUpdatedString, localized_strings));
there is a race here, and besides which localized_strings is not thread safe

instead of adding the last updated time to localized_strings, you should create
a javascript function call to pass along that string. Also you should cache it,
so you only have to read from disk once

and I think you'll find FileUtilProxy to be pretty helpful

[Kyle Horimoto, 2012-04-17 18:31:44]

https://chromiumcodereview.appspot.com/10038034/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/10038034/diff/15001/chrome/browser/ui/...
chrome/browser/ui/webui/help/help_handler.cc:196:
base::Bind(&AddLastUpdatedString, localized_strings));
On 2012/04/16 19:42:51, Evan Stade wrote:
> there is a race here, and besides which localized_strings is not thread safe
> 
> instead of adding the last updated time to localized_strings, you should
create
> a javascript function call to pass along that string. Also you should cache
it,
> so you only have to read from disk once
> 
> and I think you'll find FileUtilProxy to be pretty helpful

Done.

[Evan Stade, 2012-04-17 19:49:50]

http://codereview.chromium.org/10038034/diff/16006/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/16006/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:102: scoped_ptr<Value>
last_updated_string(NULL);
you can't put non-POD data here. Besides, the scope never ends, so there's no
point in using a scoped_ptr. Just use a Value*

also the name should be g_last_updated_string

[Kyle Horimoto, 2012-04-17 20:20:22]

http://codereview.chromium.org/10038034/diff/16006/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/16006/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:102: scoped_ptr<Value>
last_updated_string(NULL);
On 2012/04/17 19:49:50, Evan Stade wrote:
> you can't put non-POD data here. Besides, the scope never ends, so there's no
> point in using a scoped_ptr. Just use a Value*
> 
> also the name should be g_last_updated_string

Done.

[Mark Mentovai, 2012-04-17 20:26:31]

http://codereview.chromium.org/10038034/diff/21001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/21001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:252: if (g_last_updated_string ==
NULL) {
This looks racy. Can’t the application reach this point multiple times before
ProcessLsbFileInfo is ever called? In that case, ProcessLsbFileInfo will leak
slightly.

[Kyle Horimoto, 2012-04-17 21:20:03]

http://codereview.chromium.org/10038034/diff/21001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/21001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:252: if (g_last_updated_string ==
NULL) {
On 2012/04/17 20:26:31, Mark Mentovai wrote:
> This looks racy. Can’t the application reach this point multiple times before
> ProcessLsbFileInfo is ever called? In that case, ProcessLsbFileInfo will leak
> slightly.

Done.

[Mark Mentovai, 2012-04-17 22:03:31]

http://codereview.chromium.org/10038034/diff/20004/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/20004/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:108: base::Lock g_lock;
C’mon, you just fixed g_last_updated_string to not be of class type.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...

You can’t do this.

[Kyle Horimoto, 2012-04-17 23:37:29]

Sorry for the mistakes earlier, Mark. Didn't quite understand the global/static
object allocation error until I read through the document you sent. Does this
look better?

http://codereview.chromium.org/10038034/diff/20004/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/20004/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:108: base::Lock g_lock;
On 2012/04/17 22:03:31, Mark Mentovai wrote:
> C’mon, you just fixed g_last_updated_string to not be of class type.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...
> 
> You can’t do this.

Done.

[Mark Mentovai, 2012-04-18 15:59:07]

I will not LG the stuff outside of base, so you should still get someone else
familiar with the code to LG that portion.

http://codereview.chromium.org/10038034/diff/25002/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/25002/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:437: if (g_last_updated_string !=
NULL)
The only reason this works and isn’t racy is that this is guaranteed to only run
on one thread, right? You should assert that you’re on the correct thread with a
DCHECK.

[Kyle Horimoto, 2012-04-18 20:54:18]

Addressed comments, fixed a few things up.

http://codereview.chromium.org/10038034/diff/25002/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/25002/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:437: if (g_last_updated_string !=
NULL)
On 2012/04/18 15:59:07, Mark Mentovai wrote:
> The only reason this works and isn’t racy is that this is guaranteed to only
run
> on one thread, right? You should assert that you’re on the correct thread with
a
> DCHECK.

Yep, that's why it's not racy. I'll add that in.

[Evan Stade, 2012-04-19 03:40:27]

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:241:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
I disagree this is necessary, the one down below is OK because it's a function
that gets posted from a different thread, but we don't generally put these
guards on functions that are not inter-thread callbacks. But I don't care too
much.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:259: // is a chance that multiple
tabs could request the file information at
leaner comments. While this comment is all true, everything after the second
sentence is kind of excessive.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:270: // If it is not NULL, the date
has already been retrieved from the file
I would remove this comment; it's already covered by your variable-level comment
above.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:429: // version numbers of the
release. Thus, this is a close approximation of
remove last sentence

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:434: // the last update time to be
the current time.
instead of telling a lie, it's better to not show the last updated time at all
if we can't retrieve it.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:442: // already been retrieved by
another tab. In this case, allocate the Value
invert the first sentence (if string is not null...), and drop second sentence.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:444: if (g_last_updated_string ==
NULL)
if g_last_updated_string is not null, you needn't have done any of the work up
to this point in the function. Meaning if g_last_updated_string is non null,
skip straight to the punchline rather than constructing last_updated only to
throw it out.

[Mark Mentovai, 2012-04-19 03:44:50]

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:241:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Evan Stade wrote:
> I disagree this is necessary, the one down below is OK because it's a function
> that gets posted from a different thread, but we don't generally put these
> guards on functions that are not inter-thread callbacks. But I don't care too
> much.

To clarify, I really only thought it was necessary for the DCHECK to be in
ProcessLsbFileInfo tool.

[Kyle Horimoto, 2012-04-19 18:31:29]

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:241:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/04/19 03:40:27, Evan Stade wrote:
> I disagree this is necessary, the one down below is OK because it's a function
> that gets posted from a different thread, but we don't generally put these
> guards on functions that are not inter-thread callbacks. But I don't care too
> much.

Done.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:241:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/04/19 03:44:50, Mark Mentovai wrote:
> Evan Stade wrote:
> > I disagree this is necessary, the one down below is OK because it's a
function
> > that gets posted from a different thread, but we don't generally put these
> > guards on functions that are not inter-thread callbacks. But I don't care
too
> > much.
> 
> To clarify, I really only thought it was necessary for the DCHECK to be in
> ProcessLsbFileInfo tool.

Done.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:259: // is a chance that multiple
tabs could request the file information at
On 2012/04/19 03:40:27, Evan Stade wrote:
> leaner comments. While this comment is all true, everything after the second
> sentence is kind of excessive.

Done.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:270: // If it is not NULL, the date
has already been retrieved from the file
On 2012/04/19 03:40:27, Evan Stade wrote:
> I would remove this comment; it's already covered by your variable-level
comment
> above.

Done.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:429: // version numbers of the
release. Thus, this is a close approximation of
On 2012/04/19 03:40:27, Evan Stade wrote:
> remove last sentence

Done.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:434: // the last update time to be
the current time.
On 2012/04/19 03:40:27, Evan Stade wrote:
> instead of telling a lie, it's better to not show the last updated time at all
> if we can't retrieve it.

I made the "Last Updated" section hidden by default, and only shown if
help.HelpPage.setLastUpdated() is called.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:442: // already been retrieved by
another tab. In this case, allocate the Value
On 2012/04/19 03:40:27, Evan Stade wrote:
> invert the first sentence (if string is not null...), and drop second
sentence.

Done.

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:444: if (g_last_updated_string ==
NULL)
On 2012/04/19 03:40:27, Evan Stade wrote:
> if g_last_updated_string is not null, you needn't have done any of the work up
> to this point in the function. Meaning if g_last_updated_string is non null,
> skip straight to the punchline rather than constructing last_updated only to
> throw it out.

Done.

[Evan Stade, 2012-04-19 19:30:57]

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:444: if (g_last_updated_string ==
NULL)
On 2012/04/19 18:31:29, Kyle Horimoto wrote:
> On 2012/04/19 03:40:27, Evan Stade wrote:
> > if g_last_updated_string is not null, you needn't have done any of the work
up
> > to this point in the function. Meaning if g_last_updated_string is non null,
> > skip straight to the punchline rather than constructing last_updated only to
> > throw it out.
> 
> Done.

I don't see any changes. What I meant was something like:

if (!g_last_updated_string) {
  base::Time time;
  if (error == base::PLATFORM_FILE_OK) {
  ...
}

web_ui()->CallJavascriptFunction(...);

[Kyle Horimoto, 2012-04-19 20:07:33]

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/27001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:444: if (g_last_updated_string ==
NULL)
On 2012/04/19 19:30:57, Evan Stade wrote:
> On 2012/04/19 18:31:29, Kyle Horimoto wrote:
> > On 2012/04/19 03:40:27, Evan Stade wrote:
> > > if g_last_updated_string is not null, you needn't have done any of the
work
> up
> > > to this point in the function. Meaning if g_last_updated_string is non
null,
> > > skip straight to the punchline rather than constructing last_updated only
to
> > > throw it out.
> > 
> > Done.
> 
> I don't see any changes. What I meant was something like:
> 
> if (!g_last_updated_string) {
>   base::Time time;
>   if (error == base::PLATFORM_FILE_OK) {
>   ...
> }
> 
> web_ui()->CallJavascriptFunction(...);

Oops, misunderstood you the first time. Got it now.

[Evan Stade, 2012-04-19 22:26:57]

http://codereview.chromium.org/10038034/diff/36001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.html (right):

http://codereview.chromium.org/10038034/diff/36001/chrome/browser/resources/h...
chrome/browser/resources/help/help.html:65: <section id="last-updated-container"
hidden>
perhaps visibility: hidden would be better than hidden attribute (i.e. display:
none) for chromeos, because that way there won't be a flash/jiggle on load where
this div is unhidden, and everything below it has to move down. Other platforms
should I guess <if> guard this out.

http://codereview.chromium.org/10038034/diff/36001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10038034/diff/36001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:111: #endif // defined(OS_CHROMEOS)
in order to avoid this ugly ifdef, I would make weak_factory_ common to all
platforms even though it is only used on chromeos.

also, you only need ALLOW_THIS_IN_INITIALIZER_LIST for windows, so if you were
to leave the ifdefs here you wouldn't need it

[Kyle Horimoto, 2012-04-23 20:37:03]

https://chromiumcodereview.appspot.com/10038034/diff/36001/chrome/browser/res...
File chrome/browser/resources/help/help.html (right):

https://chromiumcodereview.appspot.com/10038034/diff/36001/chrome/browser/res...
chrome/browser/resources/help/help.html:65: <section id="last-updated-container"
hidden>
On 2012/04/19 22:26:57, Evan Stade wrote:
> perhaps visibility: hidden would be better than hidden attribute (i.e.
display:
> none) for chromeos, because that way there won't be a flash/jiggle on load
where
> this div is unhidden, and everything below it has to move down. Other
platforms
> should I guess <if> guard this out.

Changed to "visibility: hidden". The whole "More Info" area is <if> guarded out
for other platforms, so putting extra ones around this <section> is unnecessary.

https://chromiumcodereview.appspot.com/10038034/diff/36001/chrome/browser/ui/...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/10038034/diff/36001/chrome/browser/ui/...
chrome/browser/ui/webui/help/help_handler.cc:111: #endif // defined(OS_CHROMEOS)
On 2012/04/19 22:26:57, Evan Stade wrote:
> in order to avoid this ugly ifdef, I would make weak_factory_ common to all
> platforms even though it is only used on chromeos.
> 
> also, you only need ALLOW_THIS_IN_INITIALIZER_LIST for windows, so if you were
> to leave the ifdefs here you wouldn't need it

Done.

[Evan Stade, 2012-04-24 18:21:52]

lgtm with nit

http://codereview.chromium.org/10038034/diff/36001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.html (right):

http://codereview.chromium.org/10038034/diff/36001/chrome/browser/resources/h...
chrome/browser/resources/help/help.html:65: <section id="last-updated-container"
hidden>
On 2012/04/23 20:37:03, Kyle Horimoto wrote:
> On 2012/04/19 22:26:57, Evan Stade wrote:
> > perhaps visibility: hidden would be better than hidden attribute (i.e.
> display:
> > none) for chromeos, because that way there won't be a flash/jiggle on load
> where
> > this div is unhidden, and everything below it has to move down. Other
> platforms
> > should I guess <if> guard this out.
> 
> Changed to "visibility: hidden". The whole "More Info" area is <if> guarded
out
> for other platforms, so putting extra ones around this <section> is
unnecessary.

ah i see

http://codereview.chromium.org/10038034/diff/40001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.js (right):

http://codereview.chromium.org/10038034/diff/40001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:223:
$('last-updated-container').style.visibility = 'visible';
nit: the typical separation of presentation and logic would be to add an 'empty'
class to the container in the html, and remove that class here. Then in css,
#login-updated-container.empty { visibility: hidden; }

[Kyle Horimoto, 2012-04-24 21:28:49]

estade: does this look okay now?

http://codereview.chromium.org/10038034/diff/40001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.js (right):

http://codereview.chromium.org/10038034/diff/40001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:223:
$('last-updated-container').style.visibility = 'visible';
On 2012/04/24 18:21:52, Evan Stade wrote:
> nit: the typical separation of presentation and logic would be to add an
'empty'
> class to the container in the html, and remove that class here. Then in css,
> #login-updated-container.empty { visibility: hidden; }

Done.

[Evan Stade, 2012-04-24 21:30:46]

lgtm

[commit-bot: I haz the power, 2012-04-24 21:31:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10038034/47001

[commit-bot: I haz the power, 2012-04-24 21:54:51]

Try job failure for 10038034-47001 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-24 22:38:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10038034/49002

[commit-bot: I haz the power, 2012-04-25 01:59:47]

Change committed as 133840