Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1073983005:
Take a snapshot of the AXTree (Closed)
|
Created:
5 years, 8 months ago by sgurun-gerrit only Modified:
5 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@assist-3 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTake a snapshot of the AXTree
b/19771298
Provide a snapshot of the AXTree to webview. Depends on
https://codereview.chromium.org/1051923003/
BUG=472704
Committed: https://crrev.com/ff49f7c098655db11e160761f08c4b81b0cb6c4c
Cr-Commit-Position: refs/heads/master@{#325065}
Patch Set 1 #
Total comments: 11
Patch Set 2 : address feedback #
Total comments: 6
Patch Set 3 : address feedback after lgtm #Patch Set 4 : suppress findbugs issues #Messages
Total messages: 25 (8 generated)
sgurun@chromium.org changed reviewers: + dmazzoni@chromium.org
On 2015/04/10 07:47:40, sgurun wrote: > mailto:sgurun@chromium.org changed reviewers: > + mailto:dmazzoni@chromium.org This still needs a little bit of cleanup, snapshotting more data and some more tests but I think getting close.
https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_android.cc:49: content::Java_WebContentsImpl_onEvaluateJavaScriptResult( Is there a reason not to use "namespace content" inside this file? It doesn't make sense to me to see content:: frequently when this source file is already within content. If there's a good reason, I'd suggest some "using" declarations at the top to make the code more readable, like: using content::BrowserAccessibilityManager etc. https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_android.cc:85: // TODO(sgurun) check if root can be null here. You should probably first check if result.nodes is empty and consider that a failure if so. Otherwise, BrowserAccessibilityManager will parse the AXTreeUpdate and it will always have a valid root if so. I just realize that if it fails to parse it either calls the delegate if it has one, or exits with a fatal error if not, so you may want to consider whether you want to provide a delegate. There's no valid reason you should get a bad AXTreeUpdate, it's just a question of if you have a compromised or corrupted renderer if you want to return an error or just crash the whole program. https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java (right): https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:349: private static AXNodeData createAXNode(int id, int x, int y, int scrollX, int scrollY, This makes me think that AXNodeData (or whatever we call it) should be defined in org.chromium.content.browser.webcontents, not in org.chromium.ui. https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/AXNodeData.java (right): https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/AXNodeData.java:10: * A data struct to convey AXNodeData information to Java side. I think we should convey how this differs from similar data structures in BrowserAccessibilityManager.java. Something like: This is a lightweight data structure that encodes some key information from the accessibility tree for operations that need a quick one-time snapshot of the web content. This is different from BrowserAccessibilityManager.java, which maintains a persistent Android accessibility tree that can be queried synchronously by the Android framework. https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/AXNodeData.java:12: public class AXNodeData { It feels like this is the wrong level of abstraction or the wrong name. AXNodeData is a core accessibility data structure in Chromium code, but this class is just encoding some narrow Android-specific information so it doesn't seem right to give it exactly the same name. Historically we also don't use "AX" in Android / Java code, but I don't care too much about that. My thought is to call it something like AndroidAccessibilitySnapshotNode and put it in, e.g. content/public/android/java/src/org/chromium/content/browser/webcontents I think "Android" should be in the class name explicitly to indicate that this is an Android-specific object, not just the Android counterpart to a cross-platform object (which AXNodeData implies).
sgurun@chromium.org changed reviewers: + dtrainor@chromium.org
sgurun@chromium.org changed reviewers: + jam@chromium.org - dtrainor@chromium.org
https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_android.cc:49: content::Java_WebContentsImpl_onEvaluateJavaScriptResult( On 2015/04/10 19:20:00, dmazzoni wrote: > Is there a reason not to use "namespace content" inside this file? It doesn't > make sense to me to see content:: frequently when this source file is already > within content. > > If there's a good reason, I'd suggest some "using" declarations at the top to > make the code more readable, like: > > using content::BrowserAccessibilityManager > > etc. > Done. https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_android.cc:85: // TODO(sgurun) check if root can be null here. On 2015/04/10 19:20:00, dmazzoni wrote: > You should probably first check if result.nodes is empty and consider that a > failure if so. > > Otherwise, BrowserAccessibilityManager will parse the AXTreeUpdate and it will > always have a valid root if so. > > I just realize that if it fails to parse it either calls the delegate if it has > one, or exits with a fatal error if not, so you may want to consider whether you > want to provide a delegate. > > There's no valid reason you should get a bad AXTreeUpdate, it's just a question > of if you have a compromised or corrupted renderer if you want to return an > error or just crash the whole program. Thanks, for webview we probably don't worry too much about a corrupted renderer case since webview is single process, and if renderer is compromised/corrupted a crash is likely imminent anyway. checked for the condition result.nodes is empty. https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java (right): https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:349: private static AXNodeData createAXNode(int id, int x, int y, int scrollX, int scrollY, On 2015/04/10 19:20:00, dmazzoni wrote: > This makes me think that AXNodeData (or whatever we call it) should be defined > in org.chromium.content.browser.webcontents, not in org.chromium.ui. Done. https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/AXNodeData.java (right): https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/AXNodeData.java:10: * A data struct to convey AXNodeData information to Java side. On 2015/04/10 19:20:00, dmazzoni wrote: > I think we should convey how this differs from similar data structures in > BrowserAccessibilityManager.java. > > Something like: This is a lightweight data structure that encodes some key > information from the accessibility tree for operations that need a quick > one-time snapshot of the web content. This is different from > BrowserAccessibilityManager.java, which maintains a persistent Android > accessibility tree that can be queried synchronously by the Android framework. That is a very clear description. Thanks! https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/AXNodeData.java:12: public class AXNodeData { On 2015/04/10 19:20:00, dmazzoni wrote: > It feels like this is the wrong level of abstraction or the wrong name. > AXNodeData is a core accessibility data structure in Chromium code, but this > class is just encoding some narrow Android-specific information so it doesn't > seem right to give it exactly the same name. Historically we also don't use "AX" > in Android / Java code, but I don't care too much about that. > > My thought is to call it something like AndroidAccessibilitySnapshotNode and put > it in, e.g. > content/public/android/java/src/org/chromium/content/browser/webcontents > > I think "Android" should be in the class name explicitly to indicate that this > is an Android-specific object, not just the Android counterpart to a > cross-platform object (which AXNodeData implies). Done.
On 2015/04/10 23:24:51, sgurun wrote: > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > File content/browser/web_contents/web_contents_android.cc (right): > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > content/browser/web_contents/web_contents_android.cc:49: > content::Java_WebContentsImpl_onEvaluateJavaScriptResult( > On 2015/04/10 19:20:00, dmazzoni wrote: > > Is there a reason not to use "namespace content" inside this file? It doesn't > > make sense to me to see content:: frequently when this source file is already > > within content. > > > > If there's a good reason, I'd suggest some "using" declarations at the top to > > make the code more readable, like: > > > > using content::BrowserAccessibilityManager > > > > etc. > > > > Done. > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > content/browser/web_contents/web_contents_android.cc:85: // TODO(sgurun) check > if root can be null here. > On 2015/04/10 19:20:00, dmazzoni wrote: > > You should probably first check if result.nodes is empty and consider that a > > failure if so. > > > > Otherwise, BrowserAccessibilityManager will parse the AXTreeUpdate and it will > > always have a valid root if so. > > > > I just realize that if it fails to parse it either calls the delegate if it > has > > one, or exits with a fatal error if not, so you may want to consider whether > you > > want to provide a delegate. > > > > There's no valid reason you should get a bad AXTreeUpdate, it's just a > question > > of if you have a compromised or corrupted renderer if you want to return an > > error or just crash the whole program. > > Thanks, for webview we probably don't worry too much about a corrupted renderer > case since webview is single process, and if renderer is compromised/corrupted a > crash is likely imminent anyway. checked for the condition result.nodes is > empty. > > https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java > (right): > > https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:349: > private static AXNodeData createAXNode(int id, int x, int y, int scrollX, int > scrollY, > On 2015/04/10 19:20:00, dmazzoni wrote: > > This makes me think that AXNodeData (or whatever we call it) should be defined > > in org.chromium.content.browser.webcontents, not in org.chromium.ui. > > Done. > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > File ui/android/java/src/org/chromium/ui/AXNodeData.java (right): > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > ui/android/java/src/org/chromium/ui/AXNodeData.java:10: * A data struct to > convey AXNodeData information to Java side. > On 2015/04/10 19:20:00, dmazzoni wrote: > > I think we should convey how this differs from similar data structures in > > BrowserAccessibilityManager.java. > > > > Something like: This is a lightweight data structure that encodes some key > > information from the accessibility tree for operations that need a quick > > one-time snapshot of the web content. This is different from > > BrowserAccessibilityManager.java, which maintains a persistent Android > > accessibility tree that can be queried synchronously by the Android framework. > > That is a very clear description. Thanks! > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > ui/android/java/src/org/chromium/ui/AXNodeData.java:12: public class AXNodeData > { > On 2015/04/10 19:20:00, dmazzoni wrote: > > It feels like this is the wrong level of abstraction or the wrong name. > > AXNodeData is a core accessibility data structure in Chromium code, but this > > class is just encoding some narrow Android-specific information so it doesn't > > seem right to give it exactly the same name. Historically we also don't use > "AX" > > in Android / Java code, but I don't care too much about that. > > > > My thought is to call it something like AndroidAccessibilitySnapshotNode and > put > > it in, e.g. > > content/public/android/java/src/org/chromium/content/browser/webcontents > > > > I think "Android" should be in the class name explicitly to indicate that this > > is an Android-specific object, not just the Android counterpart to a > > cross-platform object (which AXNodeData implies). > > Done. jam@ PTAL content/ thanks!
https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_android.cc:85: // TODO(sgurun) check if root can be null here. On 2015/04/10 23:24:50, sgurun wrote: > On 2015/04/10 19:20:00, dmazzoni wrote: > > You should probably first check if result.nodes is empty and consider that a > > failure if so. > > > > Otherwise, BrowserAccessibilityManager will parse the AXTreeUpdate and it will > > always have a valid root if so. > > > > I just realize that if it fails to parse it either calls the delegate if it > has > > one, or exits with a fatal error if not, so you may want to consider whether > you > > want to provide a delegate. > > > > There's no valid reason you should get a bad AXTreeUpdate, it's just a > question > > of if you have a compromised or corrupted renderer if you want to return an > > error or just crash the whole program. > > Thanks, for webview we probably don't worry too much about a corrupted renderer > case since webview is single process, and if renderer is compromised/corrupted a > crash is likely imminent anyway. checked for the condition result.nodes is > empty. Drive by meta-comment: Single-process-ness of webview is irrelevant here. This is content code that is multi-process in clank. Also it's just generally bad even in webview-only code trust information from renderer. We should follow chrome's pattern of checking as well.
On 2015/04/10 23:47:17, boliu wrote: > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > File content/browser/web_contents/web_contents_android.cc (right): > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > content/browser/web_contents/web_contents_android.cc:85: // TODO(sgurun) check > if root can be null here. > On 2015/04/10 23:24:50, sgurun wrote: > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > You should probably first check if result.nodes is empty and consider that a > > > failure if so. > > > > > > Otherwise, BrowserAccessibilityManager will parse the AXTreeUpdate and it > will > > > always have a valid root if so. > > > > > > I just realize that if it fails to parse it either calls the delegate if it > > has > > > one, or exits with a fatal error if not, so you may want to consider whether > > you > > > want to provide a delegate. > > > > > > There's no valid reason you should get a bad AXTreeUpdate, it's just a > > question > > > of if you have a compromised or corrupted renderer if you want to return an > > > error or just crash the whole program. > > > > Thanks, for webview we probably don't worry too much about a corrupted > renderer > > case since webview is single process, and if renderer is compromised/corrupted > a > > crash is likely imminent anyway. checked for the condition result.nodes is > > empty. > > Drive by meta-comment: > > Single-process-ness of webview is irrelevant here. This is content code that is > multi-process in clank. > > Also it's just generally bad even in webview-only code trust information from > renderer. We should follow chrome's pattern of checking as well. yeah, conceptually. not hard to check, we can add it. In a single process you can't do much if renderer is compromised.
lgtm https://codereview.chromium.org/1073983005/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1073983005/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_android.cc:59: ConvertUTF16ToJavaString(env,node->GetText()); nit: space after comma (throughout) https://codereview.chromium.org/1073983005/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java (right): https://codereview.chromium.org/1073983005/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java:17: public int id; Let's delete "id" unless you have a really good reason why it's needed. https://codereview.chromium.org/1073983005/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java:19: public int childCount; Any reason for this over children.size()?
On 2015/04/10 23:25:21, sgurun wrote: > On 2015/04/10 23:24:51, sgurun wrote: > > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > > File content/browser/web_contents/web_contents_android.cc (right): > > > > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > > content/browser/web_contents/web_contents_android.cc:49: > > content::Java_WebContentsImpl_onEvaluateJavaScriptResult( > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > Is there a reason not to use "namespace content" inside this file? It > doesn't > > > make sense to me to see content:: frequently when this source file is > already > > > within content. > > > > > > If there's a good reason, I'd suggest some "using" declarations at the top > to > > > make the code more readable, like: > > > > > > using content::BrowserAccessibilityManager > > > > > > etc. > > > > > > > Done. > > > > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > > content/browser/web_contents/web_contents_android.cc:85: // TODO(sgurun) check > > if root can be null here. > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > You should probably first check if result.nodes is empty and consider that a > > > failure if so. > > > > > > Otherwise, BrowserAccessibilityManager will parse the AXTreeUpdate and it > will > > > always have a valid root if so. > > > > > > I just realize that if it fails to parse it either calls the delegate if it > > has > > > one, or exits with a fatal error if not, so you may want to consider whether > > you > > > want to provide a delegate. > > > > > > There's no valid reason you should get a bad AXTreeUpdate, it's just a > > question > > > of if you have a compromised or corrupted renderer if you want to return an > > > error or just crash the whole program. > > > > Thanks, for webview we probably don't worry too much about a corrupted > renderer > > case since webview is single process, and if renderer is compromised/corrupted > a > > crash is likely imminent anyway. checked for the condition result.nodes is > > empty. > > > > > https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... > > File > > > content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java > > (right): > > > > > https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... > > > content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:349: > > private static AXNodeData createAXNode(int id, int x, int y, int scrollX, int > > scrollY, > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > This makes me think that AXNodeData (or whatever we call it) should be > defined > > > in org.chromium.content.browser.webcontents, not in org.chromium.ui. > > > > Done. > > > > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > > File ui/android/java/src/org/chromium/ui/AXNodeData.java (right): > > > > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > > ui/android/java/src/org/chromium/ui/AXNodeData.java:10: * A data struct to > > convey AXNodeData information to Java side. > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > I think we should convey how this differs from similar data structures in > > > BrowserAccessibilityManager.java. > > > > > > Something like: This is a lightweight data structure that encodes some key > > > information from the accessibility tree for operations that need a quick > > > one-time snapshot of the web content. This is different from > > > BrowserAccessibilityManager.java, which maintains a persistent Android > > > accessibility tree that can be queried synchronously by the Android > framework. > > > > That is a very clear description. Thanks! > > > > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > > ui/android/java/src/org/chromium/ui/AXNodeData.java:12: public class > AXNodeData > > { > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > It feels like this is the wrong level of abstraction or the wrong name. > > > AXNodeData is a core accessibility data structure in Chromium code, but this > > > class is just encoding some narrow Android-specific information so it > doesn't > > > seem right to give it exactly the same name. Historically we also don't use > > "AX" > > > in Android / Java code, but I don't care too much about that. > > > > > > My thought is to call it something like AndroidAccessibilitySnapshotNode and > > put > > > it in, e.g. > > > content/public/android/java/src/org/chromium/content/browser/webcontents > > > > > > I think "Android" should be in the class name explicitly to indicate that > this > > > is an Android-specific object, not just the Android counterpart to a > > > cross-platform object (which AXNodeData implies). > > > > Done. > > jam@ PTAL content/ > > > thanks! tedchoc is an owner for these two files and he would be a better reviewer.
On 2015/04/13 14:36:41, jam wrote: > On 2015/04/10 23:25:21, sgurun wrote: > > On 2015/04/10 23:24:51, sgurun wrote: > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > > > File content/browser/web_contents/web_contents_android.cc (right): > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > > > content/browser/web_contents/web_contents_android.cc:49: > > > content::Java_WebContentsImpl_onEvaluateJavaScriptResult( > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > Is there a reason not to use "namespace content" inside this file? It > > doesn't > > > > make sense to me to see content:: frequently when this source file is > > already > > > > within content. > > > > > > > > If there's a good reason, I'd suggest some "using" declarations at the top > > to > > > > make the code more readable, like: > > > > > > > > using content::BrowserAccessibilityManager > > > > > > > > etc. > > > > > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > > > content/browser/web_contents/web_contents_android.cc:85: // TODO(sgurun) > check > > > if root can be null here. > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > You should probably first check if result.nodes is empty and consider that > a > > > > failure if so. > > > > > > > > Otherwise, BrowserAccessibilityManager will parse the AXTreeUpdate and it > > will > > > > always have a valid root if so. > > > > > > > > I just realize that if it fails to parse it either calls the delegate if > it > > > has > > > > one, or exits with a fatal error if not, so you may want to consider > whether > > > you > > > > want to provide a delegate. > > > > > > > > There's no valid reason you should get a bad AXTreeUpdate, it's just a > > > question > > > > of if you have a compromised or corrupted renderer if you want to return > an > > > > error or just crash the whole program. > > > > > > Thanks, for webview we probably don't worry too much about a corrupted > > renderer > > > case since webview is single process, and if renderer is > compromised/corrupted > > a > > > crash is likely imminent anyway. checked for the condition result.nodes is > > > empty. > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... > > > File > > > > > > content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... > > > > > > content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:349: > > > private static AXNodeData createAXNode(int id, int x, int y, int scrollX, > int > > > scrollY, > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > This makes me think that AXNodeData (or whatever we call it) should be > > defined > > > > in org.chromium.content.browser.webcontents, not in org.chromium.ui. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > > > File ui/android/java/src/org/chromium/ui/AXNodeData.java (right): > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > > > ui/android/java/src/org/chromium/ui/AXNodeData.java:10: * A data struct to > > > convey AXNodeData information to Java side. > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > I think we should convey how this differs from similar data structures in > > > > BrowserAccessibilityManager.java. > > > > > > > > Something like: This is a lightweight data structure that encodes some key > > > > information from the accessibility tree for operations that need a quick > > > > one-time snapshot of the web content. This is different from > > > > BrowserAccessibilityManager.java, which maintains a persistent Android > > > > accessibility tree that can be queried synchronously by the Android > > framework. > > > > > > That is a very clear description. Thanks! > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > > > ui/android/java/src/org/chromium/ui/AXNodeData.java:12: public class > > AXNodeData > > > { > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > It feels like this is the wrong level of abstraction or the wrong name. > > > > AXNodeData is a core accessibility data structure in Chromium code, but > this > > > > class is just encoding some narrow Android-specific information so it > > doesn't > > > > seem right to give it exactly the same name. Historically we also don't > use > > > "AX" > > > > in Android / Java code, but I don't care too much about that. > > > > > > > > My thought is to call it something like AndroidAccessibilitySnapshotNode > and > > > put > > > > it in, e.g. > > > > content/public/android/java/src/org/chromium/content/browser/webcontents > > > > > > > > I think "Android" should be in the class name explicitly to indicate that > > this > > > > is an Android-specific object, not just the Android counterpart to a > > > > cross-platform object (which AXNodeData implies). > > > > > > Done. > > > > jam@ PTAL content/ > > > > > > thanks! > > tedchoc is an owner for these two files and he would be a better reviewer. Hi Jam, TedChoc seems to OOO until 4/21.
On 2015/04/13 14:42:43, sgurun wrote: > On 2015/04/13 14:36:41, jam wrote: > > On 2015/04/10 23:25:21, sgurun wrote: > > > On 2015/04/10 23:24:51, sgurun wrote: > > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > > > > File content/browser/web_contents/web_contents_android.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > > > > content/browser/web_contents/web_contents_android.cc:49: > > > > content::Java_WebContentsImpl_onEvaluateJavaScriptResult( > > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > > Is there a reason not to use "namespace content" inside this file? It > > > doesn't > > > > > make sense to me to see content:: frequently when this source file is > > > already > > > > > within content. > > > > > > > > > > If there's a good reason, I'd suggest some "using" declarations at the > top > > > to > > > > > make the code more readable, like: > > > > > > > > > > using content::BrowserAccessibilityManager > > > > > > > > > > etc. > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/browser/web_content... > > > > content/browser/web_contents/web_contents_android.cc:85: // TODO(sgurun) > > check > > > > if root can be null here. > > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > > You should probably first check if result.nodes is empty and consider > that > > a > > > > > failure if so. > > > > > > > > > > Otherwise, BrowserAccessibilityManager will parse the AXTreeUpdate and > it > > > will > > > > > always have a valid root if so. > > > > > > > > > > I just realize that if it fails to parse it either calls the delegate if > > it > > > > has > > > > > one, or exits with a fatal error if not, so you may want to consider > > whether > > > > you > > > > > want to provide a delegate. > > > > > > > > > > There's no valid reason you should get a bad AXTreeUpdate, it's just a > > > > question > > > > > of if you have a compromised or corrupted renderer if you want to return > > an > > > > > error or just crash the whole program. > > > > > > > > Thanks, for webview we probably don't worry too much about a corrupted > > > renderer > > > > case since webview is single process, and if renderer is > > compromised/corrupted > > > a > > > > crash is likely imminent anyway. checked for the condition result.nodes is > > > > empty. > > > > > > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... > > > > File > > > > > > > > > > content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/content/public/android/java... > > > > > > > > > > content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:349: > > > > private static AXNodeData createAXNode(int id, int x, int y, int scrollX, > > int > > > > scrollY, > > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > > This makes me think that AXNodeData (or whatever we call it) should be > > > defined > > > > > in org.chromium.content.browser.webcontents, not in org.chromium.ui. > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > > > > File ui/android/java/src/org/chromium/ui/AXNodeData.java (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > > > > ui/android/java/src/org/chromium/ui/AXNodeData.java:10: * A data struct to > > > > convey AXNodeData information to Java side. > > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > > I think we should convey how this differs from similar data structures > in > > > > > BrowserAccessibilityManager.java. > > > > > > > > > > Something like: This is a lightweight data structure that encodes some > key > > > > > information from the accessibility tree for operations that need a quick > > > > > one-time snapshot of the web content. This is different from > > > > > BrowserAccessibilityManager.java, which maintains a persistent Android > > > > > accessibility tree that can be queried synchronously by the Android > > > framework. > > > > > > > > That is a very clear description. Thanks! > > > > > > > > > > > > > > https://codereview.chromium.org/1073983005/diff/1/ui/android/java/src/org/chr... > > > > ui/android/java/src/org/chromium/ui/AXNodeData.java:12: public class > > > AXNodeData > > > > { > > > > On 2015/04/10 19:20:00, dmazzoni wrote: > > > > > It feels like this is the wrong level of abstraction or the wrong name. > > > > > AXNodeData is a core accessibility data structure in Chromium code, but > > this > > > > > class is just encoding some narrow Android-specific information so it > > > doesn't > > > > > seem right to give it exactly the same name. Historically we also don't > > use > > > > "AX" > > > > > in Android / Java code, but I don't care too much about that. > > > > > > > > > > My thought is to call it something like AndroidAccessibilitySnapshotNode > > and > > > > put > > > > > it in, e.g. > > > > > content/public/android/java/src/org/chromium/content/browser/webcontents > > > > > > > > > > I think "Android" should be in the class name explicitly to indicate > that > > > this > > > > > is an Android-specific object, not just the Android counterpart to a > > > > > cross-platform object (which AXNodeData implies). > > > > > > > > Done. > > > > > > jam@ PTAL content/ > > > > > > > > > thanks! > > > > tedchoc is an owner for these two files and he would be a better reviewer. > > Hi Jam, > > TedChoc seems to OOO until 4/21. ok rubberstamp lgtm, deferring to dmazzoni
fixed issues dmazzoni raised in his lgtm'ed review. Addressing the gracious handling of potential renderer compromise (rather than immediate crashing) turned out to be a little more involved so will file a bug for this case. https://codereview.chromium.org/1073983005/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1073983005/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_android.cc:59: ConvertUTF16ToJavaString(env,node->GetText()); On 2015/04/13 06:55:44, dmazzoni wrote: > nit: space after comma (throughout) Done. https://codereview.chromium.org/1073983005/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java (right): https://codereview.chromium.org/1073983005/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java:17: public int id; On 2015/04/13 06:55:44, dmazzoni wrote: > Let's delete "id" unless you have a really good reason why it's needed. not used. Done. https://codereview.chromium.org/1073983005/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java:19: public int childCount; On 2015/04/13 06:55:44, dmazzoni wrote: > Any reason for this over children.size()? it is not used actually. Thanks for catching it!
The CQ bit was checked by sgurun@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1073983005/#ps40001 (title: "address feedback after lgtm")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1073983005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
lgtm for FindBugs suppression
The CQ bit was checked by sgurun@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org Link to the patchset: https://codereview.chromium.org/1073983005/#ps60001 (title: "suppress findbugs issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1073983005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ff49f7c098655db11e160761f08c4b81b0cb6c4c Cr-Commit-Position: refs/heads/master@{#325065} |
