Making changes to previous patch with revision Number: 174133

Cursor overlaps the text inside New message body in GMail after r174133

Fix regression after r174133 that was causing the cursor to overlap the text
inside "New message" body in GMail. Considering RIGHT-TO-LEFT MARK and
RIGHT-TO-LEFT OVERRIDE in complex text was causing trouble on Mac, not sure of
exact reason.

R=eae@chromium.org
BUG=377445
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178901

[h.joshi, 2014-07-03 10:12:47]

@Emil: PTAL
Reverting previous patch, will start looking at why on Mac only issue is
occurring and re-submit new patch.

[eae, 2014-07-07 15:04:05]

On 2014/07/03 10:12:47, h.joshi wrote:
> @Emil: PTAL
> Reverting previous patch, will start looking at why on Mac only issue is
> occurring and re-submit new patch.

LGTM

Please include the revision numbers (instead of the code review links) in the
title and description.

[eae, 2014-07-07 15:04:30]

On 2014/07/07 15:04:05, eae wrote:
> On 2014/07/03 10:12:47, h.joshi wrote:
> > @Emil: PTAL
> > Reverting previous patch, will start looking at why on Mac only issue is
> > occurring and re-submit new patch.
> 
> LGTM
> 
> Please include the revision numbers (instead of the code review links) in the
> title and description.

...and ideally a description as to what it broke.

[h.joshi, 2014-07-09 22:22:01]

@Emil: Made changes to previous patch after testing, with new 
changes issue on Mac is not faced and issue 256333 is also working.

[h.joshi, 2014-07-11 05:46:32]

@Emil: Considering RIGHT-TO-LEFT MARK and RIGHT-TO-LEFT OVERRIDE in complex were
causing issue in Mac, not very sure why 
it was causing issue in Mac only.

[Inactive, 2014-07-23 12:52:37]

h.joshi: Your changelog is not great, I would propose:

"""
Cursor overlaps the text inside New message body in GMail after r174133

Fix regression after r174133 that was causing the cursor to overlap the text
inside "New message" body in GMail. Considering RIGHT-TO-LEFT MARK and
RIGHT-TO-LEFT OVERRIDE in complex text was causing trouble on Mac, not sure of
exact reason.

R=eae@chromium.org
BUG=377445
"""

Always make sure to wrap your lines at 80 chars. Once you fix the Changelog, I
believe you can land this as eae already approved the change % changelog fixes.
Please do this ASAP as this is an important regression and this has been going
on for a while now...

[h.joshi, 2014-07-23 12:59:27]

@Chris: Thank you, did required changes
Was not checking Open Source, was tied to other task.
Will submit changes now.

[h.joshi, 2014-07-23 12:59:40]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-07-23 13:00:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/370523003/20001

[commit-bot: I haz the power, 2014-07-23 13:00:50]

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

[commit-bot: I haz the power, 2014-07-23 13:00:51]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1086.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
733c240fa2046910b0343962c1038b00f5f72b98..ece55dc76687244dc1d46dfd8a4d330a79fbb04a
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1086,3 +1086,7 @@ crbug.com/30536 [ Win7 ]
http/tests/security/contentSecurityPolicy/report-only.h
 
 crbug.com/390204 inspector/console/console-viewport-selection.html [ Pass
Failure ]
 
+# Need rebaseline for https://codereview.chromium.org/370523003/ patch which is
related to Ignorable Unicodes
+crbug.com/377445 fast/text/international/bidi-LDB-2-formatting-characters.html
[ NeedsRebaseline ]
+crbug.com/377445 fast/text/format-control.html [ NeedsRebaseline ]
+

[h.joshi, 2014-07-23 14:30:40]

When trying to update patch, getting following issue
Got error status from 'git show 593e65381a842e5bed3b0bffb41443775219b405'
trying to find solution for above git issue, will try to update and check-in
this patch by today

[Inactive, 2014-07-23 14:51:27]

On 2014/07/23 14:30:40, h.joshi wrote:
> When trying to update patch, getting following issue
> Got error status from 'git show 593e65381a842e5bed3b0bffb41443775219b405'
> trying to find solution for above git issue, will try to update and check-in
> this patch by today

Likely you messed up your git branch somehow. Could you try the following?
1. git checkout master
2. git reset --hard HEAD
3. git checkout -b 370523003
4. git cl patch 370523003
5. Resolve conflict on LayoutTests/TestExpectations by editing it
6. git add LayoutTests/TestExpectations
7. git commit
8. git cl upload && git cl try

[h.joshi, 2014-07-23 15:04:59]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-07-23 15:05:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/370523003/40001

[h.joshi, 2014-07-23 15:55:26]

The CQ bit was unchecked by h.joshi@samsung.com

[h.joshi, 2014-07-23 16:20:45]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-07-23 16:21:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/370523003/60001

[h.joshi, 2014-07-23 16:30:56]

The CQ bit was unchecked by h.joshi@samsung.com

[h.joshi, 2014-07-23 16:34:12]

@Chris: I feel Emil need to review patch as after his LGTM file was changed, 
I have updated files and trying bots.

@Emil: Pls review.

[eae, 2014-07-23 16:35:38]

Still LGTM

[Inactive, 2014-07-23 16:36:49]

On 2014/07/23 16:34:12, h.joshi wrote:
> @Chris: I feel Emil need to review patch as after his LGTM file was changed, 
> I have updated files and trying bots.

Which file has changed? I only see a minor rebase of TestExpectations.

[Inactive, 2014-07-23 16:37:25]

On 2014/07/23 16:36:49, Chris Dumez wrote:
> On 2014/07/23 16:34:12, h.joshi wrote:
> > @Chris: I feel Emil need to review patch as after his LGTM file was changed,

> > I have updated files and trying bots.
> 
> Which file has changed? I only see a minor rebase of TestExpectations.

Nevermind, Emil was super quick :)

[Inactive, 2014-07-23 16:37:43]

lgtm

[h.joshi, 2014-07-23 17:17:00]

@Emil/Chris: Thank you

[commit-bot: I haz the power, 2014-07-23 17:31:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16750)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18530)

[commit-bot: I haz the power, 2014-07-23 18:09:10]

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

[commit-bot: I haz the power, 2014-07-23 18:09:12]

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

[h.joshi, 2014-07-23 18:28:44]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-07-23 18:29:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/370523003/80001

[h.joshi, 2014-07-24 11:02:54]

Too much time is taken in win_gpu bot, yesterday it showed 450+ mins and 
now its showing 800+ mins to retry job.

[commit-bot: I haz the power, 2014-07-25 00:30:56]

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

[commit-bot: I haz the power, 2014-07-25 00:30:57]

Exceeded time limit waiting for builds to trigger.

[h.joshi, 2014-07-25 03:18:44]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-07-25 03:19:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/370523003/100001

[commit-bot: I haz the power, 2014-07-25 04:19:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-07-25 05:20:04]

Change committed as 178901

[behdad_google, 2014-07-25 14:55:27]

On 2014/07/25 05:20:04, I haz the power (commit-bot) wrote:
> Change committed as 178901

This change does NOT look good to me.  It doesn't make sense and no one
understand why it fixes the bug.  The fact that the description has "not sure of
exact reason." should have been enough to not allow it to go in.

Someone else wanted to commit the same:
https://codereview.chromium.org/392033006/

[Inactive, 2014-07-25 15:05:21]

On 2014/07/25 14:55:27, behdad_google wrote:
> On 2014/07/25 05:20:04, I haz the power (commit-bot) wrote:
> > Change committed as 178901
> 
> This change does NOT look good to me.  It doesn't make sense and no one
> understand why it fixes the bug.  The fact that the description has "not sure
of
> exact reason." should have been enough to not allow it to go in.
> 
> Someone else wanted to commit the same:
> https://codereview.chromium.org/392033006/

I agree that this needs to be investigated. We were getting pressure on the bug
to revert the h.joshi's original commit that caused the regression. h.joshi thus
submitted a partial revert until this can be figured out as the regression
looked pretty bad.