Don't overflow in LayoutText::width() on a huge length.

Don't overflow in LayoutText::width() on a huge length.

BUG=647820, 641261
Committed: https://crrev.com/197c495edac589ae3279e7a02a7248f4adb12934
Cr-Commit-Position: refs/heads/master@{#419391}

[wkorman, 2016-09-17 00:17:22]

wkorman@chromium.org changed reviewers:
+ eae@chromium.org, kojii@chromium.org

[wkorman, 2016-09-17 00:17:23]

https://codereview.chromium.org/2345633007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2345633007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1590: // TODO(wkorman):
Incorporate constraints around |from| and |len| in the preceding variant of
width() here.
Seeking feedback on this. AFAICT this method variant is also public, and is
exposed in api/LineLayoutText.h, and callsites could conceivably pass in the
same invalid data we guard against in the preceding method variant.

So we could move the shared logic used here and above into a widthInternal()
method and have them both enact the constraints, but wanted to hear your
thoughts and can do in separate change in any case.

[eae, 2016-09-17 08:35:51]

LGTM

https://codereview.chromium.org/2345633007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2345633007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1590: // TODO(wkorman):
Incorporate constraints around |from| and |len| in the preceding variant of
width() here.
On 2016/09/17 00:17:22, wkorman wrote:
> Seeking feedback on this. AFAICT this method variant is also public, and is
> exposed in api/LineLayoutText.h, and callsites could conceivably pass in the
> same invalid data we guard against in the preceding method variant.
> 
> So we could move the shared logic used here and above into a widthInternal()
> method and have them both enact the constraints, but wanted to hear your
> thoughts and can do in separate change in any case.

The other method calls this one with style(firstLine)->font()) as the Font so
you only need the check here.

[wkorman, 2016-09-17 21:16:42]

https://codereview.chromium.org/2345633007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2345633007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1590: // TODO(wkorman):
Incorporate constraints around |from| and |len| in the preceding variant of
width() here.
On 2016/09/17 08:35:51, eae wrote:
> On 2016/09/17 00:17:22, wkorman wrote:
> > Seeking feedback on this. AFAICT this method variant is also public, and is
> > exposed in api/LineLayoutText.h, and callsites could conceivably pass in the
> > same invalid data we guard against in the preceding method variant.
> > 
> > So we could move the shared logic used here and above into a widthInternal()
> > method and have them both enact the constraints, but wanted to hear your
> > thoughts and can do in separate change in any case.
> 
> The other method calls this one with style(firstLine)->font()) as the Font so
> you only need the check here.

That's true, but this method is exposed and can be called directly from other
sources (and there are a couple already though they are in line layout code).

So IMO we should guard here as well and just structure so that we don't guard
2x, but we can discuss in separate change.

[wkorman, 2016-09-17 21:17:05]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-09-17 21:17:06]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2345633007/#ps40001
(title: "Sync to head.")

[commit-bot: I haz the power, 2016-09-17 21:17:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 23:23:46]

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

[commit-bot: I haz the power, 2016-09-17 23:23:47]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wkorman, 2016-09-18 00:49:13]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-09-18 00:49:14]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2345633007/#ps60001
(title: "Tweak test ranges for Mac results.")

[commit-bot: I haz the power, 2016-09-18 00:49:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-18 02:25:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-18 02:31:59]

Description was changed from

==========
Don't overflow in LayoutText::width() on a huge length.

BUG=647820,641261
==========

to

==========
Don't overflow in LayoutText::width() on a huge length.

BUG=647820,641261

Committed: https://crrev.com/197c495edac589ae3279e7a02a7248f4adb12934
Cr-Commit-Position: refs/heads/master@{#419391}
==========

[commit-bot: I haz the power, 2016-09-18 02:32:00]

Patchset 4 (id:??) landed as
https://crrev.com/197c495edac589ae3279e7a02a7248f4adb12934
Cr-Commit-Position: refs/heads/master@{#419391}