|
|
Created:
7 years, 5 months ago by robliao Modified:
7 years, 4 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@r213016 Visibility:
Public. |
DescriptionUpdated 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 #
Messages
Total messages: 28 (0 generated)
In the meeting, we decided we would not use a lightweight async framework... for now. This delta is the result of that discussion.
https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:753: hideWelcomeToast(); hideWelcomeToast() should not be part of this. It should be called only from setToastVisible(). https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:761: onStateChange(); Suppose when we closed Chrome last time, we were in polling state (polling alarm was on). Then, on startup, it will be still on. So, onStateChange() won't do anything. However, on startup, we have to poll for cards. Couple of options how to achieve this: 1. Stop updateCardsAttempts before onStateChange here OR 2. Plumb a parameter to onStateChange forcing to make a first poll immediately even if polling is on. (Please test this case) https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:773: updateCardsAttempts.isRunning(function(currentValue) { Please unindent. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:807: (!notificationFound && visibleRequest)) if (visibleRequest != !!notifications[WELCOME_TOAST_NOTIFICATION_ID]) https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:835: var toastVisible = false; toastShouldBeVisible? Otherwise, it may seem that this variable is about current state. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:850: shouldRun = false; Already false. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:856: shouldRun = false; Already false. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:859: setToastVisible(toastVisible, function() { With new simplified tasks, you don't have to invoke finish callback. Therefore, no need for setToastVisible and setShouldRun to take a callback parameter and be executed in a cascaded way.
https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:753: hideWelcomeToast(); Done. On 2013/07/24 00:59:38, vadimt wrote: > hideWelcomeToast() should not be part of this. > It should be called only from setToastVisible(). https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:761: onStateChange(); Stopping updateCardsAttempts is best here. It keeps the rest of the code consistent that depends on this state of updateCardsAttempts consistent. On 2013/07/24 00:59:38, vadimt wrote: > Suppose when we closed Chrome last time, we were in polling state (polling alarm > was on). > Then, on startup, it will be still on. > So, onStateChange() won't do anything. > However, on startup, we have to poll for cards. Couple of options how to achieve > this: > 1. Stop updateCardsAttempts before onStateChange here > OR > 2. Plumb a parameter to onStateChange forcing to make a first poll immediately > even if polling is on. > > (Please test this case) https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:773: updateCardsAttempts.isRunning(function(currentValue) { On 2013/07/24 00:59:38, vadimt wrote: > Please unindent. Done. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:807: (!notificationFound && visibleRequest)) On 2013/07/24 00:59:38, vadimt wrote: > if (visibleRequest != !!notifications[WELCOME_TOAST_NOTIFICATION_ID]) Done. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:835: var toastVisible = false; Used shouldSetToastVisible. On 2013/07/24 00:59:38, vadimt wrote: > toastShouldBeVisible? > Otherwise, it may seem that this variable is about current state. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:850: shouldRun = false; Kept here for state machine clarity. On 2013/07/24 00:59:38, vadimt wrote: > Already false. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:856: shouldRun = false; Kept here for state machine clarity. Ideally, the variables above should be uninitialized. Initializing them provides us robustness against future bugs by providing a known valid initial start state. On 2013/07/24 00:59:38, vadimt wrote: > Already false. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:859: setToastVisible(toastVisible, function() { Is the new simplified tasks checked in? On 2013/07/24 00:59:38, vadimt wrote: > With new simplified tasks, you don't have to invoke finish callback. Therefore, > no need for setToastVisible and setShouldRun to take a callback parameter and be > executed in a cascaded way.
https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:850: shouldRun = false; On 2013/07/24 20:07:37, Robert Liao wrote: > Kept here for state machine clarity. > On 2013/07/24 00:59:38, vadimt wrote: > > Already false. > Then you should always set both variables in all branches. OR You can remove "= false" assignments. It may happen that once you do this, you'll like the succinct, declarative and easily understandable result. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:856: shouldRun = false; On 2013/07/24 20:07:37, Robert Liao wrote: > Kept here for state machine clarity. > > Ideally, the variables above should be uninitialized. Initializing them provides > us robustness against future bugs by providing a known valid initial start > state. > On 2013/07/24 00:59:38, vadimt wrote: > > Already false. > For me, assigning to false is stating "nothing is done unless explicitly told to do so". https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:859: setToastVisible(toastVisible, function() { On 2013/07/24 20:07:37, Robert Liao wrote: > Is the new simplified tasks checked in? > On 2013/07/24 00:59:38, vadimt wrote: > > With new simplified tasks, you don't have to invoke finish callback. > Therefore, > > no need for setToastVisible and setShouldRun to take a callback parameter and > be > > executed in a cascaded way. > No, but will be by the moment you'll be checking in. https://codereview.chromium.org/19822007/diff/5001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:750: function stopAndCloseDown() { You don't need this method. You already have stopPollingCards().
https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:850: shouldRun = false; State machine implementations should generally be explicit rather than implicit to make sure you cover all states you need to cover. I've removed the =falses. I think we agree to disagree here. On 2013/07/24 20:49:43, vadimt wrote: > On 2013/07/24 20:07:37, Robert Liao wrote: > > Kept here for state machine clarity. > > On 2013/07/24 00:59:38, vadimt wrote: > > > Already false. > > > > Then you should always set both variables in all branches. > OR > You can remove "= false" assignments. It may happen that once you do this, > you'll like the succinct, declarative and easily understandable result. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:856: shouldRun = false; On 2013/07/24 20:49:43, vadimt wrote: > On 2013/07/24 20:07:37, Robert Liao wrote: > > Kept here for state machine clarity. > > > > Ideally, the variables above should be uninitialized. Initializing them > provides > > us robustness against future bugs by providing a known valid initial start > > state. > > On 2013/07/24 00:59:38, vadimt wrote: > > > Already false. > > > > For me, assigning to false is stating "nothing is done unless explicitly told to > do so". Done. https://codereview.chromium.org/19822007/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:859: setToastVisible(toastVisible, function() { Do you have a delta I can pull? On 2013/07/24 20:49:43, vadimt wrote: > On 2013/07/24 20:07:37, Robert Liao wrote: > > Is the new simplified tasks checked in? > > On 2013/07/24 00:59:38, vadimt wrote: > > > With new simplified tasks, you don't have to invoke finish callback. > > Therefore, > > > no need for setToastVisible and setShouldRun to take a callback parameter > and > > be > > > executed in a cascaded way. > > > > No, but will be by the moment you'll be checking in. https://codereview.chromium.org/19822007/diff/5001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:750: function stopAndCloseDown() { On 2013/07/24 20:49:43, vadimt wrote: > You don't need this method. You already have stopPollingCards(). Done.
Cool. Now, please make sure unit tests pass. To run unit tests, build unit_tests target and run unit_tests.exe with GoogleNow*.* filter.
out\Debug\unit_tests.exe --gtest_filter=GoogleNow*.* [==========] 11 tests from 1 test case ran. (536 ms total) [ PASSED ] 11 tests.
https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:757: stopPollingCards(); I'm afraid that this scenario is possible: 1. When we closed Chrome, polling was on. alarm was registered. 2. User opens Chrome and quickly opts out from geolocation (or starts without Google Now flag, opts out and then starts with the flag again). 3. Extension gets loaded. 4. Extension calls stopPollingCards(); 5. onStateChange does nothing (correctly) because geolocation is off. 6. Alarms finally loads all alarms, and we start polling, which is wrong. Do you also think this is possible? If so, this should be addressed. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_test_util.js:35: clearWatch: emptyMock We mock in background_test_util.js only methods that are called during loading background.js. Since this method is not called during initial execution, please don't include it here. Please review all additions to this file and remove ones that are not required to simply load the page. These things need to be mocked in a test. You can use methods like makeAndRegisterMockApis for this. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:68: * Sets up the test to expect the state machine calls and send Please mention which tested method this is for. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:140: initialize(); Unit tests typically mock all callees and only check that first-level callees are invoked in a right order and with right args. So, we need to have a test for initialize() that checks that recordEvent, stopPollingCards and onStateChange are called, and also tests for these functions. Please do either: 1. Add a TODO comment to split unit tests later. OR 2. Do this in this CL if you wish. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:175: Remove empty line https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:220: TEST_F('GoogleNowBackgroundUnitTest', 'Initialize_ToastStateYes', function() { The name and comment seem out of date. We don't store Yes vs. No result.
out\Debug\unit_tests.exe --gtest_filter=GoogleNow* [==========] 11 tests from 1 test case ran. (537 ms total) [ PASSED ] 11 tests. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:757: stopPollingCards(); It is, but that bug is beyond the scope of this delta. On 2013/07/25 20:48:01, vadimt wrote: > I'm afraid that this scenario is possible: > 1. When we closed Chrome, polling was on. alarm was registered. > 2. User opens Chrome and quickly opts out from geolocation (or starts without > Google Now flag, opts out and then starts with the flag again). > 3. Extension gets loaded. > 4. Extension calls stopPollingCards(); > 5. onStateChange does nothing (correctly) because geolocation is off. > 6. Alarms finally loads all alarms, and we start polling, which is wrong. > > Do you also think this is possible? If so, this should be addressed. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_test_util.js:35: clearWatch: emptyMock On 2013/07/25 20:48:01, vadimt wrote: > We mock in background_test_util.js only methods that are called during loading > background.js. Since this method is not called during initial execution, please > don't include it here. > > Please review all additions to this file and remove ones that are not required > to simply load the page. > > These things need to be mocked in a test. You can use methods like > makeAndRegisterMockApis for this. Done. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:68: * Sets up the test to expect the state machine calls and send On 2013/07/25 20:48:01, vadimt wrote: > Please mention which tested method this is for. Done. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:140: initialize(); On 2013/07/25 20:48:01, vadimt wrote: > Unit tests typically mock all callees and only check that first-level callees > are invoked in a right order and with right args. So, we need to have a test for > initialize() that checks that recordEvent, stopPollingCards and onStateChange > are called, and also tests for these functions. Please do either: > 1. Add a TODO comment to split unit tests later. > OR > 2. Do this in this CL if you wish. Done. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:175: On 2013/07/25 20:48:01, vadimt wrote: > Remove empty line Done. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:220: TEST_F('GoogleNowBackgroundUnitTest', 'Initialize_ToastStateYes', function() { The comment is still valid. I've updated the name. We store the Yes vs. No answer in Geolocation Prefs in this case. On 2013/07/25 20:48:01, vadimt wrote: > The name and comment seem out of date. We don't store Yes vs. No result.
https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:757: stopPollingCards(); On 2013/07/25 22:35:48, Robert Liao wrote: > It is, but that bug is beyond the scope of this delta. > On 2013/07/25 20:48:01, vadimt wrote: > > I'm afraid that this scenario is possible: > > 1. When we closed Chrome, polling was on. alarm was registered. > > 2. User opens Chrome and quickly opts out from geolocation (or starts without > > Google Now flag, opts out and then starts with the flag again). > > 3. Extension gets loaded. > > 4. Extension calls stopPollingCards(); > > 5. onStateChange does nothing (correctly) because geolocation is off. > > 6. Alarms finally loads all alarms, and we start polling, which is wrong. > > > > Do you also think this is possible? If so, this should be addressed. > Please add a TODO item and plan addressing this soon, because continuing showing cards after the user opted out is a serious privacy issue. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:220: TEST_F('GoogleNowBackgroundUnitTest', 'Initialize_ToastStateYes', function() { On 2013/07/25 22:35:48, Robert Liao wrote: > The comment is still valid. I've updated the name. > > We store the Yes vs. No answer in Geolocation Prefs in this case. > On 2013/07/25 20:48:01, vadimt wrote: > > The name and comment seem out of date. We don't store Yes vs. No result. > The name and comment are still confusing. The user may have just manually checked Geolocation box after answering No or not answering at all. Why not say in name/comments that this is about geolocation box state, not about what user answered in the past? https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_test_util.js:10: debugSetStepName: emptyMock, This one is not called while the page loads, correct? https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:69: 'updateCardsAttempts.stop']); Square bracket should be on a new line. https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:115: * Sets up the test to expect the initialization calls. Please mention what function we are testing. https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:305: // Setup and expectations. Duplication of // Setup and expectations. https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:319: this.mockApis, Indent by 4.
https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:757: stopPollingCards(); This would be better expressed as a bug rather than a TODO. Neither the fix nor the workaround is local to this area. On 2013/07/26 01:00:37, vadimt wrote: > On 2013/07/25 22:35:48, Robert Liao wrote: > > It is, but that bug is beyond the scope of this delta. > > On 2013/07/25 20:48:01, vadimt wrote: > > > I'm afraid that this scenario is possible: > > > 1. When we closed Chrome, polling was on. alarm was registered. > > > 2. User opens Chrome and quickly opts out from geolocation (or starts > without > > > Google Now flag, opts out and then starts with the flag again). > > > 3. Extension gets loaded. > > > 4. Extension calls stopPollingCards(); > > > 5. onStateChange does nothing (correctly) because geolocation is off. > > > 6. Alarms finally loads all alarms, and we start polling, which is wrong. > > > > > > Do you also think this is possible? If so, this should be addressed. > > > > Please add a TODO item and plan addressing this soon, because continuing showing > cards after the user opted out is a serious privacy issue. https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:220: TEST_F('GoogleNowBackgroundUnitTest', 'Initialize_ToastStateYes', function() { This is the core user scenario we care about with the toast. I went ahead and changed the name. On 2013/07/26 01:00:37, vadimt wrote: > On 2013/07/25 22:35:48, Robert Liao wrote: > > The comment is still valid. I've updated the name. > > > > We store the Yes vs. No answer in Geolocation Prefs in this case. > > On 2013/07/25 20:48:01, vadimt wrote: > > > The name and comment seem out of date. We don't store Yes vs. No result. > > > > The name and comment are still confusing. The user may have just manually > checked Geolocation box after answering No or not answering at all. Why not say > in name/comments that this is about geolocation box state, not about what user > answered in the past? https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_test_util.js:10: debugSetStepName: emptyMock, True, but we're getting rid of this. Once we do, we can remove this here. This is easier to remove than picking through the tests. On 2013/07/26 01:00:37, vadimt wrote: > This one is not called while the page loads, correct? https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:69: 'updateCardsAttempts.stop']); On 2013/07/26 01:00:37, vadimt wrote: > Square bracket should be on a new line. Done. https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:115: * Sets up the test to expect the initialization calls. On 2013/07/26 01:00:37, vadimt wrote: > Please mention what function we are testing. Done. https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:305: // Setup and expectations. On 2013/07/26 01:00:37, vadimt wrote: > Duplication of // Setup and expectations. Done. https://codereview.chromium.org/19822007/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:319: this.mockApis, On 2013/07/26 01:00:37, vadimt wrote: > Indent by 4. Done.
lgtm Please proceed with OWNERs. https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/... chrome/browser/resources/google_now/background_test_util.js:57: savedArgs.arguments.splice(0, savedArgs.arguments.length); Why not savedArgs.arguments = []? https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:61: 'chrome.identity.getAuthToken', Should be indented by 2, I guess.
xiyuan: Please provide owner approval for all files in this CL. Thanks! https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/... chrome/browser/resources/google_now/background_test_util.js:57: savedArgs.arguments.splice(0, savedArgs.arguments.length); It is tempting to do that, but savedArgs.arguments actually gets passed around to other objects (e.g. the matchers). As a result, we must change the array itself rather than just creating and assigning a new empty array. On 2013/07/26 01:54:01, vadimt wrote: > Why not savedArgs.arguments = []? https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/46001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:61: 'chrome.identity.getAuthToken', Yup, per the style guide. Fixed the one above too. On 2013/07/26 01:54:01, vadimt wrote: > Should be indented by 2, I guess.
LGTM https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/... chrome/browser/resources/google_now/background_test_util.js:18: onLocationUpdate: emptyListener, nit: The change seems to be unnecessary. https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:267: // Tests if Google Now will invoke startPollingCards when all nit: I think the original 2-space indent is correct per style guide. "Passing Anonymous Functions When declaring an anonymous function in the list of arguments for a function call, the body of the function is indented two spaces from the left edge of the statement, or two spaces from the left edge of the function keyword. This is to make the body of the anonymous function easier to read (i.e. not be all squished up into the right half of the screen)."
https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/... chrome/browser/resources/google_now/background_test_util.js:18: onLocationUpdate: emptyListener, On 2013/07/26 17:24:51, xiyuan wrote: > nit: The change seems to be unnecessary. Done. https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/19822007/diff/56001/chrome/browser/resources/... chrome/browser/resources/google_now/background_unittest.gtestjs:267: // Tests if Google Now will invoke startPollingCards when all On 2013/07/26 17:24:51, xiyuan wrote: > nit: I think the original 2-space indent is correct per style guide. > > "Passing Anonymous Functions > > When declaring an anonymous function in the list of arguments for a function > call, the body of the function is indented two spaces from the left edge of the > statement, or two spaces from the left edge of the function keyword. This is to > make the body of the anonymous function easier to read (i.e. not be all squished > up into the right half of the screen)." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/62001
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&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/62001
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 chrome/browser/resources/google_now/background_test_util.js Hunk #2 succeeded at 22 (offset 1 line). Hunk #3 FAILED at 48. 1 out of 3 hunks FAILED -- saving rejects to file chrome/browser/resources/google_now/background_test_util.js.rej Patch: chrome/browser/resources/google_now/background_test_util.js Index: chrome/browser/resources/google_now/background_test_util.js diff --git a/chrome/browser/resources/google_now/background_test_util.js b/chrome/browser/resources/google_now/background_test_util.js index b4d2bccb9b0268e2cfa24261dfd71cf900340a0a..cce409b16f4d3f9d087de10186b5141aac202fe9 100644 --- a/chrome/browser/resources/google_now/background_test_util.js +++ b/chrome/browser/resources/google_now/background_test_util.js @@ -6,7 +6,10 @@ function emptyMock() {} function buildTaskManager() { - return {instrumentApiFunction: emptyMock}; + return { + debugSetStepName: emptyMock, + instrumentApiFunction: emptyMock, + }; } var instrumentApiFunction = emptyMock; var buildAttemptManager = emptyMock; @@ -18,6 +21,11 @@ chrome['notifications'] = { onClosed: emptyListener }; chrome['omnibox'] = {onInputEntered: emptyListener}; +chrome['preferencesPrivate'] = { + googleGeolocationAccessEnabled: { + onChange: emptyListener + } +}; chrome['runtime'] = { onInstalled: emptyListener, onStartup: emptyListener @@ -40,6 +48,11 @@ function invokeCallback(savedArgs, callbackParameter, var_args) { var callbackArguments = Array.prototype.slice.call(arguments, 2); return callFunction(function() { savedArgs.arguments[callbackParameter].apply(null, callbackArguments); + + // Mock4JS does not clear the saved args after invocation. + // To allow reuse of the same SaveMockArguments for multiple + // invocations with similar arguments, clear them here. + savedArgs.arguments.splice(0, savedArgs.arguments.length); }); }
sky: Please provide owner approval for chrome/test/data/webui/test_api.js This is a change that didn't make the initial move checkin in https://chromiumcodereview.appspot.com/19749007
chrome/test/data/OWNERS is *, and I'm not a good reviewer for js changes anyway. Please pick another reviewer (maybe one of the existing reviewers). -sky
On 2013/07/29 19:48:45, sky wrote: > chrome/test/data/OWNERS is *, and I'm not a good reviewer for js changes anyway. > Please pick another reviewer (maybe one of the existing reviewers). > > -sky sky: xiyuan has already lgtm'ed this line. We just need a rubber stamp from someone on chrome/test (unless I'm misinterpreting the '*'). Thanks!
As sky said, chrome/test/data has its own OWNERS file and it it '*'. So anyone could make changes there without owner's stamp. Did you get any error when running presubmit?
On 2013/07/29 20:00:35, xiyuan wrote: > As sky said, chrome/test/data has its own OWNERS file and it it '*'. So anyone > could make changes there without owner's stamp. > > Did you get any error when running presubmit? No. It failed on some flaky tests and conflicting changes came in after I resubmitted. I'll send it in again. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/101001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/101001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/19822007/101001
Message was sent while issue was closed.
Change committed as 214536 |