Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
447893003:
Test that simulates touch exploration and checks speech on tabs and also the empty bookmark bar. (Closed)
|
Created:
6 years, 4 months ago by evy Modified:
3 years, 9 months ago CC:
chromium-reviews, oshima+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, tfarina, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, nkostylev+watch_chromium.org, lisayin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionTest that simulates touch exploration and checks speech on tabs and also the empty bookmark bar.
BUG=393768, 395895
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289488
Patch Set 1 #Patch Set 2 : moved new tests to views #
Total comments: 22
Patch Set 3 : renamed file and added better commenting #Patch Set 4 : check for existing window.ontouchstart #
Total comments: 12
Patch Set 5 : nits #
Total comments: 13
Patch Set 6 : changed file name #Patch Set 7 : added static text to accessibility private #Messages
Total messages: 35 (2 generated)
** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc
Illegal include: "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
Because of "-chrome/browser/ui/views" from chrome/browser's include_rules. \
chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc
Illegal include: "chrome/browser/ui/views/frame/browser_view.h"
Because of "-chrome/browser/ui/views" from chrome/browser's include_rules. \
chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc
Illegal include: "chrome/browser/ui/views/tabs/tab_strip.h"
Because of "-chrome/browser/ui/views" from chrome/browser's include_rules.
Some options I can think of:
* see if I can move the whole file to another folder (depends on the include
rules in the chrome/browser/ui/views/accessibility/ for any
chrome/browser/chromeos/ files needed)
* make a separate file in chrome/browser/ui/views/accessibility/ for just these
tests I've added, and somehow link it to this file's resources for running the
tests
* keep the test in this folder and find a way to access the views I need more
abstractly, not needing to include the forbidden files.
I'm not really sure which direction to go, or if there's something else that
would work better.
Thoughts?
If you can do option 3 - that's seems the easiest. If that's not possible/tricky, I'd say go with option 2, except move all the shared machinery to some suitable test_utils file. Also, there's already chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc - see if it makes sense to add the tests there rather than creating a new file.
No more presubmit errors! :) I moved the test to views because I need to be able to create a bookmarks/tab view to check it's accessible state. The shared function that simulates touch screen in ChromeVox was added to the accessibility utils. Code review for the tests would be awesome, as well review for my code shuffling and #include choices.
Heading into a meeting, initial comments, more to follow https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.h (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.h:8: #include "content/public/browser/browser_context.h" not needed. forward declare BrowserContext: namespace content { class BrowserContext; } https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:236: nit: Only one blank line between these sections https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. I steered you wrong. This should be spoken_feedback_views_browsertest.cc like I initially suggested. (I forgot the other file was spoken_feedback_browsertest.cc) https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:14: typedef InProcessBrowserTest SpokenFeedbackViewsTest; Add a brief comment about why this lives in c/b/ui/views https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:18: IN_PROC_BROWSER_TEST_F(SpokenFeedbackViewsTest, Add a brief comment explaining what you are testing. https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:20: EXPECT_FALSE(AccessibilityManager::Get()->IsSpokenFeedbackEnabled()); I would ASSERT_FALSE, as this is a precondition for the test.
Kept going to make comments only to find jamescook@ had already made them :) https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:57: "window.ontouchstart = function() {};")); Perhaps to be a little safer: "if (!('ontouchstart' in window)) window.ontouchstart = function() {};" https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:37: EXPECT_EQ("For quick access,", monitor.GetNextUtterance()); Possibly consider expressing this in terms of instructions->text() (You'd have to split the string up yourself though; possibly not worth it - it would just mean the test wouldn't break if they changed the wording at some point.)
https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:630: if (instructions_ && instructions_->visible()) Any particular reason you're checking for visible()? None of the code above checks visibility before returning the item.
https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:57: "window.ontouchstart = function() {};")); On 2014/08/08 17:16:01, aboxhall wrote: > Perhaps to be a little safer: > "if (!('ontouchstart' in window)) window.ontouchstart = function() {};" This code was actually moved over from another file - it looks like a way of executing JavaScript from the browsercontext but I don't fully understand what's going on. https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.h (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.h:8: #include "content/public/browser/browser_context.h" On 2014/08/08 17:06:31, James Cook wrote: > not needed. forward declare BrowserContext: > > namespace content { > class BrowserContext; > } Done. https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:236: On 2014/08/08 17:06:32, James Cook wrote: > nit: Only one blank line between these sections Done. https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/08/08 17:06:32, James Cook wrote: > I steered you wrong. This should be spoken_feedback_views_browsertest.cc like I > initially suggested. (I forgot the other file was > spoken_feedback_browsertest.cc) Done. https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:14: typedef InProcessBrowserTest SpokenFeedbackViewsTest; On 2014/08/08 17:06:32, James Cook wrote: > Add a brief comment about why this lives in c/b/ui/views Done. https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:18: IN_PROC_BROWSER_TEST_F(SpokenFeedbackViewsTest, On 2014/08/08 17:06:32, James Cook wrote: > Add a brief comment explaining what you are testing. Done. https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:20: EXPECT_FALSE(AccessibilityManager::Get()->IsSpokenFeedbackEnabled()); On 2014/08/08 17:06:32, James Cook wrote: > I would ASSERT_FALSE, as this is a precondition for the test. Done. https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:37: EXPECT_EQ("For quick access,", monitor.GetNextUtterance()); On 2014/08/08 17:16:01, aboxhall wrote: > Possibly consider expressing this in terms of instructions->text() > (You'd have to split the string up yourself though; possibly not worth it - it > would just mean the test wouldn't break if they changed the wording at some > point.) I thought a bit about that, and I'm glad you brought it up - I think it's hard to know for sure how it's broken up (more of a guess and check) Dominic suggested to just do this, so I think I'll stick with it, but I'm not set on that decision. Let me know if you think it'd be better to break up the instruction accessible name string. https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:630: if (instructions_ && instructions_->visible()) On 2014/08/08 17:46:55, James Cook wrote: > Any particular reason you're checking for visible()? None of the code above > checks visibility before returning the item. Once the instructions appear, the view always exists, but once bookmarks are added it's no longer visible. I was thinking of it more from what the user experiences - if there are bookmarks added and the instructions aren't there, we shouldn't get getting the instructions view. (I can add a comment explaining this) From a "just fetching views" perspective though, I can also see the argument that if it's there and someone needs it, it's returned.
https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:57: "window.ontouchstart = function() {};")); On 2014/08/08 18:37:47, evy wrote: > On 2014/08/08 17:16:01, aboxhall wrote: > > Perhaps to be a little safer: > > "if (!('ontouchstart' in window)) window.ontouchstart = function() {};" > > This code was actually moved over from another file - it looks like a way of > executing JavaScript from the browsercontext but I don't fully understand what's > going on. Yeah, I did see it in the other file after I made this comment. Essentially what it's doing is running https://developer.chrome.com/extensions/tabs#method-executeScript in the ChromeVox extension background page context, but from C++ instead of JavaScript. I'm not sure whether it's a risk, but as it is currently you'd be overwriting window.ontouchstart if it already exists (e.g if this test somehow gets executed in a context with a touch screen) - probably not actually a problem right now, but you never know where this method will end up getting used given it's now in a util file. My suggestion is simply to check for an existing window.ontouchstart before you set it.
https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:57: "window.ontouchstart = function() {};")); On 2014/08/08 18:54:30, aboxhall wrote: > On 2014/08/08 18:37:47, evy wrote: > > On 2014/08/08 17:16:01, aboxhall wrote: > > > Perhaps to be a little safer: > > > "if (!('ontouchstart' in window)) window.ontouchstart = function() {};" > > > > This code was actually moved over from another file - it looks like a way of > > executing JavaScript from the browsercontext but I don't fully understand > what's > > going on. > > Yeah, I did see it in the other file after I made this comment. > Essentially what it's doing is running > https://developer.chrome.com/extensions/tabs#method-executeScript in the > ChromeVox extension background page context, but from C++ instead of JavaScript. > I'm not sure whether it's a risk, but as it is currently you'd be overwriting > window.ontouchstart if it already exists (e.g if this test somehow gets executed > in a context with a touch screen) - probably not actually a problem right now, > but you never know where this method will end up getting used given it's now in > a util file. My suggestion is simply to check for an existing > window.ontouchstart before you set it. Makes sense! Thanks for the explanation + suggestion
https://codereview.chromium.org/447893003/diff/60001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/447893003/diff/60001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:276: 'browser/chromeos/accessibility/accessibility_util.cc', nit: This should be in alphabetical order.
https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:37: EXPECT_EQ("For quick access,", monitor.GetNextUtterance()); On 2014/08/08 18:37:47, evy wrote: > On 2014/08/08 17:16:01, aboxhall wrote: > > Possibly consider expressing this in terms of instructions->text() > > (You'd have to split the string up yourself though; possibly not worth it - it > > would just mean the test wouldn't break if they changed the wording at some > > point.) > > I thought a bit about that, and I'm glad you brought it up - I think it's hard > to know for sure how it's broken up (more of a guess and check) > Dominic suggested to just do this, so I think I'll stick with it, but I'm not > set on that decision. > Let me know if you think it'd be better to break up the instruction accessible > name string. I think on balance it'd probably be over-engineering it; you'd also have to get the "Bookmarks" utterance which would be from bookmarks_bar->GetAccessibleState(), and I'm not sure whether that method ends up including the label contents or not... so I think this is fine for now; if it breaks we'll just cross that bridge!
LGTM with nits https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:58: "if (!('ontouchstart' in window)) window.ontouchstart = function() {};")); You should make sure these tests still pass with the check you added. The previous code looked to me like it was intentionally overwriting an existing ontouchstart. https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.h (right): https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.h:9: class BrowserContext; nit: No spaces at the front of this line. Also, we typically put these after "class Browser;" https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:36: #include "extensions/browser/extension_host.h" You can probably remove this #include and the one below. And maybe the extensions_constants.h one above. https://codereview.chromium.org/447893003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc:23: // These tests fetch views from the browser and triggers an accessibilty event One comment per test, please. When these tests fail the person looking at the failure will probably be rushed and you can save them a second or two if the comment is right next to the test. https://codereview.chromium.org/447893003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc:51: EXPECT_FALSE(AccessibilityManager::Get()->IsSpokenFeedbackEnabled()); ASSERT_FALSE like above
https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_chromeos_browsertest.cc:37: EXPECT_EQ("For quick access,", monitor.GetNextUtterance()); On 2014/08/08 20:01:52, aboxhall wrote: > On 2014/08/08 18:37:47, evy wrote: > > On 2014/08/08 17:16:01, aboxhall wrote: > > > Possibly consider expressing this in terms of instructions->text() > > > (You'd have to split the string up yourself though; possibly not worth it - > it > > > would just mean the test wouldn't break if they changed the wording at some > > > point.) > > > > I thought a bit about that, and I'm glad you brought it up - I think it's hard > > to know for sure how it's broken up (more of a guess and check) > > Dominic suggested to just do this, so I think I'll stick with it, but I'm not > > set on that decision. > > Let me know if you think it'd be better to break up the instruction accessible > > name string. > > I think on balance it'd probably be over-engineering it; you'd also have to get > the "Bookmarks" utterance which would be from > bookmarks_bar->GetAccessibleState(), and I'm not sure whether that method ends > up including the label contents or not... so I think this is fine for now; if it > breaks we'll just cross that bridge! Acknowledged. https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:58: "if (!('ontouchstart' in window)) window.ontouchstart = function() {};")); On 2014/08/08 20:26:46, James Cook wrote: > You should make sure these tests still pass with the check you added. The > previous code looked to me like it was intentionally overwriting an existing > ontouchstart. They do still pass. Should I keep the change or leave the original? https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.h (right): https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.h:9: class BrowserContext; On 2014/08/08 20:26:46, James Cook wrote: > nit: No spaces at the front of this line. Also, we typically put these after > "class Browser;" Done. https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:36: #include "extensions/browser/extension_host.h" On 2014/08/08 20:26:46, James Cook wrote: > You can probably remove this #include and the one below. And maybe the > extensions_constants.h one above. Done. https://codereview.chromium.org/447893003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc:23: // These tests fetch views from the browser and triggers an accessibilty event On 2014/08/08 20:26:46, James Cook wrote: > One comment per test, please. When these tests fail the person looking at the > failure will probably be rushed and you can save them a second or two if the > comment is right next to the test. Done. https://codereview.chromium.org/447893003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc:51: EXPECT_FALSE(AccessibilityManager::Get()->IsSpokenFeedbackEnabled()); On 2014/08/08 20:26:46, James Cook wrote: > ASSERT_FALSE like above Done. https://codereview.chromium.org/447893003/diff/60001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/447893003/diff/60001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:276: 'browser/chromeos/accessibility/accessibility_util.cc', On 2014/08/08 19:22:40, lisayin wrote: > nit: This should be in alphabetical order. Done.
+sky for OWNERS review Spoken feedback tests for the tabs/bookmark changes you reviewed earlier.
https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:630: if (instructions_ && instructions_->visible()) Is there a reason for the NULL check? instructions_ is never set to NULL.
https://codereview.chromium.org/447893003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:58: "if (!('ontouchstart' in window)) window.ontouchstart = function() {};")); If this works I would keep it this way.
https://codereview.chromium.org/447893003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:58: "if (!('ontouchstart' in window)) window.ontouchstart = function() {};")); On 2014/08/08 23:31:37, James Cook wrote: > If this works I would keep it this way. Acknowledged. https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:630: if (instructions_ && instructions_->visible()) On 2014/08/08 23:30:03, sky wrote: > Is there a reason for the NULL check? instructions_ is never set to NULL. Is there a chance that code could be added in the future to set it to NULL? Or should that never happen (it should only be set to not visible, and the code assumes that/tests ensure that)
lgtm Good but just one small concern https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc (right): https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc:6: #include "chrome/browser/chromeos/accessibility/accessibility_manager.h" It doesn't make sense to me that a file in chrome/browser/ui/views should include chromeos-only files. Note that chrome/browser/ui/views is compiled on Windows. Does this fail to compile on Windows? If you added chromeos to the filename that would make more sense to me. I know "spoken feedback" is a Chrome OS specific term, but not everyone would know that.
https://codereview.chromium.org/447893003/diff/80001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/447893003/diff/80001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:262: 'dependencies': [ If the files are added under chromeos==1, will they only compile for ChomeOS?
https://codereview.chromium.org/447893003/diff/80001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/447893003/diff/80001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:262: 'dependencies': [ On 2014/08/11 14:00:15, evy wrote: > If the files are added under chromeos==1, will they only compile for ChomeOS? Yes, but I think that could be confusing because people not familiar with "spoken feedback" won't get any clue that those tests are Chrome-OS-only. I suggest putting chromeos into the file name, because the default gyp rules will *automatically* make it only compile on Chrome OS, and it will also make it more obvious to anyone who discovers the file. https://codereview.chromium.org/447893003/diff/80001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:299: 'browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc', Note that every other file in this section has 'chromeos" in the path somewhere.
LGTM once dmazzoni's concerns are addressed. https://codereview.chromium.org/447893003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:48: // ChromeVox looks at whether 'ontouchstart' exists to know whether trivial: comment could be reflowed (clang-format should take care of this for you)
https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:630: if (instructions_ && instructions_->visible()) On 2014/08/09 02:26:52, evy wrote: > On 2014/08/08 23:30:03, sky wrote: > > Is there a reason for the NULL check? instructions_ is never set to NULL. > > Is there a chance that code could be added in the future to set it to NULL? Or > should that never happen (it should only be set to not visible, and the code > assumes that/tests ensure that) If we start allowing it to be NULL we would have to revisit any existing code.
https://codereview.chromium.org/447893003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_util.cc (right): https://codereview.chromium.org/447893003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_util.cc:48: // ChromeVox looks at whether 'ontouchstart' exists to know whether On 2014/08/11 15:48:27, aboxhall wrote: > trivial: comment could be reflowed (clang-format should take care of this for > you) Done. https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/447893003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:630: if (instructions_ && instructions_->visible()) On 2014/08/11 16:26:58, sky wrote: > On 2014/08/09 02:26:52, evy wrote: > > On 2014/08/08 23:30:03, sky wrote: > > > Is there a reason for the NULL check? instructions_ is never set to NULL. > > > > Is there a chance that code could be added in the future to set it to NULL? Or > > should that never happen (it should only be set to not visible, and the code > > assumes that/tests ensure that) > > If we start allowing it to be NULL we would have to revisit any existing code. Done. https://codereview.chromium.org/447893003/diff/80001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/447893003/diff/80001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:299: 'browser/ui/views/accessibility/spoken_feedback_views_browsertest.cc', On 2014/08/11 15:14:29, dmazzoni wrote: > Note that every other file in this section has 'chromeos" in the path somewhere. Done.
LGTM
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/447893003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/447893003/120001
Message was sent while issue was closed.
Committed patchset #7 (120001) as 289488
This got reverted because of accessibility_util linking to ExecuteScript in content_tests:test_support. I think maybe accessibility_util shouldn't be calling a function in test_support - we should move SimulateTouchScreenInChromeVoxForTest to a source file that's compiled only for tests, rather than a file that's compiled into Chrome.
jamescook@chromium.org changed reviewers: - jamescook@chromium.org
aboxhall@chromium.org changed reviewers: - aboxhall@chromium.org |
