Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737043003:
Add support for attribute change callbacks in AXNodeData. (Closed)
|
Created:
3 years, 9 months ago by dmazzoni Modified:
3 years, 9 months ago Reviewers:
aboxhall CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for attribute change callbacks in AXNodeData.
The idea here is that instead of Blink firing events like "name changed",
"value changed", or "checked state changed", Blink should be able to just
fire a generic notification that a node changed, and then each platform
should be able to trigger platform-specific events as needed, based on
actual changes to the attributes of an accessibilitiy node.
This change just adds support for attribute change callbacks, so it's
easy to write code that gets called whenever any particular attribute
changes.
Quite tedious, but I wanted to make sure this was both comprehensive and
exhaustively tested so we can feel confident using it.
BUG=699438
Review-Url: https://codereview.chromium.org/2737043003
Cr-Commit-Position: refs/heads/master@{#456425}
Committed: https://chromium.googlesource.com/chromium/src/+/3ab5385cf6be9069d26b135647dc2df6e1f33e3f
Patch Set 1 #
Total comments: 2
Patch Set 2 : Yay for template magic #
Total comments: 10
Patch Set 3 : Address feedback, lambda in local var #
Messages
Total messages: 21 (13 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + aboxhall@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Still going through tests but otherwise looks great, with one possibly infeasible suggestion. https://codereview.chromium.org/2737043003/diff/1/ui/accessibility/ax_tree.cc File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2737043003/diff/1/ui/accessibility/ax_tree.cc... ui/accessibility/ax_tree.cc:338: delegate_->OnStringAttributeChanged(this, node, old_strings[i].first, Can we fix this duplication somehow? Something something templated method and function pointers? Not worth it?
The CQ bit was checked by dmazzoni@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...
https://codereview.chromium.org/2737043003/diff/1/ui/accessibility/ax_tree.cc File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2737043003/diff/1/ui/accessibility/ax_tree.cc... ui/accessibility/ax_tree.cc:338: delegate_->OnStringAttributeChanged(this, node, old_strings[i].first, On 2017/03/09 00:46:06, aboxhall wrote: > Can we fix this duplication somehow? Something something templated method and > function pointers? Not worth it? Figured it out! What do you think? It's about 90% better, far less duplication, but about 25% more "magic". :) https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:50: template <typename K, typename V, typename F> The typename F here is the only part I'm not thrilled about. I couldn't figure out the right signature for F since because a lamba that captures is not interchangeable with a function pointer. I could probably use std::function, but I'm not sure that would compile away. Apparently using <typename F> when you want to pass a lambda is a pretty common trick - just let the compiler figure out what the type is.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with suggestions. https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:50: template <typename K, typename V, typename F> On 2017/03/10 23:40:26, dmazzoni wrote: > The typename F here is the only part I'm not thrilled about. > I couldn't figure out the right signature for F since > because a lamba that captures is not interchangeable with > a function pointer. I could probably use std::function, but > I'm not sure that would compile away. > > Apparently using <typename F> when you want to pass a > lambda is a pretty common trick - just let the compiler > figure out what the type is. I had a play with this because I wanted to understand everything going on here! I don't understand what you mean about the lambda "compiling away". Do lambdas have some kind of "pretend I'm not here" magic? Otherwise, yeah, the typename F is a bit weird but I think it's ok. https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:55: // void (*callback)(K, const V&, const V&)) { Stray commented out code :) https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:62: } else { Early out instead of else {} ? https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:371: [this, node](AXStringAttribute attr, const std::string& old_string, Maybe pull the lambda out into a local variable for clarity? It's kind of a lot going on otherwise.
Thanks! https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:50: template <typename K, typename V, typename F> On 2017/03/13 03:29:29, aboxhall wrote: > On 2017/03/10 23:40:26, dmazzoni wrote: > > The typename F here is the only part I'm not thrilled about. > > I couldn't figure out the right signature for F since > > because a lamba that captures is not interchangeable with > > a function pointer. I could probably use std::function, but > > I'm not sure that would compile away. > > > > Apparently using <typename F> when you want to pass a > > lambda is a pretty common trick - just let the compiler > > figure out what the type is. > > I had a play with this because I wanted to understand everything going on here! > > I don't understand what you mean about the lambda "compiling away". Do lambdas > have some kind of "pretend I'm not here" magic? No, but presumably the overhead of a lambda is not much more than any other function call, whereas std::function is a whole object and comes with more overhead. > Otherwise, yeah, the typename F is a bit weird but I think it's ok. Sounds good https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:55: // void (*callback)(K, const V&, const V&)) { On 2017/03/13 03:29:29, aboxhall wrote: > Stray commented out code :) Done. https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:62: } else { On 2017/03/13 03:29:29, aboxhall wrote: > Early out instead of else {} ? Good idea! Helps readability. https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:371: [this, node](AXStringAttribute attr, const std::string& old_string, On 2017/03/13 03:29:29, aboxhall wrote: > Maybe pull the lambda out into a local variable for clarity? It's kind of a lot > going on otherwise. Done.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aboxhall@chromium.org Link to the patchset: https://codereview.chromium.org/2737043003/#ps40001 (title: "Address feedback, lambda in local var")
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": 1489422761594480,
"parent_rev": "b2a374ecdadcd8ebb5f6b9478458725ca25768e0", "commit_rev":
"3ab5385cf6be9069d26b135647dc2df6e1f33e3f"}
Message was sent while issue was closed.
Description was changed from ========== Add support for attribute change callbacks in AXNodeData. The idea here is that instead of Blink firing events like "name changed", "value changed", or "checked state changed", Blink should be able to just fire a generic notification that a node changed, and then each platform should be able to trigger platform-specific events as needed, based on actual changes to the attributes of an accessibilitiy node. This change just adds support for attribute change callbacks, so it's easy to write code that gets called whenever any particular attribute changes. Quite tedious, but I wanted to make sure this was both comprehensive and exhaustively tested so we can feel confident using it. BUG=699438 ========== to ========== Add support for attribute change callbacks in AXNodeData. The idea here is that instead of Blink firing events like "name changed", "value changed", or "checked state changed", Blink should be able to just fire a generic notification that a node changed, and then each platform should be able to trigger platform-specific events as needed, based on actual changes to the attributes of an accessibilitiy node. This change just adds support for attribute change callbacks, so it's easy to write code that gets called whenever any particular attribute changes. Quite tedious, but I wanted to make sure this was both comprehensive and exhaustively tested so we can feel confident using it. BUG=699438 Review-Url: https://codereview.chromium.org/2737043003 Cr-Commit-Position: refs/heads/master@{#456425} Committed: https://chromium.googlesource.com/chromium/src/+/3ab5385cf6be9069d26b135647dc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3ab5385cf6be9069d26b135647dc...
Message was sent while issue was closed.
(Still lgtm) https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2737043003/diff/20001/ui/accessibility/ax_tre... ui/accessibility/ax_tree.cc:50: template <typename K, typename V, typename F> On 2017/03/13 16:32:37, dmazzoni wrote: > On 2017/03/13 03:29:29, aboxhall wrote: > > On 2017/03/10 23:40:26, dmazzoni wrote: > > > The typename F here is the only part I'm not thrilled about. > > > I couldn't figure out the right signature for F since > > > because a lamba that captures is not interchangeable with > > > a function pointer. I could probably use std::function, but > > > I'm not sure that would compile away. > > > > > > Apparently using <typename F> when you want to pass a > > > lambda is a pretty common trick - just let the compiler > > > figure out what the type is. > > > > I had a play with this because I wanted to understand everything going on > here! > > > > I don't understand what you mean about the lambda "compiling away". Do lambdas > > have some kind of "pretend I'm not here" magic? > > No, but presumably the overhead of a lambda is not much > more than any other function call, whereas std::function is > a whole object and comes with more overhead. > > > Otherwise, yeah, the typename F is a bit weird but I think it's ok. > > Sounds good Oh! In that case would base::BindRepeating() have worked? I had a play with that yesterday. You would have to make some local wrapper functions to get a consistent type signature, but then you could get rid of the F magic. Not really a clear win. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
