Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1137393003:
Add style information to the snapshot node (Closed)
|
Created:
5 years, 7 months ago by sgurun-gerrit only Modified:
5 years, 6 months ago CC:
android-webview-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd style information (font size, bold, italic, etc) and color to the snapshot.
BUG=490893
Committed: https://crrev.com/2fe5ccafafe4419c2c5c6a827b50a1d027057fb1
Cr-Commit-Position: refs/heads/master@{#332082}
Patch Set 1 #
Total comments: 2
Patch Set 2 : add tests and fix style checks #Patch Set 3 : #
Total comments: 10
Patch Set 4 : address code review #
Total comments: 2
Patch Set 5 : address code review #Patch Set 6 : gn support #
Total comments: 5
Patch Set 7 : address sievers review #
Total comments: 2
Patch Set 8 : address code review (dependency and non-existent style) #
Total comments: 7
Patch Set 9 : put android specific section of GN files in is_android block #Patch Set 10 : rebased and fixed gyp/gn files for non-android targets #Patch Set 11 : add content dependency to ui_accessibility_java for GN #Messages
Total messages: 62 (15 generated)
sgurun@chromium.org changed reviewers: + dmazzoni@chromium.org
Hey Dominic, I have a draft here for collecting style info. I don't get the data I expect to receive at the moment. Do you mind answering my comment? thanks. https://codereview.chromium.org/1137393003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_android.cc:66: int text_style = node->GetIntAttribute(ui::AX_ATTR_TEXT_STYLE); is this the right way of getting color, bkgcolor, size, etc? I wrote a unit test and don't get the right data (always 0 for color for example)
dmazzoni@chromium.org changed reviewers: + nektar@chromium.org
+nektar Nektarios, do you see any potential issues with this change? https://codereview.chromium.org/1137393003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_android.cc:63: int color = node->GetIntAttribute(ui::AX_ATTR_COLOR_VALUE); This is the wrong attribute for text color - COLOR_VALUE is the value of <input type=color>, you just want AX_ATTR_COLOR.
Not very familiar with the Android code, but make sure you use COLOR and not COLOR_VALUE as the attribute for foreground color as Dominic also said. Nektarios. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/19 18:22:34, chromium-reviews wrote: > Not very familiar with the Android code, but make sure you use COLOR and > not COLOR_VALUE as the attribute for foreground color as Dominic also said. > Nektarios. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. ok this fixes the color issue indeed.
sgurun@chromium.org changed reviewers: + hush@chromium.org
On 2015/05/19 21:59:22, sgurun wrote: > On 2015/05/19 18:22:34, chromium-reviews wrote: > > Not very familiar with the Android code, but make sure you use COLOR and > > not COLOR_VALUE as the attribute for foreground color as Dominic also said. > > Nektarios. > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > ok this fixes the color issue indeed. PTAL: dmazzoni: ui/accessibility and overall hush: android_webview/
lgtm with some minor comments https://codereview.chromium.org/1137393003/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/1137393003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:547: static class AccessibilityCallbackHelper extends CallbackHelper { make this class private static? https://codereview.chromium.org/1137393003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:556: public AccessibilitySnapshotNode getValue() { how about "getRootAXNode"? https://codereview.chromium.org/1137393003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:628: assertEquals(11.0, child.textSize, 0.01); Can explain why there is a small difference in textSize? https://codereview.chromium.org/1137393003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java (right): https://codereview.chromium.org/1137393003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java:21: public int color, bkgcolor; bkgcolor --> bgcolor
https://codereview.chromium.org/1137393003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java (right): https://codereview.chromium.org/1137393003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java:23: public float textSize; another question: why is textSize in float instead of int? Other sizes like x, y, scrollX, scrollY, width, height are all in int already
On 2015/05/22 00:27:57, hush wrote: > https://codereview.chromium.org/1137393003/diff/40001/content/public/android/... > File > content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java > (right): > > https://codereview.chromium.org/1137393003/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java:23: > public float textSize; > another question: > why is textSize in float instead of int? > Other sizes like x, y, scrollX, scrollY, width, height are all in int already
https://codereview.chromium.org/1137393003/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/1137393003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:547: static class AccessibilityCallbackHelper extends CallbackHelper { On 2015/05/22 00:22:22, hush wrote: > make this class private static? Done. https://codereview.chromium.org/1137393003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:556: public AccessibilitySnapshotNode getValue() { On 2015/05/22 00:22:22, hush wrote: > how about "getRootAXNode"? AXNode expecially the name AX is specifically reserved for accessibility tree in a platform independent manner. Here we go with the name AccessibilitySnapshotNode, to emphasize that it is a light weight data structure separate from AX nodes. https://codereview.chromium.org/1137393003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:628: assertEquals(11.0, child.textSize, 0.01); On 2015/05/22 00:22:22, hush wrote: > Can explain why there is a small difference in textSize? because you cannot equal compare floats? https://codereview.chromium.org/1137393003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java (right): https://codereview.chromium.org/1137393003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java:21: public int color, bkgcolor; On 2015/05/22 00:22:23, hush wrote: > bkgcolor --> bgcolor Done. https://codereview.chromium.org/1137393003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java:23: public float textSize; On 2015/05/22 00:27:57, hush wrote: > another question: > why is textSize in float instead of int? > Other sizes like x, y, scrollX, scrollY, width, height are all in int already it comes as float from blink.
https://codereview.chromium.org/1137393003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java (right): https://codereview.chromium.org/1137393003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:28: // These values should be same as the AXTextStyle enumerations, see ax_enums.h Would it be possible to avoid duplicating these, e.g.: https://sites.google.com/a/google.com/clank/engineering/accessing-c-enums-in-... If not, please add a comment to ax_enums.idl with a pointer to this source file and a reminder to keep them in sync.
https://codereview.chromium.org/1137393003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java (right): https://codereview.chromium.org/1137393003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:28: // These values should be same as the AXTextStyle enumerations, see ax_enums.h On 2015/05/26 18:22:23, dmazzoni wrote: > Would it be possible to avoid duplicating these, e.g.: > > https://sites.google.com/a/google.com/clank/engineering/accessing-c-enums-in-... > > If not, please add a comment to ax_enums.idl with a pointer to this source file > and a reminder to keep them in sync. ok done.
The CQ bit was checked by sgurun@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from hush@chromium.org Link to the patchset: https://codereview.chromium.org/1137393003/#ps100001 (title: "gn support")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1137393003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1137393003/diff/100001/ui/accessibility/ax_en... File ui/accessibility/ax_enums.idl (right): https://codereview.chromium.org/1137393003/diff/100001/ui/accessibility/ax_en... ui/accessibility/ax_enums.idl:436: text_style_line_through = 8 Oh, good catch - these should have been powers of two before
sgurun@chromium.org changed reviewers: + pasko@chromium.org, tedchoc@chromium.org
On 2015/05/27 15:46:39, dmazzoni wrote: > lgtm > > https://codereview.chromium.org/1137393003/diff/100001/ui/accessibility/ax_en... > File ui/accessibility/ax_enums.idl (right): > > https://codereview.chromium.org/1137393003/diff/100001/ui/accessibility/ax_en... > ui/accessibility/ax_enums.idl:436: text_style_line_through = 8 > Oh, good catch - these should have been powers of two before tedchoc@, need owners review for files under content/ (-content.gyp which I don't think you are an owner ) pasko@ need owners review for build/android/gyp/java_cpp_enum.py
sgurun@chromium.org changed reviewers: + sievers@chromium.org
On 2015/05/27 16:55:44, sgurun wrote: > On 2015/05/27 15:46:39, dmazzoni wrote: > > lgtm > > > > > https://codereview.chromium.org/1137393003/diff/100001/ui/accessibility/ax_en... > > File ui/accessibility/ax_enums.idl (right): > > > > > https://codereview.chromium.org/1137393003/diff/100001/ui/accessibility/ax_en... > > ui/accessibility/ax_enums.idl:436: text_style_line_through = 8 > > Oh, good catch - these should have been powers of two before > > tedchoc@, need owners review for files under content/ (-content.gyp which I > don't think you are an owner ) > pasko@ need owners review for build/android/gyp/java_cpp_enum.py sievers@, need owners review for content/content.gyp
On 2015/05/27 16:57:04, sgurun wrote: > On 2015/05/27 16:55:44, sgurun wrote: > > On 2015/05/27 15:46:39, dmazzoni wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/1137393003/diff/100001/ui/accessibility/ax_en... > > > File ui/accessibility/ax_enums.idl (right): > > > > > > > > > https://codereview.chromium.org/1137393003/diff/100001/ui/accessibility/ax_en... > > > ui/accessibility/ax_enums.idl:436: text_style_line_through = 8 > > > Oh, good catch - these should have been powers of two before > > > > tedchoc@, need owners review for files under content/ (-content.gyp which I > > don't think you are an owner ) > > pasko@ need owners review for build/android/gyp/java_cpp_enum.py > > sievers@, need owners review for content/content.gyp It's weird that the enums under ui/ would have a rule to generate the Java counterpart in content/. Can you add it to a target in ui instead and then depend include the Java file from there?
https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:63: int color = node->GetIntAttribute(ui::AX_ATTR_COLOR); GetIntAttribute returns 0 when this isn't defined. For color, that will default to transparent on android, which probably isn't what you want. Should this use the function that returns a boolean and only use the color if defined otherwise default to 0xFF000000 ? Same for bgcolor and size, should we default to some decent constant if undefined?
On 2015/05/27 18:45:39, Ted C wrote: > https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... > File content/browser/web_contents/web_contents_android.cc (right): > > https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... > content/browser/web_contents/web_contents_android.cc:63: int color = > node->GetIntAttribute(ui::AX_ATTR_COLOR); > GetIntAttribute returns 0 when this isn't defined. For color, that will default > to transparent on android, which probably isn't what you want. > > Should this use the function that returns a boolean and only use the color if > defined otherwise default to 0xFF000000 ? > > Same for bgcolor and size, should we default to some decent constant if > undefined? sievers comment addressed, thinking on Ted's comment.
https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:63: int color = node->GetIntAttribute(ui::AX_ATTR_COLOR); On 2015/05/27 18:45:39, Ted C wrote: > GetIntAttribute returns 0 when this isn't defined. For color, that will default > to transparent on android, which probably isn't what you want. > > Should this use the function that returns a boolean and only use the color if > defined otherwise default to 0xFF000000 ? > > Same for bgcolor and size, should we default to some decent constant if > undefined? good point, I actually missed it. It is more attractive to pass a decent constant (for not making JNI interface even more crowded) but then I don't know what a decent constant would be.
https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:63: int color = node->GetIntAttribute(ui::AX_ATTR_COLOR); On 2015/05/27 19:27:24, sgurun wrote: > On 2015/05/27 18:45:39, Ted C wrote: > > GetIntAttribute returns 0 when this isn't defined. For color, that will > default > > to transparent on android, which probably isn't what you want. > > > > Should this use the function that returns a boolean and only use the color if > > defined otherwise default to 0xFF000000 ? > > > > Same for bgcolor and size, should we default to some decent constant if > > undefined? > > good point, I actually missed it. It is more attractive to pass a decent > constant (for not making JNI interface even more crowded) but then I don't know > what a decent constant would be. ok, talked to Dominic about it. We will check if size exists or not. if size exists, we will read all the attributes and pass to Java, otherwise, we will not pass anything.
On Wed, May 27, 2015 at 12:42 PM, <sgurun@chromium.org> wrote: > ok, talked to Dominic about it. We will check if size exists or not. if > size exists, we will read all the attributes and pass to Java, > otherwise, we will not pass anything. And to clarify, the reason for this is that we don't distinguish between zero-valued and missing int attributes, so we need to fix this and make it unambiguous somehow. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1137393003/diff/120001/content/content.gyp File content/content.gyp (right): https://codereview.chromium.org/1137393003/diff/120001/content/content.gyp#ne... content/content.gyp:442: '../ui/accessibility/accessibility.gyp:ax_enumerations_java', Shouldn't you get this automatically by depending on accessibility (which content_common already depends on)?
https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:63: int color = node->GetIntAttribute(ui::AX_ATTR_COLOR); On 2015/05/27 19:42:26, sgurun wrote: > On 2015/05/27 19:27:24, sgurun wrote: > > On 2015/05/27 18:45:39, Ted C wrote: > > > GetIntAttribute returns 0 when this isn't defined. For color, that will > > default > > > to transparent on android, which probably isn't what you want. > > > > > > Should this use the function that returns a boolean and only use the color > if > > > defined otherwise default to 0xFF000000 ? > > > > > > Same for bgcolor and size, should we default to some decent constant if > > > undefined? > > > > good point, I actually missed it. It is more attractive to pass a decent > > constant (for not making JNI interface even more crowded) but then I don't > know > > what a decent constant would be. > > ok, talked to Dominic about it. We will check if size exists or not. if size > exists, we will read all the attributes and pass to Java, otherwise, we will not > pass anything. Done. https://codereview.chromium.org/1137393003/diff/120001/content/content.gyp File content/content.gyp (right): https://codereview.chromium.org/1137393003/diff/120001/content/content.gyp#ne... content/content.gyp:442: '../ui/accessibility/accessibility.gyp:ax_enumerations_java', On 2015/05/27 19:55:57, sievers wrote: > Shouldn't you get this automatically by depending on accessibility (which > content_common already depends on)? Done.
On 2015/05/29 00:35:25, sgurun wrote: > https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... > File content/browser/web_contents/web_contents_android.cc (right): > > https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... > content/browser/web_contents/web_contents_android.cc:63: int color = > node->GetIntAttribute(ui::AX_ATTR_COLOR); > On 2015/05/27 19:42:26, sgurun wrote: > > On 2015/05/27 19:27:24, sgurun wrote: > > > On 2015/05/27 18:45:39, Ted C wrote: > > > > GetIntAttribute returns 0 when this isn't defined. For color, that will > > > default > > > > to transparent on android, which probably isn't what you want. > > > > > > > > Should this use the function that returns a boolean and only use the color > > if > > > > defined otherwise default to 0xFF000000 ? > > > > > > > > Same for bgcolor and size, should we default to some decent constant if > > > > undefined? > > > > > > good point, I actually missed it. It is more attractive to pass a decent > > > constant (for not making JNI interface even more crowded) but then I don't > > know > > > what a decent constant would be. > > > > ok, talked to Dominic about it. We will check if size exists or not. if size > > exists, we will read all the attributes and pass to Java, otherwise, we will > not > > pass anything. > > Done. > > https://codereview.chromium.org/1137393003/diff/120001/content/content.gyp > File content/content.gyp (right): > > https://codereview.chromium.org/1137393003/diff/120001/content/content.gyp#ne... > content/content.gyp:442: > '../ui/accessibility/accessibility.gyp:ax_enumerations_java', > On 2015/05/27 19:55:57, sievers wrote: > > Shouldn't you get this automatically by depending on accessibility (which > > content_common already depends on)? > > Done. sievers, no more content/gyp change, you are off the hook now :)
On 2015/05/29 00:36:36, sgurun wrote: > On 2015/05/29 00:35:25, sgurun wrote: > > > https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... > > File content/browser/web_contents/web_contents_android.cc (right): > > > > > https://codereview.chromium.org/1137393003/diff/100001/content/browser/web_co... > > content/browser/web_contents/web_contents_android.cc:63: int color = > > node->GetIntAttribute(ui::AX_ATTR_COLOR); > > On 2015/05/27 19:42:26, sgurun wrote: > > > On 2015/05/27 19:27:24, sgurun wrote: > > > > On 2015/05/27 18:45:39, Ted C wrote: > > > > > GetIntAttribute returns 0 when this isn't defined. For color, that will > > > > default > > > > > to transparent on android, which probably isn't what you want. > > > > > > > > > > Should this use the function that returns a boolean and only use the > color > > > if > > > > > defined otherwise default to 0xFF000000 ? > > > > > > > > > > Same for bgcolor and size, should we default to some decent constant if > > > > > undefined? > > > > > > > > good point, I actually missed it. It is more attractive to pass a decent > > > > constant (for not making JNI interface even more crowded) but then I don't > > > know > > > > what a decent constant would be. > > > > > > ok, talked to Dominic about it. We will check if size exists or not. if size > > > exists, we will read all the attributes and pass to Java, otherwise, we will > > not > > > pass anything. > > > > Done. > > > > https://codereview.chromium.org/1137393003/diff/120001/content/content.gyp > > File content/content.gyp (right): > > > > > https://codereview.chromium.org/1137393003/diff/120001/content/content.gyp#ne... > > content/content.gyp:442: > > '../ui/accessibility/accessibility.gyp:ax_enumerations_java', > > On 2015/05/27 19:55:57, sievers wrote: > > > Shouldn't you get this automatically by depending on accessibility (which > > > content_common already depends on)? > > > > Done. > > sievers, no more content/gyp change, you are off the hook now :) pasko: ping build/android/gyp/* tedchoc: ptal content/* dmazzoni: files have changed in ui/accessibility/*
still lgtm
sgurun@chromium.org changed reviewers: + jbudorick@chromium.org - pasko@chromium.org
On 2015/05/29 06:52:54, dmazzoni wrote: > still lgtm jbudorick@ please review build/android/*
jbudorick@chromium.org changed reviewers: + cjhopman@chromium.org
https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... File build/android/gyp/java_cpp_enum.py (right): https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... build/android/gyp/java_cpp_enum.py:19: # This script can parse .idl files however, at present it ignores special Why are we adding .idl support here instead of handling it separately?
On 2015/05/29 17:01:02, jbudorick wrote: > https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... > File build/android/gyp/java_cpp_enum.py (right): > > https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... > build/android/gyp/java_cpp_enum.py:19: # This script can parse .idl files > however, at present it ignores special > Why are we adding .idl support here instead of handling it separately? content/* - lgtm
https://codereview.chromium.org/1137393003/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:71: color = node->GetIntAttribute(ui::AX_ATTR_COLOR); can you add a DCHECK above each one of these using Has<XXX>Attribute?
https://codereview.chromium.org/1137393003/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:71: color = node->GetIntAttribute(ui::AX_ATTR_COLOR); On 2015/05/29 17:04:29, Ted C wrote: > can you add a DCHECK above each one of these using Has<XXX>Attribute? No, we deliberately filter out zero values and empty strings from attributes so if you want the value of the attribute including possibly zero, this is the right way to do it.
https://codereview.chromium.org/1137393003/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:71: color = node->GetIntAttribute(ui::AX_ATTR_COLOR); On 2015/05/29 17:06:01, dmazzoni wrote: > On 2015/05/29 17:04:29, Ted C wrote: > > can you add a DCHECK above each one of these using Has<XXX>Attribute? > > No, we deliberately filter out zero values and empty strings from attributes so > if you want the value of the attribute including possibly zero, this is the > right way to do it. Huh? So HasFloatAttribute returns false if the value is 0? I'm more concerned that all these attributes are associated with size, which seems odd/bad/incorrect to me. I'd rather have some means of asserting that is true and that if size is specified then so is everything else. So if there is no way to do it currently, it seems like something we should add and a TODO should be here referencing the bug for it.
https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... File build/android/gyp/java_cpp_enum.py (right): https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... build/android/gyp/java_cpp_enum.py:19: # This script can parse .idl files however, at present it ignores special On 2015/05/29 17:01:01, jbudorick wrote: > Why are we adding .idl support here instead of handling it separately? do you mean have another script just to parse idl files. as far as I can see, their syntax is very similar to C++ files so it would not buy us much duplicating the script. Or did you have something else in your mind?
https://codereview.chromium.org/1137393003/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/1137393003/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:71: color = node->GetIntAttribute(ui::AX_ATTR_COLOR); On 2015/05/29 17:10:37, Ted C wrote: > On 2015/05/29 17:06:01, dmazzoni wrote: > > On 2015/05/29 17:04:29, Ted C wrote: > > > can you add a DCHECK above each one of these using Has<XXX>Attribute? > > > > No, we deliberately filter out zero values and empty strings from attributes > so > > if you want the value of the attribute including possibly zero, this is the > > right way to do it. > > Huh? > > So HasFloatAttribute returns false if the value is 0? I'm more concerned that > all these attributes are associated with size, which seems odd/bad/incorrect to > me. > > I'd rather have some means of asserting that is true and that if size is > specified > then so is everything else. So if there is no way to do it currently, it seems > like something we should add and a TODO should be here referencing the bug for > it. I will add a todo and a crbug and we can discuss how to do it better there. maybe a better way to encapsulate this information is having something like hasStyleInformation hiding all the internal details.
On Fri, May 29, 2015 at 10:10 AM, <tedchoc@chromium.org> wrote: > > can you add a DCHECK above each one of these using >> > Has<XXX>Attribute? > > No, we deliberately filter out zero values and empty strings from >> > attributes so > >> if you want the value of the attribute including possibly zero, this >> > is the > >> right way to do it. >> > > Huh? > > So HasFloatAttribute returns false if the value is 0? That's correct. We do this because we have ~100 attributes and on any given node, usually only a handful are anything other than zero or the empty string. When this is bad semantically for a specific attribute, we should pick a different encoding that avoids this problem. > I'm more > concerned that > all these attributes are associated with size, which seems > odd/bad/incorrect to > me. The right way to do it would probably be to have an attribute indicating if we have style information for this node or not. To do this right would need a Blink change, I suggested filing a bug and assigning it to nektar@, who implemented these style attributes, to clean it up. > I'd rather have some means of asserting that is true and that if size is > specified > then so is everything else. So if there is no way to do it currently, > it seems > like something we should add and a TODO should be here referencing the > bug for it. > > > https://codereview.chromium.org/1137393003/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/29 17:31:44, dmazzoni wrote: > On Fri, May 29, 2015 at 10:10 AM, <mailto:tedchoc@chromium.org> wrote: > > > > can you add a DCHECK above each one of these using > >> > > Has<XXX>Attribute? > > > > No, we deliberately filter out zero values and empty strings from > >> > > attributes so > > > >> if you want the value of the attribute including possibly zero, this > >> > > is the > > > >> right way to do it. > >> > > > > Huh? > > > > So HasFloatAttribute returns false if the value is 0? > > > That's correct. We do this because we have ~100 attributes and on any given > node, usually only a handful are anything other than zero or the empty > string. > > When this is bad semantically for a specific attribute, we should pick a > different encoding that avoids this problem. > > > > I'm more > > concerned that > > all these attributes are associated with size, which seems > > odd/bad/incorrect to > > me. > > > The right way to do it would probably be to have an attribute indicating if > we have style information for this node or not. To do this right would need > a Blink change, I suggested filing a bug and assigning it to nektar@, who > implemented these style attributes, to clean it up. > > > > I'd rather have some means of asserting that is true and that if size is > > specified > > then so is everything else. So if there is no way to do it currently, > > it seems > > like something we should add and a TODO should be here referencing the > > bug for it. > > > > > > https://codereview.chromium.org/1137393003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. filed crbug/493778. as a side track, I think bugs are a lot more meaningful than a TODO, which are generally forgotten.
On 2015/05/29 17:44:51, sgurun wrote: > On 2015/05/29 17:31:44, dmazzoni wrote: > > On Fri, May 29, 2015 at 10:10 AM, <mailto:tedchoc@chromium.org> wrote: > > > > > > can you add a DCHECK above each one of these using > > >> > > > Has<XXX>Attribute? > > > > > > No, we deliberately filter out zero values and empty strings from > > >> > > > attributes so > > > > > >> if you want the value of the attribute including possibly zero, this > > >> > > > is the > > > > > >> right way to do it. > > >> > > > > > > Huh? > > > > > > So HasFloatAttribute returns false if the value is 0? > > > > > > That's correct. We do this because we have ~100 attributes and on any given > > node, usually only a handful are anything other than zero or the empty > > string. > > > > When this is bad semantically for a specific attribute, we should pick a > > different encoding that avoids this problem. > > > > > > > I'm more > > > concerned that > > > all these attributes are associated with size, which seems > > > odd/bad/incorrect to > > > me. > > > > > > The right way to do it would probably be to have an attribute indicating if > > we have style information for this node or not. To do this right would need > > a Blink change, I suggested filing a bug and assigning it to nektar@, who > > implemented these style attributes, to clean it up. > > > > > > > I'd rather have some means of asserting that is true and that if size is > > > specified > > > then so is everything else. So if there is no way to do it currently, > > > it seems > > > like something we should add and a TODO should be here referencing the > > > bug for it. > > > > > > > > > https://codereview.chromium.org/1137393003/ > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > filed crbug/493778. as a side track, I think bugs are a lot more meaningful than > a TODO, which are generally forgotten. Yeah, I pretty much tell anyone that adds a TODO without a bug to remove it since it will never get fixed otherwise.
build/android/ lgtm https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... File build/android/gyp/java_cpp_enum.py (right): https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... build/android/gyp/java_cpp_enum.py:19: # This script can parse .idl files however, at present it ignores special On 2015/05/29 at 17:16:07, sgurun wrote: > On 2015/05/29 17:01:01, jbudorick wrote: > > Why are we adding .idl support here instead of handling it separately? > > do you mean have another script just to parse idl files. as far as I can see, their syntax is very similar to C++ files so it would not buy us much duplicating the script. > > Or did you have something else in your mind? As discussed offline, I'm ok with filing a bug here to clean this up s.t. "java_cpp_enum.py" isn't handling .idls -- it's not particularly intuitive.
On 2015/05/29 18:21:51, jbudorick wrote: > build/android/ lgtm > > https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... > File build/android/gyp/java_cpp_enum.py (right): > > https://codereview.chromium.org/1137393003/diff/140001/build/android/gyp/java... > build/android/gyp/java_cpp_enum.py:19: # This script can parse .idl files > however, at present it ignores special > On 2015/05/29 at 17:16:07, sgurun wrote: > > On 2015/05/29 17:01:01, jbudorick wrote: > > > Why are we adding .idl support here instead of handling it separately? > > > > do you mean have another script just to parse idl files. as far as I can see, > their syntax is very similar to C++ files so it would not buy us much > duplicating the script. > > > > Or did you have something else in your mind? > > As discussed offline, I'm ok with filing a bug here to clean this up s.t. > "java_cpp_enum.py" isn't handling .idls -- it's not particularly intuitive. filed crbug/493803 for this. thanks, one cl generated two bugs :)
The CQ bit was checked by sgurun@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hush@chromium.org Link to the patchset: https://codereview.chromium.org/1137393003/#ps140001 (title: "address code review (dependency and non-existent style)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1137393003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
ok the bots finally started coming green. Basically, my gn and gyp files had problems: gn file did not have the right dependency for android (cjhopman identified the dependency issue) and it was not importing android rules. and gyp file was not conditionalizing the targets right so they were failing on non-android targets. tedchoc: PTAL content/public/android/BUILD.gn dmazzoni: gyp/gn files in ui/accessibility
lgtm
The CQ bit was checked by sgurun@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hush@chromium.org, jbudorick@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/1137393003/#ps200001 (title: "add content dependency to ui_accessibility_java for GN")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1137393003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2fe5ccafafe4419c2c5c6a827b50a1d027057fb1 Cr-Commit-Position: refs/heads/master@{#332082} |
