Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887913004:
[Night Light] CL4: Automatic schedule backend. (Closed)
Description[Night Light] CL4: Automatic schedule backend.
Implements the backend work for the NightLight automatic scheduling.
BUG=705816
TEST=ash_unittests --gtest_filter=NightLightTest.*
TEST=ash_unittests --gtest_filter=TimeOfDayTest.*
Review-Url: https://codereview.chromium.org/2887913004
Cr-Commit-Position: refs/heads/master@{#477700}
Committed: https://chromium.googlesource.com/chromium/src/+/0057494ad8732195794a7b32078424f92a5fce41
Patch Set 1 #Patch Set 2 : WIP2 #Patch Set 3 : Working #
Total comments: 52
Patch Set 4 : James' comments #
Total comments: 7
Patch Set 5 : Comments #
Total comments: 1
Patch Set 6 : Skip test on mash_unittests #
Total comments: 8
Patch Set 7 : Rebase #Patch Set 8 : mcasas and jshin comments #Patch Set 9 : Rebase #
Total comments: 2
Patch Set 10 : jshin's comments #Patch Set 11 : Remove all geolocation code for now. #
Total comments: 6
Patch Set 12 : Nits #
Messages
Total messages: 52 (30 generated)
Description was changed from ========== [Night Light] CL4: Automatic schedule backend. WIP BUG=705816 ========== to ========== [Night Light] CL4: Automatic schedule backend. Implements the backend work for the NightLight automatic scheduling. BUG=705816 TEST=ash_unittests --gtest_filter=NightLightTest. ==========
afakhry@chromium.org changed reviewers: + jamescook@chromium.org
James, I hope you are in a better health today! This CL is a bit dense, and I suggest looking at the tests first to understand the expected behavior.
https://codereview.chromium.org/2887913004/diff/40001/ash/public/cpp/ash_pref... File ash/public/cpp/ash_pref_names.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/public/cpp/ash_pref... ash/public/cpp/ash_pref_names.cc:24: // 2 -> NightLight schedule times is customly set by the user. super-nit: "is custom set" or "are explicitly set" https://codereview.chromium.org/2887913004/diff/40001/ash/public/cpp/ash_pref... ash/public/cpp/ash_pref_names.cc:33: // See ash::NightLightTime. Nice docs for the prefs, btw. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:45: device::GeolocationProvider::GetInstance()->AddLocationUpdateCallback( optional: "using device::GeolocationProvider" at global scope and caching the provider pointer in a local might make the line wrapping better here. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:47: base::Unretained(this)), Double-checking: Does |this| outlive GeolocationProvider? https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:348: bool immediate_target_status = false; optional: |should_enable|? |enable_now|? https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:354: // Start: 6:00 PM today, End: 6:00 AM tomorrow, Now: 4:00PM. super-nit: "4:00 PM" with space, be consistent throughout file. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:392: DCHECK_GE(end_time, now); I find these DCHECKs very helpful, thanks. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:419: GetEnabled() ? false : true, AnimationDurationType::kLong)); nit: !GetEnabled() https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_controller.h (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.h:113: void OverrideDelegateForTesting(std::unique_ptr<Delegate> delegate); optional: SetDelegateForTesting() is a more common name https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.h:140: void RefreshScheduleTimer(base::Time start_time, nit: docs? https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.h:162: AnimationDurationType consumed_animation_type_ = optional: |last_animation_type_|? I'm a little confused about "consumed". https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_controller_unittest.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:290: // Start time is at 3:00 PM and end timeis at 8:00 PM. nit: timeis -> time is https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:324: // The ASCII art is super-helpful, thanks for doing it. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:340: // User toggles either from the SystemMenu or the System Settings toggle nit: SystemMenu -> system menu https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:371: } This test is a little long. Could it be broken into multiple pieces? It would be nice if it could be 2-3 tests, each of which tested 2-3 state transitions. I'm OK with copy/pasting some setup code, like the parts that set the schedule. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:382: controller->SetScheduleType(NightLightController::ScheduleType::kNever); optional name idea: kNever might be clearer as kNone. At first read, "never" seems like "never use night light", whereas it really means "don't use a schedule". https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:463: } Nice test suite. Thorough and easy to read. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_time.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.cc:6: #include <base/i18n/time_formatting.h> nit: blank line above, and no <> https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.cc:7: #include <base/strings/utf_string_conversions.h> no <> https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.cc:52: CHECK(base::Time::FromLocalExploded(now, &result)); Will this CHECK-fail and crash at 12:30 AM during daylight savings time transition? Maybe return a well-known time, or error? https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_time.h (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:6: #define ASH_SYSTEM_NIGHT_LIGHT_NIGHT_LIGHT_TIME_H_ I think this class should be called TimeOfDay, since that's what it does. tools/git/move_source_file.py to the rescue! https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:18: class ASH_EXPORT NightLightTime { Aside: I'm a little surprised we don't already have a time-of-day class, but I can't find one. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:25: NightLightTime(const NightLightTime& other); Does = default work here? https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:32: NightLightTime& operator=(const NightLightTime& rhs); Does = default work here? nit: move up near NightLightTime(const NightLightTime&) https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:41: base::Time ToTime() const; This might be clearer as ToTimeToday(). Also, document what happens if the time-of-day doesn't exist. For example, 12:30 AM during daylight savings time. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_time_unittest.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time_unittest.cc:33: base::Time now_today = base::Time::Now(); Can you use a fixed time, rather than Now()? Now + 24 hours is not the same time of day across daylight savings time. https://memegen.googleplex.com/11477755
Description was changed from ========== [Night Light] CL4: Automatic schedule backend. Implements the backend work for the NightLight automatic scheduling. BUG=705816 TEST=ash_unittests --gtest_filter=NightLightTest. ========== to ========== [Night Light] CL4: Automatic schedule backend. Implements the backend work for the NightLight automatic scheduling. BUG=705816 TEST=ash_unittests --gtest_filter=NightLightTest.* TEST=ash_unittests --gtest_filter=TimeOfDayTest.* ==========
afakhry@chromium.org changed reviewers: + mcasas@chromium.org
+mcasas@ for a question about using the GeolocationProvider API. https://codereview.chromium.org/2887913004/diff/40001/ash/public/cpp/ash_pref... File ash/public/cpp/ash_pref_names.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/public/cpp/ash_pref... ash/public/cpp/ash_pref_names.cc:24: // 2 -> NightLight schedule times is customly set by the user. On 2017/05/23 16:34:57, James Cook wrote: > super-nit: "is custom set" or "are explicitly set" Done. https://codereview.chromium.org/2887913004/diff/40001/ash/public/cpp/ash_pref... ash/public/cpp/ash_pref_names.cc:33: // See ash::NightLightTime. On 2017/05/23 16:34:57, James Cook wrote: > Nice docs for the prefs, btw. Thanks! https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:45: device::GeolocationProvider::GetInstance()->AddLocationUpdateCallback( On 2017/05/23 16:34:58, James Cook wrote: > optional: "using device::GeolocationProvider" at global scope and caching the > provider pointer in a local might make the line wrapping better here. Done. I also moved the creation of the delegate to OnActiveUserSessionChanged(). https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:47: base::Unretained(this)), On 2017/05/23 16:34:58, James Cook wrote: > Double-checking: Does |this| outlive GeolocationProvider? I don't think so. GeolocationProviderImpl is a singleton https://cs.chromium.org/chromium/src/device/geolocation/geolocation_provider_... so I think it will be destroyed at shutdown. |this| delegate will be destroyed with NightLightController which will be destroyed with Shell, and I think that happens before the destruction of singletons. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:348: bool immediate_target_status = false; On 2017/05/23 16:34:58, James Cook wrote: > optional: |should_enable|? |enable_now|? Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:354: // Start: 6:00 PM today, End: 6:00 AM tomorrow, Now: 4:00PM. On 2017/05/23 16:34:58, James Cook wrote: > super-nit: "4:00 PM" with space, be consistent throughout file. Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:392: DCHECK_GE(end_time, now); On 2017/05/23 16:34:58, James Cook wrote: > I find these DCHECKs very helpful, thanks. Acknowledged. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:419: GetEnabled() ? false : true, AnimationDurationType::kLong)); On 2017/05/23 16:34:58, James Cook wrote: > nit: !GetEnabled() Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_controller.h (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.h:113: void OverrideDelegateForTesting(std::unique_ptr<Delegate> delegate); On 2017/05/23 16:34:58, James Cook wrote: > optional: SetDelegateForTesting() is a more common name Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.h:140: void RefreshScheduleTimer(base::Time start_time, On 2017/05/23 16:34:58, James Cook wrote: > nit: docs? Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller.h:162: AnimationDurationType consumed_animation_type_ = On 2017/05/23 16:34:58, James Cook wrote: > optional: |last_animation_type_|? I'm a little confused about "consumed". Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_controller_unittest.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:290: // Start time is at 3:00 PM and end timeis at 8:00 PM. On 2017/05/23 16:34:58, James Cook wrote: > nit: timeis -> time is Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:324: // On 2017/05/23 16:34:58, James Cook wrote: > The ASCII art is super-helpful, thanks for doing it. You're welcome! I always find myself needing this kind of documentation: https://cs.chromium.org/chromium/src/ash/display/display_manager_unittest.cc?..., otherwise I get lost. :D https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:340: // User toggles either from the SystemMenu or the System Settings toggle On 2017/05/23 16:34:58, James Cook wrote: > nit: SystemMenu -> system menu Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:371: } On 2017/05/23 16:34:58, James Cook wrote: > This test is a little long. Could it be broken into multiple pieces? It would be > nice if it could be 2-3 tests, each of which tested 2-3 state transitions. I'm > OK with copy/pasting some setup code, like the parts that set the schedule. Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:382: controller->SetScheduleType(NightLightController::ScheduleType::kNever); On 2017/05/23 16:34:58, James Cook wrote: > optional name idea: kNever might be clearer as kNone. At first read, "never" > seems like "never use night light", whereas it really means "don't use a > schedule". I just named it after the string we will show in the system settings when there's no schedule. But yes, your suggestion makes more sense. Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_controller_unittest.cc:463: } On 2017/05/23 16:34:59, James Cook wrote: > Nice test suite. Thorough and easy to read. Acknowledged. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_time.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.cc:6: #include <base/i18n/time_formatting.h> On 2017/05/23 16:34:59, James Cook wrote: > nit: blank line above, and no <> Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.cc:7: #include <base/strings/utf_string_conversions.h> On 2017/05/23 16:34:59, James Cook wrote: > no <> Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.cc:52: CHECK(base::Time::FromLocalExploded(now, &result)); On 2017/05/23 16:34:59, James Cook wrote: > Will this CHECK-fail and crash at 12:30 AM during daylight savings time > transition? Maybe return a well-known time, or error? It seems that we have code to handle this case: https://cs.chromium.org/chromium/src/base/time/time_exploded_posix.cc?q=Time:... I'm going to follow the same pattern done in Time::LocalMidnight(): https://cs.chromium.org/chromium/src/base/time/time.cc?q=Time::LocalMidnight&... Since local midnight has the same issue with daylight savings. Please take a look. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_time.h (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:6: #define ASH_SYSTEM_NIGHT_LIGHT_NIGHT_LIGHT_TIME_H_ On 2017/05/23 16:34:59, James Cook wrote: > I think this class should be called TimeOfDay, since that's what it does. > > tools/git/move_source_file.py to the rescue! Done. Eclipse is very good at this! Just Highlight the class, press Alt+Shift+R, Rename the class, Press Ctrl+Enter, review the changes, Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:18: class ASH_EXPORT NightLightTime { On 2017/05/23 16:34:59, James Cook wrote: > Aside: I'm a little surprised we don't already have a time-of-day class, but I > can't find one. Yes, me too. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:25: NightLightTime(const NightLightTime& other); On 2017/05/23 16:34:59, James Cook wrote: > Does = default work here? Yes. Done. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:32: NightLightTime& operator=(const NightLightTime& rhs); On 2017/05/23 16:34:59, James Cook wrote: > Does = default work here? > > nit: move up near NightLightTime(const NightLightTime&) Done. I also added a test. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time.h:41: base::Time ToTime() const; On 2017/05/23 16:34:59, James Cook wrote: > This might be clearer as ToTimeToday(). > > Also, document what happens if the time-of-day doesn't exist. For example, 12:30 > AM during daylight savings time. Done. Check my comment in the .cc file. https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... File ash/system/night_light/night_light_time_unittest.cc (right): https://codereview.chromium.org/2887913004/diff/40001/ash/system/night_light/... ash/system/night_light/night_light_time_unittest.cc:33: base::Time now_today = base::Time::Now(); On 2017/05/23 16:34:59, James Cook wrote: > Can you use a fixed time, rather than Now()? > > Now + 24 hours is not the same time of day across daylight savings time. > > https://memegen.googleplex.com/11477755 Done. https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:47: geolocation_provider->UserDidOptIntoLocationServices(); +mcasas@ to review if this is the right way to use this API.
Code LGTM but do not land until mcasas has reviewed and geolocation opt-in is clarified. https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:41: NightLightControllerDelegateImpl() { I suggest a DCHECK in this constructor that an active user session is started. That makes it very clear in the code that the device owner has gone through OOBE and agreed to terms of service. You could comment that as well. I would also cite your privacy-review bug somewhere. https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... File ash/system/night_light/time_of_day.cc (right): https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... ash/system/night_light/time_of_day.cc:47: NOTREACHED(); super-nit: I still think FromLocalExploded() can fail in pathological daylight savings time transitions (specifically, for TimeOfDay == 2:30 AM on the "spring forward" DST transition that goes from 01:59 to 03:00 instantaneously). I would remove the NOTREACHED() and comment that DST is weird and it's OK to have a funky time in that rare case. https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... File ash/system/night_light/time_of_day.h (right): https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... ash/system/night_light/time_of_day.h:40: // the NULL time will be returned. nit: "NULL time" seems weird. Maybe Time() or "epoch".
afakhry@chromium.org changed reviewers: + jshin@chromium.org
Thanks, James! +jshin@chromium.org for OWNERS review of adding '+third_party/icu' to the DEPS file. https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... ash/system/night_light/night_light_controller.cc:41: NightLightControllerDelegateImpl() { On 2017/05/24 15:42:46, James Cook wrote: > I suggest a DCHECK in this constructor that an active user session is started. > That makes it very clear in the code that the device owner has gone through OOBE > and agreed to terms of service. You could comment that as well. > > I would also cite your privacy-review bug somewhere. Done. https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... File ash/system/night_light/time_of_day.cc (right): https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... ash/system/night_light/time_of_day.cc:47: NOTREACHED(); On 2017/05/24 15:42:46, James Cook wrote: > super-nit: I still think FromLocalExploded() can fail in pathological daylight > savings time transitions (specifically, for TimeOfDay == 2:30 AM on the "spring > forward" DST transition that goes from 01:59 to 03:00 instantaneously). I would > remove the NOTREACHED() and comment that DST is weird and it's OK to have a > funky time in that rare case. Done. https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... File ash/system/night_light/time_of_day.h (right): https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... ash/system/night_light/time_of_day.h:40: // the NULL time will be returned. On 2017/05/24 15:42:47, James Cook wrote: > nit: "NULL time" seems weird. Maybe Time() or "epoch". Done. https://codereview.chromium.org/2887913004/diff/80001/ash/DEPS File ash/DEPS (right): https://codereview.chromium.org/2887913004/diff/80001/ash/DEPS#newcode22 ash/DEPS:22: "+third_party/icu", +jshin@chromium.org for OWNERS review of this DEPS change.
On 2017/05/24 16:25:35, afakhry wrote: > Thanks, James! > > mailto:+jshin@chromium.org for OWNERS review of adding '+third_party/icu' to the DEPS > file. > > https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... > File ash/system/night_light/night_light_controller.cc (right): > > https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... > ash/system/night_light/night_light_controller.cc:41: > NightLightControllerDelegateImpl() { > On 2017/05/24 15:42:46, James Cook wrote: > > I suggest a DCHECK in this constructor that an active user session is started. > > That makes it very clear in the code that the device owner has gone through > OOBE > > and agreed to terms of service. You could comment that as well. > > > > I would also cite your privacy-review bug somewhere. > > Done. > > https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... > File ash/system/night_light/time_of_day.cc (right): > > https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... > ash/system/night_light/time_of_day.cc:47: NOTREACHED(); > On 2017/05/24 15:42:46, James Cook wrote: > > super-nit: I still think FromLocalExploded() can fail in pathological daylight > > savings time transitions (specifically, for TimeOfDay == 2:30 AM on the > "spring > > forward" DST transition that goes from 01:59 to 03:00 instantaneously). I > would > > remove the NOTREACHED() and comment that DST is weird and it's OK to have a > > funky time in that rare case. > > Done. > > https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... > File ash/system/night_light/time_of_day.h (right): > > https://codereview.chromium.org/2887913004/diff/60001/ash/system/night_light/... > ash/system/night_light/time_of_day.h:40: // the NULL time will be returned. > On 2017/05/24 15:42:47, James Cook wrote: > > nit: "NULL time" seems weird. Maybe Time() or "epoch". > > Done. > > https://codereview.chromium.org/2887913004/diff/80001/ash/DEPS > File ash/DEPS (right): > > https://codereview.chromium.org/2887913004/diff/80001/ash/DEPS#newcode22 > ash/DEPS:22: "+third_party/icu", > mailto:+jshin@chromium.org for OWNERS review of this DEPS change. jshin@chromium.org and mcasas@chromium.org, friendly ping. Thank you!
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by afakhry@chromium.org
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
afakhry@chromium.org changed reviewers: + jungshik@google.com
Also question to jshin, can we make third_party/icu/source/i18n/astro.h a public
header? I need this to calculate sunset and sunrise times. Currently the bots
are failing with:
/b/c/b/chromeos_daisy_chromium_compile_only_ng/src/buildtools/linux64/gn gen
//out/Release --check
-> returned 1
ERROR at //ash/system/night_light/night_light_controller.cc:15:11: Including a
private header.
#include "third_party/icu/source/i18n/astro.h"
^----------------------------------
This file is private to the target //third_party/icu:icui18n
GN gen failed: 1
https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:53: geolocation_subscription_ = geolocation_provider->AddLocationUpdateCallback( This will also enable Wifi-based location via [1], but I'm not sure if consent-wise needs more permissions or not. The code to retrieve position looks good, I wonder if you could use a mojo interface though? (Will ease servicification down the line). [1] https://cs.chromium.org/chromium/src/device/geolocation/network_location_prov...
https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:53: geolocation_subscription_ = geolocation_provider->AddLocationUpdateCallback( On 2017/05/25 16:52:27, mcasas wrote: > This will also enable Wifi-based location via [1], I believe privacy will have an issue with sending Wifi data to google without user consent! Is there a way to force it not to? I thought enable_high_accuracy = false should take care of that. > but I'm not sure if consent-wise needs more permissions > or not. The code to retrieve position looks good, I wonder > if you could use a mojo interface though? (Will ease > servicification down the line). My understanding is the geoposition mojo interface are not yet ready, are they? Can you please give an example about how to use them if they are? > > [1] > https://cs.chromium.org/chromium/src/device/geolocation/network_location_prov...
jshin@chromium.org changed reviewers: - jungshik@google.com
filed https://bugs.chromium.org/p/chromium/issues/detail?id=726654 https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:15: #include "third_party/icu/source/i18n/astro.h" You're not supposed to use a non-public/internal API. All public headers are in source/i18n/unicode and source/common/unicode. I thought I had added a guard against including private headers in third_party/icu/BUILD.gn. Yes, I did, but I may not have done the right thing. https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:75: return base::Time::FromDoubleT(astro.getSunRiseSet(sunrise) / 1000.0); According to the comment in the source file, this method only works well when the CalendarAstromer's time is near "local" noon. (By 'local', I think it means local sidereal time, but not sure). When you construct CalendarAstronomer with {long, lat}, the time is set to the current time. So, it may not work well.
On 2017/05/26 10:03:48, jungshik at Google wrote: > filed https://bugs.chromium.org/p/chromium/issues/detail?id=726654 > > https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... > File ash/system/night_light/night_light_controller.cc (right): > > https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... > ash/system/night_light/night_light_controller.cc:15: #include > "third_party/icu/source/i18n/astro.h" > You're not supposed to use a non-public/internal API. All public headers are in > source/i18n/unicode and source/common/unicode. I'll propose that this class be made public, but I'm not sure how the upstream will respond.
https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:53: geolocation_subscription_ = geolocation_provider->AddLocationUpdateCallback( On 2017/05/25 17:49:59, afakhry wrote: > On 2017/05/25 16:52:27, mcasas wrote: > > This will also enable Wifi-based location via [1], > > I believe privacy will have an issue with sending Wifi data to google without > user consent! Is there a way to force it not to? I thought enable_high_accuracy > = false should take care of that. I'm afraid |enable_high_accuracy| doesn't do that, and that's because it maps the equivalent Spec term which leaves plenty of leeway for the UA to implement it in any it sees fit (and even we leave that further to the OS in some cases e.g. Android). That said, I could imagine having some way to explicitly configure low accuracy sources. > > > but I'm not sure if consent-wise needs more permissions > > or not. The code to retrieve position looks good, I wonder > > if you could use a mojo interface though? (Will ease > > servicification down the line). > > My understanding is the geoposition mojo interface are not yet ready, are they? > Can you please give an example about how to use them if they are? > WebKit uses the Geolocation mojo API [1], which acts similarly to what you are doing here (albeit using WTF types [2]). The Geoloc service is currently offered by content/browser [3], and in due time it should be offered by //services/device. [1] https://cs.chromium.org/chromium/src/device/geolocation/public/interfaces/geo... [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/geoloc... [3] https://cs.chromium.org/chromium/src/content/public/app/mojo/content_browser_... > > > > [1] > > > https://cs.chromium.org/chromium/src/device/geolocation/network_location_prov... >
On 2017/05/26 10:03:48, jungshik at Google wrote: > filed https://bugs.chromium.org/p/chromium/issues/detail?id=726654 > > https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... > File ash/system/night_light/night_light_controller.cc (right): > > https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... > ash/system/night_light/night_light_controller.cc:15: #include > "third_party/icu/source/i18n/astro.h" > You're not supposed to use a non-public/internal API. All public headers are in > source/i18n/unicode and source/common/unicode. > > I thought I had added a guard against including private headers in > third_party/icu/BUILD.gn. Yes, I did, but I may not have done the right thing. > > https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... > ash/system/night_light/night_light_controller.cc:75: return > base::Time::FromDoubleT(astro.getSunRiseSet(sunrise) / 1000.0); > According to the comment in the source file, this method only works well when > the CalendarAstromer's time is near "local" noon. (By 'local', I think it means > local sidereal time, but not sure). > > When you construct CalendarAstronomer with {long, lat}, the time is set to the > current time. So, it may not work well. See what Android does (taken from https://android.googlesource.com/platform/frameworks/base/+/21cc4bcd01266446b... ) final CalendarAstronomer ca = new CalendarAstronomer( location.getLongitude(), location.getLatitude()); final Calendar noon = Calendar.getInstance(); noon.setTimeInMillis(timeMillis); noon.set(Calendar.HOUR_OF_DAY, 12); noon.set(Calendar.MINUTE, 0); noon.set(Calendar.SECOND, 0); noon.set(Calendar.MILLISECOND, 0); ca.setTime(noon.getTimeInMillis()); long sunriseTimeMillis = ca.getSunRiseSet(true /* rise */); long sunsetTimeMillis = ca.getSunRiseSet(false /* rise */); ......
https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:15: #include "third_party/icu/source/i18n/astro.h" On 2017/05/26 10:03:48, jungshik at Google wrote: > You're not supposed to use a non-public/internal API. All public headers are in > source/i18n/unicode and source/common/unicode. > > I thought I had added a guard against including private headers in > third_party/icu/BUILD.gn. Yes, I did, but I may not have done the right thing. > > Yes, gn gen --check catches this. Please let's push for making this a public API. Thanks! https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:53: geolocation_subscription_ = geolocation_provider->AddLocationUpdateCallback( On 2017/05/26 15:39:17, mcasas wrote: > On 2017/05/25 17:49:59, afakhry wrote: > > On 2017/05/25 16:52:27, mcasas wrote: > > > This will also enable Wifi-based location via [1], > > > > I believe privacy will have an issue with sending Wifi data to google without > > user consent! Is there a way to force it not to? I thought > enable_high_accuracy > > = false should take care of that. > > I'm afraid |enable_high_accuracy| doesn't do that, and that's > because it maps the equivalent Spec term which leaves > plenty of leeway for the UA to implement it in any it sees fit > (and even we leave that further to the OS in some cases e.g. > Android). That said, I could imagine having some way to > explicitly configure low accuracy sources. > > > > > > but I'm not sure if consent-wise needs more permissions > > > or not. The code to retrieve position looks good, I wonder > > > if you could use a mojo interface though? (Will ease > > > servicification down the line). > > > > My understanding is the geoposition mojo interface are not yet ready, are > they? > > Can you please give an example about how to use them if they are? > > > > WebKit uses the Geolocation mojo API [1], which acts similarly to > what you are doing here (albeit using WTF types [2]). The Geoloc > service is currently offered by content/browser [3], and in due time > it should be offered by //services/device. > > > [1] > https://cs.chromium.org/chromium/src/device/geolocation/public/interfaces/geo... > [2] > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/geoloc... > [3] > https://cs.chromium.org/chromium/src/content/public/app/mojo/content_browser_... > > > > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/device/geolocation/network_location_prov... > > > Since now we know we can't send wifi data without user consent, I uploaded a CL https://codereview.chromium.org/2901413006 in which I make the NetworkLocationProvider not send wifi data if it was started with enable_high_accuracy is set to false. Please take a look. Then I will use the mojo APIs if that CL is a good enough solution to the problem. https://codereview.chromium.org/2887913004/diff/100001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:75: return base::Time::FromDoubleT(astro.getSunRiseSet(sunrise) / 1000.0); On 2017/05/26 10:03:48, jungshik at Google wrote: > According to the comment in the source file, this method only works well when > the CalendarAstromer's time is near "local" noon. (By 'local', I think it means > local sidereal time, but not sure). > > When you construct CalendarAstronomer with {long, lat}, the time is set to the > current time. So, it may not work well. Thanks for catching this. Done. Please take a look.
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jshin@ Now that the astro.h has been made public and the trybots are not complaining anymore, Can you please approve the ash/DEPS change to include icu? Thank you.
ash icu dependency lgtm. Please, address the following issue. https://codereview.chromium.org/2887913004/diff/160001/ash/system/night_light... File ash/system/night_light/time_of_day_unittest.cc (right): https://codereview.chromium.org/2887913004/diff/160001/ash/system/night_light... ash/system/night_light/time_of_day_unittest.cc:31: EXPECT_EQ("6:32 PM", time1.ToString()); This will fail in some locales because the format is different. e.g. it can be '18:32" in some locales. In other locales, "PM" (location and string) can be different, etc. You can use this. test::ScopedRestoreICUDefaultLocale restore_locale; i18n::SetICUDefaultLocale("en_US");
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jshin, thanks! james, we started the UI review for the permission dialog. I will upload a patch containing the implementation of that once it's finalized. https://codereview.chromium.org/2887913004/diff/160001/ash/system/night_light... File ash/system/night_light/time_of_day_unittest.cc (right): https://codereview.chromium.org/2887913004/diff/160001/ash/system/night_light... ash/system/night_light/time_of_day_unittest.cc:31: EXPECT_EQ("6:32 PM", time1.ToString()); On 2017/06/05 23:17:43, jungshik at Google wrote: > This will fail in some locales because the format is different. e.g. it can be > '18:32" in some locales. In other locales, "PM" (location and string) can be > different, etc. > > You can use this. > > test::ScopedRestoreICUDefaultLocale restore_locale; > i18n::SetICUDefaultLocale("en_US"); Done. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
James, I removed all geolocation code for now, until we finalize the user permission dialog. Please take a look at NightLightControllerDelegateImpl.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM with a nit and some optional ideas https://codereview.chromium.org/2887913004/diff/200001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2887913004/diff/200001/ash/BUILD.gn#newcode857 ash/BUILD.gn:857: "//device/geolocation", Not needed for this CL. https://codereview.chromium.org/2887913004/diff/200001/ash/system/night_light... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/200001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:66: astro.setTime(noon.ToTimeToday().ToDoubleT() * 1000.0); optional: you could introduce local variables that have ms and sec in the names: double noon_today_sec = noon.ToTimeToday().ToDoubleT(); astro.setTime(noon_in_sec * 1000.0); ... double sun_rise_set_ms = astro.getSunRiseSet(sunrise); return base::Time::FromDoubleT(sun_rise_set_in_ms / 1000.0); https://codereview.chromium.org/2887913004/diff/200001/ash/system/night_light... File ash/system/night_light/night_light_controller.h (right): https://codereview.chromium.org/2887913004/diff/200001/ash/system/night_light... ash/system/night_light/night_light_controller.h:42: enum class AnimationDurationType { optional: This could just be AnimationDuration (and the getters animation_duration() and last_animation_duration())
https://codereview.chromium.org/2887913004/diff/200001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2887913004/diff/200001/ash/BUILD.gn#newcode857 ash/BUILD.gn:857: "//device/geolocation", On 2017/06/07 01:00:14, James Cook wrote: > Not needed for this CL. Done. https://codereview.chromium.org/2887913004/diff/200001/ash/system/night_light... File ash/system/night_light/night_light_controller.cc (right): https://codereview.chromium.org/2887913004/diff/200001/ash/system/night_light... ash/system/night_light/night_light_controller.cc:66: astro.setTime(noon.ToTimeToday().ToDoubleT() * 1000.0); On 2017/06/07 01:00:14, James Cook wrote: > optional: you could introduce local variables that have ms and sec in the names: > > double noon_today_sec = noon.ToTimeToday().ToDoubleT(); > astro.setTime(noon_in_sec * 1000.0); > > ... > > double sun_rise_set_ms = astro.getSunRiseSet(sunrise); > return base::Time::FromDoubleT(sun_rise_set_in_ms / 1000.0); Done. https://codereview.chromium.org/2887913004/diff/200001/ash/system/night_light... File ash/system/night_light/night_light_controller.h (right): https://codereview.chromium.org/2887913004/diff/200001/ash/system/night_light... ash/system/night_light/night_light_controller.h:42: enum class AnimationDurationType { On 2017/06/07 01:00:14, James Cook wrote: > optional: This could just be AnimationDuration (and the getters > animation_duration() and last_animation_duration()) Done.
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2887913004/#ps220001 (title: "Nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496852400607930,
"parent_rev": "f5a32df768b6d95dc80d4f2dd900ff16bfed00aa", "commit_rev":
"0057494ad8732195794a7b32078424f92a5fce41"}
Message was sent while issue was closed.
Description was changed from ========== [Night Light] CL4: Automatic schedule backend. Implements the backend work for the NightLight automatic scheduling. BUG=705816 TEST=ash_unittests --gtest_filter=NightLightTest.* TEST=ash_unittests --gtest_filter=TimeOfDayTest.* ========== to ========== [Night Light] CL4: Automatic schedule backend. Implements the backend work for the NightLight automatic scheduling. BUG=705816 TEST=ash_unittests --gtest_filter=NightLightTest.* TEST=ash_unittests --gtest_filter=TimeOfDayTest.* Review-Url: https://codereview.chromium.org/2887913004 Cr-Commit-Position: refs/heads/master@{#477700} Committed: https://chromium.googlesource.com/chromium/src/+/0057494ad8732195794a7b320784... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/0057494ad8732195794a7b320784... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
