Itemize and layout text lazily in RenderTextWin.

Itemize and layout text lazily in RenderTextWin.

This matches what is done in render_text_linux.cc
and avoids doing unnecessary work unless needed.

BUG=none
TEST=Run chrome.exe with command-line --use-pure-views. Edit text in the omnibox. It should work as before.

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

[Alexei Svitkine (slow), 2011-11-22 18:55:19]

msw: review
sky: OWNERS review

[Alexei Svitkine (slow), 2011-11-28 19:32:29]

On 2011/11/22 18:55:19, Alexei Svitkine wrote:
> msw: review
> sky: OWNERS review

ping

[Alexei Svitkine (slow), 2011-11-28 20:24:55]

This change overlaps a little bit with:

http://codereview.chromium.org/8536047/

So it will need to be adjusted if that one goes in first, or vice versa.

[msw, 2011-11-28 21:15:45]

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:148: string_width_(0) {
Init needs_layout_

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:293: SelectionModel
RenderTextWin::LeftEndSelectionModel() {
Do we need EnsureLayout on these protected methods:
LeftEndSelectionModel, RightEndSelectionModel, GetSubstringBounds.
Otherwise, please |DCHECK(!needs_layout_);|

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:389: needs_layout_ = true;
Add a simple comment like "Layout is performed lazily as needed for
drawing/metrics."

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:392: void RenderTextWin::EnsureLayout() {
This should be below IndexOfAdjacentGrapheme to match decl order.

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:398: LayoutVisualText();
I think we actually need to reset |string_width_| here, or you could set
|string_width_ = 0| and then check |if (!runs_.empty())| for early-termination
at the top of LayoutVisualText.

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:598: SelectionModel
RenderTextWin::FirstSelectionModelInsideRun(
Should these private methods |DCHECK(!needs_layout_);|:
[First|Last]SelectionModelInsideRun, [Left|Right]SelectionModel

[sky, 2011-11-28 21:56:49]

I'm rubber stamping, but you need to wait for approval from Mike: LGTM

[Alexei Svitkine (slow), 2011-11-28 21:58:13]

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:148: string_width_(0) {
On 2011/11/28 21:15:46, msw wrote:
> Init needs_layout_

Embarrassing! Done.

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:293: SelectionModel
RenderTextWin::LeftEndSelectionModel() {
On 2011/11/28 21:15:46, msw wrote:
> Do we need EnsureLayout on these protected methods:
> LeftEndSelectionModel, RightEndSelectionModel, GetSubstringBounds.
> Otherwise, please |DCHECK(!needs_layout_);|

Done.

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:389: needs_layout_ = true;
On 2011/11/28 21:15:46, msw wrote:
> Add a simple comment like "Layout is performed lazily as needed for
> drawing/metrics."

Done.

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:392: void RenderTextWin::EnsureLayout() {
On 2011/11/28 21:15:46, msw wrote:
> This should be below IndexOfAdjacentGrapheme to match decl order.

Done.

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:398: LayoutVisualText();
On 2011/11/28 21:15:46, msw wrote:
> I think we actually need to reset |string_width_| here, or you could set
> |string_width_ = 0| and then check |if (!runs_.empty())| for early-termination
> at the top of LayoutVisualText.

Good catch! Done.

http://codereview.chromium.org/8633019/diff/1/ui/gfx/render_text_win.cc#newco...
ui/gfx/render_text_win.cc:598: SelectionModel
RenderTextWin::FirstSelectionModelInsideRun(
On 2011/11/28 21:15:46, msw wrote:
> Should these private methods |DCHECK(!needs_layout_);|:

As they currently are, they don't need it. However, I've made
[Left|Right]SelectionModel have the DCHECK() - explanation below.

> [First|Last]SelectionModelInsideRun

These shouldn't have it because they don't use any state from layout directly -
only parameters and return values of other functions, specifically
IndexOfAdjacentGrapheme(). That function does need layout, but does call
EnsureLayout().

Since they don't touch layout state (from the class) directly, they shouldn't
need to check it.

> [First|Last]SelectionModelInsideRun, [Left|Right]SelectionModel

These both get called only via entry points that do EnsureLayout(). Furthermore,
both call GetRunContainingPosition() near the start, which has the DCHECK().

But, since they end up using layout state following that call, I've added the
DCHECK()'s to these, to protect against refactoring changes.

[msw, 2011-11-28 22:18:44]

LGTM; thanks!

[commit-bot: I haz the power, 2011-11-29 15:30:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8633019/11001

[commit-bot: I haz the power, 2011-11-29 17:14:09]

Try job failure for 8633019-11001 (retry) on linux_rel for steps "browser_tests,
ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-29 17:34:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8633019/11001

[commit-bot: I haz the power, 2011-11-29 18:55:30]

Change committed as 111983