Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522033002:
Generalized fix for serialization error/reset issues (Closed)
|
Created:
4 years, 1 month ago by David Tseng Modified:
4 years, 1 month ago Reviewers:
dmazzoni CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGeneralized fix for serialization error/reset issues
Whenever we see an AX tree fail to unserialize, request it be reset on the source end.
TEST=in hotrod, repeatedly toggle on/off ChromeVox. Verify that we re-populate the tree when we get any events (e.g. focus).
BUG=667819
Committed: https://crrev.com/38c63d7344c76d9fa58c84db6112daebbfefd3ab
Cr-Commit-Position: refs/heads/master@{#433985}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Generalized fix for serialization error/reset issues #Patch Set 3 : Fix test. #
Messages
Total messages: 22 (15 generated)
dtseng@chromium.org changed reviewers: + dmazzoni@chromium.org
Hopefully we can merge this into m55 at least. Long term, you or I should work on making source trees track client trees (in the event we begin having more than ChromeVox or one screen reader hitting the tree)
Description was changed from ========== Generalized fix for serialization error/reset issues Whenever we see an AX tree fail to unserialize, request it be reset on the source end. TEST=in hotrod, repeatedly toggle on/off ChromeVox. Verify that we re-populate the tree when we get any events (e.g. focus). BUG= ========== to ========== Generalized fix for serialization error/reset issues Whenever we see an AX tree fail to unserialize, request it be reset on the source end. TEST=in hotrod, repeatedly toggle on/off ChromeVox. Verify that we re-populate the tree when we get any events (e.g. focus). BUG=667819 ==========
https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/extensions/... File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/extensions/... chrome/renderer/extensions/automation_internal_custom_bindings.cc:998: while (state_shifter && state_shifter <= ui::AX_STATE_LAST) { Is this related? https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/extensions/... chrome/renderer/extensions/automation_internal_custom_bindings.cc:1167: v8::Number::New(isolate, tree_id)); nit: fits on one line https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/resources/e... File chrome/renderer/resources/extensions/automation_custom_bindings.js (right): https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/resources/e... chrome/renderer/resources/extensions/automation_custom_bindings.js:350: automationInternal.onAccessibilityTreeSerializationError.addListener(function(id) { nit: wrap https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/resources/e... chrome/renderer/resources/extensions/automation_custom_bindings.js:351: // Possibly clear native focus. Is this necessary? Won't it fix itself when the new frame is retrieved? https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/resources/e... chrome/renderer/resources/extensions/automation_custom_bindings.js:355: automationInternal.enableFrame(id); Does calling enableFrame() force it to reset? I think we should either add a comment to enableFrame saying that it must reset (otherwise we could accidentally try to optimize away redundant calls to enableFrame), or else we should add a separate resetFrame call.
https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/extensions/... File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/extensions/... chrome/renderer/extensions/automation_internal_custom_bindings.cc:998: while (state_shifter && state_shifter <= ui::AX_STATE_LAST) { On 2016/11/22 18:11:42, dmazzoni wrote: > Is this related? Somewhat. When toggling off/on, we get incomplete tree data and among other things, start accessing bogous state values. https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/extensions/... chrome/renderer/extensions/automation_internal_custom_bindings.cc:1167: v8::Number::New(isolate, tree_id)); On 2016/11/22 18:11:42, dmazzoni wrote: > nit: fits on one line Done. https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/resources/e... File chrome/renderer/resources/extensions/automation_custom_bindings.js (right): https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/resources/e... chrome/renderer/resources/extensions/automation_custom_bindings.js:350: automationInternal.onAccessibilityTreeSerializationError.addListener(function(id) { On 2016/11/22 18:11:42, dmazzoni wrote: > nit: wrap Done. https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/resources/e... chrome/renderer/resources/extensions/automation_custom_bindings.js:351: // Possibly clear native focus. On 2016/11/22 18:11:42, dmazzoni wrote: > Is this necessary? Won't it fix itself when the new frame is retrieved? Navigation could still occur before then, but likely not for long before the reserialized frame comes back. I removed it. https://codereview.chromium.org/2522033002/diff/1/chrome/renderer/resources/e... chrome/renderer/resources/extensions/automation_custom_bindings.js:355: automationInternal.enableFrame(id); On 2016/11/22 18:11:42, dmazzoni wrote: > Does calling enableFrame() force it to reset? > > I think we should either add a comment to enableFrame saying that > it must reset (otherwise we could accidentally try to optimize away > redundant calls to enableFrame), or else we should add a separate > resetFrame call. Let's rename this to enableOrResetFrame (in another cl). That's how it's been used and it's automation only (tree only mode).
lgtm
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
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: linux_chromium_chromeos_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 dtseng@chromium.org to run a CQ dry run
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: This issue passed the CQ dry run.
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2522033002/#ps40001 (title: "Fix test.")
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": 1479849690788520,
"parent_rev": "53909ca6793aee68531fc4d291d5851d9020a559", "commit_rev":
"7cab049df9858e1e8df5beb2afe675dbae84d638"}
Message was sent while issue was closed.
Description was changed from ========== Generalized fix for serialization error/reset issues Whenever we see an AX tree fail to unserialize, request it be reset on the source end. TEST=in hotrod, repeatedly toggle on/off ChromeVox. Verify that we re-populate the tree when we get any events (e.g. focus). BUG=667819 ========== to ========== Generalized fix for serialization error/reset issues Whenever we see an AX tree fail to unserialize, request it be reset on the source end. TEST=in hotrod, repeatedly toggle on/off ChromeVox. Verify that we re-populate the tree when we get any events (e.g. focus). BUG=667819 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Generalized fix for serialization error/reset issues Whenever we see an AX tree fail to unserialize, request it be reset on the source end. TEST=in hotrod, repeatedly toggle on/off ChromeVox. Verify that we re-populate the tree when we get any events (e.g. focus). BUG=667819 ========== to ========== Generalized fix for serialization error/reset issues Whenever we see an AX tree fail to unserialize, request it be reset on the source end. TEST=in hotrod, repeatedly toggle on/off ChromeVox. Verify that we re-populate the tree when we get any events (e.g. focus). BUG=667819 Committed: https://crrev.com/38c63d7344c76d9fa58c84db6112daebbfefd3ab Cr-Commit-Position: refs/heads/master@{#433985} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/38c63d7344c76d9fa58c84db6112daebbfefd3ab Cr-Commit-Position: refs/heads/master@{#433985} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
