Fixes 105893

Upon calling RenderText::SelectRange, the selection style should be applied.

The layout is generated via IsCursorablePosition, however it seems as if it needs to be regenerated once the selection model is set.

The bug manifests itself as the text selected text being the wrong color when the omnibox text is selected via Ctrl-L

BUG=105893
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112704

[pkotwicz, 2011-11-30 23:01:59]

This fixes a bug with the selected text color (Bug 105893).
Please send me your comments, I am not very familiar with this region of code.

[msw, 2011-11-30 23:24:33]

Actually, this looks like a regression from Xiaomei's crrev.com/112188, which
removed the UpdateLayout call amidst moving RenderText::SetSelectionModel.
It looks like that change was intentional (she includes a note in the CL
description, but doesn't say why). Perhaps that was a mistake?
Replacing that call to UpdateLayout should do the trick, add a comment for good
measure, something like "Selection updates require layout to show the selected
text color.".

[xji, 2011-12-01 08:48:04]

On 2011/11/30 23:24:33, msw wrote:
> Actually, this looks like a regression from Xiaomei's crrev.com/112188, which
> removed the UpdateLayout call amidst moving RenderText::SetSelectionModel.
> It looks like that change was intentional (she includes a note in the CL
> description, but doesn't say why). Perhaps that was a mistake?
> Replacing that call to UpdateLayout should do the trick, add a comment for
good
> measure, something like "Selection updates require layout to show the selected
> text color.".

Michael is right, I removed UpdateLayout() from SetSelectionModel() in r112188.
I tested with ctrl-l, but I was not using URL, and apparently, the highlight is
correct.

The patch fixes the problem.
But my question is: 
why SelectRange()/SetSelectionModel() changes the selected text's foreground
color scheme, which requires a re-layout?
Isn't the color change should be triggered by applying new style (a new
foreground), which will trigger re-layout?

In another word, when pressing ctrl-l to trigger select all, why it does not
trigger apply new style (for the color scheme changes)? Functionality wise, I do
not think SelectRange/SetSelectionModel should trigger style change.

It triggered relayout before is because highlight is mixed in layout previously.

[msw, 2011-12-01 19:07:50]

Good point, if we can update color and redraw without going through layout,
that'd be a good performance enhancement. We shouldn't re-layout as each
character is selected in a drag-selection, etc.

This requires a bit of work, as color is applied via StyleRanges which break the
text into runs (RenderText::ApplyCompositionAndSelectionStyles). I think we'd
need to disambiguate color (and other styles that shouldn't break text runs)
from pre-layout style settings.

[xji, 2011-12-01 20:29:10]

On 2011/12/01 19:07:50, msw wrote:
> Good point, if we can update color and redraw without going through layout,
> that'd be a good performance enhancement. We shouldn't re-layout as each
> character is selected in a drag-selection, etc.
> 
> This requires a bit of work, as color is applied via StyleRanges which break
the
> text into runs (RenderText::ApplyCompositionAndSelectionStyles). I think we'd
> need to disambiguate color (and other styles that shouldn't break text runs)
> from pre-layout style settings.

Good point, and it brings it even further.
In my previous reply, I was not that far yet (sorry for the confusion). I meant
that SelectionRange() and SetSelectionModel() should not trigger layout. Color
change is done by setting style which should trigger layout.
(probably we did not UpdateLayout somewhere that we should).

[pkotwicz, 2011-12-01 22:07:59]

As Michael suggested, I am putting back UpdateLayer in SetSelectionModel and
will tackle the perf problem in a separate patch

[msw, 2011-12-01 22:13:57]

LGTM

[xji, 2011-12-01 22:37:52]

On 2011/12/01 22:13:57, msw wrote:
> LGTM

LGTM.
Thanks for explaining the problem. I missed the ApplySelectionStyle() part in
layout.
Hopefully, after http://codereview.chromium.org/8725002, we could get rid of
ApplyCompositionAndSelectionStyle() from Pango and get rid of this relayout too.

[pkotwicz, 2011-12-01 23:18:03]

Cool, I created a bug so that we don't lose track of reremoving UpdateLayout
from SetSelectionModel

[sky, 2011-12-02 00:02:00]

LGTM

[commit-bot: I haz the power, 2011-12-02 14:27:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/8761005/7001

[commit-bot: I haz the power, 2011-12-02 15:46:10]

Change committed as 112704