Avoid drawing emphasis marks over ellipsis glyphs

Avoid drawing emphasis marks over ellipsis glyphs

Take m_fromIndex and m_toIndex into account and accumulate initial advance instead of using "0" non-drawing glyphs.

BUG=408544
R=dominik.rottsches@intel.com, behdad
TEST=fast/text/emphasis-ellipsis-complextext.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185955

[Habib Virji, 2014-09-30 11:17:25]

habib.virji@samsung.com changed reviewers:
+ dominik.rottsches@intel.com

[Dominik Röttsches, 2014-09-30 12:27:01]

https://codereview.chromium.org/613193002/diff/20001/LayoutTests/fast/text/em...
File LayoutTests/fast/text/emphasis-ellipsis-complextext.html (right):

https://codereview.chromium.org/613193002/diff/20001/LayoutTests/fast/text/em...
LayoutTests/fast/text/emphasis-ellipsis-complextext.html:18: <div>abcd efg
hijk<div>
Could you add a line with dir=rtl and latin text?

https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1027: glyphToAdd = 0;
Can m_toIndex be factored into the clusterEnd computation. Then there will not
be a need for special casing here.

[Habib Virji, 2014-09-30 13:28:40]

Thanks Dominik. Some clarifications and queries.

https://codereview.chromium.org/613193002/diff/20001/LayoutTests/fast/text/em...
File LayoutTests/fast/text/emphasis-ellipsis-complextext.html (right):

https://codereview.chromium.org/613193002/diff/20001/LayoutTests/fast/text/em...
LayoutTests/fast/text/emphasis-ellipsis-complextext.html:18: <div>abcd efg
hijk<div>
On 2014/09/30 12:27:00, Dominik Röttsches wrote:
> Could you add a line with dir=rtl and latin text?

You mean something like this right?
<div dir="rtl">abcd efg hijk<div>
<div dir="rtl">كتاب ألف ليلة وليلة</div>

<div>Af̀fiZ Default Font</div>

https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1027: glyphToAdd = 0;
On 2014/09/30 12:27:00, Dominik Röttsches wrote:
> Can m_toIndex be factored into the clusterEnd computation. Then there will not
> be a need for special casing here.

Reason to add special handling was returning before graphemes cluster loop was
resulting in emphasis marks were not corresponding to letters.
Other issue was clusterEnd, just holds index. What value of index should be set
to get glyphToAdd as zero.

[Dominik Röttsches, 2014-09-30 13:48:24]

https://codereview.chromium.org/613193002/diff/20001/LayoutTests/fast/text/em...
File LayoutTests/fast/text/emphasis-ellipsis-complextext.html (right):

https://codereview.chromium.org/613193002/diff/20001/LayoutTests/fast/text/em...
LayoutTests/fast/text/emphasis-ellipsis-complextext.html:18: <div>abcd efg
hijk<div>
On 2014/09/30 13:28:40, Habib Virji wrote:
> On 2014/09/30 12:27:00, Dominik Röttsches wrote:
> > Could you add a line with dir=rtl and latin text?
> 
> You mean something like this right?
> <div dir="rtl">abcd efg hijk<div>

Yes.

> <div dir="rtl">كتاب ألف ليلة وليلة</div>

Arabic should be RTL by default in this case, so no need to add the direction
here.

<div>Af̀fiZ Default Font</div>

We're not testing for ligatures here, so this line wouldn't be needed.

https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1027: glyphToAdd = 0;
Yes, I understood your analysis. Please take a look at graphemesInCluster, which
is used in the loop termination condition: j < graphemesInCluster. Then check
countGraphemesInCluster - there a clusterEnd is specified as argument.
clusterEnd is using all characters of the run - I think the bounds check against
m_toIndex should be in line 1016, where clusterEnd is calculated. Then there is
no need for extra special casing during the loop.

[Habib Virji, 2014-09-30 14:01:24]

"Yes, I understood your analysis. Please take a look at graphemesInCluster,
which
is used in the loop termination condition: j < graphemesInCluster. Then check
countGraphemesInCluster - there a clusterEnd is specified as argument.
clusterEnd is using all characters of the run - I think the bounds check against
m_toIndex should be in line 1016, where clusterEnd is calculated. Then there is
no need for extra special casing during the loop."

Thanks for quick response. Yes in my previous comment, i was referring to
clusterEnd.

clusterEnd current condition for rtl is ==> clusterEnd = currentCharacterIndex;

With your suggested change ==> clusterEnd = currentCharacterIndex >= m_toIndex ?
(What should be the index value here) : currentCharacterIndex;

countGraphemeesInCluster takes both clusterStart and clusterEnd. Here if
clusterEnd is m_toIndex, it will not work. I am not sure what value will
correspond here for clusterEnd.

[behdad_google, 2014-09-30 14:10:50]

behdad@google.com changed reviewers:
+ behdad@google.com

[behdad_google, 2014-09-30 14:10:51]

lgtm after applying Dominik's feedback.

[Dominik Röttsches, 2014-10-01 07:35:35]

https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1027: glyphToAdd = 0;
I looked at this a bit more closely again - this fix works, but we would still
be generating too large GlyphBuffers. We just keep filling the GlyphBuffer with
0 glyphs, for which no emphasis marks get drawn down the line. This can be okay
as a first iteration - then we can put a FIXME here and say that in fact the
glyphBuffer should not get populated with extra 0s, which get placed there
because we're not taking the m_fromIndex and m_toIndex into account correctly.

If we go for this kind of initial fix: The opposite test cases - and I would ask
you to try that - can be constructed, too: Where the ellipsis is drawn on the
other side, and there is an emphasis drawn over it because we're not taking
m_fromIndex into account either. Then the fix would need to be extended for
that.

The right way to fix this, however, is similar to what the 
fillGlyphBufferFromHarfBuzzRun method is doing: When we're still outside the
bounds of [m_fromIndex, m_toIndex] we should shift the start offset of the glyph
buffer until we reach the start position, and only then add glyphs to it that
are within those bounds. That way the glyphBuffer size is constrained to
actually placing glyphs in it for characters that are within the [m_fromIndex,
m_toIndex] range.

I appreciate your analysis of the issue, and you found the root cause. That's
great -  It'd be great now if you could fix the problem at the source.

Please note that Emil landed some changes to HarfBuzzShaper in
https://codereview.chromium.org/607483002/ on splitting advances and offsets -
make sure you are rebasing on top of that one.

[Habib Virji, 2014-10-01 07:38:47]

On 2014/10/01 07:35:35, Dominik Röttsches wrote:
>
https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
> File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):
> 
>
https://codereview.chromium.org/613193002/diff/20001/Source/platform/fonts/ha...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1027: glyphToAdd = 0;
> I looked at this a bit more closely again - this fix works, but we would still
> be generating too large GlyphBuffers. We just keep filling the GlyphBuffer
with
> 0 glyphs, for which no emphasis marks get drawn down the line. This can be
okay
> as a first iteration - then we can put a FIXME here and say that in fact the
> glyphBuffer should not get populated with extra 0s, which get placed there
> because we're not taking the m_fromIndex and m_toIndex into account correctly.
> 
> If we go for this kind of initial fix: The opposite test cases - and I would
ask
> you to try that - can be constructed, too: Where the ellipsis is drawn on the
> other side, and there is an emphasis drawn over it because we're not taking
> m_fromIndex into account either. Then the fix would need to be extended for
> that.
> 
> The right way to fix this, however, is similar to what the 
> fillGlyphBufferFromHarfBuzzRun method is doing: When we're still outside the
> bounds of [m_fromIndex, m_toIndex] we should shift the start offset of the
glyph
> buffer until we reach the start position, and only then add glyphs to it that
> are within those bounds. That way the glyphBuffer size is constrained to
> actually placing glyphs in it for characters that are within the [m_fromIndex,
> m_toIndex] range.
> 
> I appreciate your analysis of the issue, and you found the root cause. That's
> great -  It'd be great now if you could fix the problem at the source.
> 
> Please note that Emil landed some changes to HarfBuzzShaper in
> https://codereview.chromium.org/607483002/ on splitting advances and offsets -
> make sure you are rebasing on top of that one.

Thanks Dominik, make sense. Will work on the issue and update accordingly.

[Habib Virji, 2014-10-03 16:08:14]

Thanks Dominik for previous comment. Apologise it is taking time to resolve
this.

Issue with following Emil latest changes was fillGlyphBufferFromHarfBuzzRun
relies on offset. But apparently offset are not available in
fillGlyphBufferForTextEmphasis. 

If we try to restrict in between bound, then emphasis start on top of ellipsis.
As in the case of example text, index starts from 15 and goes till 0. Text start
from offset 11 and ends at 0. If we skip ellipsis, then emphasis start from top
of ellipsis but truncated, since glyphBuffer did not advance. 

I did try to make fillGlyphBufferForTextEmphasis use GlyphBufferWithOffset(with
required changes in Font,etc) but still it does not support offset values as
fillGlyphBufferFromHarfBuzzRun has. 

I have done a change to previous submission to store glyph offsets for ellipsis,
and then add in the glyphBuffer in one go. (It does not meet the result, but is
just an alternate to previous submit)

Will again try on Monday to update to use GlyphBufferWithOffset as it is only
alternate.

[Habib Virji, 2014-10-17 14:11:57]

@dominik: I had a look at the issue again. I am not making much progress than
patch#3.

Issue is basically, glyphAdv and currentAdvance when added for the first
character after it is in range of m_fromIndex and m_toIndex, it makes width
length correct but emphasis is placed on the start of the glyph and not at the
end.

This is an example of how it looks:
 *  *** 
...1234

That's the reason as in patch#3 i introduced adding in glyphBuffer, that works
correctly with 0.  

Let me know if it can be pushed in with fixme?

[Habib Virji, 2014-10-20 14:48:34]

habib.virji@samsung.com changed reviewers:
- eae@chromium.org

[Habib Virji, 2014-10-21 15:45:45]

On 2014/10/17 14:11:57, Habib Virji wrote:
> @dominik: I had a look at the issue again. I am not making much progress than
> patch#3.
> 
> Issue is basically, glyphAdv and currentAdvance when added for the first
> character after it is in range of m_fromIndex and m_toIndex, it makes width
> length correct but emphasis is placed on the start of the glyph and not at the
> end.
> 
> This is an example of how it looks:
>  *  *** 
> ...1234
> 
> That's the reason as in patch#3 i introduced adding in glyphBuffer, that works
> correctly with 0.  
> 
> Let me know if it can be pushed in with fixme?

@dominik:PTAL

[Habib Virji, 2014-10-22 13:00:32]

@behdad: have tried pinging dominik. Is it okay to land the issue?

[behdad_google, 2014-10-22 19:12:59]

On 2014/10/22 13:00:32, Habib Virji wrote:
> @behdad: have tried pinging dominik. Is it okay to land the issue?

No.  Please wait for him.

[Habib Virji, 2014-11-06 14:24:31]

@dominik: ping

[Dominik Röttsches, 2014-11-13 09:01:55]

I was out of office for three weeks, got back today, will take a closer look as
soon as I find the time.

[Habib Virji, 2014-11-13 15:01:22]

On 2014/11/13 09:01:55, Dominik Röttsches wrote:
> I was out of office for three weeks, got back today, will take a closer look
as
> soon as I find the time.

Thanks Dominik. C

[Habib Virji, 2014-11-13 15:02:01]

On 2014/11/13 09:01:55, Dominik Röttsches wrote:
> I was out of office for three weeks, got back today, will take a closer look
as
> soon as I find the time.

Thanks Dominik. 

CL needs update to the latest master, but if changes are okay will add FIXME and
update to latest master.

[Dominik Röttsches, 2014-11-14 10:10:47]

There are some improvements in this patch, in that you're accumulating the
initial advance now, that's good. But we're still adding too many glyphs. I
believe, you only partially addressed my review comments.

Also, in general, please always rebase before asking for a review since there
maybe unexpected problems in rebasing which may cost reviewer's time. Like in
this case, there has been another GlyphBuffer semantics change which this patch
is probably affected by.

What's your response to my previous request to extend the test case?

> If we go for this kind of initial fix: The opposite test cases - and I would
ask
you to try that - can be constructed, too: Where the ellipsis is drawn on the
other side, and there is an emphasis drawn over it because we're not taking
m_fromIndex into account either. Then the fix would need to be extended for
that.

More detailed comments below.

https://codereview.chromium.org/613193002/diff/40001/LayoutTests/fast/text/em...
File LayoutTests/fast/text/emphasis-ellipsis-complextext.html (right):

https://codereview.chromium.org/613193002/diff/40001/LayoutTests/fast/text/em...
LayoutTests/fast/text/emphasis-ellipsis-complextext.html:20: <div dir="rtl">abcd
efg hijk<div>
Sorry, now the direction is redundant (since it's already in the general DIV
CSS). What I meant: If possible, we need the ellipsis be placed on either side,
so once for RTL once for LTR. So that we can cover the m_fromIndex and m_toIndex
case in the test.

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:989: bool isRTL = m_run.rtl();
I wouldn't create a redundant copy here.

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:990: float glyphAdv = 0;
Please do not use abbreviations in variable naming.
You probably want to call this "advanceSoFar" as in the non-emphasis version of
the function, or better "initialAdvance".

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1000: clusterStart =
currentRun->startIndex() + (isRTL ? currentRun->numCharacters() :
glyphToCharacterIndexes[0]);
As far as I can see, this is functionally equivalent to the old code. Why touch
this?

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1014: glyphBuffer->add(0,
currentRun->fontData(), glyphAdv);
Instead of already adding a glyph here, the first glyph should be shifted by the
value in "initialAdvance".

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1020: uint16_t clusterEnd =
isRTL ? currentCharacterIndex :
Same as above, code is functionally identical to the previous code, why
reformat?

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1031:
glyphBuffer->add(glyphToAdd, currentRun->fontData(), glyphAdvanceX);
So you'd need to use your accumulated initial advance here, then reset it to 0.

[Habib Virji, 2014-11-24 11:52:31]

Thanks dominik for reviewing and sorry for delay in replying back as I was on
holiday last 2 weeks.

Reason for adding empty glyph is position of emphasis not correctly placed and
is explained below for line 1014. 

Apologise for not updating to latest since it was quite for sometime in
pipeline, so did not update frequently.

I did not fathom correctly test case you asked for, have understood now from
your comments, will update it.

https://codereview.chromium.org/613193002/diff/40001/LayoutTests/fast/text/em...
File LayoutTests/fast/text/emphasis-ellipsis-complextext.html (right):

https://codereview.chromium.org/613193002/diff/40001/LayoutTests/fast/text/em...
LayoutTests/fast/text/emphasis-ellipsis-complextext.html:20: <div dir="rtl">abcd
efg hijk<div>
Make sense, will try to upload a test covering the issue.

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:989: bool isRTL = m_run.rtl();
Reason was it is used quite a lot below. Would remove it if not relevant.

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:990: float glyphAdv = 0;
Will update to initialAdvance.

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1000: clusterStart =
currentRun->startIndex() + (isRTL ? currentRun->numCharacters() :
glyphToCharacterIndexes[0]);
Agreed. The change was to ease in following code, will remove it.

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1014: glyphBuffer->add(0,
currentRun->fontData(), glyphAdv);
That's the issue I struggled with. Adding initialAdvance to the first glyph
results in emphasis mark on top of middle of the last 4 characters. As show
below, first 3 character glyph advances are stored in initialAdvance, but when
adding emphasis mark on top of first arabic character, it places it on the
middle of last 4 position (3 dots and 1 arabic character). Due to this reason I
had to add empty glyph for ellipsis to position, emphasis mark correctly.

For example:
 .  .. 
...الخ

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1020: uint16_t clusterEnd =
isRTL ? currentCharacterIndex :
On 2014/11/14 10:10:47, Dominik Röttsches wrote:
> Same as above, code is functionally identical to the previous code, why
> reformat?

Acknowledged.

https://codereview.chromium.org/613193002/diff/40001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1031:
glyphBuffer->add(glyphToAdd, currentRun->fontData(), glyphAdvanceX);
This is what i tried, but due to wrong position of emphasis mark, did not try
out.

[Habib Virji, 2014-11-24 16:48:26]

@Dominik: Closing this CL as not able to update it. Created new CL:
https://codereview.chromium.org/753233002/

[Dominik Röttsches, 2014-11-24 16:54:32]

On 2014/11/24 16:48:26, Habib Virji wrote:
> @Dominik: Closing this CL as not able to update it. Created new CL:
> https://codereview.chromium.org/753233002/

I do not understand why you were unable to update this CL. Moved files do not
cause a restriction to update a CL and creating a new CL makes it harder for the
reviewer to follow and identify changes from previous revisions of the work.

[Habib Virji, 2014-11-24 16:58:30]

On 2014/11/24 16:54:32, Dominik Röttsches wrote:
> On 2014/11/24 16:48:26, Habib Virji wrote:
> > @Dominik: Closing this CL as not able to update it. Created new CL:
> > https://codereview.chromium.org/753233002/
> 
> I do not understand why you were unable to update this CL. Moved files do not
> cause a restriction to update a CL and creating a new CL makes it harder for
the
> reviewer to follow and identify changes from previous revisions of the work.

Many apologise, issue is my computer does not have old branch and git CL patch
is failing to update because of change path and code is quite changed.

[Dominik Röttsches, 2014-11-24 17:02:19]

> Many apologise, issue is my computer does not have old branch and git CL patch
> is failing to update because of change path and code is quite changed.

The old branch is not required to update a CL. You can set a branch's associated
issue using "git cl issue <#>". You don't need the "git cl patch" tool for
updating a CL.

[Habib Virji, 2014-11-24 17:07:46]

On 2014/11/24 17:02:19, Dominik Röttsches wrote:
> > Many apologise, issue is my computer does not have old branch and git CL
patch
> > is failing to update because of change path and code is quite changed.
> 
> The old branch is not required to update a CL. You can set a branch's
associated
> issue using "git cl issue <#>". You don't need the "git cl patch" tool for
> updating a CL.

Thanks Dominik, was not aware about "git cl issue". This CL is also updated now.
Sorry for ignorance about the git cl issue usage.

[Dominik Röttsches, 2014-11-24 17:15:42]

Alright, thanks. Please close the new one and update the issue description here,
for example:

"Avoid drawing emphasis marks over ellipsis glyphs

Take m_fromIndex and m_toIndex into account and 
accumulate initial advance instead of using "0" non-drawing glyphs.

BUG=408544
R=dominik.rottsches@intel.com, behdad
TEST=fast/text/emphasis-ellipsis-complextext.html"

[Habib Virji, 2014-11-24 17:21:05]

Thanks. Closed new issue and updated the current one.

[Dominik Röttsches, 2014-11-24 17:24:52]

LGTM.

[Habib Virji, 2014-11-24 17:25:54]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-11-24 17:26:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613193002/80001

[commit-bot: I haz the power, 2014-11-24 19:26:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-24 19:26:11]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38211)

[Habib Virji, 2014-11-25 11:49:58]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-11-25 11:51:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613193002/100001

[commit-bot: I haz the power, 2014-11-25 12:30:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 12:30:30]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: LayoutTests/TestExpectations
  |diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
  |index
52a6105e15417fcdb1b363806c25e6d3d157d85e..7689c5c89078da29bd35ed962823fc8755637cd0
100644
  |--- a/LayoutTests/TestExpectations
  |+++ b/LayoutTests/TestExpectations
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
52a6105e15417fcdb1b363806c25e6d3d157d85e..7689c5c89078da29bd35ed962823fc8755637cd0
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -86,6 +86,10 @@ crbug.com/310679 [ Mac ]
fast/events/scale-and-scroll-iframe-window.html [ Image
 crbug.com/310679 [ Mac ] fast/frames/frame-set-rotation-hit.html [ Failure
ImageOnlyFailure Pass ]
 crbug.com/310679 [ Mac ] fast/frames/frame-set-scaling-hit.html [ Failure
ImageOnlyFailure Pass ]
 
+crbug.com/408544 fast/text/emphasis-ellipsis-complextext.html [ NeedsRebaseline
]
+crbug.com/408544
virtual/antialiasedtext/fast/text/emphasis-ellipsis-complextext.html [
NeedsRebaseline ]
+crbug.com/408544 virtual/textblob/fast/text/emphasis-ellipsis-complextext.html
[ NeedsRebaseline ]
+
 # Tests failing since applyPageScaleFactorInCompositor enabled
 crbug.com/225184 fast/repaint/relayout-fixed-position-after-scale.html [
ImageOnlyFailure ]

[Habib Virji, 2014-11-25 12:46:44]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-11-25 12:47:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613193002/120001

[commit-bot: I haz the power, 2014-11-25 12:48:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 12:48:15]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: LayoutTests/TestExpectations
  |diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
  |index
41327b8985ace31a6b598bfc4bed1e0e83852f44..3c197fe357acdffa5933e7774360878287fd745f
100644
  |--- a/LayoutTests/TestExpectations
  |+++ b/LayoutTests/TestExpectations
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
41327b8985ace31a6b598bfc4bed1e0e83852f44..3c197fe357acdffa5933e7774360878287fd745f
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -86,6 +86,10 @@ crbug.com/310679 [ Mac ]
fast/events/scale-and-scroll-iframe-window.html [ Image
 crbug.com/310679 [ Mac ] fast/frames/frame-set-rotation-hit.html [ Failure
ImageOnlyFailure Pass ]
 crbug.com/310679 [ Mac ] fast/frames/frame-set-scaling-hit.html [ Failure
ImageOnlyFailure Pass ]
 
+crbug.com/408544 fast/text/emphasis-ellipsis-complextext.html [ NeedsRebaseline
]
+crbug.com/408544
virtual/antialiasedtext/fast/text/emphasis-ellipsis-complextext.html [
NeedsRebaseline ]
+crbug.com/408544 virtual/textblob/fast/text/emphasis-ellipsis-complextext.html
[ NeedsRebaseline ]
+
 # Tests failing since applyPageScaleFactorInCompositor enabled
 crbug.com/225184 fast/repaint/relayout-fixed-position-after-scale.html [
ImageOnlyFailure ]

[Habib Virji, 2014-11-25 12:54:38]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-11-25 12:55:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613193002/140001

[commit-bot: I haz the power, 2014-11-25 12:55:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 12:55:39]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: LayoutTests/TestExpectations
  |diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
  |index
41327b8985ace31a6b598bfc4bed1e0e83852f44..b45cdb48336ef43af00ffdb04a48d6ef13e59823
100644
  |--- a/LayoutTests/TestExpectations
  |+++ b/LayoutTests/TestExpectations
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
41327b8985ace31a6b598bfc4bed1e0e83852f44..b45cdb48336ef43af00ffdb04a48d6ef13e59823
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1400,3 +1400,7 @@ crbug.com/434690
virtual/implsidepainting/inspector/tracing/worker-js-profile.ht
 crbug.com/435342 media/video-seeking.html [ Pass Failure ]
 
 crbug.com/435525 [ Mac Debug ] svg/W3C-SVG-1.1/struct-frag-02-t.svg [
NeedsRebaseline ]
+
+crbug.com/408544 fast/text/emphasis-ellipsis-complextext.html [ NeedsRebaseline
]
+crbug.com/408544
virtual/antialiasedtext/fast/text/emphasis-ellipsis-complextext.html [
NeedsRebaseline ]
+crbug.com/408544 virtual/textblob/fast/text/emphasis-ellipsis-complextext.html
[ NeedsRebaseline ]

[Habib Virji, 2014-11-25 13:19:41]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-11-25 13:19:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613193002/140001

[commit-bot: I haz the power, 2014-11-25 13:19:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 13:19:58]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: LayoutTests/TestExpectations
  |diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
  |index
41327b8985ace31a6b598bfc4bed1e0e83852f44..b45cdb48336ef43af00ffdb04a48d6ef13e59823
100644
  |--- a/LayoutTests/TestExpectations
  |+++ b/LayoutTests/TestExpectations
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
41327b8985ace31a6b598bfc4bed1e0e83852f44..b45cdb48336ef43af00ffdb04a48d6ef13e59823
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1400,3 +1400,7 @@ crbug.com/434690
virtual/implsidepainting/inspector/tracing/worker-js-profile.ht
 crbug.com/435342 media/video-seeking.html [ Pass Failure ]
 
 crbug.com/435525 [ Mac Debug ] svg/W3C-SVG-1.1/struct-frag-02-t.svg [
NeedsRebaseline ]
+
+crbug.com/408544 fast/text/emphasis-ellipsis-complextext.html [ NeedsRebaseline
]
+crbug.com/408544
virtual/antialiasedtext/fast/text/emphasis-ellipsis-complextext.html [
NeedsRebaseline ]
+crbug.com/408544 virtual/textblob/fast/text/emphasis-ellipsis-complextext.html
[ NeedsRebaseline ]

[Habib Virji, 2014-11-25 13:22:33]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-11-25 13:22:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613193002/160001

[commit-bot: I haz the power, 2014-11-25 13:52:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 13:52:10]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: LayoutTests/TestExpectations
  |diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
  |index
41327b8985ace31a6b598bfc4bed1e0e83852f44..eb553d4030b739f6be64823956cf7236c3ff5b78
100644
  |--- a/LayoutTests/TestExpectations
  |+++ b/LayoutTests/TestExpectations
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
41327b8985ace31a6b598bfc4bed1e0e83852f44..eb553d4030b739f6be64823956cf7236c3ff5b78
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1,3 +1,7 @@
+crbug.com/408544 fast/text/emphasis-ellipsis-complextext.html [ NeedsRebaseline
]
+crbug.com/408544
virtual/antialiasedtext/fast/text/emphasis-ellipsis-complextext.html [
NeedsRebaseline ]
+crbug.com/408544 virtual/textblob/fast/text/emphasis-ellipsis-complextext.html
[ NeedsRebaseline ]
+
 crbug.com/434560
virtual/slimmingpaint/fast/table/td-width-fifty-percent-regression.html [
ImageOnlyFailure ]
 
 # This directly has manual tests that don't have to run with run-webkit-tests

[Habib Virji, 2014-11-25 14:05:27]

The CQ bit was checked by habib.virji@samsung.com

[Habib Virji, 2014-11-25 14:24:17]

The CQ bit was unchecked by habib.virji@samsung.com

[Habib Virji, 2014-11-25 14:29:51]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-11-25 14:55:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613193002/200001

[commit-bot: I haz the power, 2014-11-25 14:56:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 14:56:02]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: LayoutTests/TestExpectations
  |diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
  |index
41327b8985ace31a6b598bfc4bed1e0e83852f44..037368c7e758f0b7951cd012416f2e37bb6f860e
100644
  |--- a/LayoutTests/TestExpectations
  |+++ b/LayoutTests/TestExpectations
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
41327b8985ace31a6b598bfc4bed1e0e83852f44..037368c7e758f0b7951cd012416f2e37bb6f860e
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1,3 +1,7 @@
+crbug.com/408544 fast/text/emphasis-ellipsis-complextext.html [ NeedsRebaseline
]
+crbug.com/408544 [ Win Mac ]
virtual/antialiasedtext/fast/text/emphasis-ellipsis-complextext.html [
NeedsRebaseline ]
+crbug.com/408544 virtual/textblob/fast/text/emphasis-ellipsis-complextext.html
[ NeedsRebaseline ]
+
 crbug.com/434560
virtual/slimmingpaint/fast/table/td-width-fifty-percent-regression.html [
ImageOnlyFailure ]
 
 # This directly has manual tests that don't have to run with run-webkit-tests

[Habib Virji, 2014-11-25 15:15:52]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-11-25 15:16:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613193002/220001

[commit-bot: I haz the power, 2014-11-25 17:02:32]

Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185955

[Habib Virji, 2015-02-18 14:09:22]

Patchset #13 (id:240001) has been deleted