Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
614103007:
Error in popup on Link (Closed)
|
Created:
6 years, 2 months ago by Roman Sorokin (ftl) Modified:
6 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, sadrul, tfarina, chromium-apps-reviews_chromium.org, kalyank, peter+watch_chromium.org, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFixed Multiprofile Error cutoff/displayed incompletely on Link
BUG=415213
TEST=Please see the bug crbug.com/415213 for steps to reproduce.
Somehow sum of width of the words is greater by one pixel than width of the concatenation of these words in this particular case ("The administrator for this account has disallowed multiple sign-in." message and Link board). When we try to get preferred size we break the message into two lines and get maximum width of these two strings. We get the width which is smaller by one pixel. And then during rendering again we try to break the message into two lines by adding new words one by one and it does not fit in this width. So it does not fit into the preferred size and lose the last word.
Committed: https://crrev.com/370fe4b5db1c9412093225f7a6e1adc18f8aedb2
Cr-Commit-Position: refs/heads/master@{#303650}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Update after review #
Total comments: 4
Patch Set 3 : Update after review and rebase #Patch Set 4 : Update after review #
Total comments: 1
Patch Set 5 : For testing #Patch Set 6 : For testing #Patch Set 7 : Added test #
Total comments: 2
Patch Set 8 : Set test to enabled on all platforms #Patch Set 9 : Update after review #Messages
Total messages: 44 (7 generated)
rsorokin@chromium.org changed reviewers: + asvitkine@chromium.org
rsorokin@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine@ Problem is that max width for the error string here https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/canvas_skia... and here https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/text_elider... are different (193 vs 194). So it causes rendering carry-over the last word of the first line to the second line and it causes loss of the last word of the message. Can you please help me with the right solution? I don't think mine is good enough.
Can you make the CL description more descriptive and possibly add a TEST= line that specifies how QA can verify the fix? https://codereview.chromium.org/614103007/diff/1/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/614103007/diff/1/ui/gfx/text_elider.cc#newcod... ui/gfx/text_elider.cc:567: float* max_width_; I suggest not making this a pointer, but rather a member of the class with a getter. Then, in the wrapper function, assign it to the ptr passed into it via the getter.
Alexei, please review. Is there any way to create test for this case (it is only reproduces on Pixel(Link))
https://codereview.chromium.org/614103007/diff/20001/ui/gfx/canvas_skia.cc File ui/gfx/canvas_skia.cc (right): https://codereview.chromium.org/614103007/diff/20001/ui/gfx/canvas_skia.cc#ne... ui/gfx/canvas_skia.cc:196: w = std::max(w, string_size.width()); I actually don't understand your fix. Can you clarify in what circumstances the initial |w| (returned by your modified code) would be greater than string_size.width() for each string in the broken-up array? And why that makes a difference? Might be worth putting this explanation in the CL description. https://codereview.chromium.org/614103007/diff/20001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/614103007/diff/20001/ui/gfx/text_elider.cc#ne... ui/gfx/text_elider.cc:506: float GetMaxBrokenPixelWidth() const { return max_broken_pixel_width_; } Nit: This should just be max_broken_pixel_width() since its a trivial accessor.
https://codereview.chromium.org/614103007/diff/20001/ui/gfx/canvas_skia.cc File ui/gfx/canvas_skia.cc (right): https://codereview.chromium.org/614103007/diff/20001/ui/gfx/canvas_skia.cc#ne... ui/gfx/canvas_skia.cc:196: w = std::max(w, string_size.width()); See the CL description, please On 2014/10/06 17:59:58, Alexei Svitkine wrote: > I actually don't understand your fix. > > Can you clarify in what circumstances the initial |w| (returned by your modified > code) would be greater than string_size.width() for each string in the broken-up > array? And why that makes a difference? > > Might be worth putting this explanation in the CL description. https://codereview.chromium.org/614103007/diff/20001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/614103007/diff/20001/ui/gfx/text_elider.cc#ne... ui/gfx/text_elider.cc:506: float GetMaxBrokenPixelWidth() const { return max_broken_pixel_width_; } On 2014/10/06 17:59:58, Alexei Svitkine wrote: > Nit: This should just be max_broken_pixel_width() since its a trivial accessor. Done.
Thanks for the description. I don't think this is the right fix. It seems like a hack to workaround the issue. If it was localized, it may be OK, but the fact that it involves changing the function API is too much for a hack. Have you considered any of the following alternative solutions? 1. Checking the actual values that are being summed (i.e. the widths of individual words vs. the concatenated string) and making sure the issue is not because of floating point truncation. What are the actual values? 2. If indeed its not due to floating point truncation, to fix the code to not do a "sum of widths" if indeed that sum of widths doesn't correspond to the width of the concatenated string. 3. Don't draw the string in multiline mode, instead just draw each line by itself in single line mode. That way, there's no extra breaking that happens at the time of painting. I think this is worth doing regardless of 1 and 2, since running the wrapping code again is not efficient.
Thanks for your answer. 1. It is not floating point truncation. Somehow on Link width of "The administrator " is 105 and width of "for" is 17 but width of "The administrator for" is 121. 2. Now I've done it. 3. We can't do it. Initially we calculate preferred size of the text. We can't guarantee size will not change during actual rendering.
Please review
Thanks, I like this fix better. Can you write a unit test for your specific observed example? https://codereview.chromium.org/614103007/diff/50001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/614103007/diff/50001/ui/gfx/text_elider.cc#ne... ui/gfx/text_elider.cc:686: // equal to width of concatenation of these strings (see crbug.com/415213). Nit: Put this comment above the "float new_width =" line.
Can you help me with this test? It is only reproduces on Link. I believe it is font-specific. Do you know similar font-specific tests? On 2014/10/09 13:31:03, Alexei Svitkine wrote: > Thanks, I like this fix better. > > Can you write a unit test for your specific observed example? > > https://codereview.chromium.org/614103007/diff/50001/ui/gfx/text_elider.cc > File ui/gfx/text_elider.cc (right): > > https://codereview.chromium.org/614103007/diff/50001/ui/gfx/text_elider.cc#ne... > ui/gfx/text_elider.cc:686: // equal to width of concatenation of these strings > (see crbug.com/415213). > Nit: Put this comment above the "float new_width =" line.
Can you check if any of our bots do have that font? e.g. write a unit test with code that verifies if that font was used and run try jobs and check their outputs. If we have at least one bot that does have it, then that's good enough for me. Otherwise, we can loop in other font folks like derat@ and see what they suggest. On Thu, Oct 9, 2014 at 9:38 AM, <rsorokin@chromium.org> wrote: > Can you help me with this test? It is only reproduces on Link. I believe > it is > font-specific. Do you know similar font-specific tests? > > > On 2014/10/09 13:31:03, Alexei Svitkine wrote: > >> Thanks, I like this fix better. >> > > Can you write a unit test for your specific observed example? >> > > https://codereview.chromium.org/614103007/diff/50001/ui/ >> gfx/text_elider.cc >> File ui/gfx/text_elider.cc (right): >> > > > https://codereview.chromium.org/614103007/diff/50001/ui/ > gfx/text_elider.cc#newcode686 > >> ui/gfx/text_elider.cc:686: // equal to width of concatenation of these >> strings >> (see crbug.com/415213). >> Nit: Put this comment above the "float new_width =" line. >> > > > > https://codereview.chromium.org/614103007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #7 (id:110001) has been deleted
Patchset #6 (id:90001) has been deleted
Patchset #5 (id:70001) has been deleted
Sorry for the long response. I made this test https://codereview.chromium.org/614103007/diff/130001/ui/gfx/text_elider_unit... "Noto Sans UI,ui-sans, 12px" is the font that was using in the buggy popup. But it does not seem to fail on linux, win or mac.
But it does fail on ChromeOS bots? Or not even? On Wed, Oct 22, 2014 at 9:42 AM, <rsorokin@chromium.org> wrote: > Sorry for the long response. > > I made this test > https://codereview.chromium.org/614103007/diff/130001/ui/ > gfx/text_elider_unittest.cc > "Noto Sans UI,ui-sans, 12px" is the font that was using in the buggy > popup. But > it does not seem to fail on linux, win or mac. > > https://codereview.chromium.org/614103007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I tested on linux_chromium_chromeos_rel - does not fail On 2014/10/22 13:57:51, Alexei Svitkine wrote: > But it does fail on ChromeOS bots? Or not even? > > On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> wrote: > > > Sorry for the long response. > > > > I made this test > > https://codereview.chromium.org/614103007/diff/130001/ui/ > > gfx/text_elider_unittest.cc > > "Noto Sans UI,ui-sans, 12px" is the font that was using in the buggy > > popup. But > > it does not seem to fail on linux, win or mac. > > > > https://codereview.chromium.org/614103007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
+derat who might be able to help. Daniel, do you know if our bots have ChromeOS fonts installed, and if not, is there a way we can get them installed? On Wed, Oct 22, 2014 at 10:02 AM, <rsorokin@chromium.org> wrote: > I tested on linux_chromium_chromeos_rel - does not fail > > On 2014/10/22 13:57:51, Alexei Svitkine wrote: > >> But it does fail on ChromeOS bots? Or not even? >> > > On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> wrote: >> > > > Sorry for the long response. >> > >> > I made this test >> > https://codereview.chromium.org/614103007/diff/130001/ui/ >> > gfx/text_elider_unittest.cc >> > "Noto Sans UI,ui-sans, 12px" is the font that was using in the buggy >> > popup. But >> > it does not seem to fail on linux, win or mac. >> > >> > https://codereview.chromium.org/614103007/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/614103007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The script looks like it's probably outdated (i.e. installing notofonts-20121206.tar.gz even though 20140815 looks like it's the latest), but see build/linux/install-chromeos-fonts.py, which gets run by install-build-deps.sh --chromeos-fonts. I have no idea about how the bots were set up, though. I can update the script, at least. On Wed, Oct 22, 2014 at 8:07 AM, Alexei Svitkine <asvitkine@chromium.org> wrote: > +derat who might be able to help. > > Daniel, do you know if our bots have ChromeOS fonts installed, and if not, > is there a way we can get them installed? > > On Wed, Oct 22, 2014 at 10:02 AM, <rsorokin@chromium.org> wrote: > >> I tested on linux_chromium_chromeos_rel - does not fail >> >> On 2014/10/22 13:57:51, Alexei Svitkine wrote: >> >>> But it does fail on ChromeOS bots? Or not even? >>> >> >> On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> wrote: >>> >> >> > Sorry for the long response. >>> > >>> > I made this test >>> > https://codereview.chromium.org/614103007/diff/130001/ui/ >>> > gfx/text_elider_unittest.cc >>> > "Noto Sans UI,ui-sans, 12px" is the font that was using in the buggy >>> > popup. But >>> > it does not seem to fail on linux, win or mac. >>> > >>> > https://codereview.chromium.org/614103007/ >>> > >>> >> >> To unsubscribe from this group and stop receiving emails from it, send an >>> >> email >> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >>> >> >> >> https://codereview.chromium.org/614103007/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Still does not seem to fail. Are there any other ways we can test this Link-specific bug? This could be font or render specific. On 2014/10/22 14:26:37, Daniel Erat wrote: > The script looks like it's probably outdated (i.e. > installing notofonts-20121206.tar.gz even though 20140815 looks like it's > the latest), but see build/linux/install-chromeos-fonts.py, which gets run > by install-build-deps.sh --chromeos-fonts. I have no idea about how the > bots were set up, though. > > I can update the script, at least. > > On Wed, Oct 22, 2014 at 8:07 AM, Alexei Svitkine <mailto:asvitkine@chromium.org> > wrote: > > > +derat who might be able to help. > > > > Daniel, do you know if our bots have ChromeOS fonts installed, and if not, > > is there a way we can get them installed? > > > > On Wed, Oct 22, 2014 at 10:02 AM, <mailto:rsorokin@chromium.org> wrote: > > > >> I tested on linux_chromium_chromeos_rel - does not fail > >> > >> On 2014/10/22 13:57:51, Alexei Svitkine wrote: > >> > >>> But it does fail on ChromeOS bots? Or not even? > >>> > >> > >> On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> wrote: > >>> > >> > >> > Sorry for the long response. > >>> > > >>> > I made this test > >>> > https://codereview.chromium.org/614103007/diff/130001/ui/ > >>> > gfx/text_elider_unittest.cc > >>> > "Noto Sans UI,ui-sans, 12px" is the font that was using in the buggy > >>> > popup. But > >>> > it does not seem to fail on linux, win or mac. > >>> > > >>> > https://codereview.chromium.org/614103007/ > >>> > > >>> > >> > >> To unsubscribe from this group and stop receiving emails from it, send an > >>> > >> email > >> > >>> to mailto:chromium-reviews+unsubscribe@chromium.org. > >>> > >> > >> > >> https://codereview.chromium.org/614103007/ > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I'd guess that your test is failing due to subpixel positioning, which we only enable on high-DPI Chrome OS devices like link. Can you repro the failure elsewhere if you update IsBrowserTextSubpixelPositioningEnabled() in ui/gfx/font_render_params_linux.cc to always return true? On Mon, Oct 27, 2014 at 8:59 AM, <rsorokin@chromium.org> wrote: > Still does not seem to fail. > Are there any other ways we can test this Link-specific bug? This could be > font > or render specific. > > On 2014/10/22 14:26:37, Daniel Erat wrote: > >> The script looks like it's probably outdated (i.e. >> installing notofonts-20121206.tar.gz even though 20140815 looks like it's >> the latest), but see build/linux/install-chromeos-fonts.py, which gets >> run >> by install-build-deps.sh --chromeos-fonts. I have no idea about how the >> bots were set up, though. >> > > I can update the script, at least. >> > > On Wed, Oct 22, 2014 at 8:07 AM, Alexei Svitkine >> > <mailto:asvitkine@chromium.org> > >> wrote: >> > > > +derat who might be able to help. >> > >> > Daniel, do you know if our bots have ChromeOS fonts installed, and if >> not, >> > is there a way we can get them installed? >> > >> > On Wed, Oct 22, 2014 at 10:02 AM, <mailto:rsorokin@chromium.org> wrote: >> > >> >> I tested on linux_chromium_chromeos_rel - does not fail >> >> >> >> On 2014/10/22 13:57:51, Alexei Svitkine wrote: >> >> >> >>> But it does fail on ChromeOS bots? Or not even? >> >>> >> >> >> >> On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> >> wrote: >> >>> >> >> >> >> > Sorry for the long response. >> >>> > >> >>> > I made this test >> >>> > https://codereview.chromium.org/614103007/diff/130001/ui/ >> >>> > gfx/text_elider_unittest.cc >> >>> > "Noto Sans UI,ui-sans, 12px" is the font that was using in the buggy >> >>> > popup. But >> >>> > it does not seem to fail on linux, win or mac. >> >>> > >> >>> > https://codereview.chromium.org/614103007/ >> >>> > >> >>> >> >> >> >> To unsubscribe from this group and stop receiving emails from it, >> send an >> >>> >> >> email >> >> >> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >>> >> >> >> >> >> >> https://codereview.chromium.org/614103007/ >> >> >> > >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/614103007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Daniel, do you know if we have any tests that run in high-DPI mode? If not, should we? Else it's kind of scary to ship code in configs that aren't covered by our tests. On Mon, Oct 27, 2014 at 11:04 AM, Daniel Erat <derat@chromium.org> wrote: > I'd guess that your test is failing due to subpixel positioning, which we > only enable on high-DPI Chrome OS devices like link. Can you repro the > failure elsewhere if you update IsBrowserTextSubpixelPositioningEnabled() > in ui/gfx/font_render_params_linux.cc to always return true? > > On Mon, Oct 27, 2014 at 8:59 AM, <rsorokin@chromium.org> wrote: > >> Still does not seem to fail. >> Are there any other ways we can test this Link-specific bug? This could >> be font >> or render specific. >> >> On 2014/10/22 14:26:37, Daniel Erat wrote: >> >>> The script looks like it's probably outdated (i.e. >>> installing notofonts-20121206.tar.gz even though 20140815 looks like it's >>> the latest), but see build/linux/install-chromeos-fonts.py, which gets >>> run >>> by install-build-deps.sh --chromeos-fonts. I have no idea about how the >>> bots were set up, though. >>> >> >> I can update the script, at least. >>> >> >> On Wed, Oct 22, 2014 at 8:07 AM, Alexei Svitkine >>> >> <mailto:asvitkine@chromium.org> >> >>> wrote: >>> >> >> > +derat who might be able to help. >>> > >>> > Daniel, do you know if our bots have ChromeOS fonts installed, and if >>> not, >>> > is there a way we can get them installed? >>> > >>> > On Wed, Oct 22, 2014 at 10:02 AM, <mailto:rsorokin@chromium.org> >>> wrote: >>> > >>> >> I tested on linux_chromium_chromeos_rel - does not fail >>> >> >>> >> On 2014/10/22 13:57:51, Alexei Svitkine wrote: >>> >> >>> >>> But it does fail on ChromeOS bots? Or not even? >>> >>> >>> >> >>> >> On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> >>> wrote: >>> >>> >>> >> >>> >> > Sorry for the long response. >>> >>> > >>> >>> > I made this test >>> >>> > https://codereview.chromium.org/614103007/diff/130001/ui/ >>> >>> > gfx/text_elider_unittest.cc >>> >>> > "Noto Sans UI,ui-sans, 12px" is the font that was using in the >>> buggy >>> >>> > popup. But >>> >>> > it does not seem to fail on linux, win or mac. >>> >>> > >>> >>> > https://codereview.chromium.org/614103007/ >>> >>> > >>> >>> >>> >> >>> >> To unsubscribe from this group and stop receiving emails from it, >>> send an >>> >>> >>> >> email >>> >> >>> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >>> >>> >>> >> >>> >> >>> >> https://codereview.chromium.org/614103007/ >>> >> >>> > >>> > >>> >> >> To unsubscribe from this group and stop receiving emails from it, send an >>> >> email >> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >>> >> >> >> >> https://codereview.chromium.org/614103007/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oshima might know (whether we run any tests in high-DPI mode, specifically in the context of forcing subpixel positioning). On Mon, Oct 27, 2014 at 9:10 AM, Alexei Svitkine <asvitkine@chromium.org> wrote: > Daniel, do you know if we have any tests that run in high-DPI mode? > > If not, should we? Else it's kind of scary to ship code in configs that > aren't covered by our tests. > > On Mon, Oct 27, 2014 at 11:04 AM, Daniel Erat <derat@chromium.org> wrote: > >> I'd guess that your test is failing due to subpixel positioning, which we >> only enable on high-DPI Chrome OS devices like link. Can you repro the >> failure elsewhere if you update IsBrowserTextSubpixelPositioningEnabled() >> in ui/gfx/font_render_params_linux.cc to always return true? >> >> On Mon, Oct 27, 2014 at 8:59 AM, <rsorokin@chromium.org> wrote: >> >>> Still does not seem to fail. >>> Are there any other ways we can test this Link-specific bug? This could >>> be font >>> or render specific. >>> >>> On 2014/10/22 14:26:37, Daniel Erat wrote: >>> >>>> The script looks like it's probably outdated (i.e. >>>> installing notofonts-20121206.tar.gz even though 20140815 looks like >>>> it's >>>> the latest), but see build/linux/install-chromeos-fonts.py, which gets >>>> run >>>> by install-build-deps.sh --chromeos-fonts. I have no idea about how the >>>> bots were set up, though. >>>> >>> >>> I can update the script, at least. >>>> >>> >>> On Wed, Oct 22, 2014 at 8:07 AM, Alexei Svitkine >>>> >>> <mailto:asvitkine@chromium.org> >>> >>>> wrote: >>>> >>> >>> > +derat who might be able to help. >>>> > >>>> > Daniel, do you know if our bots have ChromeOS fonts installed, and if >>>> not, >>>> > is there a way we can get them installed? >>>> > >>>> > On Wed, Oct 22, 2014 at 10:02 AM, <mailto:rsorokin@chromium.org> >>>> wrote: >>>> > >>>> >> I tested on linux_chromium_chromeos_rel - does not fail >>>> >> >>>> >> On 2014/10/22 13:57:51, Alexei Svitkine wrote: >>>> >> >>>> >>> But it does fail on ChromeOS bots? Or not even? >>>> >>> >>>> >> >>>> >> On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> >>>> wrote: >>>> >>> >>>> >> >>>> >> > Sorry for the long response. >>>> >>> > >>>> >>> > I made this test >>>> >>> > https://codereview.chromium.org/614103007/diff/130001/ui/ >>>> >>> > gfx/text_elider_unittest.cc >>>> >>> > "Noto Sans UI,ui-sans, 12px" is the font that was using in the >>>> buggy >>>> >>> > popup. But >>>> >>> > it does not seem to fail on linux, win or mac. >>>> >>> > >>>> >>> > https://codereview.chromium.org/614103007/ >>>> >>> > >>>> >>> >>>> >> >>>> >> To unsubscribe from this group and stop receiving emails from it, >>>> send an >>>> >>> >>>> >> email >>>> >> >>>> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >>>> >>> >>>> >> >>>> >> >>>> >> https://codereview.chromium.org/614103007/ >>>> >> >>>> > >>>> > >>>> >>> >>> To unsubscribe from this group and stop receiving emails from it, send >>>> an >>>> >>> email >>> >>>> to mailto:chromium-reviews+unsubscribe@chromium.org. >>>> >>> >>> >>> >>> https://codereview.chromium.org/614103007/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That helped! On 2014/10/27 15:04:41, Daniel Erat wrote: > I'd guess that your test is failing due to subpixel positioning, which we > only enable on high-DPI Chrome OS devices like link. Can you repro the > failure elsewhere if you update IsBrowserTextSubpixelPositioningEnabled() > in ui/gfx/font_render_params_linux.cc to always return true? > > On Mon, Oct 27, 2014 at 8:59 AM, <mailto:rsorokin@chromium.org> wrote: > > > Still does not seem to fail. > > Are there any other ways we can test this Link-specific bug? This could be > > font > > or render specific. > > > > On 2014/10/22 14:26:37, Daniel Erat wrote: > > > >> The script looks like it's probably outdated (i.e. > >> installing notofonts-20121206.tar.gz even though 20140815 looks like it's > >> the latest), but see build/linux/install-chromeos-fonts.py, which gets > >> run > >> by install-build-deps.sh --chromeos-fonts. I have no idea about how the > >> bots were set up, though. > >> > > > > I can update the script, at least. > >> > > > > On Wed, Oct 22, 2014 at 8:07 AM, Alexei Svitkine > >> > > <mailto:asvitkine@chromium.org> > > > >> wrote: > >> > > > > > +derat who might be able to help. > >> > > >> > Daniel, do you know if our bots have ChromeOS fonts installed, and if > >> not, > >> > is there a way we can get them installed? > >> > > >> > On Wed, Oct 22, 2014 at 10:02 AM, <mailto:rsorokin@chromium.org> wrote: > >> > > >> >> I tested on linux_chromium_chromeos_rel - does not fail > >> >> > >> >> On 2014/10/22 13:57:51, Alexei Svitkine wrote: > >> >> > >> >>> But it does fail on ChromeOS bots? Or not even? > >> >>> > >> >> > >> >> On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> > >> wrote: > >> >>> > >> >> > >> >> > Sorry for the long response. > >> >>> > > >> >>> > I made this test > >> >>> > https://codereview.chromium.org/614103007/diff/130001/ui/ > >> >>> > gfx/text_elider_unittest.cc > >> >>> > "Noto Sans UI,ui-sans, 12px" is the font that was using in the buggy > >> >>> > popup. But > >> >>> > it does not seem to fail on linux, win or mac. > >> >>> > > >> >>> > https://codereview.chromium.org/614103007/ > >> >>> > > >> >>> > >> >> > >> >> To unsubscribe from this group and stop receiving emails from it, > >> send an > >> >>> > >> >> email > >> >> > >> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> >>> > >> >> > >> >> > >> >> https://codereview.chromium.org/614103007/ > >> >> > >> > > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > > > > > https://codereview.chromium.org/614103007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
It seems this helped only on my machine and on bots it still does not fail. On 2014/10/27 15:48:41, Roman Sorokin wrote: > That helped! > > On 2014/10/27 15:04:41, Daniel Erat wrote: > > I'd guess that your test is failing due to subpixel positioning, which we > > only enable on high-DPI Chrome OS devices like link. Can you repro the > > failure elsewhere if you update IsBrowserTextSubpixelPositioningEnabled() > > in ui/gfx/font_render_params_linux.cc to always return true? > > > > On Mon, Oct 27, 2014 at 8:59 AM, <mailto:rsorokin@chromium.org> wrote: > > > > > Still does not seem to fail. > > > Are there any other ways we can test this Link-specific bug? This could be > > > font > > > or render specific. > > > > > > On 2014/10/22 14:26:37, Daniel Erat wrote: > > > > > >> The script looks like it's probably outdated (i.e. > > >> installing notofonts-20121206.tar.gz even though 20140815 looks like it's > > >> the latest), but see build/linux/install-chromeos-fonts.py, which gets > > >> run > > >> by install-build-deps.sh --chromeos-fonts. I have no idea about how the > > >> bots were set up, though. > > >> > > > > > > I can update the script, at least. > > >> > > > > > > On Wed, Oct 22, 2014 at 8:07 AM, Alexei Svitkine > > >> > > > <mailto:asvitkine@chromium.org> > > > > > >> wrote: > > >> > > > > > > > +derat who might be able to help. > > >> > > > >> > Daniel, do you know if our bots have ChromeOS fonts installed, and if > > >> not, > > >> > is there a way we can get them installed? > > >> > > > >> > On Wed, Oct 22, 2014 at 10:02 AM, <mailto:rsorokin@chromium.org> wrote: > > >> > > > >> >> I tested on linux_chromium_chromeos_rel - does not fail > > >> >> > > >> >> On 2014/10/22 13:57:51, Alexei Svitkine wrote: > > >> >> > > >> >>> But it does fail on ChromeOS bots? Or not even? > > >> >>> > > >> >> > > >> >> On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> > > >> wrote: > > >> >>> > > >> >> > > >> >> > Sorry for the long response. > > >> >>> > > > >> >>> > I made this test > > >> >>> > https://codereview.chromium.org/614103007/diff/130001/ui/ > > >> >>> > gfx/text_elider_unittest.cc > > >> >>> > "Noto Sans UI,ui-sans, 12px" is the font that was using in the buggy > > >> >>> > popup. But > > >> >>> > it does not seem to fail on linux, win or mac. > > >> >>> > > > >> >>> > https://codereview.chromium.org/614103007/ > > >> >>> > > > >> >>> > > >> >> > > >> >> To unsubscribe from this group and stop receiving emails from it, > > >> send an > > >> >>> > > >> >> email > > >> >> > > >> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. > > >> >>> > > >> >> > > >> >> > > >> >> https://codereview.chromium.org/614103007/ > > >> >> > > >> > > > >> > > > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > >> > > > email > > > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > >> > > > > > > > > > > > > https://codereview.chromium.org/614103007/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
Do you have any thoughts how to make it fail on the bots? On 2014/10/28 08:07:21, Roman Sorokin wrote: > It seems this helped only on my machine and on bots it still does not fail. > > On 2014/10/27 15:48:41, Roman Sorokin wrote: > > That helped! > > > > On 2014/10/27 15:04:41, Daniel Erat wrote: > > > I'd guess that your test is failing due to subpixel positioning, which we > > > only enable on high-DPI Chrome OS devices like link. Can you repro the > > > failure elsewhere if you update IsBrowserTextSubpixelPositioningEnabled() > > > in ui/gfx/font_render_params_linux.cc to always return true? > > > > > > On Mon, Oct 27, 2014 at 8:59 AM, <mailto:rsorokin@chromium.org> wrote: > > > > > > > Still does not seem to fail. > > > > Are there any other ways we can test this Link-specific bug? This could be > > > > font > > > > or render specific. > > > > > > > > On 2014/10/22 14:26:37, Daniel Erat wrote: > > > > > > > >> The script looks like it's probably outdated (i.e. > > > >> installing notofonts-20121206.tar.gz even though 20140815 looks like it's > > > >> the latest), but see build/linux/install-chromeos-fonts.py, which gets > > > >> run > > > >> by install-build-deps.sh --chromeos-fonts. I have no idea about how the > > > >> bots were set up, though. > > > >> > > > > > > > > I can update the script, at least. > > > >> > > > > > > > > On Wed, Oct 22, 2014 at 8:07 AM, Alexei Svitkine > > > >> > > > > <mailto:asvitkine@chromium.org> > > > > > > > >> wrote: > > > >> > > > > > > > > > +derat who might be able to help. > > > >> > > > > >> > Daniel, do you know if our bots have ChromeOS fonts installed, and if > > > >> not, > > > >> > is there a way we can get them installed? > > > >> > > > > >> > On Wed, Oct 22, 2014 at 10:02 AM, <mailto:rsorokin@chromium.org> wrote: > > > >> > > > > >> >> I tested on linux_chromium_chromeos_rel - does not fail > > > >> >> > > > >> >> On 2014/10/22 13:57:51, Alexei Svitkine wrote: > > > >> >> > > > >> >>> But it does fail on ChromeOS bots? Or not even? > > > >> >>> > > > >> >> > > > >> >> On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org> > > > >> wrote: > > > >> >>> > > > >> >> > > > >> >> > Sorry for the long response. > > > >> >>> > > > > >> >>> > I made this test > > > >> >>> > https://codereview.chromium.org/614103007/diff/130001/ui/ > > > >> >>> > gfx/text_elider_unittest.cc > > > >> >>> > "Noto Sans UI,ui-sans, 12px" is the font that was using in the > buggy > > > >> >>> > popup. But > > > >> >>> > it does not seem to fail on linux, win or mac. > > > >> >>> > > > > >> >>> > https://codereview.chromium.org/614103007/ > > > >> >>> > > > > >> >>> > > > >> >> > > > >> >> To unsubscribe from this group and stop receiving emails from it, > > > >> send an > > > >> >>> > > > >> >> email > > > >> >> > > > >> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > >> >>> > > > >> >> > > > >> >> > > > >> >> https://codereview.chromium.org/614103007/ > > > >> >> > > > >> > > > > >> > > > > >> > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > > >> > > > > email > > > > > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > >> > > > > > > > > > > > > > > > > https://codereview.chromium.org/614103007/ > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
asvitkine@chromium.org changed reviewers: + oshima@chromium.org
+oshima Oshima, any idea if we have a way to run HighDPI tests on the bots? (Seems derat@ suggested asking you a couple comments up, but you weren't cc'd?)
I don't know why the same change would be failing on your local machine but passing on bots. On Wed, Nov 5, 2014 at 9:31 AM, <rsorokin@chromium.org> wrote: > Do you have any thoughts how to make it fail on the bots? > > On 2014/10/28 08:07:21, Roman Sorokin wrote: > >> It seems this helped only on my machine and on bots it still does not >> fail. >> > > On 2014/10/27 15:48:41, Roman Sorokin wrote: >> > That helped! >> > >> > On 2014/10/27 15:04:41, Daniel Erat wrote: >> > > I'd guess that your test is failing due to subpixel positioning, >> which we >> > > only enable on high-DPI Chrome OS devices like link. Can you repro the >> > > failure elsewhere if you update IsBrowserTextSubpixelPositioni >> ngEnabled() >> > > in ui/gfx/font_render_params_linux.cc to always return true? >> > > >> > > On Mon, Oct 27, 2014 at 8:59 AM, <mailto:rsorokin@chromium.org> >> wrote: >> > > >> > > > Still does not seem to fail. >> > > > Are there any other ways we can test this Link-specific bug? This >> could >> > be > >> > > > font >> > > > or render specific. >> > > > >> > > > On 2014/10/22 14:26:37, Daniel Erat wrote: >> > > > >> > > >> The script looks like it's probably outdated (i.e. >> > > >> installing notofonts-20121206.tar.gz even though 20140815 looks >> like >> > it's > >> > > >> the latest), but see build/linux/install-chromeos-fonts.py, which >> gets >> > > >> run >> > > >> by install-build-deps.sh --chromeos-fonts. I have no idea about >> how the >> > > >> bots were set up, though. >> > > >> >> > > > >> > > > I can update the script, at least. >> > > >> >> > > > >> > > > On Wed, Oct 22, 2014 at 8:07 AM, Alexei Svitkine >> > > >> >> > > > <mailto:asvitkine@chromium.org> >> > > > >> > > >> wrote: >> > > >> >> > > > >> > > > > +derat who might be able to help. >> > > >> > >> > > >> > Daniel, do you know if our bots have ChromeOS fonts installed, >> and if >> > > >> not, >> > > >> > is there a way we can get them installed? >> > > >> > >> > > >> > On Wed, Oct 22, 2014 at 10:02 AM, <mailto:rsorokin@chromium.org> >> > wrote: > >> > > >> > >> > > >> >> I tested on linux_chromium_chromeos_rel - does not fail >> > > >> >> >> > > >> >> On 2014/10/22 13:57:51, Alexei Svitkine wrote: >> > > >> >> >> > > >> >>> But it does fail on ChromeOS bots? Or not even? >> > > >> >>> >> > > >> >> >> > > >> >> On Wed, Oct 22, 2014 at 9:42 AM, <mailto:rsorokin@chromium.org >> > >> > > >> wrote: >> > > >> >>> >> > > >> >> >> > > >> >> > Sorry for the long response. >> > > >> >>> > >> > > >> >>> > I made this test >> > > >> >>> > https://codereview.chromium.org/614103007/diff/130001/ui/ >> > > >> >>> > gfx/text_elider_unittest.cc >> > > >> >>> > "Noto Sans UI,ui-sans, 12px" is the font that was using in >> the >> buggy >> > > >> >>> > popup. But >> > > >> >>> > it does not seem to fail on linux, win or mac. >> > > >> >>> > >> > > >> >>> > https://codereview.chromium.org/614103007/ >> > > >> >>> > >> > > >> >>> >> > > >> >> >> > > >> >> To unsubscribe from this group and stop receiving emails from >> it, >> > > >> send an >> > > >> >>> >> > > >> >> email >> > > >> >> >> > > >> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > >> >>> >> > > >> >> >> > > >> >> >> > > >> >> https://codereview.chromium.org/614103007/ >> > > >> >> >> > > >> > >> > > >> > >> > > >> >> > > > >> > > > To unsubscribe from this group and stop receiving emails from it, >> send >> > an > >> > > >> >> > > > email >> > > > >> > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > >> >> > > > >> > > > >> > > > >> > > > https://codereview.chromium.org/614103007/ >> > > > >> > > >> > > To unsubscribe from this group and stop receiving emails from it, >> send an >> > email >> > > to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/614103007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I replied in the bug. The width of the text changes unlinearly for scale factor so you should first check if the code is using the right scale factor to calculate the width, not 1.0f. (I've seen similar issue) On Wed, Nov 5, 2014 at 8:35 AM, <asvitkine@chromium.org> wrote: > +oshima > > Oshima, any idea if we have a way to run HighDPI tests on the bots? > > (Seems derat@ suggested asking you a couple comments up, but you weren't > cc'd?) > > https://codereview.chromium.org/614103007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Somehow the bug got fixed by itself during this month. I suggest we can leave this test, so at least it will catch regression for this specific message. (This test fails on the codebase month before now) On 2014/11/05 18:07:31, oshima wrote: > I replied in the bug. The width of the text changes unlinearly for scale > factor so you should first check if the code is using the right scale > factor to calculate the width, not 1.0f. (I've seen similar issue) > > > > On Wed, Nov 5, 2014 at 8:35 AM, <mailto:asvitkine@chromium.org> wrote: > > > +oshima > > > > Oshima, any idea if we have a way to run HighDPI tests on the bots? > > > > (Seems derat@ suggested asking you a couple comments up, but you weren't > > cc'd?) > > > > https://codereview.chromium.org/614103007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I wonder if the issue was fixed when Harfbuzz was enabled by default. Having a test makes sense. Though, I'm hoping we don't need it to be platform dependent. https://codereview.chromium.org/614103007/diff/170001/ui/gfx/text_elider_unit... File ui/gfx/text_elider_unittest.cc (right): https://codereview.chromium.org/614103007/diff/170001/ui/gfx/text_elider_unit... ui/gfx/text_elider_unittest.cc:688: DISABLED_ElideRectangleTextCheckConcatWidthEqualsSumOfWidths Does this mean it fails on other platforms?
https://codereview.chromium.org/614103007/diff/170001/ui/gfx/text_elider_unit... File ui/gfx/text_elider_unittest.cc (right): https://codereview.chromium.org/614103007/diff/170001/ui/gfx/text_elider_unit... ui/gfx/text_elider_unittest.cc:688: DISABLED_ElideRectangleTextCheckConcatWidthEqualsSumOfWidths I think no, (have not checked yet). I guess I could enable it for all the platforms. On 2014/11/06 15:05:46, Alexei Svitkine wrote: > Does this mean it fails on other platforms?
glad that harfbuzz fixed the issue :) On 2014/11/05 16:35:22, Alexei Svitkine wrote: > +oshima > > Oshima, any idea if we have a way to run HighDPI tests on the bots? > > (Seems derat@ suggested asking you a couple comments up, but you weren't cc'd?) Regarding this question, other high DPI related tests that we have for cros are basically doing the same (set the scale factor parameter as if it comes from system). Only potential issue that I can think of is that bots may have different font configs, and it may be falling through to something different than what's being specified in the test? Is there a way to verify that?
Actually it does not work on other platforms since SetFontRenderParamsDeviceScaleFactor is available on ChromeOS only. On 2014/11/06 15:54:34, oshima wrote: > glad that harfbuzz fixed the issue :) > > On 2014/11/05 16:35:22, Alexei Svitkine wrote: > > +oshima > > > > Oshima, any idea if we have a way to run HighDPI tests on the bots? > > > > (Seems derat@ suggested asking you a couple comments up, but you weren't > cc'd?) > > Regarding this question, other high DPI related tests that we have for cros > are basically doing the same (set the scale factor parameter as if it comes from > system). > > Only potential issue that I can think of is that bots may have different font > configs, > and it may be falling through to something different than what's being specified > in the test? > Is there a way to verify that?
Then I suggest just putting an ifdef chromeos block around the test, rather than redefining the test name to be DISABLED. On Fri, Nov 7, 2014 at 11:24 AM, <rsorokin@chromium.org> wrote: > Actually it does not work on other platforms since > SetFontRenderParamsDeviceScaleFactor is available on ChromeOS only. > > > On 2014/11/06 15:54:34, oshima wrote: > >> glad that harfbuzz fixed the issue :) >> > > On 2014/11/05 16:35:22, Alexei Svitkine wrote: >> > +oshima >> > >> > Oshima, any idea if we have a way to run HighDPI tests on the bots? >> > >> > (Seems derat@ suggested asking you a couple comments up, but you >> weren't >> cc'd?) >> > > Regarding this question, other high DPI related tests that we have for >> cros >> are basically doing the same (set the scale factor parameter as if it >> comes >> > from > >> system). >> > > Only potential issue that I can think of is that bots may have different >> font >> configs, >> and it may be falling through to something different than what's being >> > specified > >> in the test? >> Is there a way to verify that? >> > > > https://codereview.chromium.org/614103007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Alexei, please review. Do you think it is still worth to commit my fix? Cause the bug potentially can occur on a different message. In text elider we still consider that sum of widths is equal to the width of concatenation of the words. On 2014/11/07 16:29:44, Alexei Svitkine wrote: > Then I suggest just putting an ifdef chromeos block around the test, rather > than redefining the test name to be DISABLED. > > On Fri, Nov 7, 2014 at 11:24 AM, <mailto:rsorokin@chromium.org> wrote: > > > Actually it does not work on other platforms since > > SetFontRenderParamsDeviceScaleFactor is available on ChromeOS only. > > > > > > On 2014/11/06 15:54:34, oshima wrote: > > > >> glad that harfbuzz fixed the issue :) > >> > > > > On 2014/11/05 16:35:22, Alexei Svitkine wrote: > >> > +oshima > >> > > >> > Oshima, any idea if we have a way to run HighDPI tests on the bots? > >> > > >> > (Seems derat@ suggested asking you a couple comments up, but you > >> weren't > >> cc'd?) > >> > > > > Regarding this question, other high DPI related tests that we have for > >> cros > >> are basically doing the same (set the scale factor parameter as if it > >> comes > >> > > from > > > >> system). > >> > > > > Only potential issue that I can think of is that bots may have different > >> font > >> configs, > >> and it may be falling through to something different than what's being > >> > > specified > > > >> in the test? > >> Is there a way to verify that? > >> > > > > > > https://codereview.chromium.org/614103007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by rsorokin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/614103007/210001
Message was sent while issue was closed.
Committed patchset #9 (id:210001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/370fe4b5db1c9412093225f7a6e1adc18f8aedb2 Cr-Commit-Position: refs/heads/master@{#303650} |
