Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2443353003:
The Android ViewStructure font size should be in physical pixels (Closed)
|
Created:
4 years, 1 month ago by dmazzoni Modified:
4 years, 1 month ago CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, jam, yuzo+watch_chromium.org, je_julie, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe Android ViewStructure font size should be in physical pixels
The font size in AccessibilityNodeInfo is the computed style for that
element. We need to convert it twice:
1. Convert that to global CSS coordinates, applying any transformations that
might affect the font size, such as CSS scale transformations. Covered by a
test.
2. Convert from CSS pixels to physical pixels. Not currently covered by a
test but seems straightforward if that's the correct way to interpret this
value.
BUG=658906
Committed: https://crrev.com/c0148879eff4f35f89c2882dae706a3b097c2c4a
Cr-Commit-Position: refs/heads/master@{#427436}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address feedback #
Total comments: 2
Patch Set 3 : Reword Android pixels to physical pixels #
Messages
Total messages: 29 (16 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + sgurun@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/24 22:30:21, dmazzoni wrote: lgtm. need an owner's lgtm though.
https://codereview.chromium.org/2443353003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/2443353003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_android.cc:109: size = node->GetFloatAttribute(ui::AX_ATTR_FONT_SIZE); I prefer this line to move right above line 114 below, so related things are close to each other.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dmazzoni@chromium.org 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...
dmazzoni@chromium.org changed reviewers: + boliu@chromium.org
+boliu for owners review https://codereview.chromium.org/2443353003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/2443353003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_android.cc:109: size = node->GetFloatAttribute(ui::AX_ATTR_FONT_SIZE); On 2016/10/24 22:47:35, sgurun wrote: > I prefer this line to move right above line 114 below, so related things are > close to each other. Done.
"Android pixels" isn't a thing. I think you mean "physical pixels". Can you update the CL description and code comments? Looking at your CL description 1. is RelativeToAbsoluteBounds, 2. is RenderCoordinates.fromLocalCssToPix? fromLocalCssToPix also covers "page scale factor" (aka "page zoom"), so is that factor included twice? https://codereview.chromium.org/2443353003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/2443353003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_android.cc:113: size = node->GetFloatAttribute(ui::AX_ATTR_FONT_SIZE); nit: can you pass this directly into RectF constructor instead of assigning to |size|? seeing size being updated twice looks odd..
Description was changed from ========== The Android ViewStructure font size should be in Android pixels The font size in AccessibilityNodeInfo is the computed style for that element. We need to convert it twice: 1. Convert that to global CSS coordinates, applying any transformations that might affect the font size. This also takes care of page zoom. Covered by a test. 2. Convert from CSS pixels to Android pixels. Not currently covered by a test but seems straightforward if that's the correct way to interpret this value. BUG=658906 ========== to ========== The Android ViewStructure font size should be in physical pixels The font size in AccessibilityNodeInfo is the computed style for that element. We need to convert it twice: 1. Convert that to global CSS coordinates, applying any transformations that might affect the font size. This also takes care of page zoom. Covered by a test. 2. Convert from CSS pixels to physical pixels. Not currently covered by a test but seems straightforward if that's the correct way to interpret this value. BUG=658906 ==========
Description was changed from ========== The Android ViewStructure font size should be in physical pixels The font size in AccessibilityNodeInfo is the computed style for that element. We need to convert it twice: 1. Convert that to global CSS coordinates, applying any transformations that might affect the font size. This also takes care of page zoom. Covered by a test. 2. Convert from CSS pixels to physical pixels. Not currently covered by a test but seems straightforward if that's the correct way to interpret this value. BUG=658906 ========== to ========== The Android ViewStructure font size should be in physical pixels The font size in AccessibilityNodeInfo is the computed style for that element. We need to convert it twice: 1. Convert that to global CSS coordinates, applying any transformations that might affect the font size, such as CSS scale transformations. Covered by a test. 2. Convert from CSS pixels to physical pixels. Not currently covered by a test but seems straightforward if that's the correct way to interpret this value. BUG=658906 ==========
On 2016/10/25 17:20:19, boliu wrote: > "Android pixels" isn't a thing. I think you mean "physical pixels". Can you > update the CL description and code comments? Done > Looking at your CL description > 1. is RelativeToAbsoluteBounds, 2. is RenderCoordinates.fromLocalCssToPix? > > fromLocalCssToPix also covers "page scale factor" (aka "page zoom"), so is that > factor included twice? You're right, updated comment. The first one covers things like CSS scale transforms but not page zoom.
https://codereview.chromium.org/2443353003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/2443353003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_android.cc:113: size = node->GetFloatAttribute(ui::AX_ATTR_FONT_SIZE); On 2016/10/25 17:20:19, boliu wrote: > nit: can you pass this directly into RectF constructor instead of assigning to > |size|? seeing size being updated twice looks odd.. Done.
lgtm
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org Link to the patchset: https://codereview.chromium.org/2443353003/#ps40001 (title: "Reword Android pixels to physical pixels")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== The Android ViewStructure font size should be in physical pixels The font size in AccessibilityNodeInfo is the computed style for that element. We need to convert it twice: 1. Convert that to global CSS coordinates, applying any transformations that might affect the font size, such as CSS scale transformations. Covered by a test. 2. Convert from CSS pixels to physical pixels. Not currently covered by a test but seems straightforward if that's the correct way to interpret this value. BUG=658906 ========== to ========== The Android ViewStructure font size should be in physical pixels The font size in AccessibilityNodeInfo is the computed style for that element. We need to convert it twice: 1. Convert that to global CSS coordinates, applying any transformations that might affect the font size, such as CSS scale transformations. Covered by a test. 2. Convert from CSS pixels to physical pixels. Not currently covered by a test but seems straightforward if that's the correct way to interpret this value. BUG=658906 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== The Android ViewStructure font size should be in physical pixels The font size in AccessibilityNodeInfo is the computed style for that element. We need to convert it twice: 1. Convert that to global CSS coordinates, applying any transformations that might affect the font size, such as CSS scale transformations. Covered by a test. 2. Convert from CSS pixels to physical pixels. Not currently covered by a test but seems straightforward if that's the correct way to interpret this value. BUG=658906 ========== to ========== The Android ViewStructure font size should be in physical pixels The font size in AccessibilityNodeInfo is the computed style for that element. We need to convert it twice: 1. Convert that to global CSS coordinates, applying any transformations that might affect the font size, such as CSS scale transformations. Covered by a test. 2. Convert from CSS pixels to physical pixels. Not currently covered by a test but seems straightforward if that's the correct way to interpret this value. BUG=658906 Committed: https://crrev.com/c0148879eff4f35f89c2882dae706a3b097c2c4a Cr-Commit-Position: refs/heads/master@{#427436} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c0148879eff4f35f89c2882dae706a3b097c2c4a Cr-Commit-Position: refs/heads/master@{#427436} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
