Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2863833003:
Fix primary cause of flakiness of DumpAccessibilityEvents tests on Win (Closed)
|
Created:
3 years, 7 months ago by dmazzoni Modified:
3 years, 7 months ago Reviewers:
nektarios CC:
aboxhall+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dougt+watch_chromium.org, dtseng+watch_chromium.org, jam, je_julie, nektar+watch_chromium.org, yuzo+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix primary cause of flakiness of DumpAccessibilityEvents tests on Win
Normally we suppress focus events when the native window isn't itself
focused. This was causing test flakiness for tests that specifically
listen for those focus events; add a way to suppress them to make the
tests more robust.
BUG=719030
Review-Url: https://codereview.chromium.org/2863833003
Cr-Commit-Position: refs/heads/master@{#470589}
Committed: https://chromium.googlesource.com/chromium/src/+/d252421685e84c1f262f200ef54f79d159be47a7
Patch Set 1 #Patch Set 2 : Address feedback #Patch Set 3 : Fix logic #
Messages
Total messages: 33 (18 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + nektar@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
+bool g_never_suppress_focus_events_for_testing = false;
Why the "G" prefix? If you put this in an unnamed namespace, does it need to be
global?
+ if (!g_never_suppress_focus_events_for_testing) {
Isn't it better to have two conditions that would control whether focus events
would be bypassed? If the g_focus_change_callback_for_testing is not NULL or the
g_never_suppress_focus_events_for_testing flag is true, bypass the event.
if (g_focus_change_callback_for_testing.Get().is_null())
In the case you add this back, are you sure the Get().is_null() call is
necessary, or is there a corresponding boolean operator (operator!)?
void BrowserAccessibilityManager::SetFocusChangeCallbackForTesting(
const base::Closure& callback) {
g_focus_change_callback_for_testing.Get() = callback;
+ g_never_suppress_focus_events_for_testing = true;
I don't think you need to turn the flag on here. Just test for the callback when
the focus event is going to be fired.
These are two separate testing methods and I don't think we should mix them.
lgtm
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by nektar@chromium.org
Aren't you going to respond to my comments? I approved because my comments were not that important but I was waiting for a response too. Sorry for the confusion.
On 2017/05/08 19:25:18, nektarios wrote: > Aren't you going to respond to my comments? > I approved because my comments were not that important but I was waiting for a > response too. > Sorry for the confusion. Oops, sorry - I wasn't paying attention and saw the lgtm by itself in the last message so I clicked commit. I'll respond now.
On 2017/05/08 18:56:59, nektarios wrote:
> +bool g_never_suppress_focus_events_for_testing = false;
> Why the "G" prefix? If you put this in an unnamed namespace, does it need to
be
> global?
Moved to unnamed namespace, good idea. However it's still a global.
I'm just trying to follow conventions used in most other source files -
unless there was a PSA I'm not aware of, globals are still supposed to
have a g_ prefix. A code search shows there are hundreds and hundreds of
variables starting with g_ in chrome/ and content/, so this isn't
inconsistent with the rest of the code.
> + if (!g_never_suppress_focus_events_for_testing) {
> Isn't it better to have two conditions that would control whether focus events
> would be bypassed? If the g_focus_change_callback_for_testing is not NULL or
the
> g_never_suppress_focus_events_for_testing flag is true, bypass the event.
Sure, done.
> if (g_focus_change_callback_for_testing.Get().is_null())
> In the case you add this back, are you sure the Get().is_null() call is
> necessary, or is there a corresponding boolean operator (operator!)?
Done.
> void BrowserAccessibilityManager::SetFocusChangeCallbackForTesting(
> const base::Closure& callback) {
> g_focus_change_callback_for_testing.Get() = callback;
> + g_never_suppress_focus_events_for_testing = true;
>
> I don't think you need to turn the flag on here. Just test for the callback
when
> the focus event is going to be fired.
> These are two separate testing methods and I don't think we should mix them.
OK, done.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org Link to the patchset: https://codereview.chromium.org/2863833003/#ps20001 (title: "Address feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org Link to the patchset: https://codereview.chromium.org/2863833003/#ps40001 (title: "Fix logic")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494429185915930,
"parent_rev": "5ed4ad2180627b00b88ccbe53bbe0710b33f9778", "commit_rev":
"d252421685e84c1f262f200ef54f79d159be47a7"}
Message was sent while issue was closed.
Description was changed from ========== Fix primary cause of flakiness of DumpAccessibilityEvents tests on Win Normally we suppress focus events when the native window isn't itself focused. This was causing test flakiness for tests that specifically listen for those focus events; add a way to suppress them to make the tests more robust. BUG=719030 ========== to ========== Fix primary cause of flakiness of DumpAccessibilityEvents tests on Win Normally we suppress focus events when the native window isn't itself focused. This was causing test flakiness for tests that specifically listen for those focus events; add a way to suppress them to make the tests more robust. BUG=719030 Review-Url: https://codereview.chromium.org/2863833003 Cr-Commit-Position: refs/heads/master@{#470589} Committed: https://chromium.googlesource.com/chromium/src/+/d252421685e84c1f262f200ef54f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d252421685e84c1f262f200ef54f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
