Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
358693004:
Added touch event permutations test to touch_exploration_controller. (Closed)
DescriptionAdded touch event permutations test to touch_exploration_controller.
All permutations of three fingers pressing, moving, and releasing are now being tested. Fingers that were recorded as pressed during the permutation but not released yet are released, and then there is a check for no fingers down.
BUG=388520
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281195
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281924
Patch Set 1 #
Total comments: 10
Patch Set 2 : more efficient factorials #Patch Set 3 : updated NOTREACHED() logging and prevented the test from reaching the NOTREACHED() #
Total comments: 12
Patch Set 4 : explained the algorithm #
Total comments: 6
Patch Set 5 : Updated comments and organization #
Total comments: 8
Patch Set 6 : #
Total comments: 15
Patch Set 7 : #Patch Set 8 : rebased #Patch Set 9 : rebased from newest passthrough #Patch Set 10 : permutations doesn't run VLOGS and doesn't break the DCHECK anymore #
Total comments: 5
Patch Set 11 : #Patch Set 12 : test now only runs in release mode because it times out in debug mode #Patch Set 13 : rebased #Patch Set 14 : rebased off of gestures #Patch Set 15 : rebased #Patch Set 16 : rebased again because failure to apply the patch #
Messages
Total messages: 32 (0 generated)
https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:952: int Factorial(int n) { Put this inside the "namespace {" at the top, so that we don't get a link error if someone else tries to add a function called Factorial. I guess we should do that for ConfirmEventsAreTouchAndEqual too, but that can be a separate change. https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:992: int index; Declare all of these variables inside the block scope where they're used - so in the case of "index", inside the second "for" loop. Otherwise it appears as if you might care about the value of these variables after the end of the loop. https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:999: index = (i / Factorial(num_events - 1)); This logic looks correct but inefficient. Calling Factorial inside this inner loop is doing a lot of extra computation, it's computing the same Factorial over and over again. Try to do it this way: make a copy of "i" into a new variable, like "permutation_index" or something like that. Then in each inner loop, the index is (permutation_index % j) and then divide permutation_index by j. Essentially you keep modifying permutation_index as you go. https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:1014: // order. If there is a NOTREACHED() for touch moved hit in We should probably change the NOTREACHED to a LOG(WARNING), then. NOTREACHED means "this should never happen".
https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:952: int Factorial(int n) { On 2014/06/26 18:16:43, dmazzoni wrote: > Put this inside the "namespace {" at the top, so that we don't get a link error > if someone else tries to add a function called Factorial. > > I guess we should do that for ConfirmEventsAreTouchAndEqual too, but that can be > a separate change. > Done. https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:992: int index; On 2014/06/26 18:16:43, dmazzoni wrote: > Declare all of these variables inside the block scope where they're used - so in > the case of "index", inside the second "for" loop. > > Otherwise it appears as if you might care about the value of these variables > after the end of the loop. Done. https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:999: index = (i / Factorial(num_events - 1)); On 2014/06/26 18:16:43, dmazzoni wrote: > This logic looks correct but inefficient. Calling Factorial inside this inner > loop is doing a lot of extra computation, it's computing the same Factorial over > and over again. > > Try to do it this way: make a copy of "i" into a new variable, like > "permutation_index" or something like that. Then in each inner loop, the index > is (permutation_index % j) and then divide permutation_index by j. Essentially > you keep modifying permutation_index as you go. I'm not quite sure what you meant, but I did something that modifies the factorial value as I go - tell me what you think. https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:1014: // order. If there is a NOTREACHED() for touch moved hit in On 2014/06/26 18:16:43, dmazzoni wrote: > We should probably change the NOTREACHED to a LOG(WARNING), then. > > NOTREACHED means "this should never happen". The thing is, this should never happen when the user is actually interacting. It just happens in this test. Do you want me to stop releases from being deployed if that finger isn't currently pressed? Or should we just change it to a LOG(WARNING)
https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:1014: // order. If there is a NOTREACHED() for touch moved hit in On 2014/06/26 22:31:05, evy wrote: > On 2014/06/26 18:16:43, dmazzoni wrote: > > We should probably change the NOTREACHED to a LOG(WARNING), then. > > > > NOTREACHED means "this should never happen". > > The thing is, this should never happen when the user is actually interacting. It > just happens in this test. Do you want me to stop releases from being deployed > if that finger isn't currently pressed? Or should we just change it to a > LOG(WARNING) What about if the user already has fingers down when they enable ChromeVox? Couldn't it happen then? Basically if it could happen even in really obscure cases like that, it's not appropriate for NOTREACHED.
https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:1014: // order. If there is a NOTREACHED() for touch moved hit in On 2014/06/27 07:25:16, dmazzoni wrote: > On 2014/06/26 22:31:05, evy wrote: > > On 2014/06/26 18:16:43, dmazzoni wrote: > > > We should probably change the NOTREACHED to a LOG(WARNING), then. > > > > > > NOTREACHED means "this should never happen". > > > > The thing is, this should never happen when the user is actually interacting. > It > > just happens in this test. Do you want me to stop releases from being deployed > > if that finger isn't currently pressed? Or should we just change it to a > > LOG(WARNING) > > What about if the user already has fingers down when they enable ChromeVox? > Couldn't it happen then? > > Basically if it could happen even in really obscure cases like that, it's not > appropriate for NOTREACHED. Okay, a touch moved is now properly handled.
Alice, want to take a pass?
https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:969: all_events.push_back( This could be a for-loop over touch_id - it'd also solve the "what's that third argument again" mystery (I had to look it up). https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:991: for (int i = 0; i < num_permutations; i++) { Since this is not used as a simple index on the next line, perhaps name it 'p' or something more descriptive as a reminder of what it represents. (Then 'j' could become 'i' - since it doesn't really represent something straightforward I couldn't think of a more readable name for it.) (In emacs, in case you haven't come across this before: M-< M-x replace-regexp \bi\b p - or whatever you want the new variable to be called) https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:993: std::vector<bool> fingers_pressed(3, false); My favourite vector constructor! https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:997: for (int j = num_events - 1; j >= 0; j--) { I can't fully understand this code (just the bit about calculating |index|), so I can't work through whether it's correct or not. Could you write an explanation of what it's doing? https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:999: if (j == num_events - 1) { This could be a switch statement (and you could pull num_events - 1 out into a variable named something like last_event to make it easier to visually parse). https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1017: // order. If there is a NOTREACHED() for touch moved hit in Will NOTREACHED() cause the test to crash in debug mode?
https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:969: all_events.push_back( On 2014/06/27 18:08:02, aboxhall wrote: > This could be a for-loop over touch_id - it'd also solve the "what's that third > argument again" mystery (I had to look it up). Done. Great idea. https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:991: for (int i = 0; i < num_permutations; i++) { On 2014/06/27 18:08:02, aboxhall wrote: > Since this is not used as a simple index on the next line, perhaps name it 'p' > or something more descriptive as a reminder of what it represents. (Then 'j' > could become 'i' - since it doesn't really represent something straightforward I > couldn't think of a more readable name for it.) > > (In emacs, in case you haven't come across this before: > M-< > M-x replace-regexp > \bi\b > p > - or whatever you want the new variable to be called) Done. https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:993: std::vector<bool> fingers_pressed(3, false); On 2014/06/27 18:08:01, aboxhall wrote: > My favourite vector constructor! :D https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:997: for (int j = num_events - 1; j >= 0; j--) { On 2014/06/27 18:08:02, aboxhall wrote: > I can't fully understand this code (just the bit about calculating |index|), so > I can't work through whether it's correct or not. Could you write an explanation > of what it's doing? I renamed some variables and added tons of comments - let me know what you think! https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:999: if (j == num_events - 1) { On 2014/06/27 18:08:02, aboxhall wrote: > This could be a switch statement (and you could pull num_events - 1 out into a > variable named something like last_event to make it easier to visually parse). I'm not sure if this comment applies - can you read over my new explanations and if you still want a switch statement let me know what you mean? https://codereview.chromium.org/358693004/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1017: // order. If there is a NOTREACHED() for touch moved hit in On 2014/06/27 18:08:01, aboxhall wrote: > Will NOTREACHED() cause the test to crash in debug mode? Yup - I just fixed that problem, but forgot to change this comment. Thanks!
https://codereview.chromium.org/358693004/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:972: gfx::Point(10 * touch_id + 1, 10 * touch_id + 2), Suggestion: you could just have x and y values that you increment each time you use them instead of this; might be a bit easier to read. e.g. int x = 30, y = 30; for (...) { ... gfx::Point(x++, y++), ... (or even one value, but it might be a little tricky to name) https://codereview.chromium.org/358693004/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:992: // current_factorial starts at factorial(num_events - 1) This comment is now out of date. https://codereview.chromium.org/358693004/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1005: index = p / (current_num_permutations / events_left); Thanks, I actually mostly understand this now! However, I think we can still make it clearer. These blocks are all equivalent, right? p % num_permutations === p and if events_left == 1 then current_num_permutations will also be 1, and 1 % 1 === 0 So you could collapse this down to just the lines inside the if (events_left > 1) block. Then it's just a matter of understanding why those couple of lines are the right thing to do :) Perhaps we could rewrite it to be clearer, something like ("thinking out loud" here, this is not necessarily the best way to write it; you might well come up with something better)... int sub_permutation = p % current_num_permutations; int remaining_permutations = current_num_permutations / events_left; index = sub_permutation / remaining_permutations; current_num_permutations = remaining_permutations; Then you could structure your comments around those lines and explain why each one makes sense.
https://codereview.chromium.org/358693004/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:972: gfx::Point(10 * touch_id + 1, 10 * touch_id + 2), On 2014/06/27 22:37:26, aboxhall wrote: > Suggestion: you could just have x and y values that you increment each time you > use them instead of this; might be a bit easier to read. > > e.g. > int x = 30, y = 30; > for (...) { > ... gfx::Point(x++, y++), ... > > (or even one value, but it might be a little tricky to name) Done. https://codereview.chromium.org/358693004/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:992: // current_factorial starts at factorial(num_events - 1) On 2014/06/27 22:37:26, aboxhall wrote: > This comment is now out of date. Done. https://codereview.chromium.org/358693004/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1005: index = p / (current_num_permutations / events_left); On 2014/06/27 22:37:26, aboxhall wrote: > Thanks, I actually mostly understand this now! However, I think we can still > make it clearer. > > These blocks are all equivalent, right? > p % num_permutations === p > and if events_left == 1 then current_num_permutations will also be 1, and 1 % 1 > === 0 > > So you could collapse this down to just the lines inside the if (events_left > > 1) block. > > Then it's just a matter of understanding why those couple of lines are the right > thing to do :) > > Perhaps we could rewrite it to be clearer, something like ("thinking out loud" > here, this is not necessarily the best way to write it; you might well come up > with something better)... > int sub_permutation = p % current_num_permutations; > int remaining_permutations = current_num_permutations / events_left; > index = sub_permutation / remaining_permutations; > current_num_permutations = remaining_permutations; > > Then you could structure your comments around those lines and explain why each > one makes sense. Done. I'm not sure if this is clear enough yet. Can you take a look and let me know what you think?
Getting very close - I really love the worked (a, b, c, d) examples. https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:970: int x = 10*touch_id + 1; I think you might want to initialise these outside the for loop (and then increment them at the end of the loop as well), otherwise you will have events which are occurring at the same point but with different touch_ids. Also, is there any reason you can't just increment them by one? https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1005: // But how do we find the index for current_num_permutations? This sentence took me a couple of goes to understand. How about "To find the permutation within the part of the sequence we're currently looking at, we need a number..." https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1019: // So to figure out what current event we want to choose, we divide This step is still a little unclear to me. I think you explained the (current_num_permutations / events_left) part more clearly in the previous patch. I do think it might be clearer if you pulled that number out into a local variable, particularly since you set current_num_permutations to the same number below. https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1030: current_num_permutations /= events_left; I think it's worth noting that this is just set-up for the next iteration.
https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:970: int x = 10*touch_id + 1; On 2014/06/30 18:19:36, aboxhall wrote: > I think you might want to initialise these outside the for loop (and then > increment them at the end of the loop as well), otherwise you will have events > which are occurring at the same point but with different touch_ids. > > Also, is there any reason you can't just increment them by one? Oh, yeah I guess I can just increment by one each. I need to keep it inside the loop through, I think, because the events need to occur at different points depending on the touch ids, which is why x and y are initialized here using the touch_id value https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1005: // But how do we find the index for current_num_permutations? On 2014/06/30 18:19:36, aboxhall wrote: > This sentence took me a couple of goes to understand. How about "To find the > permutation within the part of the sequence we're currently looking at, we need > a number..." Done. https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1019: // So to figure out what current event we want to choose, we divide On 2014/06/30 18:19:36, aboxhall wrote: > This step is still a little unclear to me. I think you explained the > (current_num_permutations / events_left) part more clearly in the previous > patch. > > I do think it might be clearer if you pulled that number out into a local > variable, particularly since you set current_num_permutations to the same number > below. Done. https://codereview.chromium.org/358693004/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1030: current_num_permutations /= events_left; On 2014/06/30 18:19:36, aboxhall wrote: > I think it's worth noting that this is just set-up for the next iteration. Done.
lgtm with nits (i.e. feel free to submit after you fix up the last few things) https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:974: x++; nit: you can use this inline, e.g. gfx::Point(x++, y++) https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:984: // I'm going to explain this algorithm, and use an example in parenthesis. nit: "parentheses" https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:993: // Branching out from the first event, there are num_permutations nit: newline before each of these long comments I think this comment could go before setting current_num_permutations on line 1026, instead of here - I think it explains what you're doing there better than the current comment there. https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1024: // current_num_permutations to the factorail of the next number of nit: typo "factorail" https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1032: // Note that thre are 6 that start with a because there are 6 nit: typo "thre" nit: start with 'a' instead of start with a https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1034: int index = nit: fits on one line.
Oh - you should probably wait for Dominic's LGTM too though!
Oh - you should probably wait for Dominic's LGTM too though!
Can you upload again with the latest changes that address Alice's comments? On Mon, Jun 30, 2014 at 12:41 PM, <aboxhall@chromium.org> wrote: > Oh - you should probably wait for Dominic's LGTM too though! > > https://codereview.chromium.org/358693004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm I love the new comments. Great job. https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:66: if (n == 2) Nit: you don't actually need this case, it works correctly otherwise when n = 2. https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:993: // Branching out from the first event, there are num_permutations Another suggestion: the convention in Chrome is to use the pipe character ("|") to denote variable names in comments. That makes it easier to read when you have variable names that are also English works. For example: // Branching out from the first event, there are |num_permutations| // permutations, and each value of |p| is associated with... You don't have to change every single one, but consider going through and doing that where it'd help readability.
https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:66: if (n == 2) On 2014/06/30 20:14:44, dmazzoni wrote: > Nit: you don't actually need this case, it works correctly otherwise when n = 2. Oh, whoops. Thanks for catching that https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:974: x++; On 2014/06/30 19:24:23, aboxhall wrote: > nit: you can use this inline, e.g. gfx::Point(x++, y++) Done. https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:984: // I'm going to explain this algorithm, and use an example in parenthesis. On 2014/06/30 19:24:24, aboxhall wrote: > nit: "parentheses" Done. https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:993: // Branching out from the first event, there are num_permutations On 2014/06/30 19:24:24, aboxhall wrote: > nit: newline before each of these long comments > > I think this comment could go before setting current_num_permutations on line > 1026, instead of here - I think it explains what you're doing there better than > the current comment there. Done. https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1024: // current_num_permutations to the factorail of the next number of On 2014/06/30 19:24:25, aboxhall wrote: > nit: typo "factorail" Done. https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1032: // Note that thre are 6 that start with a because there are 6 On 2014/06/30 19:24:24, aboxhall wrote: > nit: typo "thre" > nit: start with 'a' instead of start with a Done. https://codereview.chromium.org/358693004/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1034: int index = On 2014/06/30 19:24:24, aboxhall wrote: > nit: fits on one line. Done.
still lgtm
I changed a few things because I was failing some trybot tests (trying to stop the tap timer when it wasn't running) and also I made it so there are never VLOGS when this test runs.
lgtm with nits https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:51: if (turn_on) You can just say VLOG_on_ = turn_on, no "if" needed https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:94: void SwitchVLOG(bool turn_on); I'd call this SwitchVLOGForTesting as-is. I think I'd prefer that you name it something that makes it more clear why you might want to do this, something like: void SuppressVLOGsForTesting(bool suppress); (This reverses the way you have the logic now - i.e. "true" would mean to not log.) Add a very short comment saying this is for tests that would generate too many logs otherwise. https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:862: Delete extra line
lgtm
https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:51: if (turn_on) On 2014/07/02 23:05:57, dmazzoni wrote: > You can just say VLOG_on_ = turn_on, no "if" needed Done. https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/358693004/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:94: void SwitchVLOG(bool turn_on); On 2014/07/02 23:05:57, dmazzoni wrote: > I'd call this SwitchVLOGForTesting as-is. > > I think I'd prefer that you name it something that makes it more clear why you > might want to do this, something like: > > void SuppressVLOGsForTesting(bool suppress); > > (This reverses the way you have the logic now - i.e. "true" would mean to not > log.) > > Add a very short comment saying this is for tests that would generate too many > logs otherwise. Done.
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/358693004/200001
Message was sent while issue was closed.
Change committed as 281195
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/367263005/ by nkostylev@chromium.org. The reason for reverting is: New test is failing on Linux ChromiumOS Tests (dbg) http://goo.gl/rwFMMz.
Message was sent while issue was closed.
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/358693004/320001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 281924 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
