Set caret width based on device scale factor

Set caret width based on device scale factor

Moves caretWidth() function from LayoutObject to FrameView
and computes the width according to the device scale factor.

COMPONENT=LayoutObject, FrameView, Caret
BUG=655410
Review-Url: https://codereview.chromium.org/2683233005
Cr-Commit-Position: refs/heads/master@{#450160}
Committed: https://chromium.googlesource.com/chromium/src/+/3353063df1bbda55628a8c1e0a0d1f1289d80b78

[malaykeshav, 2017-02-09 21:46:09]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 21:46:43]

Dry run: 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, 2017-02-09 23:48:25]

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

[commit-bot: I haz the power, 2017-02-09 23:48:27]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2017-02-10 00:06:25]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 00:07:02]

Dry run: 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, 2017-02-10 00:11:45]

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

[commit-bot: I haz the power, 2017-02-10 00:11:46]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2017-02-10 00:50:19]

malaykeshav@chromium.org changed reviewers:
+ oshima@chromium.org

[malaykeshav, 2017-02-10 00:50:20]

PTAL

[malaykeshav, 2017-02-10 00:52:46]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 00:53:40]

Dry run: 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, 2017-02-10 00:59:27]

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

[commit-bot: I haz the power, 2017-02-10 00:59:28]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-02-10 19:55:16]

alternatively, you can add FrameView::getCaretWidth().

I'll leave it to owner's review.

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

https://codereview.chromium.org/2683233005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4548:
LayoutSize(caretWidth(frameView()->getHostWindow()), size().height()));
just get noce

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

https://codereview.chromium.org/2683233005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1181: x = maxX -
caretWidth(frameView()->getHostWindow());
can you just get once in this function?

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

https://codereview.chromium.org/2683233005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutText.cpp:769:
caretWidth(frameView()->getHostWindow()) / 2;
ditto

[malaykeshav, 2017-02-10 21:00:55]

Resolved comments

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

https://codereview.chromium.org/2683233005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4548:
LayoutSize(caretWidth(frameView()->getHostWindow()), size().height()));
On 2017/02/10 at 19:55:16, oshima wrote:
> just get noce

Done

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

https://codereview.chromium.org/2683233005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1181: x = maxX -
caretWidth(frameView()->getHostWindow());
On 2017/02/10 at 19:55:16, oshima wrote:
> can you just get once in this function?

Done

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

https://codereview.chromium.org/2683233005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutText.cpp:769:
caretWidth(frameView()->getHostWindow()) / 2;
On 2017/02/10 at 19:55:16, oshima wrote:
> ditto

Done

[malaykeshav, 2017-02-10 21:00:57]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 21:01:27]

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

[oshima, 2017-02-10 21:09:34]

lgtm with one q.

There's no layout test with caret?

[commit-bot: I haz the power, 2017-02-10 21:23:51]

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

[commit-bot: I haz the power, 2017-02-10 21:23:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[malaykeshav, 2017-02-10 21:50:13]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[malaykeshav, 2017-02-10 21:50:36]

On 2017/02/10 at 21:09:34, oshima wrote:
> lgtm with one q.
> 
> There's no layout test with caret?

Couldnt find one related to width. There were some related to color and height.

[commit-bot: I haz the power, 2017-02-10 21:51:08]

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

[malaykeshav, 2017-02-10 22:39:59]

malaykeshav@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[malaykeshav, 2017-02-10 22:40:00]

PTAL

[Xianzhu, 2017-02-10 22:53:55]

https://codereview.chromium.org/2683233005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2683233005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:104: static LayoutUnit
gCaretWidth(host->windowToViewportScalar(1));
Does host->windowToViewportScalar(1) always return the same value in a process?

[oshima, 2017-02-10 22:57:23]

https://codereview.chromium.org/2683233005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2683233005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:104: static LayoutUnit
gCaretWidth(host->windowToViewportScalar(1));
On 2017/02/10 22:53:55, Xianzhu wrote:
> Does host->windowToViewportScalar(1) always return the same value in a
process?

No, two tabs may have different scale factor.

[malaykeshav, 2017-02-10 23:02:32]

Patchset #2 (id:20001) has been deleted

[Xianzhu, 2017-02-10 23:04:13]

https://codereview.chromium.org/2683233005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2683233005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:104: static LayoutUnit
gCaretWidth(host->windowToViewportScalar(1));
On 2017/02/10 22:57:23, oshima wrote:
> On 2017/02/10 22:53:55, Xianzhu wrote:
> > Does host->windowToViewportScalar(1) always return the same value in a
> process?
> 
> No, two tabs may have different scale factor.

Then the above code is incorrect because the static variable is initialized only
once.

Please remove this function from LayoutObject.h/cpp and add

LayoutUnit FrameView::caretWidth() const {
  return getHostWindow()->windowToViewportScalar(1);
}

[malaykeshav, 2017-02-10 23:05:54]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 23:06:24]

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

[malaykeshav, 2017-02-10 23:18:08]

Patchset #3 (id:60001) has been deleted

[malaykeshav, 2017-02-10 23:19:00]

Description was changed from

==========
Set caret width based on device scale factor

Updates caretWidth() function to accept HostWindow as an argument to
scale caret width based on device scale factor.

COMPONENT=LayoutObject, LayoutObject subclasses, Caret
BUG=655410
==========

to

==========
Set caret width based on device scale factor

Moves caretWidth() function from LayoutObject to FrameView
and computes the width according to the device scale factor.

COMPONENT=LayoutObject, FrameView, Caret
BUG=655410
==========

[malaykeshav, 2017-02-10 23:19:04]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 23:19:42]

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

[malaykeshav, 2017-02-10 23:32:31]

Patchset #3 (id:80001) has been deleted

[malaykeshav, 2017-02-10 23:32:34]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 23:34:07]

Dry run: 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, 2017-02-11 01:22:54]

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

[commit-bot: I haz the power, 2017-02-11 01:22:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[malaykeshav, 2017-02-13 19:00:57]

Patchset #3 (id:100001) has been deleted

[malaykeshav, 2017-02-13 19:01:03]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 19:02:19]

Dry run: 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, 2017-02-13 19:05:00]

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

[commit-bot: I haz the power, 2017-02-13 19:05:01]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[malaykeshav, 2017-02-13 19:08:21]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 19:09:06]

Dry run: 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, 2017-02-13 20:46:51]

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

[commit-bot: I haz the power, 2017-02-13 20:46:52]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2017-02-13 21:11:04]

PTAL

[Xianzhu, 2017-02-13 21:36:00]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-13 21:36:01]

lgtm

[Xianzhu, 2017-02-13 21:36:01]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2683233005/#ps140001
(title: "Merge with ToT")

[commit-bot: I haz the power, 2017-02-13 21:37:02]

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, 2017-02-13 21:47:05]

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

[commit-bot: I haz the power, 2017-02-13 21:47:06]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[malaykeshav, 2017-02-13 21:47:17]

The CQ bit was checked by malaykeshav@chromium.org

[commit-bot: I haz the power, 2017-02-13 21:48:17]

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

[malaykeshav, 2017-02-13 21:53:49]

The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 21:54:36]

Dry run: 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, 2017-02-13 23:33:51]

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

[commit-bot: I haz the power, 2017-02-13 23:33:51]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2017-02-13 23:50:36]

The CQ bit was checked by malaykeshav@chromium.org

[malaykeshav, 2017-02-13 23:50:37]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2683233005/#ps160001
(title: "Sync with ToT again")

[commit-bot: I haz the power, 2017-02-13 23:51:47]

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, 2017-02-13 23:59:20]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487029836764460,
"parent_rev": "549d1501a66ed2ec0283149f65c71d64569183aa", "commit_rev":
"3353063df1bbda55628a8c1e0a0d1f1289d80b78"}

[commit-bot: I haz the power, 2017-02-13 23:59:58]

Description was changed from

==========
Set caret width based on device scale factor

Moves caretWidth() function from LayoutObject to FrameView
and computes the width according to the device scale factor.

COMPONENT=LayoutObject, FrameView, Caret
BUG=655410
==========

to

==========
Set caret width based on device scale factor

Moves caretWidth() function from LayoutObject to FrameView
and computes the width according to the device scale factor.

COMPONENT=LayoutObject, FrameView, Caret
BUG=655410

Review-Url: https://codereview.chromium.org/2683233005
Cr-Commit-Position: refs/heads/master@{#450160}
Committed:
https://chromium.googlesource.com/chromium/src/+/3353063df1bbda55628a8c1e0a0d...
==========

[commit-bot: I haz the power, 2017-02-14 00:00:00]

Committed patchset #5 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/3353063df1bbda55628a8c1e0a0d...