Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(273)

Issue 19822007: Updated Google Now to Check the Geolocation Access Preference (Closed)

Created:
7 years, 5 months ago by robliao
Modified:
7 years, 4 months ago
Reviewers:
xiyuan, vadimt
CC:
chromium-reviews, arv+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@r213016
Visibility:
Public.

Description

Updated Google Now to Check the Geolocation Access Preference Hooked up the brand new chrome.preferencesPrivate.googleGeolocationAccessEnabled API to the startup path as well as control behavior on preference enable/disable events. Restructured the code for easy state machine changes. BUG=164227 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214536

Patch Set 1 #

Total comments: 22

Patch Set 2 : CR Feedback #

Total comments: 2

Patch Set 3 : CR Feedback #

Patch Set 4 : Update Unit Tests #

Total comments: 16

Patch Set 5 : CR Feedback #

Total comments: 10

Patch Set 6 : CR Feedback #

Total comments: 4

Patch Set 7 : Spacing Fix #

Total comments: 4

Patch Set 8 : CR Quick Fixup #

Patch Set 9 : Sync and Resolve Conflicts #

Unified diffs Side-by-side diffs Delta from patch set Stats (+444 lines, -118 lines) Patch
M chrome/browser/resources/google_now/background.js View 1 2 3 4 5 6 7 8 12 chunks +199 lines, -40 lines 0 comments Download
M chrome/browser/resources/google_now/background_test_util.js View 1 2 3 4 5 6 7 8 2 chunks +9 lines, -1 line 0 comments Download
M chrome/browser/resources/google_now/background_unittest.gtestjs View 1 2 3 4 5 6 7 8 4 chunks +212 lines, -74 lines 0 comments Download
M chrome/browser/resources/google_now/utility.js View 1 2 3 4 5 6 7 8 3 chunks +19 lines, -3 lines 0 comments Download
M chrome/test/data/webui/test_api.js View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (0 generated)
robliao
In the meeting, we decided we would not use a lightweight async framework... for now. ...
7 years, 5 months ago (2013-07-23 23:13:33 UTC) #1
vadimt
https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/google_now/background.js#newcode753 chrome/browser/resources/google_now/background.js:753: hideWelcomeToast(); hideWelcomeToast() should not be part of this. It ...
7 years, 5 months ago (2013-07-24 00:59:38 UTC) #2
robliao
https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/google_now/background.js#newcode753 chrome/browser/resources/google_now/background.js:753: hideWelcomeToast(); Done. On 2013/07/24 00:59:38, vadimt wrote: > hideWelcomeToast() ...
7 years, 5 months ago (2013-07-24 20:07:37 UTC) #3
vadimt
https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/google_now/background.js#newcode850 chrome/browser/resources/google_now/background.js:850: shouldRun = false; On 2013/07/24 20:07:37, Robert Liao wrote: ...
7 years, 5 months ago (2013-07-24 20:49:42 UTC) #4
robliao
https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/google_now/background.js#newcode850 chrome/browser/resources/google_now/background.js:850: shouldRun = false; State machine implementations should generally be ...
7 years, 5 months ago (2013-07-24 21:07:06 UTC) #5
vadimt
Cool. Now, please make sure unit tests pass. To run unit tests, build unit_tests target ...
7 years, 5 months ago (2013-07-24 21:27:54 UTC) #6
robliao
out\Debug\unit_tests.exe --gtest_filter=GoogleNow*.* [==========] 11 tests from 1 test case ran. (536 ms total) [ PASSED ...
7 years, 5 months ago (2013-07-25 17:29:45 UTC) #7
vadimt
https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/google_now/background.js#newcode757 chrome/browser/resources/google_now/background.js:757: stopPollingCards(); I'm afraid that this scenario is possible: 1. ...
7 years, 5 months ago (2013-07-25 20:48:01 UTC) #8
robliao
out\Debug\unit_tests.exe --gtest_filter=GoogleNow* [==========] 11 tests from 1 test case ran. (537 ms total) [ PASSED ...
7 years, 5 months ago (2013-07-25 22:35:48 UTC) #9
vadimt
https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/google_now/background.js#newcode757 chrome/browser/resources/google_now/background.js:757: stopPollingCards(); On 2013/07/25 22:35:48, Robert Liao wrote: > It ...
7 years, 5 months ago (2013-07-26 01:00:37 UTC) #10
robliao
https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/google_now/background.js#newcode757 chrome/browser/resources/google_now/background.js:757: stopPollingCards(); This would be better expressed as a bug ...
7 years, 5 months ago (2013-07-26 01:12:17 UTC) #11
vadimt
lgtm Please proceed with OWNERs. https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/google_now/background_test_util.js File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/google_now/background_test_util.js#newcode57 chrome/browser/resources/google_now/background_test_util.js:57: savedArgs.arguments.splice(0, savedArgs.arguments.length); Why not ...
7 years, 5 months ago (2013-07-26 01:54:00 UTC) #12
robliao
xiyuan: Please provide owner approval for all files in this CL. Thanks! https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/google_now/background_test_util.js File chrome/browser/resources/google_now/background_test_util.js ...
7 years, 5 months ago (2013-07-26 08:11:23 UTC) #13
xiyuan
LGTM https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/google_now/background_test_util.js File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/google_now/background_test_util.js#newcode18 chrome/browser/resources/google_now/background_test_util.js:18: onLocationUpdate: emptyListener, nit: The change seems to be ...
7 years, 5 months ago (2013-07-26 17:24:51 UTC) #14
robliao
https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/google_now/background_test_util.js File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/google_now/background_test_util.js#newcode18 chrome/browser/resources/google_now/background_test_util.js:18: onLocationUpdate: emptyListener, On 2013/07/26 17:24:51, xiyuan wrote: > nit: ...
7 years, 5 months ago (2013-07-26 19:04:32 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/62001
7 years, 5 months ago (2013-07-26 19:31:04 UTC) #16
commit-bot: I haz the power
Retried try job too often on win7_aura for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&number=63892
7 years, 4 months ago (2013-07-28 00:29:48 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/62001
7 years, 4 months ago (2013-07-28 02:54:16 UTC) #18
commit-bot: I haz the power
Failed to apply patch for chrome/browser/resources/google_now/background_test_util.js: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 4 months ago (2013-07-28 02:54:19 UTC) #19
robliao
sky: Please provide owner approval for chrome/test/data/webui/test_api.js This is a change that didn't make the ...
7 years, 4 months ago (2013-07-29 18:02:47 UTC) #20
sky
chrome/test/data/OWNERS is *, and I'm not a good reviewer for js changes anyway. Please pick ...
7 years, 4 months ago (2013-07-29 19:48:45 UTC) #21
robliao
On 2013/07/29 19:48:45, sky wrote: > chrome/test/data/OWNERS is *, and I'm not a good reviewer ...
7 years, 4 months ago (2013-07-29 19:57:59 UTC) #22
xiyuan
As sky said, chrome/test/data has its own OWNERS file and it it '*'. So anyone ...
7 years, 4 months ago (2013-07-29 20:00:35 UTC) #23
robliao
On 2013/07/29 20:00:35, xiyuan wrote: > As sky said, chrome/test/data has its own OWNERS file ...
7 years, 4 months ago (2013-07-29 20:06:30 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/101001
7 years, 4 months ago (2013-07-29 20:14:47 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/101001
7 years, 4 months ago (2013-07-30 01:41:15 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/101001
7 years, 4 months ago (2013-07-30 12:13:55 UTC) #27
commit-bot: I haz the power
7 years, 4 months ago (2013-07-31 03:17:01 UTC) #28
Message was sent while issue was closed.
Change committed as 214536

Powered by Google App Engine
This is Rietveld 408576698