Revert 4939383c95030cf963cf5a70ede23c57b212baa4 from M39

Revert 4939383c95030cf963cf5a70ede23c57b212baa4 from M39

Causing blanks/boxes on mixed CJK configurations.

A direct revert wasn't sufficient, I had to add internal:: to LinkedFontsIterator and re-add the implementation of ChooseFallbackFont.

R=asvitkine@chromium.org, ckocagil@chromium.org
BUG=422142
TEST=zh-hans WinXP, Chrome UI set to Korean, things don't disappear

Committed: https://chromium.googlesource.com/chromium/src/+/ca0bd02de9c18071a74d8aa496f8ddbdb85729a7

[scottmg, 2014-10-22 18:46:49]

Patchset #1 (id:1) has been deleted

[scottmg, 2014-10-22 18:49:22]

Same as https://codereview.chromium.org/663373005/ but for 39.

[Alexei Svitkine (slow), 2014-10-22 18:51:40]

LGTM.

Should we also do this on TOT for now, so that we have matching behavior there
until we have a better fix? (I realise Harfbuzz is on TOT now, but I fear we
might disable it before branch point and re-introduce this.)

[scottmg, 2014-10-22 18:55:04]

Committed patchset #1 (id:20001) manually as
ca0bd02de9c18071a74d8aa496f8ddbdb85729a7 (presubmit successful).

[ckocagil, 2014-10-22 18:55:56]

lgtm

On 2014/10/22 18:51:40, Alexei Svitkine wrote:
> LGTM.
> 
> Should we also do this on TOT for now, so that we have matching behavior there
> until we have a better fix? (I realise Harfbuzz is on TOT now, but I fear we
> might disable it before branch point and re-introduce this.)

RenderTextWin is already fixed on ToT by a different CL instead of a revert.
https://codereview.chromium.org/637953010/

RenderTextHarfBuzz hasn't been fixed yet, I will make that a beta release
blocker.

[Alexei Svitkine (slow), 2014-10-22 18:58:17]

I thought the other CL wasn't enough to fully fix the issue, hence why we
didn't merge it back? Scott?

On Wed, Oct 22, 2014 at 2:55 PM, <ckocagil@chromium.org> wrote:

> lgtm
>
> On 2014/10/22 18:51:40, Alexei Svitkine wrote:
>
>> LGTM.
>>
>
>  Should we also do this on TOT for now, so that we have matching behavior
>> there
>> until we have a better fix? (I realise Harfbuzz is on TOT now, but I fear
>> we
>> might disable it before branch point and re-introduce this.)
>>
>
> RenderTextWin is already fixed on ToT by a different CL instead of a
> revert.
> https://codereview.chromium.org/637953010/
>
> RenderTextHarfBuzz hasn't been fixed yet, I will make that a beta release
> blocker.
>
> https://codereview.chromium.org/638413003/
>

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

[scottmg, 2014-10-22 19:07:12]

I'm not sure if it was or not, but a revert seemed simpler for old
branches, and was easier to apply.

It seems reasonable to me to try to work on fixing on 40, but we should
definitely make sure QA manually confirms that we haven't regressed on 40
before it goes out.

On Wed, Oct 22, 2014 at 11:57 AM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> I thought the other CL wasn't enough to fully fix the issue, hence why we
> didn't merge it back? Scott?
>
> On Wed, Oct 22, 2014 at 2:55 PM, <ckocagil@chromium.org> wrote:
>
>> lgtm
>>
>> On 2014/10/22 18:51:40, Alexei Svitkine wrote:
>>
>>> LGTM.
>>>
>>
>>  Should we also do this on TOT for now, so that we have matching behavior
>>> there
>>> until we have a better fix? (I realise Harfbuzz is on TOT now, but I
>>> fear we
>>> might disable it before branch point and re-introduce this.)
>>>
>>
>> RenderTextWin is already fixed on ToT by a different CL instead of a
>> revert.
>> https://codereview.chromium.org/637953010/
>>
>> RenderTextHarfBuzz hasn't been fixed yet, I will make that a beta release
>> blocker.
>>
>> https://codereview.chromium.org/638413003/
>>
>
>  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.

[Alexei Svitkine (slow), 2014-10-22 19:17:21]

Scott, would you mind trying TOT with your repro VM (when you disable
harfbuzz?

I'd like to understand if the TOT fix is really fixing the issue or not...
If not, I still prefer we apply the same fix on TOT until we have something
better. If it does fix the issue, all is good % making the same change to
harfbuzz which we have a new bug for.

On Wed, Oct 22, 2014 at 3:07 PM, Scott Graham <scottmg@chromium.org> wrote:

> I'm not sure if it was or not, but a revert seemed simpler for old
> branches, and was easier to apply.
>
> It seems reasonable to me to try to work on fixing on 40, but we should
> definitely make sure QA manually confirms that we haven't regressed on 40
> before it goes out.
>
> On Wed, Oct 22, 2014 at 11:57 AM, Alexei Svitkine <asvitkine@chromium.org>
> wrote:
>
>> I thought the other CL wasn't enough to fully fix the issue, hence why we
>> didn't merge it back? Scott?
>>
>> On Wed, Oct 22, 2014 at 2:55 PM, <ckocagil@chromium.org> wrote:
>>
>>> lgtm
>>>
>>> On 2014/10/22 18:51:40, Alexei Svitkine wrote:
>>>
>>>> LGTM.
>>>>
>>>
>>>  Should we also do this on TOT for now, so that we have matching
>>>> behavior there
>>>> until we have a better fix? (I realise Harfbuzz is on TOT now, but I
>>>> fear we
>>>> might disable it before branch point and re-introduce this.)
>>>>
>>>
>>> RenderTextWin is already fixed on ToT by a different CL instead of a
>>> revert.
>>> https://codereview.chromium.org/637953010/
>>>
>>> RenderTextHarfBuzz hasn't been fixed yet, I will make that a beta release
>>> blocker.
>>>
>>> https://codereview.chromium.org/638413003/
>>>
>>
>>  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.

[scottmg, 2014-10-22 19:23:04]

Oh, with it disabled, I see. pbommana tried that here
https://code.google.com/p/chromium/issues/detail?id=422142#c65 so yes, I
think we just need to fix the non-disabled path at TOT.

On Wed, Oct 22, 2014 at 12:16 PM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> Scott, would you mind trying TOT with your repro VM (when you disable
> harfbuzz?
>
> I'd like to understand if the TOT fix is really fixing the issue or not...
> If not, I still prefer we apply the same fix on TOT until we have something
> better. If it does fix the issue, all is good % making the same change to
> harfbuzz which we have a new bug for.
>
> On Wed, Oct 22, 2014 at 3:07 PM, Scott Graham <scottmg@chromium.org>
> wrote:
>
>> I'm not sure if it was or not, but a revert seemed simpler for old
>> branches, and was easier to apply.
>>
>> It seems reasonable to me to try to work on fixing on 40, but we should
>> definitely make sure QA manually confirms that we haven't regressed on 40
>> before it goes out.
>>
>> On Wed, Oct 22, 2014 at 11:57 AM, Alexei Svitkine <asvitkine@chromium.org
>> > wrote:
>>
>>> I thought the other CL wasn't enough to fully fix the issue, hence why
>>> we didn't merge it back? Scott?
>>>
>>> On Wed, Oct 22, 2014 at 2:55 PM, <ckocagil@chromium.org> wrote:
>>>
>>>> lgtm
>>>>
>>>> On 2014/10/22 18:51:40, Alexei Svitkine wrote:
>>>>
>>>>> LGTM.
>>>>>
>>>>
>>>>  Should we also do this on TOT for now, so that we have matching
>>>>> behavior there
>>>>> until we have a better fix? (I realise Harfbuzz is on TOT now, but I
>>>>> fear we
>>>>> might disable it before branch point and re-introduce this.)
>>>>>
>>>>
>>>> RenderTextWin is already fixed on ToT by a different CL instead of a
>>>> revert.
>>>> https://codereview.chromium.org/637953010/
>>>>
>>>> RenderTextHarfBuzz hasn't been fixed yet, I will make that a beta
>>>> release
>>>> blocker.
>>>>
>>>> https://codereview.chromium.org/638413003/
>>>>
>>>
>>>  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.
>

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

[ckocagil, 2014-10-23 04:50:12]

On 2014/10/22 18:58:17, Alexei Svitkine wrote:
> I thought the other CL wasn't enough to fully fix the issue, hence why we
> didn't merge it back? Scott?

I forgot to remind Scott to disable HarfBuzz while testing on ToT, so we came to
the wrong conclusion that there were separate issues, one which my CL fixed and
one that it didn't.

We still didn't merge it back because reverting is safer.

[Alexei Svitkine (slow), 2014-10-23 15:26:53]

Ah, that makes sense - thanks for double checking.

On Thu, Oct 23, 2014 at 12:50 AM, <ckocagil@chromium.org> wrote:

> On 2014/10/22 18:58:17, Alexei Svitkine wrote:
>
>> I thought the other CL wasn't enough to fully fix the issue, hence why we
>> didn't merge it back? Scott?
>>
>
> I forgot to remind Scott to disable HarfBuzz while testing on ToT, so we
> came to
> the wrong conclusion that there were separate issues, one which my CL
> fixed and
> one that it didn't.
>
> We still didn't merge it back because reverting is safer.
>
> https://codereview.chromium.org/638413003/
>

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