Scrollbar width is set using thickness to compute correct rect size

Scrollbar width is set using thickness to compute correct rect size

BUG=637264
COMPONENT=ScrollableArea, Webkit
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/5556b4bf5c650643cc5aea0f17a72b1e9e29e316
Cr-Commit-Position: refs/heads/master@{#422210}

[malaykeshav, 2016-09-27 22:51:08]

Description was changed from

==========
Scrollbar width is set using thickness to compute correct rect size

BUG=637264
COMPONENT=ScrollableArea, Webkit
==========

to

==========
Scrollbar width is set using thickness to compute correct rect size

BUG=637264
COMPONENT=ScrollableArea, Webkit
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[malaykeshav, 2016-09-27 22:51:41]

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

[commit-bot: I haz the power, 2016-09-27 22:52:04]

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

[malaykeshav, 2016-09-27 22:52:18]

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

[malaykeshav, 2016-09-27 22:52:18]

PTAL Updates how scrollbar width is set during dsf change.

[oshima, 2016-09-27 23:29:41]

could you please add a layout test for this?

I can tell you where and how, so please ping me when you have time.

[commit-bot: I haz the power, 2016-09-28 00:29:40]

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

[commit-bot: I haz the power, 2016-09-28 00:29:41]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2016-09-28 23:03:27]

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

[commit-bot: I haz the power, 2016-09-28 23:03:52]

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

[malaykeshav, 2016-09-28 23:04:58]

The CQ bit was unchecked by malaykeshav@chromium.org

[malaykeshav, 2016-09-28 23:05:15]

Patchset #2 (id:20001) has been deleted

[malaykeshav, 2016-09-28 23:05:32]

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

[commit-bot: I haz the power, 2016-09-28 23:06:00]

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, 2016-09-29 00:06:40]

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

[commit-bot: I haz the power, 2016-09-29 00:06:40]

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

[malaykeshav, 2016-09-29 01:07:39]

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

[commit-bot: I haz the power, 2016-09-29 01:08: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, 2016-09-29 02:31:55]

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

[commit-bot: I haz the power, 2016-09-29 02:31:56]

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, 2016-09-29 17:28:36]

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

[commit-bot: I haz the power, 2016-09-29 17:28:51]

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, 2016-09-29 18:06:49]

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

[commit-bot: I haz the power, 2016-09-29 18:06:50]

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

[malaykeshav, 2016-09-29 22:05:48]

Patchset #6 (id:120001) has been deleted

[malaykeshav, 2016-09-29 22:05:53]

Patchset #5 (id:100001) has been deleted

[malaykeshav, 2016-09-29 22:05:59]

Patchset #4 (id:80001) has been deleted

[malaykeshav, 2016-09-29 22:09:38]

Patchset #4 (id:140001) has been deleted

[malaykeshav, 2016-09-29 22:10:12]

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

[commit-bot: I haz the power, 2016-09-29 22:10:43]

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

[malaykeshav, 2016-09-29 22:53:41]

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

[malaykeshav, 2016-09-29 22:53:42]

PTAL.

[Xianzhu, 2016-09-29 23:00:04]

Please include test expectations on one platform.

fwiw you can create test expectations by running:
run-webkit-tests
fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html
--reset-results

https://codereview.chromium.org/2377453004/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2377453004/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:256: crbug.com/637264 [ Linux
Mac Win ] fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html  [
NeedsRebaseline ]
The new test needs rebaseline on all platforms, so you can just omit the [ Linux
Mac Win ] part.

[commit-bot: I haz the power, 2016-09-29 23:33:09]

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

[commit-bot: I haz the power, 2016-09-29 23:33:10]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2016-09-30 01:38:20]

On 2016/09/29 at 23:00:04, wangxianzhu wrote:
> Please include test expectations on one platform.
> 
> fwiw you can create test expectations by running:
> run-webkit-tests
fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html
--reset-results
> 
>
https://codereview.chromium.org/2377453004/diff/160001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2377453004/diff/160001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:256: crbug.com/637264 [ Linux
Mac Win ] fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html  [
NeedsRebaseline ]
> The new test needs rebaseline on all platforms, so you can just omit the [
Linux Mac Win ] part.

Done

[malaykeshav, 2016-09-30 01:38:28]

Patchset #5 (id:180001) has been deleted

[malaykeshav, 2016-09-30 01:38:44]

Patchset #5 (id:200001) has been deleted

[malaykeshav, 2016-09-30 01:44:12]

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

[commit-bot: I haz the power, 2016-09-30 01:44:37]

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, 2016-09-30 01:47:42]

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

[commit-bot: I haz the power, 2016-09-30 01:47:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Xianzhu, 2016-09-30 03:36:14]

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html
(right):

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:3:
<head>
Normally we omit <html>, <head>, <body> in layout tests.

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:5:
window.enablePixelTesting = true;
Please remove the above line. It's no use here.

You may have seen some tests using it. These tests also includes some script
files which will read the variable.

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:7:
setTimeout(function() {testRunner.notifyDone();}, 0);
Is the setTimeout necessary?

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:17:
if (window.testRunner) {
Line 16 should be also protected by if (window.testRunner).
Or you can just add 'if (window.testRunner)' before line 22 and remove all other
guards.

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:22:
window.addEventListener("load", startTest, false);
You can use shorter form:
  onload = startTest;
or replace line 15 with
  onload = function() {

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:28:
<div style="width:400px; height: 200px; background-color:red"></div>
Remove "background-color: red". Normally we use red to indicate failure.

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:31:
</html>
We prefer reference tests to pixel tests because the former can avoid pixel and
text expectations and platform baselines.

Just create a file named
scrollbar-appearance-decrease-device-scale-factor-expected.html beside this file
containing the following contents:
<!DOCTYPE html>
<div id="div" style="width:200px; height:120px; overflow:auto">
  <br>This should be a scrollable text box with horizontal and vertical scroll.
  <div style="width:400px; height: 200px; background-color:red"></div>
</div>

The layout test runner script will run both html file and compare their pixel
outputs. The test passes if the two files generate the same output.

You can keep the other test as a pixel test.

[malaykeshav, 2016-09-30 17:53:30]

Patchset #6 (id:240001) has been deleted

[malaykeshav, 2016-09-30 17:53:59]

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

[commit-bot: I haz the power, 2016-09-30 17:54:37]

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

[malaykeshav, 2016-09-30 18:23:39]

Updated tests.
PTAL

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html
(right):

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:3:
<head>
On 2016/09/30 at 03:36:14, Xianzhu wrote:
> Normally we omit <html>, <head>, <body> in layout tests.

Done

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:5:
window.enablePixelTesting = true;
On 2016/09/30 at 03:36:14, Xianzhu wrote:
> Please remove the above line. It's no use here.
> 
> You may have seen some tests using it. These tests also includes some script
files which will read the variable.

Done

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:7:
setTimeout(function() {testRunner.notifyDone();}, 0);
On 2016/09/30 at 03:36:14, Xianzhu wrote:
> Is the setTimeout necessary?

Removed.

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:17:
if (window.testRunner) {
On 2016/09/30 at 03:36:14, Xianzhu wrote:
> Line 16 should be also protected by if (window.testRunner).
> Or you can just add 'if (window.testRunner)' before line 22 and remove all
other guards.

Done

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:22:
window.addEventListener("load", startTest, false);
On 2016/09/30 at 03:36:14, Xianzhu wrote:
> You can use shorter form:
>   onload = startTest;
> or replace line 15 with
>   onload = function() {

Done

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:28:
<div style="width:400px; height: 200px; background-color:red"></div>
On 2016/09/30 at 03:36:14, Xianzhu wrote:
> Remove "background-color: red". Normally we use red to indicate failure.

Done

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:31:
</html>
On 2016/09/30 at 03:36:14, Xianzhu wrote:
> We prefer reference tests to pixel tests because the former can avoid pixel
and text expectations and platform baselines.
> 
> Just create a file named
scrollbar-appearance-decrease-device-scale-factor-expected.html beside this file
containing the following contents:
> <!DOCTYPE html>
> <div id="div" style="width:200px; height:120px; overflow:auto">
>   <br>This should be a scrollable text box with horizontal and vertical
scroll.
>   <div style="width:400px; height: 200px; background-color:red"></div>
> </div>
> 
> The layout test runner script will run both html file and compare their pixel
outputs. The test passes if the two files generate the same output.
> 
> You can keep the other test as a pixel test.

The two files will not be _exactly_ the same due to the scaling. There is always
a difference of a pixel or two due to loss from non-integral values during
scaling.

[Xianzhu, 2016-09-30 18:36:21]

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html
(right):

https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:31:
</html>
On 2016/09/30 18:23:39, malaykeshav wrote:
> On 2016/09/30 at 03:36:14, Xianzhu wrote:
> > We prefer reference tests to pixel tests because the former can avoid pixel
> and text expectations and platform baselines.
> > 
> > Just create a file named
> scrollbar-appearance-decrease-device-scale-factor-expected.html beside this
file
> containing the following contents:
> > <!DOCTYPE html>
> > <div id="div" style="width:200px; height:120px; overflow:auto">
> >   <br>This should be a scrollable text box with horizontal and vertical
> scroll.
> >   <div style="width:400px; height: 200px; background-color:red"></div>
> > </div>
> > 
> > The layout test runner script will run both html file and compare their
pixel
> outputs. The test passes if the two files generate the same output.
> > 
> > You can keep the other test as a pixel test.
> 
> The two files will not be _exactly_ the same due to the scaling. There is
always
> a difference of a pixel or two due to loss from non-integral values during
> scaling.

This looks weird to me. The final scale is 1 which is the same as the initial
scale and the scale of reference html. We should do full repaint and full
re-rasterization after scale change, so the final pixels should be exactly the
same as the original and the reference. Can you share the pixel results?

[Xianzhu, 2016-09-30 18:58:35]

On 2016/09/30 18:36:21, Xianzhu wrote:
>
https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html
> (right):
> 
>
https://codereview.chromium.org/2377453004/diff/220001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/fast/hidpi/scrollbar-appearance-decrease-device-scale-factor.html:31:
> </html>
> On 2016/09/30 18:23:39, malaykeshav wrote:
> > On 2016/09/30 at 03:36:14, Xianzhu wrote:
> > > We prefer reference tests to pixel tests because the former can avoid
pixel
> > and text expectations and platform baselines.
> > > 
> > > Just create a file named
> > scrollbar-appearance-decrease-device-scale-factor-expected.html beside this
> file
> > containing the following contents:
> > > <!DOCTYPE html>
> > > <div id="div" style="width:200px; height:120px; overflow:auto">
> > >   <br>This should be a scrollable text box with horizontal and vertical
> > scroll.
> > >   <div style="width:400px; height: 200px; background-color:red"></div>
> > > </div>
> > > 
> > > The layout test runner script will run both html file and compare their
> pixel
> > outputs. The test passes if the two files generate the same output.
> > > 
> > > You can keep the other test as a pixel test.
> > 
> > The two files will not be _exactly_ the same due to the scaling. There is
> always
> > a difference of a pixel or two due to loss from non-integral values during
> > scaling.
> 
> This looks weird to me. The final scale is 1 which is the same as the initial
> scale and the scale of reference html. We should do full repaint and full
> re-rasterization after scale change, so the final pixels should be exactly the
> same as the original and the reference. Can you share the pixel results?

malaykeshav@ just showed me the pixel results. It seems that the text layout is
incorrect after scaled up then down. This looks like a bug to me but should not
block this CL. 

Can you file a bug?

lgtm for this CL.

[commit-bot: I haz the power, 2016-09-30 20:15:34]

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

[commit-bot: I haz the power, 2016-09-30 20:15:35]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2016-09-30 20:34:33]

The CQ bit was checked by malaykeshav@chromium.org

[commit-bot: I haz the power, 2016-09-30 20:35:00]

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-30 20:40:47]

Committed patchset #6 (id:260001)

[commit-bot: I haz the power, 2016-09-30 20:42:55]

Description was changed from

==========
Scrollbar width is set using thickness to compute correct rect size

BUG=637264
COMPONENT=ScrollableArea, Webkit
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Scrollbar width is set using thickness to compute correct rect size

BUG=637264
COMPONENT=ScrollableArea, Webkit
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/5556b4bf5c650643cc5aea0f17a72b1e9e29e316
Cr-Commit-Position: refs/heads/master@{#422210}
==========

[commit-bot: I haz the power, 2016-09-30 20:42:56]

Patchset 6 (id:??) landed as
https://crrev.com/5556b4bf5c650643cc5aea0f17a72b1e9e29e316
Cr-Commit-Position: refs/heads/master@{#422210}

[malaykeshav, 2016-10-02 00:55:59]

Patchset #7 (id:280001) has been deleted