Issue 247663003: Date and Time dialog for when the clock isn't synced.

Issue 247663003: Date and Time dialog for when the clock isn't synced. (Closed)

Created:
6 years, 8 months ago by michaelpg

Modified:
6 years, 8 months ago

Reviewers:
stevenjb, Nikita (slow), Daniel Erat, Dan Beam, Alexei Svitkine (slow), sky

CC:
chromium-reviews, dbeam+watch-options_chromium.org, sadrul, nkostylev+watch_chromium.org, ben+ash_chromium.org, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, arv+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Date and Time dialog for when the clock isn't synced. If the time has not been set automatically via network syncing, the system clock can be changed. This dialog allows the user to change the date, time and time zone when possible. The motivation is to unbrick devices with the wrong time that need to be rolled out into time-sensitive networks. Documentation of the changes to Chrome OS and Ash for this CL is here: https://docs.google.com/a/google.com/drawings/d/1T3demthtROnXf1iE31p5aIPcQzK9SmjfE9EjnJbZ4zs System Time Manual Update UI design doc: https://docs.google.com/a/google.com/document/d/1djzhBrtbx-52Gctp3Fd5MIosARbTwQh_lMmd_qUnqgo Screenshot with timezone: https://drive.google.com/a/google.com/file/d/0B6HSBrih6pNUd3p2SFBoVktjVzQ Screenshot from settings page, no timezone: https://drive.google.com/a/google.com/file/d/0B6HSBrih6pNUXzk0TjNiT0tKMTQ BUG=232066 TEST=SetTimeWebUITest, DateTimeOptionsWebUITest R=stevenjb@chromium.org,nkostylev@chromium.org,dbeam@chromium.org,derat@chromium.org,asvitkine@chromium.org TBR=sky@chromium.org # TBR for adding resources to chrome/browser/chrome_resources.grd Please review: stevenjb@chromium.org - ash/system - chromeos/dbus derat@chromium.org: - chrome/browser/chromeos - chrome/browser/ui/ash - chromeos/dbus (optional) nkostylev@chromium.org: - chrome/app - chrome/browser/chromeos - chrome/browser/resources/chromeos - chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc - chrome/browser/ui/webui/chromeos - chrome/browser/ui/webui/options/chromeos - chrome/browser/browser_resources.grd - chrome/chrome_*.gypi - chrome/common dbeam@chromium.org: - chrome/browser/resources/options - chrome/browser/ui/webui/options/*.cc sky@chromium.org: - chrome/browser/browser_resources.grd Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266431

Patch Set 1 : Set Time Dialog #

Total comments: 144

Patch Set 2 : Addressed feedback #

Total comments: 26

Patch Set 3 : Addressed new feedback #

Patch Set 4 : Browser tests #

Total comments: 2

Patch Set 5 : Separate actions for date tray view #

Total comments: 6

Patch Set 6 : #

Total comments: 44

Patch Set 7 : Moar edits #

Total comments: 16

Patch Set 8 : #

Total comments: 18

Patch Set 9 : Observe system clock #

Total comments: 8

Patch Set 10 : a11y #

Total comments: 5

Patch Set 11 : Feedback #

Patch Set 12 : rebase, single quotes in browsertest js #

Created: 6 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1020 lines, -40 lines)			Patch
M	ash/system/chromeos/system_clock_observer.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+5 lines, -0 lines	0 comments	Download
M	ash/system/chromeos/system_clock_observer.cc	View	1 2 3 4 5 6 7 8 9 10	2 chunks	+8 lines, -1 line	0 comments	Download
M	ash/system/date/clock_observer.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	ash/system/date/date_default_view.cc	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	ash/system/date/date_view.h	View	1 2 3 4	2 chunks	+4 lines, -4 lines	0 comments	Download
M	ash/system/date/date_view.cc	View	1 2 3 4 5 6	3 chunks	+12 lines, -10 lines	0 comments	Download
M	ash/system/date/tray_date.h	View	1 2 3 4 5 6 7 8 9 10	2 chunks	+11 lines, -2 lines	0 comments	Download
M	ash/system/date/tray_date.cc	View	1 2 3 4 5 6 7 8 9 10	3 chunks	+18 lines, -1 line	0 comments	Download
M	ash/system/tray/default_system_tray_delegate.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	ash/system/tray/default_system_tray_delegate.cc	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	ash/system/tray/system_tray_delegate.h	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	ash/system/tray/system_tray_notifier.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	ash/system/tray/system_tray_notifier.cc	View		1 chunk	+6 lines, -0 lines	0 comments	Download
M	chrome/app/chromeos_strings.grdp	View	1 2 3 4 5 6 7 8 9	2 chunks	+22 lines, -0 lines	0 comments	Download
M	chrome/browser/browser_resources.grd	View	1 2	1 chunk	+5 lines, -0 lines	0 comments	Download
A +	chrome/browser/chromeos/set_time_dialog.h	View	1 2	2 chunks	+15 lines, -13 lines	0 comments	Download
A	chrome/browser/chromeos/set_time_dialog.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+85 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/chromeos/set_time.css	View	1 2	1 chunk	+45 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/chromeos/set_time.html	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+39 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/chromeos/set_time.js	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+200 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/options/browser_options.html	View	1 2	1 chunk	+7 lines, -1 line	0 comments	Download
M	chrome/browser/resources/options/browser_options.js	View	1 2 3 4 5 6 7 8 9 10	3 chunks	+25 lines, -1 line	0 comments	Download
M	chrome/browser/ui/ash/system_tray_delegate_chromeos.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/ash/system_tray_delegate_chromeos.cc	View	1 2 3 4 5	2 chunks	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/ash/system_tray_delegate_win.cc	View	1 2 3 4 5	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc	View		2 chunks	+3 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/webui/chromeos/set_time_ui.h	View	1	1 chunk	+25 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/webui/chromeos/set_time_ui.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+146 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/webui/chromeos/set_time_ui_browsertest.js	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+37 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/webui/options/chromeos/date_time_options_browsertest.js	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+38 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/webui/options/chromeos/date_time_options_handler.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+47 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+81 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/webui/options/options_ui.cc	View		2 chunks	+3 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser_chromeos.gypi	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser_ui.gypi	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+4 lines, -0 lines	0 comments	Download
M	chrome/chrome_tests.gypi	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -0 lines	0 comments	Download
M	chrome/common/url_constants.h	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	chrome/common/url_constants.cc	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	chromeos/dbus/fake_system_clock_client.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	chromeos/dbus/fake_system_clock_client.cc	View		1 chunk	+7 lines, -0 lines	0 comments	Download
M	chromeos/dbus/system_clock_client.h	View	1 2	2 chunks	+15 lines, -3 lines	0 comments	Download
M	chromeos/dbus/system_clock_client.cc	View	1 2	5 chunks	+73 lines, -3 lines	0 comments	Download
M	tools/metrics/actions/actions.xml	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+5 lines, -0 lines	0 comments	Download

Messages

Total messages: 45 (0 generated)

Expand Messages | Collapse Messages

Nikita (slow)

Michael, can you please specify which parts of this CL should each of us review.

6 years, 8 months ago (2014-04-23 10:27:41 UTC) #1

michaelpg

On 2014/04/23 10:27:41, Nikita Kostylev wrote: > Michael, can you please specify which parts of ...

6 years, 8 months ago (2014-04-23 12:50:09 UTC) #2

Daniel Erat

i looked at the following: - chrome/browser/chromeos - chrome/browser/ui/ash - chromeos/dbus https://codereview.chromium.org/247663003/diff/20001/chrome/browser/chromeos/set_time_dialog.h File chrome/browser/chromeos/set_time_dialog.h (right): ...

6 years, 8 months ago (2014-04-23 16:33:22 UTC) #3

Dan Beam

out of curiosity, did you check (with someone like ben@) that using WebUI is OK ...

6 years, 8 months ago (2014-04-23 17:51:44 UTC) #4

michaelpg

Thanks for the detailed review, Dan! I haven't written much JavaScript at Google yet. On ...

6 years, 8 months ago (2014-04-23 18:40:06 UTC) #5

Thanks for the detailed review, Dan! I haven't written much JavaScript at Google
yet.

On 2014/04/23 17:51:44, Dan Beam wrote:
> out of curiosity, did you check (with someone like ben@) that using WebUI is
OK
> for this?
> 

It was mentioned & discussed in the design doc. The main reason to use WebUI was
the existence of well-tested and accessible HTML5 date and time controls. But
no, I didn't think to ask ben@. Would you mind elaborating on the process I
should have used to get this OKed?

[snipped. apparently we have a 10000 character limit in replies?]

>
https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/options/chromeos/date_time_options_handler.h
> (right):
> 
>
https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/options/chromeos/date_time_options_handler.h:31:
virtual
> void SystemClockCanSetTimeChanged(bool can_set_time) OVERRIDE;
> nit: there is a school of folks that don't bother putting base class methods
in
> private: sections when deriving.  this is because callers can do:
> 
>   static_cast<Base*>(derived)->PrivateMethod()

Yes, PrivateMethod can be called either way, but using the derived pointer will
look more "wrong" in code reviews.

I like private in this case because it declares my intention for
SystemClockCanSetTimeChanged to be called through the observer interface. It
forces one to use the observer as is (logically, but not to the compiler as you
point out).

> 
> but either way this is popular in chrome code (I don't understand why but my
C++
> fu isn't great)

I don't feel strongly about it, so I'm happy to do whatever is more common.

> 
>
https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/options/chromeos/date_time_options_handler.h:33: //
> Callback for showing the set time dialog from the settings page.
> please mention which arguments this handler expect and the exact message name
> (e.g.  Callback for the "showSetTime" message)

Dan Beam

fyi: here's some screenshots of this dialog in action - http://imgur.com/a/zyzyX

6 years, 8 months ago (2014-04-23 19:48:04 UTC) #6

stevenjb

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc#newcode489 chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:489: GetUserLoginStatus() == ash::user::LOGGED_IN_NONE) { nit: Only call GetUserLoginStatus() once. ...

6 years, 8 months ago (2014-04-23 21:32:03 UTC) #7

michaelpg

Thanks for looking. https://codereview.chromium.org/247663003/diff/20001/chrome/browser/chromeos/set_time_dialog.h File chrome/browser/chromeos/set_time_dialog.h (right): https://codereview.chromium.org/247663003/diff/20001/chrome/browser/chromeos/set_time_dialog.h#newcode25 chrome/browser/chromeos/set_time_dialog.h:25: protected: On 2014/04/23 16:33:22, Daniel Erat ...

6 years, 8 months ago (2014-04-24 01:32:22 UTC) #8

Nikita (slow)

Can you please provide screenshots? Does this dialog honor 12h/24h pref? https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources/chromeos/set_time.css File chrome/browser/resources/chromeos/set_time.css (right): ...

6 years, 8 months ago (2014-04-24 14:31:52 UTC) #9

stevenjb

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/chromeos/set_time_ui.cc File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right): https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/chromeos/set_time_ui.cc#newcode49 chrome/browser/ui/webui/chromeos/set_time_ui.cc:49: SetTimeMessageHandler::SetTimeMessageHandler() { On 2014/04/24 01:32:23, Michael Giuffrida wrote: > ...

6 years, 8 months ago (2014-04-24 17:06:22 UTC) #10

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:49:
SetTimeMessageHandler::SetTimeMessageHandler() {
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 17:51:45, Dan Beam wrote:
> > just put the implementations right after the method prototype, e.g.
> > 
> >   class SetTimeMessageHandler ... {
> >    public:
> >     SetTimeMessageHandler() {
> >       system::TimezoneSettings::GetInstance()->AddObserver(this);
> >     }
> 
> We try to avoid inlining most functions.
Actually, that is something of a judgement call. We never want to inline
non-trivial or setter/getter functions in a header, but for local classes,
either is fine. Scanning this directory there is a mix of both, so whichever you
prefer. (FWIW, for a small class like this, I prefer inlining the
implementation, but I wouldn't necessarily change it).

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:115: if (login_status ==
ash::user::LOGGED_IN_LOCKED ||
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> Will this dialog be available from lock screen?
> I don't think that's necessary.

The logic and the comment above are inverted which is confusing; it took me a
bit to parse this. Also, I think !LoginState::Get()->IsUserLoggedIn() is better
/ more clear here, per Nikita's comment.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc
(right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc:33:
base::Unretained(this)));
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> nit: Align Bind parameters.

For new files, I highly recommend using clang-format. (Tip: In your branch, with
a clean tree, run 'git cl format' and it will re-format only files you have
changed without committing anything. You can revert noisy changes in existing
files).

michaelpg

I've added updated screenshots to the description. Unfortunately, the Chrome <input type="time"> control does not ...

6 years, 8 months ago (2014-04-24 18:52:31 UTC) #11

I've added updated screenshots to the description.

Unfortunately, the Chrome <input type="time"> control does not respect Chrome
OS's 24-hour clock preferences -- it decides 12 vs. 24-hour based on the locale,
and that behavior can't be overridden. Perhaps we could look into integrating
the 24-hour preference into the system locale if that's not being done properly?

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:49:
SetTimeMessageHandler::SetTimeMessageHandler() {
On 2014/04/24 17:06:23, stevenjb wrote:
> On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > just put the implementations right after the method prototype, e.g.
> > > 
> > >   class SetTimeMessageHandler ... {
> > >    public:
> > >     SetTimeMessageHandler() {
> > >       system::TimezoneSettings::GetInstance()->AddObserver(this);
> > >     }
> > 
> > We try to avoid inlining most functions.
> Actually, that is something of a judgement call. We never want to inline
> non-trivial or setter/getter functions in a header, but for local classes,
> either is fine. Scanning this directory there is a mix of both, so whichever
you
> prefer. (FWIW, for a small class like this, I prefer inlining the
> implementation, but I wouldn't necessarily change it).

The C++ Do's & Dont's file has a few arguments for following the same inlining
heuristics in .cc files but like you say it's not wrong either way. I'll leave
this as is.

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:117: login_status ==
ash::user::LOGGED_IN_NONE) {
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 21:32:04, stevenjb wrote:
> > An easier way to do this is LoginState::Get()->IsUserLoggedIn()
> 
> I'm intentionally using the Ash status. Specifically if the screen is locked,
we
> should include the timezone in the dialog:
> 
> if (!LoginState::Get()->IsUserLoggedIn()) {
>   // Include the timezone dropdown, since we're not logged in
>   // (meaning we didn't launch from Settings)
> }
> // Uh-oh, if the screen is locked we aren't including the timezone dropdown.

OK - so we won't show the dialog at all if the screen is locked.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/set_time.css (right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.css:15: display: -webkit-flex;
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> display: flex;

Done.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.css:21: display: block;
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> nit: Is it feasible to make this display: flex?

Done.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/set_time.html (right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.html:24: <input type="date"
id="date">
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> nit: Place id attribute first for all elements in this file.

Done.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.html:222: <div
class="settings-row" id="set-time" hidden>
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> nit: Place id attribute first.

Done.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:299: if
($('set-time-button'))
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> nit: Hide behind if (cr.isChromeOS) ?

That's what I had originally, I think dbeam was suggesting this, how about using
both?

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:84: void
SetTimeMessageHandler::TimezoneChanged(const icu::TimeZone& timezone) {
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> Do you need the same one OnTimeChanged()?

Hmm, sure. It's a bit weird because if the time changes (for reasons other than
the dialog setting the time) that most likely means the time has synced, in
which case the dialog is no longer necessary. But the behavior in that case is
to just to leave the dialog open, whatever time the user sets won't be applied
and the dialog won't be accessible after that.

So in short, yes there's no harm in updating the dialog's time when the time
changes.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:115: if (login_status ==
ash::user::LOGGED_IN_LOCKED ||
On 2014/04/24 17:06:23, stevenjb wrote:
> On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> > Will this dialog be available from lock screen?
> > I don't think that's necessary.
> 
> The logic and the comment above are inverted which is confusing; it took me a
> bit to parse this. Also, I think !LoginState::Get()->IsUserLoggedIn() is
better
> / more clear here, per Nikita's comment.

I didn't realize the Settings button isn't available on the lock screen. I'll
leave this dialog out of the lock screen and set the tray date item as
non-actionable.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc
(right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc:31:
web_ui()->RegisterMessageCallback("showSetTime",
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> nit: Put "showSetTime" on next line.

Done.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc:33:
base::Unretained(this)));
On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> nit: Align Bind parameters.

Done.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc:33:
base::Unretained(this)));
On 2014/04/24 17:06:23, stevenjb wrote:
> On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> > nit: Align Bind parameters.
> 
> For new files, I highly recommend using clang-format. (Tip: In your branch,
with
> a clean tree, run 'git cl format' and it will re-format only files you have
> changed without committing anything. You can revert noisy changes in existing
> files).
> 

Done.

Daniel Erat

https://codereview.chromium.org/247663003/diff/100001/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/247663003/diff/100001/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc#newcode482 chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:482: SetTimeDialog::ShowDialog(GetNativeWindow()); this method seems like a strange place to ...

6 years, 8 months ago (2014-04-24 19:49:59 UTC) #12

michaelpg

Changed the tray delegate logic per derat@'s suggestion. @asvitkine, I've added "ShowSetTimeDialog" to tools/metrics/actions/actions.xml. https://codereview.chromium.org/247663003/diff/100001/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc ...

6 years, 8 months ago (2014-04-24 20:37:11 UTC) #13

Alexei Svitkine (slow)

https://codereview.chromium.org/247663003/diff/130001/tools/metrics/actions/actions.xml File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/247663003/diff/130001/tools/metrics/actions/actions.xml#newcode9966 tools/metrics/actions/actions.xml:9966: <description>Please enter the description of the metric.</description> Please fill ...

6 years, 8 months ago (2014-04-24 20:44:32 UTC) #14

Daniel Erat

https://codereview.chromium.org/247663003/diff/130001/chrome/browser/ui/ash/system_tray_delegate_win.cc File chrome/browser/ui/ash/system_tray_delegate_win.cc (right): https://codereview.chromium.org/247663003/diff/130001/chrome/browser/ui/ash/system_tray_delegate_win.cc#newcode106 chrome/browser/ui/ash/system_tray_delegate_win.cc:106: virtual void ShowSetTimeSetings() OVERRIDE { probably doesn't compile (should ...

6 years, 8 months ago (2014-04-24 20:46:18 UTC) #15

michaelpg

6 years, 8 months ago (2014-04-24 20:55:47 UTC) #16

Dan Beam

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources/chromeos/set_time.js File chrome/browser/resources/chromeos/set_time.js (right): https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources/chromeos/set_time.js#newcode33 chrome/browser/resources/chromeos/set_time.js:33: this.maxDate_.setYear(this.minDate_.getFullYear() + 20); On 2014/04/24 01:32:23, Michael Giuffrida wrote: ...

6 years, 8 months ago (2014-04-24 21:32:22 UTC) #18

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/set_time.js (right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.js:33:
this.maxDate_.setYear(this.minDate_.getFullYear() + 20);
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 17:51:45, Dan Beam wrote:
> > what happens at 2018-01-19 03:14:07 UTC?
> 
> tlsdate accepts times up to 20 years after the build time. I've asked about
this
> but haven't heard back. I'll see if I can get an explanation.
> 
> But to answer your question, Skynet goes live.

i assume you mean the year changes to 1970 because Date overflows?

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.js:120: return;
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 17:51:45, Dan Beam wrote:
> > I'm confused, why we would ever hit this?
> 
> We wouldn't, this code is vestigial.

don't write vestigal code ;)

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.js:136: chrome.send('setTimezone',
[e.target.value]);
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 17:51:45, Dan Beam wrote:
> > nit: e.currentTarget
> 
> Not sure I follow. is that to make sure we get the value of the select, not
the
> option element?

this is to make your code impervious to event delegation

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.js:144: chrome.send('DialogClose');
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 17:51:45, Dan Beam wrote:
> > s/DialogClose/dialogClose/
> 
> I know, but no.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=%5C%22dialogclose%5C%...

rename it

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:299: if (cr.isChromeOS) {
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 17:51:45, Dan Beam wrote:
> > nit:
> > 
> >   if ($('set-time-button'))
> > 
> > just like themes-native-button above
> 
> get rid of the isChromeOS check? or just add this as well?

get right of the isChromeOS check, full code would be:

  if ($('set-time-button'))
    $('set-time-button').onclick = this.handleSetTime_.bind(this);

> 
> themes-native-button is wrapped in an if (cr.isChromeOS) {} else {...}. 

wrap the HTML in a:

<if expr="chromeos">
      <!-- set time section -->
</if>

which is basically an #if defined(OS_CHROMEOS) for HTML

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:301: }
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 17:51:45, Dan Beam wrote:
> > nit: no curlies
> 
> based on this comment I guess you mean "get rid of the isChromeOS check".

right

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1742: canSetTimeChanged_:
function(canSetTime) {
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 17:51:45, Dan Beam wrote:
> > nit: can* methods sound like they'll return a boolean whether something's
> > allowed.  updateSetTimeVisibility_ instead (while this method only controls
> > visibility, at least)?
> 
> How about toggleCanSetTime?

I think it's better to set it explicitly one way or another rather than toggle.

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:49:
SetTimeMessageHandler::SetTimeMessageHandler() {
On 2014/04/24 17:06:23, stevenjb wrote:
> On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > just put the implementations right after the method prototype, e.g.
> > > 
> > >   class SetTimeMessageHandler ... {
> > >    public:
> > >     SetTimeMessageHandler() {
> > >       system::TimezoneSettings::GetInstance()->AddObserver(this);
> > >     }
> > 
> > We try to avoid inlining most functions.
> Actually, that is something of a judgement call. We never want to inline
> non-trivial or setter/getter functions in a header, but for local classes,
> either is fine. Scanning this directory there is a mix of both, so whichever
you
> prefer. (FWIW, for a small class like this, I prefer inlining the
> implementation, but I wouldn't necessarily change it).

i think it'll result in shorter, simpler code (which definitely sounds worth it)

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc
(right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc:29:
DBusThreadManager::Get()->GetSystemClockClient()->RemoveObserver(this);
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 17:51:45, Dan Beam wrote:
> > can this class go down before InitializeHandler()?  probably not... but it
> might
> > be worthwhile to know what happens if you try to remove an observer that
> doesn't
> > exist.
> 
> I looked but there isn't a simple answer to the first question. However, there
> is no harm in calling RemoveObserver on an observer list with an observer that
> isn't in the list (other than the nanoseconds wasted searching for the
> observer). I've added a comment to SystemClockClient to emphasize this.

that's fine

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc:41: }
On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> On 2014/04/23 21:32:04, stevenjb wrote:
> > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > one wonders why this is pure virtual if some classes don't need it...
> > Frequently that is done so that implementations don't forget to include it.
> Not
> > sure that it matters either way here.

right, here I don't think it matters much

> > 
> 
> Interestingly it is the only pure virtual method in OptionsPageUIHandler.

yes, I know... probably doesn't need to be.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:299: if
($('set-time-button'))
On 2014/04/24 18:52:32, Michael Giuffrida wrote:
> On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> > nit: Hide behind if (cr.isChromeOS) ?
> 
> That's what I had originally, I think dbeam was suggesting this, how about
using
> both?

that makes little sense to me

Dan Beam

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resources/chromeos/set_time.js File chrome/browser/resources/chromeos/set_time.js (right): https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resources/chromeos/set_time.js#newcode53 chrome/browser/resources/chromeos/set_time.js:53: if (currentTimezoneId !== '') { if (currentTimezoneId) https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resources/chromeos/set_time.js#newcode98 chrome/browser/resources/chromeos/set_time.js:98: ...

6 years, 8 months ago (2014-04-24 21:54:32 UTC) #19

michaelpg

Thanks, Dan. I believe this addresses everything you've brought up so far, except for the ...

6 years, 8 months ago (2014-04-24 22:42:47 UTC) #20

Thanks, Dan. I believe this addresses everything you've brought up so far,
except for the "DialogClose" method that I'll create a separate CL for.

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/set_time.js (right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.js:33:
this.maxDate_.setYear(this.minDate_.getFullYear() + 20);
On 2014/04/24 21:32:23, Dan Beam wrote:
> On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > what happens at 2018-01-19 03:14:07 UTC?
> > 
> > tlsdate accepts times up to 20 years after the build time. I've asked about
> this
> > but haven't heard back. I'll see if I can get an explanation.
> > 
> > But to answer your question, Skynet goes live.
> 
> i assume you mean the year changes to 1970 because Date overflows?

No, Date doesn't overflow. The message will be passed correctly to tlsdate,
which will refuse to set the time because it's more than 20 years from now.
Since the system time can't be set past this time, it makes sense to set this
time as the max for the input field.

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.js:136: chrome.send('setTimezone',
[e.target.value]);
On 2014/04/24 21:32:23, Dan Beam wrote:
> On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > nit: e.currentTarget
> > 
> > Not sure I follow. is that to make sure we get the value of the select, not
> the
> > option element?
> 
> this is to make your code impervious to event delegation

Done.

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.js:144: chrome.send('DialogClose');
On 2014/04/24 21:32:23, Dan Beam wrote:
> On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > s/DialogClose/dialogClose/
> > 
> > I know, but no.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%5C%22dialogclose%5C%...
> 
> rename it

OK. Since that'll require changing 10 more files and grabbing more owners, do
you mind if I submit the change as a separate CL?

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:299: if (cr.isChromeOS) {
On 2014/04/24 21:32:23, Dan Beam wrote:
> On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > nit:
> > > 
> > >   if ($('set-time-button'))
> > > 
> > > just like themes-native-button above
> > 
> > get rid of the isChromeOS check? or just add this as well?
> 
> get right of the isChromeOS check, full code would be:
> 
>   if ($('set-time-button'))
>     $('set-time-button').onclick = this.handleSetTime_.bind(this);
> 
> > 
> > themes-native-button is wrapped in an if (cr.isChromeOS) {} else {...}. 
> 
> wrap the HTML in a:
> 
> <if expr="chromeos">
>       <!-- set time section -->
> </if>
> 
> which is basically an #if defined(OS_CHROMEOS) for HTML

OK, we'll do it that way (the HTML already is wrapped in that if tag)

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:301: }
On 2014/04/24 21:32:23, Dan Beam wrote:
> On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > nit: no curlies
> > 
> > based on this comment I guess you mean "get rid of the isChromeOS check".
> 
> right

Done.

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1742: canSetTimeChanged_:
function(canSetTime) {
On 2014/04/24 21:32:23, Dan Beam wrote:
> On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > nit: can* methods sound like they'll return a boolean whether something's
> > > allowed.  updateSetTimeVisibility_ instead (while this method only
controls
> > > visibility, at least)?
> > 
> > How about toggleCanSetTime?
> 
> I think it's better to set it explicitly one way or another rather than
toggle.

Done.

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:49:
SetTimeMessageHandler::SetTimeMessageHandler() {
On 2014/04/24 21:32:23, Dan Beam wrote:
> On 2014/04/24 17:06:23, stevenjb wrote:
> > On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > > just put the implementations right after the method prototype, e.g.
> > > > 
> > > >   class SetTimeMessageHandler ... {
> > > >    public:
> > > >     SetTimeMessageHandler() {
> > > >       system::TimezoneSettings::GetInstance()->AddObserver(this);
> > > >     }
> > > 
> > > We try to avoid inlining most functions.
> > Actually, that is something of a judgement call. We never want to inline
> > non-trivial or setter/getter functions in a header, but for local classes,
> > either is fine. Scanning this directory there is a mix of both, so whichever
> you
> > prefer. (FWIW, for a small class like this, I prefer inlining the
> > implementation, but I wouldn't necessarily change it).
> 
> i think it'll result in shorter, simpler code (which definitely sounds worth
it)

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
File chrome/browser/resources/chromeos/set_time.js (right):

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:53: if (currentTimezoneId !== '')
{
On 2014/04/24 21:54:33, Dan Beam wrote:
> if (currentTimezoneId)

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:98: var secondsRemaining = 60 -
now.getSeconds();  // 0 <= getSeconds() < 60
On 2014/04/24 21:54:33, Dan Beam wrote:
> remove commented code

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:124: if (e.target.validity.valid
&& e.target.value !== '') {
On 2014/04/24 21:54:33, Dan Beam wrote:
> if (e.target.validity.valid && e.target.value)

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:161: window.outerHeight -
window.innerHeight;  // title bar
On 2014/04/24 21:54:33, Dan Beam wrote:
> chrome webui code (and chrome code in general) doesn't use end-of-line
comments
> like (e.g.) closure does

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:170: * @return {Object} A pair of
strings in the following format:
On 2014/04/24 21:54:33, Dan Beam wrote:
> @return {{date: string, time: string}} Date is an RFC 3339 formatted date and
> time is a HH:MM formatted time.

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:174: *  }
On 2014/04/24 21:54:33, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:185: };
On 2014/04/24 21:54:33, Dan Beam wrote:
> nit:
> 
>   return {date: localDate.toISOString().slice(0, 10),
>           time: localDate.toISOString().slice(11, 16)};

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:205:
settime.TimeSetter.getInstance()));
On 2014/04/24 21:54:33, Dan Beam wrote:
> document.addEventListener('DOMContentLoaded', function() {
>   settime.TimeSetter.getInstance().initialize();
> });
> 
> is less lines and a hell of lot easier to read :)

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:299: if (cr.isChromeOS &&
$('set-time-button'))
On 2014/04/24 21:54:33, Dan Beam wrote:
> i still think this makes more sense to be based on whether the node exists.
> 
> at this layer, the code only cares that the node exists so it can wire up a
> click handler (and not cause a null pointer exception by calling
> $('set-time-button').doSomething()).
> 
> i also don't see a useful case where we'd include this node in the HTML (but
> hidden, maybe) but then not use it... this just slows down the page and
confuses
> other engineers, I'd assume.
> 
> hence, why I'd ifdef out the code in HTML and use if ($('set-time-button'))
here

Yep. The HTML is already ifdef'd out, so there's no reason to use cr.isChromeOS.
Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1741: toggleCanSetTime_:
function(canSetTime) {
On 2014/04/24 21:54:33, Dan Beam wrote:
> if this were a toggle function, it'd look something like this:
> 
>   $('time-synced-explanation').hidden = !$('time-synced-explanation').hidden;
>   $('set-time').hidden = !$('set-time').hidden;
> 
> why not use set*?

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1753: }
On 2014/04/24 21:54:33, Dan Beam wrote:
> nit: },

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:47: 
On 2014/04/24 21:54:33, Dan Beam wrote:
> document these (why/how are they called, what are the arguments, etc.)

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:97: }
On 2014/04/24 21:54:33, Dan Beam wrote:
> combine private classes in .cc so I don't have to look/change 2 different
places
> in the code

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:130:
values.SetDouble("buildTime", build_time.ToJsTime());
On 2014/04/24 21:54:33, Dan Beam wrote:
> nit:
> 
>   values.SetDouble("buildTime", base::GetBuildTime().ToJsTime());

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/set_time_ui_browsertest.js (right):

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui_browsertest.js:6: 
On 2014/04/24 21:54:33, Dan Beam wrote:
> /**
>  * Doc comment talking about how cool SetTimeWebUITest is.
>  * @constructor
>  * @extends {testing.Test}
>  */

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui_browsertest.js:13: * Browse to set
time dialog.
On 2014/04/24 21:54:33, Dan Beam wrote:
> @override

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui_browsertest.js:14: **/
On 2014/04/24 21:54:33, Dan Beam wrote:
> **/ -> */

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui_browsertest.js:21: var setTime =
settime.TimeSetter;
On 2014/04/24 21:54:33, Dan Beam wrote:
> nit: this is done everywhere else in webui as
> 
>   var TimeSetter = settime.TimeSetter;

Done. Makes sense, I want the object, not the namespace.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui_browsertest.js:26:
assertEquals("America/New_York", $('timezone-select').value);
On 2014/04/24 21:54:33, Dan Beam wrote:
> use |timezone| or remove it (preferably remove it as it wouldn't save you any
> lines of code)

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/date_time_options_browsertest.js
(right):

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/date_time_options_browsertest.js:14:
**/
On 2014/04/24 21:54:33, Dan Beam wrote:
> same general nits in this file as the other one

Done.

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc
(right):

https://codereview.chromium.org/247663003/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc:37:
base::DictionaryValue* localized_strings) {
On 2014/04/24 21:54:33, Dan Beam wrote:
> still don't see the point of this...

OK, let's give it a default implementation!

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/date_time_options_handler.h
(right):

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/date_time_options_handler.h:35: // Only
expose the button and dialog of the system time can be set.
s/of/if

Dan Beam

very very close https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources/chromeos/set_time.js File chrome/browser/resources/chromeos/set_time.js (right): https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources/chromeos/set_time.js#newcode144 chrome/browser/resources/chromeos/set_time.js:144: chrome.send('DialogClose'); On 2014/04/24 22:42:47, Michael Giuffrida ...

6 years, 8 months ago (2014-04-25 00:05:48 UTC) #21

stevenjb

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/ui/webui/chromeos/set_time_ui.cc File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right): https://codereview.chromium.org/247663003/diff/190001/chrome/browser/ui/webui/chromeos/set_time_ui.cc#newcode74 chrome/browser/ui/webui/chromeos/set_time_ui.cc:74: NOTREACHED(); On 2014/04/25 00:05:49, Dan Beam wrote: > this ...

6 years, 8 months ago (2014-04-25 01:41:10 UTC) #22

Dan Beam

6 years, 8 months ago (2014-04-25 02:15:52 UTC) #23

michaelpg

6 years, 8 months ago (2014-04-25 02:40:23 UTC) #24

Dan Beam

6 years, 8 months ago (2014-04-25 02:44:53 UTC) #25

Nikita (slow)

How are you hanling l10n? Specifically this dialog contents may not fit into the fixed ...

6 years, 8 months ago (2014-04-25 06:51:20 UTC) #26

michaelpg

On 2014/04/25 06:51:20, Nikita Kostylev wrote: > How are you hanling l10n? > > Specifically ...

6 years, 8 months ago (2014-04-25 07:17:10 UTC) #27

Alexei Svitkine (slow)

https://codereview.chromium.org/247663003/diff/150001/tools/metrics/actions/actions.xml File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/247663003/diff/150001/tools/metrics/actions/actions.xml#newcode9964 tools/metrics/actions/actions.xml:9964: <action name="ShowSetTimeDialog"> I know there's existing bad precedent, but ...

6 years, 8 months ago (2014-04-25 15:04:11 UTC) #28

Nikita (slow)

lgtm https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources/options/browser_options.js File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/247663003/diff/60001/chrome/browser/resources/options/browser_options.js#newcode299 chrome/browser/resources/options/browser_options.js:299: if ($('set-time-button')) On 2014/04/24 18:52:32, Michael Giuffrida wrote: ...

6 years, 8 months ago (2014-04-25 17:06:17 UTC) #29

michaelpg

OK, please take another look, hopefully this can try the CQ today! https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources/chromeos/set_time.js File chrome/browser/resources/chromeos/set_time.js ...

6 years, 8 months ago (2014-04-25 20:05:13 UTC) #30

OK, please take another look, hopefully this can try the CQ today!

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/set_time.js (right):

https://codereview.chromium.org/247663003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/set_time.js:33:
this.maxDate_.setYear(this.minDate_.getFullYear() + 20);
On 2014/04/24 22:42:47, Michael Giuffrida wrote:
> On 2014/04/24 21:32:23, Dan Beam wrote:
> > On 2014/04/24 01:32:23, Michael Giuffrida wrote:
> > > On 2014/04/23 17:51:45, Dan Beam wrote:
> > > > what happens at 2018-01-19 03:14:07 UTC?
> > > 
> > > tlsdate accepts times up to 20 years after the build time. I've asked
about
> > this
> > > but haven't heard back. I'll see if I can get an explanation.
> > > 
> > > But to answer your question, Skynet goes live.
> > 
> > i assume you mean the year changes to 1970 because Date overflows?
> 
> No, Date doesn't overflow. The message will be passed correctly to tlsdate,
> which will refuse to set the time because it's more than 20 years from now.
> Since the system time can't be set past this time, it makes sense to set this
> time as the max for the input field.

I've suggested a hard cutoff to drewry@. He liked the idea, so this will be
updated when tlsdate is updated.

The cutoff is 12/31/2179. 2180 K is the melting point of Chromium.

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/247663003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:115: if (login_status ==
ash::user::LOGGED_IN_LOCKED ||
On 2014/04/25 17:06:18, Nikita Kostylev wrote:
> On 2014/04/24 18:52:32, Michael Giuffrida wrote:
> > On 2014/04/24 17:06:23, stevenjb wrote:
> > > On 2014/04/24 14:31:53, Nikita Kostylev wrote:
> > > > Will this dialog be available from lock screen?
> > > > I don't think that's necessary.
> > > 
> > > The logic and the comment above are inverted which is confusing; it took
me
> a
> > > bit to parse this. Also, I think !LoginState::Get()->IsUserLoggedIn() is
> > better
> > > / more clear here, per Nikita's comment.
> > 
> > I didn't realize the Settings button isn't available on the lock screen.
I'll
> > leave this dialog out of the lock screen and set the tray date item as
> > non-actionable.
> 
> Do you still need LOGGED_IN_LOCKED here?

No, it is being removed.

https://codereview.chromium.org/247663003/diff/150001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/247663003/diff/150001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:9964: <action name="ShowSetTimeDialog">
On 2014/04/25 15:04:12, Alexei Svitkine wrote:
> I know there's existing bad precedent, but it's best practice to prefix the
name
> of the metric with a category followed by an underscore. That way, related
> metrics can be grouped together.
> 
> Could you do it here?

Done.

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/resource...
File chrome/browser/resources/chromeos/set_time.html (right):

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.html:15: <script
src="set_time.js"></script>
On 2014/04/25 00:05:49, Dan Beam wrote:
> ^ can you move any of these to the bottom of the page?  it should unblock the
> rendering...

Done. Doesn't seem to help much though. But I'm not seeing any lag on my Pixel.
Building for Falco now.

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/resource...
File chrome/browser/resources/chromeos/set_time.js (right):

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:149: chrome.send('DialogClose');
On 2014/04/25 17:06:18, Nikita Kostylev wrote:
> nit: dialogClose

Yes. I'm leaving it as is for now, otherwise tests will fail. When the CL to
rename DialogClose lands
I'll update this and submit at some point after that.

https://chromiumcodereview.appspot.com/251623003/

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/resource...
chrome/browser/resources/chromeos/set_time.js:163:
window.resizeTo(window.outerWidth, newHeight);
On 2014/04/25 17:06:18, Nikita Kostylev wrote:
> So this does resize the dialog as well?

It resizes.. everything? As with a normal browser window, setting the size of
the window object resizes the whole dialog (window + titlebar).

====Title====
  Content 1
  Content 2

Here the height of the contents is 2, the title bar adds 1, so
window.outerHeight is 3. If I wanted to fit a third line, I would call
window.resizeTo(width, 4). Now window.outerHeight is 4.

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:74: NOTREACHED();
On 2014/04/25 02:44:54, Dan Beam wrote:
> On 2014/04/25 02:40:24, Michael Giuffrida wrote:
> > On 2014/04/25 02:15:53, Dan Beam wrote:
> > > On 2014/04/25 01:41:11, stevenjb wrote:
> > > > On 2014/04/25 00:05:49, Dan Beam wrote:
> > > > > this only fires in debug builds, so either:
> > > > > 
> > > > >   CHECK(args->GetInteger(0, &seconds));
> > > > > 
> > > > > to make release builds crash as well OR
> > > > > 
> > > > >   if (!args->GetInteger(0, &seconds)) {
> > > > >     NOTREACHED();
> > > > >     return;
> > > > >   }
> > > > > 
> > > > > unless |SetTime()| has some resilience
> > > > This is a programming error, so we shouldn't handle it, i.e. we should
> crash
> > > in
> > > > Debug builds or fail otherwise. Having 'seconds' undefined should 'fail
> > > > otherwise', i.e. this should be fine.
> > > > 
> > > > 
> > > 
> > > |seconds| is currently an uninitialized int that can be anything from
> INT_MAX
> > > <=> INT_MIN, correct?  how can this be fine?
> > > 
> > > we should probably be initializing |seconds| to some sane-ish default and
> > either
> > > crashing or failing gracefully in release and always crashing in debug
when
> > > there's no arg.
> > 
> > Agreed. Worst case scenario is if |seconds| is uninitialized as a number
that
> > tlsdate will accept (some date in the next 20 years), because the SetTime
call
> > will "succeed".
> > 
> > I don't think it's worth crashing though (in release), returning should be
> fine.
> > 
> > But, speaking of INT_MAX. This is only good for dates until 2038. Should we
> > treat the arg as a double and convert it to int64, which is what SetTime
(and
> > tlsdate) actually take?
> 
> yes

Done.

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:86: NOTREACHED();
On 2014/04/25 00:05:49, Dan Beam wrote:
> same

Done.

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/date_time_options_handler.h
(right):

https://codereview.chromium.org/247663003/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/date_time_options_handler.h:35: // Only
expose the button and dialog of the system time can be set.
On 2014/04/24 22:42:48, Michael Giuffrida wrote:
> s/of/if

Done.

Dan Beam

lgtm w/nits https://codereview.chromium.org/247663003/diff/250001/chrome/browser/resources/chromeos/set_time.js File chrome/browser/resources/chromeos/set_time.js (right): https://codereview.chromium.org/247663003/diff/250001/chrome/browser/resources/chromeos/set_time.js#newcode89 chrome/browser/resources/chromeos/set_time.js:89: $('date').value === '' || $('time').value === '') ...

6 years, 8 months ago (2014-04-25 22:02:42 UTC) #31

Alexei Svitkine (slow)

lgtm with a suggestion https://codereview.chromium.org/247663003/diff/290001/tools/metrics/actions/actions.xml File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/247663003/diff/290001/tools/metrics/actions/actions.xml#newcode9789 tools/metrics/actions/actions.xml:9789: <action name="SetTimeDialog_Show"> Nit: How about: ...

6 years, 8 months ago (2014-04-25 22:25:26 UTC) #32

stevenjb

non-webui lgtm w/nits https://codereview.chromium.org/247663003/diff/270001/ash/system/chromeos/system_clock_observer.cc File ash/system/chromeos/system_clock_observer.cc (right): https://codereview.chromium.org/247663003/diff/270001/ash/system/chromeos/system_clock_observer.cc#newcode41 ash/system/chromeos/system_clock_observer.cc:41: ->GetSystemClockClient()->CanSetTime(); nit: I think it would ...

6 years, 8 months ago (2014-04-25 22:53:31 UTC) #33

michaelpg

Thanks all! https://codereview.chromium.org/247663003/diff/250001/chrome/browser/resources/chromeos/set_time.js File chrome/browser/resources/chromeos/set_time.js (right): https://codereview.chromium.org/247663003/diff/250001/chrome/browser/resources/chromeos/set_time.js#newcode89 chrome/browser/resources/chromeos/set_time.js:89: $('date').value === '' || $('time').value === '') ...

6 years, 8 months ago (2014-04-25 23:50:28 UTC) #34