Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2758733003:
Enable --prefer-compositing-to-lcd-text on Chrome OS by default. (Closed)
|
Created:
3 years, 9 months ago by bokan Modified:
3 years, 9 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, fuzzing_chromium.org, jam, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, tbuckley, Sebastien Gabriel Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable --prefer-compositing-to-lcd-text on Chrome OS by default.
This patch enables the flag on Chrome OS so that we prefer getting composited
scrollers rather than LCD text antialiasing. We do this in order to get
animated overlay scrollbars in Chrome OS which are being enabled in M59.
This means that text in scrollers outside the main frame may lose LCD text
antialiasing. However, text in the main frame should be unaffected.
BUG=307091
Review-Url: https://codereview.chromium.org/2746363009
Cr-Original-Commit-Position: refs/heads/master@{#457838}
Review-Url: https://codereview.chromium.org/2758733003
Cr-Commit-Position: refs/heads/master@{#459310}
Committed: https://chromium.googlesource.com/chromium/src/+/e6e68ae222d21bdb5e872cf132925bc5440ccdcb
Patch Set 1 #Patch Set 2 : Fix bad patch #
Total comments: 1
Patch Set 3 : ChromeOS -> Chrome OS #Messages
Total messages: 30 (14 generated)
Description was changed from ========== Enable --prefer-compositing-to-lcd-text on ChromeOS by default. This patch enables the flag on ChromeOS so that we prefer getting composited scrollers rather than LCD text antialiasing. We do this in order to get animated overlay scrollbar for ChromeOS which will be turned on by defualt shortly. This means that text in scrollers outside the main frame may lose LCD text antialiasing. However, text in the main frame should be unaffected. BUG=702064 Review-Url: https://codereview.chromium.org/2746363009 Cr-Commit-Position: refs/heads/master@{#457838} ========== to ========== Enable --prefer-compositing-to-lcd-text on ChromeOS by default. This patch enables the flag on ChromeOS so that we prefer getting composited scrollers rather than LCD text antialiasing. We do this in order to get animated overlay scrollbar for ChromeOS which will be turned on by defualt shortly. This means that text in scrollers outside the main frame may lose LCD text antialiasing. However, text in the main frame should be unaffected. BUG=702064 Review-Url: https://codereview.chromium.org/2746363009 Cr-Commit-Position: refs/heads/master@{#457838} ==========
bokan@chromium.org changed reviewers: + estade@chromium.org
Hey Evan, I realise you're not an OWNER of this file but none of the OWNERs are ChromeOS related. If you stamp (or find someone who should) I'll add an OWNER.
The CQ bit was checked by bokan@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...
Description was changed from ========== Enable --prefer-compositing-to-lcd-text on ChromeOS by default. This patch enables the flag on ChromeOS so that we prefer getting composited scrollers rather than LCD text antialiasing. We do this in order to get animated overlay scrollbar for ChromeOS which will be turned on by defualt shortly. This means that text in scrollers outside the main frame may lose LCD text antialiasing. However, text in the main frame should be unaffected. BUG=702064 Review-Url: https://codereview.chromium.org/2746363009 Cr-Commit-Position: refs/heads/master@{#457838} ========== to ========== Enable --prefer-compositing-to-lcd-text on ChromeOS by default. This patch enables the flag on ChromeOS so that we prefer getting composited scrollers rather than LCD text antialiasing. We do this in order to get animated overlay scrollbar for ChromeOS which will be turned on by defualt shortly. This means that text in scrollers outside the main frame may lose LCD text antialiasing. However, text in the main frame should be unaffected. BUG=307091 Review-Url: https://codereview.chromium.org/2746363009 Cr-Commit-Position: refs/heads/master@{#457838} ==========
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_...)
I assume the appropriate parties understand this tradeoff so lgtm +derat FYI
derat@chromium.org changed reviewers: + derat@chromium.org
https://codereview.chromium.org/2758733003/diff/20001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2758733003/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:333: // On Android, we never have subpixel antialiasing. On ChromeOS we prefer to nit: "Chrome OS" is the proper formulation (the OS_CHROMEOS macro is unfortunate)
Description was changed from ========== Enable --prefer-compositing-to-lcd-text on ChromeOS by default. This patch enables the flag on ChromeOS so that we prefer getting composited scrollers rather than LCD text antialiasing. We do this in order to get animated overlay scrollbar for ChromeOS which will be turned on by defualt shortly. This means that text in scrollers outside the main frame may lose LCD text antialiasing. However, text in the main frame should be unaffected. BUG=307091 Review-Url: https://codereview.chromium.org/2746363009 Cr-Commit-Position: refs/heads/master@{#457838} ========== to ========== Enable --prefer-compositing-to-lcd-text on Chrome OS by default. This patch enables the flag on Chrome OS so that we prefer getting composited scrollers rather than LCD text antialiasing. We do this in order to get animated overlay scrollbars in Chrome OS which are being enabled in M59. This means that text in scrollers outside the main frame may lose LCD text antialiasing. However, text in the main frame should be unaffected. BUG=307091 Review-Url: https://codereview.chromium.org/2746363009 Cr-Commit-Position: refs/heads/master@{#457838} ==========
On 2017/03/20 23:00:35, Daniel Erat wrote: > https://codereview.chromium.org/2758733003/diff/20001/content/renderer/render... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/2758733003/diff/20001/content/renderer/render... > content/renderer/render_view_impl.cc:333: // On Android, we never have subpixel > antialiasing. On ChromeOS we prefer to > nit: "Chrome OS" is the proper formulation (the OS_CHROMEOS macro is > unfortunate) Done
thanks, lgtm (with the caveat that i don't know anything about this renderer code) :-)
bokan@chromium.org changed reviewers: + aelias@chromium.org
+aelias@ for OWNER As a note: this was proposed in chromeos-ui-review@ and received go-ahead from owners there. If you know who might have been originally responsible for the ChromeOS decision on this flag might be helpful to loop them in.
In my opinion, if you're going this far, you should also disable LCD text on the main frame. I believe it's better to have equally blurry text everywhere instead of visual inconsistency -- this implementation will make side-by-side text in the same font look different, or sometimes appear as a flicker between blurry <-> nonblurry if a compositing condition is set/removed on some text. Furthermore, you'd get uniformly superior scrolling performance. It looks like this detail was not specifically discussed on the UI review: https://groups.google.com/a/google.com/forum/#!topic/chromeos-ui-review/sLMLh... . +tbuckley
On 2017/03/22 21:19:18, aelias wrote: > In my opinion, if you're going this far, you should also disable LCD text on the > main frame. I believe it's better to have equally blurry text everywhere > instead of visual inconsistency -- this implementation will make side-by-side > text in the same font look different, or sometimes appear as a flicker between > blurry <-> nonblurry if a compositing condition is set/removed on some text. > Furthermore, you'd get uniformly superior scrolling performance. > > It looks like this detail was not specifically discussed on the UI review: > https://groups.google.com/a/google.com/forum/#!topic/chromeos-ui-review/sLMLh... > . +tbuckley Good point, I hadn't thought of that. However, I've tried this out and updated my demo page (http://bokand.github.io/lcdtext.html) with a checkbox to let you toggle compositing on the right paragraph. Even knowing to look for it, and using an Acer Chromebook 13 which has a less dense display, it took me a while to notice any change at all. It's a very subtle effect. Given that it's so hard to tell, I think we should keep LCD text in the main frame. It's main benefit is reducing eyestrain when reading long documents which are most likely going to be in the main frame anyway. It'd be a shame to lose that. +sgabriel@ also.
It would be also a shame not to gain the scroll performance on the main frame. And we've always paid great care to making sure rendering is completely pixel-identical in composited element compared to noncomposited, and now you're intentionally violating that invariant, without a particularly strong reason to do so, just an inclination. Anyway, as far as code goes, content/renderer lgtm. This decision can always easily be revisited later if you want to try this.
On 2017/03/23 18:28:09, aelias wrote: > It would be also a shame not to gain the scroll performance on the main frame. FWIW, the main frame is always composited *and* gets lcd text. It's possible to do that for the main frame only because we know what the background is and we bake it directly into the scrolling contents. So we're already getting the perf gain for main frame. > And we've always paid great care to making sure rendering is completely > pixel-identical in composited element compared to noncomposited, and now you're > intentionally violating that invariant, without a particularly strong reason to > do so, just an inclination. Maybe I'm misunderstanding your point, but we're not identical today - if a page applies a transform or some other layer-requiring property we lose LCD text and the content change is not pixel-identical. Or do you mean that content in a scroller will now look slightly different than if it was in the main frame? In that case, I would say that already happens (albeit less often) if the page applies certain styles. Also, if the author cares they can get back LCD text by setting an opaque background and contains: paint. I'm also not sure I see the value if the difference is user imperceptible - what's the reason we've been so intentional about this? > Anyway, as far as code goes, content/renderer lgtm. This decision can always > easily be revisited later if you want to try this.
OK, I had forgotten about the scrolling and transform points, I guess that changes the calculus to make this more reasonable. > Also, if the author cares they can get back LCD text by setting an opaque background and contains: paint. I'm also not sure I see the value if the difference is user imperceptible - what's the reason we've been so intentional about this? You're using a particularly slack definition of "user imperceptible" here. Obviously, it is perceptible or there would be no point to LCD text. Anyway, the main reason is that transition flickers and side-by-side differences create an impression of shoddiness and glitchiness, even if they are slightly below the level of consciousness. This kind of thing can be the difference between a experience that feels crisp and polished or not. Aside from the text question, this mostly comes up in the context of edge antialiasing. I haven't seen what this looks like on ChromeOS, it's possible that it has an especially light implementation of LCD text. I wonder how your example would compare on Windows. If it looks the same, we might as well try it out on all platforms to try for universal impl-thread scrolling.
On 2017/03/23 19:12:57, aelias wrote: > OK, I had forgotten about the scrolling and transform points, I guess that > changes the calculus to make this more reasonable. > > > Also, if the author cares they can get back LCD text by setting an opaque > background and contains: paint. I'm also not sure I see the value if the > difference is user imperceptible - what's the reason we've been so intentional > about this? > > You're using a particularly slack definition of "user imperceptible" here. > Obviously, it is perceptible or there would be no point to LCD text. Anyway, > the main reason is that transition flickers and side-by-side differences create > an impression of shoddiness and glitchiness, even if they are slightly below the > level of consciousness. This kind of thing can be the difference between a > experience that feels crisp and polished or not. Aside from the text question, > this mostly comes up in the context of edge antialiasing. > > I haven't seen what this looks like on ChromeOS, it's possible that it has an > especially light implementation of LCD text. I wonder how your example would > compare on Windows. If it looks the same, we might as well try it out on all > platforms to try for universal impl-thread scrolling. I'll try it out on a Windows machine when I get access to one next week. Do you know who would own the decision on the Windows/Mac side? In the mean time, I'm going to land this since this is critical for CrOS overlay scrollbars. We can revisit with feedback from real usage if needed.
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org Link to the patchset: https://codereview.chromium.org/2758733003/#ps40001 (title: "ChromeOS -> Chrome OS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/23 22:28:31, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... FWIW we have subpixel AA enabled in most of native UI but disables in specific parts, so there's lots of side-by-side inconsistency, and derat@ is probably the only person who ever consciously notices.
On 2017/03/23 23:02:23, Evan Stade wrote: > On 2017/03/23 22:28:31, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > FWIW we have subpixel AA enabled in most of native UI but disables in specific > parts, so there's lots of side-by-side inconsistency, and derat@ is probably the > only person who ever consciously notices. that is the sad truth.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490308076426120,
"parent_rev": "ddb47b17e0242686c4f354217ba8e10df6000c08", "commit_rev":
"e6e68ae222d21bdb5e872cf132925bc5440ccdcb"}
Message was sent while issue was closed.
Description was changed from ========== Enable --prefer-compositing-to-lcd-text on Chrome OS by default. This patch enables the flag on Chrome OS so that we prefer getting composited scrollers rather than LCD text antialiasing. We do this in order to get animated overlay scrollbars in Chrome OS which are being enabled in M59. This means that text in scrollers outside the main frame may lose LCD text antialiasing. However, text in the main frame should be unaffected. BUG=307091 Review-Url: https://codereview.chromium.org/2746363009 Cr-Commit-Position: refs/heads/master@{#457838} ========== to ========== Enable --prefer-compositing-to-lcd-text on Chrome OS by default. This patch enables the flag on Chrome OS so that we prefer getting composited scrollers rather than LCD text antialiasing. We do this in order to get animated overlay scrollbars in Chrome OS which are being enabled in M59. This means that text in scrollers outside the main frame may lose LCD text antialiasing. However, text in the main frame should be unaffected. BUG=307091 Review-Url: https://codereview.chromium.org/2746363009 Cr-Original-Commit-Position: refs/heads/master@{#457838} Review-Url: https://codereview.chromium.org/2758733003 Cr-Commit-Position: refs/heads/master@{#459310} Committed: https://chromium.googlesource.com/chromium/src/+/e6e68ae222d21bdb5e872cf13292... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e6e68ae222d21bdb5e872cf13292... |
