Issue 988763005: Change for Win XP

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+312 lines, -51 lines)			Patch
M	ui/gfx/render_text.h	View	1 2 3 4	5 chunks	+14 lines, -3 lines	0 comments	Download
M	ui/gfx/render_text.cc	View	1 2 3 4	9 chunks	+43 lines, -22 lines	0 comments	Download
M	ui/gfx/render_text_harfbuzz.h	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	ui/gfx/render_text_harfbuzz.cc	View	1 2 3 4	6 chunks	+36 lines, -5 lines	0 comments	Download
M	ui/gfx/render_text_mac.cc	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	ui/gfx/render_text_unittest.cc	View	1 2 3	5 chunks	+197 lines, -20 lines	0 comments	Download
M	ui/gfx/text_constants.h	View	1 2 3 4	1 chunk	+19 lines, -0 lines	0 comments	Download

Messages

Total messages: 15 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dschuyler

This may be a fix for the windows XP unit test drawing outside of the ...

5 years, 9 months ago (2015-03-07 01:28:04 UTC) #2

dschuyler

Thew newest patch should also address ChromeOS drawing above the draw rectangle.

5 years, 9 months ago (2015-03-07 02:03:00 UTC) #3

Jun Mukai

NOT LGTM Please revert your previous patch first. This solution (excluding some bots) does not ...

5 years, 9 months ago (2015-03-07 02:19:02 UTC) #7

dschuyler

On 2015/03/07 02:19:02, Jun Mukai wrote: > NOT LGTM > Please revert your previous patch ...

5 years, 9 months ago (2015-03-07 02:24:30 UTC) #9

Jun Mukai

On 2015/03/07 02:24:30, dschuyler wrote: > On 2015/03/07 02:19:02, Jun Mukai wrote: > > NOT ...

5 years, 9 months ago (2015-03-07 02:31:25 UTC) #10

chromium-reviews

Ah, the justification is that it may not have previously worked correctly. I changed/improved the ...

5 years, 9 months ago (2015-03-07 02:43:46 UTC) #11

Ah, the justification is that it may not have previously worked correctly.
I changed/improved the unit test to write the text out in the canvas,
rather than up against the left edge where it would be clipped.  The prior
method of testing would not show the one pixel line error on Windows and
Mac OS and the larger error on ChromeOS.  With the justification of it
'being like that before' and 'not busted by me in this change list'; I felt
it was reasonable to add the #if defines.  That's what I was thinking
anyway - it's up to the judgement of others whether that is a reasonable
justification.

All that said, reverting the other check-in will also stop the try bot from
failing.

On Fri, Mar 6, 2015 at 6:31 PM, <mukai@chromium.org> wrote:

> On 2015/03/07 02:24:30, dschuyler wrote:
>
>> On 2015/03/07 02:19:02, Jun Mukai wrote:
>> > NOT LGTM
>> > Please revert your previous patch first. This solution (excluding some
>> bots)
>> > does not sound correct because I don't see the reason why it fails.
>> > What justifies the exclusion?
>> >
>> >
>>
>
> https://codereview.chromium.org/988763005/diff/20001/ui/
> gfx/render_text_unittest.cc
>
>> > File ui/gfx/render_text_unittest.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/988763005/diff/20001/ui/
> gfx/render_text_unittest.cc#newcode2705
>
>> > ui/gfx/render_text_unittest.cc:2705: // TODO(mukai): On ChromeOS text
>> draws
>> > above the GetStringSize rect.
>> > Please do not do like this. Excluding specific platform is acceptable
>> only
>>
> why
>
>> > you understand why the failure happens -- what is the actual reason?
>> Does
>>
> it
>
>> > because of fontconfig?  Does it because of specific font settings?
>> > Also, please do not assign TODO like this to someone else. That's your
>> failure,
>> > you should fix this.
>> >
>> >
>>
>
> https://codereview.chromium.org/988763005/diff/20001/ui/
> gfx/render_text_unittest.cc#newcode2721
>
>> > ui/gfx/render_text_unittest.cc:2721: // if it is ignoring the
>> SetDisplayRect
>> > shift by kTestSize.
>> > Why not just #if defined(OS_MACOSX) ?
>>
>
>  I apologize, I made an assumption based on the other exclusions in the
>> same
>>
> unit
>
>> test, excluding by platform.
>> The revert of the other change list
>>
> (https://codereview.chromium.org/985923002/)
>
>> sounds like the way to go then.
>>
>
> As I said, if you have justifications for exclusion, that's fine.
> But adding ifdef just because you see failures doesn't sound correct.
>
> https://codereview.chromium.org/988763005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Jun Mukai

On 2015/03/07 02:43:46, chromium-reviews wrote: > Ah, the justification is that it may not have ...

5 years, 9 months ago (2015-03-07 02:50:43 UTC) #12

On 2015/03/07 02:43:46, chromium-reviews wrote:
> Ah, the justification is that it may not have previously worked correctly.
> I changed/improved the unit test to write the text out in the canvas,
> rather than up against the left edge where it would be clipped.  The prior
> method of testing would not show the one pixel line error on Windows and
> Mac OS and the larger error on ChromeOS.  With the justification of it
> 'being like that before' and 'not busted by me in this change list'; I felt
> it was reasonable to add the #if defines.  That's what I was thinking
> anyway - it's up to the judgement of others whether that is a reasonable
> justification.
> 
> All that said, reverting the other check-in will also stop the try bot from
> failing.
> 

You explained what happened, but I want to know why they happened
and why it's platform specific. Why it's clipped? Is that
because of the font settings? Font library bug? Other platform bugs?

Also, your TODOs aren't clear enough. What do you expect?
And these TODOs should be yours, not mine.

> On Fri, Mar 6, 2015 at 6:31 PM, <mailto:mukai@chromium.org> wrote:
> 
> > On 2015/03/07 02:24:30, dschuyler wrote:
> >
> >> On 2015/03/07 02:19:02, Jun Mukai wrote:
> >> > NOT LGTM
> >> > Please revert your previous patch first. This solution (excluding some
> >> bots)
> >> > does not sound correct because I don't see the reason why it fails.
> >> > What justifies the exclusion?
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/988763005/diff/20001/ui/
> > gfx/render_text_unittest.cc
> >
> >> > File ui/gfx/render_text_unittest.cc (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/988763005/diff/20001/ui/
> > gfx/render_text_unittest.cc#newcode2705
> >
> >> > ui/gfx/render_text_unittest.cc:2705: // TODO(mukai): On ChromeOS text
> >> draws
> >> > above the GetStringSize rect.
> >> > Please do not do like this. Excluding specific platform is acceptable
> >> only
> >>
> > why
> >
> >> > you understand why the failure happens -- what is the actual reason?
> >> Does
> >>
> > it
> >
> >> > because of fontconfig?  Does it because of specific font settings?
> >> > Also, please do not assign TODO like this to someone else. That's your
> >> failure,
> >> > you should fix this.
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/988763005/diff/20001/ui/
> > gfx/render_text_unittest.cc#newcode2721
> >
> >> > ui/gfx/render_text_unittest.cc:2721: // if it is ignoring the
> >> SetDisplayRect
> >> > shift by kTestSize.
> >> > Why not just #if defined(OS_MACOSX) ?
> >>
> >
> >  I apologize, I made an assumption based on the other exclusions in the
> >> same
> >>
> > unit
> >
> >> test, excluding by platform.
> >> The revert of the other change list
> >>
> > (https://codereview.chromium.org/985923002/)
> >
> >> sounds like the way to go then.
> >>
> >
> > As I said, if you have justifications for exclusion, that's fine.
> > But adding ifdef just because you see failures doesn't sound correct.
> >
> > https://codereview.chromium.org/988763005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

chromium-reviews

Hi, Debugging to that extend will take more time, of course. Thank you for explaining ...

5 years, 9 months ago (2015-03-07 03:02:09 UTC) #13

Hi,

Debugging to that extend will take more time, of course.  Thank you for
explaining further.

Will it be well enough for now to revert the CL?

On Fri, Mar 6, 2015 at 6:50 PM, <mukai@chromium.org> wrote:

> On 2015/03/07 02:43:46, chromium-reviews wrote:
>
>> Ah, the justification is that it may not have previously worked correctly.
>> I changed/improved the unit test to write the text out in the canvas,
>> rather than up against the left edge where it would be clipped.  The prior
>> method of testing would not show the one pixel line error on Windows and
>> Mac OS and the larger error on ChromeOS.  With the justification of it
>> 'being like that before' and 'not busted by me in this change list'; I
>> felt
>> it was reasonable to add the #if defines.  That's what I was thinking
>> anyway - it's up to the judgement of others whether that is a reasonable
>> justification.
>>
>
>  All that said, reverting the other check-in will also stop the try bot
>> from
>> failing.
>>
>
>
> You explained what happened, but I want to know why they happened
> and why it's platform specific. Why it's clipped? Is that
> because of the font settings? Font library bug? Other platform bugs?
>
> Also, your TODOs aren't clear enough. What do you expect?
> And these TODOs should be yours, not mine.
>
>
>  On Fri, Mar 6, 2015 at 6:31 PM, <mailto:mukai@chromium.org> wrote:
>>
>
>  > On 2015/03/07 02:24:30, dschuyler wrote:
>> >
>> >> On 2015/03/07 02:19:02, Jun Mukai wrote:
>> >> > NOT LGTM
>> >> > Please revert your previous patch first. This solution (excluding
>> some
>> >> bots)
>> >> > does not sound correct because I don't see the reason why it fails.
>> >> > What justifies the exclusion?
>> >> >
>> >> >
>> >>
>> >
>> > https://codereview.chromium.org/988763005/diff/20001/ui/
>> > gfx/render_text_unittest.cc
>> >
>> >> > File ui/gfx/render_text_unittest.cc (right):
>> >> >
>> >> >
>> >>
>> >
>> > https://codereview.chromium.org/988763005/diff/20001/ui/
>> > gfx/render_text_unittest.cc#newcode2705
>> >
>> >> > ui/gfx/render_text_unittest.cc:2705: // TODO(mukai): On ChromeOS
>> text
>> >> draws
>> >> > above the GetStringSize rect.
>> >> > Please do not do like this. Excluding specific platform is acceptable
>> >> only
>> >>
>> > why
>> >
>> >> > you understand why the failure happens -- what is the actual reason?
>> >> Does
>> >>
>> > it
>> >
>> >> > because of fontconfig?  Does it because of specific font settings?
>> >> > Also, please do not assign TODO like this to someone else. That's
>> your
>> >> failure,
>> >> > you should fix this.
>> >> >
>> >> >
>> >>
>> >
>> > https://codereview.chromium.org/988763005/diff/20001/ui/
>> > gfx/render_text_unittest.cc#newcode2721
>> >
>> >> > ui/gfx/render_text_unittest.cc:2721: // if it is ignoring the
>> >> SetDisplayRect
>> >> > shift by kTestSize.
>> >> > Why not just #if defined(OS_MACOSX) ?
>> >>
>> >
>> >  I apologize, I made an assumption based on the other exclusions in the
>> >> same
>> >>
>> > unit
>> >
>> >> test, excluding by platform.
>> >> The revert of the other change list
>> >>
>> > (https://codereview.chromium.org/985923002/)
>> >
>> >> sounds like the way to go then.
>> >>
>> >
>> > As I said, if you have justifications for exclusion, that's fine.
>> > But adding ifdef just because you see failures doesn't sound correct.
>> >
>> > https://codereview.chromium.org/988763005/
>> >
>>
>
>  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/988763005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Jun Mukai

Yes I'm waiting for the revert too. Do you want me to revert your CL?

5 years, 9 months ago (2015-03-07 03:28:52 UTC) #14

chromium-reviews

5 years, 9 months ago (2015-03-07 05:43:03 UTC) #15

Sorry for any inconvenience.  Please do revert it.

On Fri, Mar 6, 2015 at 7:28 PM, <mukai@chromium.org> wrote:

> Yes I'm waiting for the revert too. Do you want me to revert your CL?
>
> https://codereview.chromium.org/988763005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages