Updating custom scrollbar style

Updating custom scrollbar style

Triggering custom scrollbar style update and adjusting the scrollbar
rect(this process is done during style recalc).This shall solve other
issues of custom scrollbar during page scale. As when the page is 
scaled the style is recalculated, if the thickness is changed then
owning renderer is set for childneedslayout. This layout shall
happen after style recalc is done, which avoids a re-layout of 
scrollbar while updating scrollbar geometry.

For brief explanation on custom-scrollbar behaviour/issues please check
https://codereview.chromium.org/503583002/#msg46

BUG=430519, 406676, 390895, 375200, 424925
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184986

[MuVen, 2014-11-05 17:06:52]

sataya.m@samsung.com changed reviewers:
+ esprehn@chromium.org

[MuVen, 2014-11-05 17:07:07]

PTAL.

Thanks,

[esprehn, 2014-11-05 19:49:20]

This needs a test.

[MuVen, 2014-11-06 15:33:58]

Hi esprehn,

PTAL. I have clubed all the issues in this patch and added tests accordingly.

Thanks,
MuVen.

[rune, 2014-11-06 16:44:26]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2014-11-06 16:44:27]

I don't know the scrollbar code well enough to say if the implementation of
recalculateCustomScrollbarStyle is correct.

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-changing-style.html (right):

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:16:
testRunner.dumpAsTextWithPixelResults();
You should try make a reference test instead. We should avoid using pixel tests
when not necessary. This test is guaranteed to require different platform
expectations for pixel results.

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:38:
setTimeout(function() {addstyle();}, 1000);
A one second timeout makes this test really slow. You should be able to make a
test for this without timeouts forcing recalc/layout with offsetTop or similar.

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html
(right):

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html:18:
testRunner.notifyDone();
Do you really need a wait/notify here? If so, won't this be too early?

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html:34:
<li>Chrome did not crash</li>
Was this list necessary?

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:339: }
Indentation looks wrong.

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:350: }
Indentation looks wrong.

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:136: virtual void
recalculateCustomScrollbarStyle();
Why virtual?

[MuVen, 2014-11-06 17:35:16]

@rune, Please find my comments. Please let me know your views on this,
accordingly i shall modify the testcases.

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-changing-style.html (right):

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:16:
testRunner.dumpAsTextWithPixelResults();
Sure, will make ref test.

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:38:
setTimeout(function() {addstyle();}, 1000);
Will make the timeout for 1 millisecond.Is that ok ?

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html
(right):

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html:34:
<li>Chrome did not crash</li>
On 2014/11/06 16:44:27, rune wrote:
> Was this list necessary?

well the list is created, just when the page is zoomed from 0 to 2.5 
scale will enable scrollbar.

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:339: }
On 2014/11/06 16:44:27, rune wrote:
> Indentation looks wrong.

will modify.

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:350: }
On 2014/11/06 16:44:27, rune wrote:
> Indentation looks wrong.

will modify.

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:137: virtual void
recalculateScrollbarOverlayStyle();
oops, copy paste problem. sorry i didnt notice. Thanks, i shall correct this
one.

[pdr., 2014-11-06 17:58:16]

pdr@chromium.org changed reviewers:
+ skobes@chromium.org

[MuVen, 2014-11-06 18:49:56]

PTAL. addressed review comments.

Thanks,
Muven.

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-changing-style.html (right):

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:16:
testRunner.dumpAsTextWithPixelResults();
On 2014/11/06 17:35:15, muven wrote:
> Sure, will make ref test.

Done.

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:38:
setTimeout(function() {addstyle();}, 1000);
On 2014/11/06 17:35:15, muven wrote:
> Will make the timeout for 1 millisecond.Is that ok ?

Done.

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html
(right):

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html:18:
testRunner.notifyDone();
On 2014/11/06 16:44:27, rune wrote:
> Do you really need a wait/notify here? If so, won't this be too early?
not needed. removed. Done.

https://codereview.chromium.org/702913002/diff/40001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html:34:
<li>Chrome did not crash</li>
On 2014/11/06 17:35:15, muven wrote:
> On 2014/11/06 16:44:27, rune wrote:
> > Was this list necessary?
> 
> well the list is created, just when the page is zoomed from 0 to 2.5 
> scale will enable scrollbar.

Done.

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:339: }
On 2014/11/06 17:35:15, muven wrote:
> On 2014/11/06 16:44:27, rune wrote:
> > Indentation looks wrong.
> 
> will modify.

Done.

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:350: }
On 2014/11/06 17:35:15, muven wrote:
> On 2014/11/06 16:44:27, rune wrote:
> > Indentation looks wrong.
> 
> will modify.

Done.

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/702913002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:137: virtual void
recalculateScrollbarOverlayStyle();
On 2014/11/06 17:35:16, muven wrote:
> oops, copy paste problem. sorry i didnt notice. Thanks, i shall correct this
> one.

Done.

[skobes, 2014-11-07 00:14:00]

https://codereview.chromium.org/702913002/diff/60001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-changing-style.html (right):

https://codereview.chromium.org/702913002/diff/60001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:19: var sheet =
document.head.appendChild(style).sheet;
I think this would be easier to understand if the calls to createElement and
appendChild were moved above the addRule initializer, e.g.:

var styleElement = document.createElement("style");
document.head.appendChild(styleElement);
var addRule = function(selector, properties) {
    ...
};

https://codereview.chromium.org/702913002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/702913002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:336:
m_horizontalScrollbar->setFrameRect(adjustScrollbarRectForResizer(hBarRect,
m_horizontalScrollbar.get()));
Calling Scrollbar::styleChanged() makes sense, but we already compute the frame
rects of the scrollbars (and call setFrameRect + invalidate) in
FrameView::updateScrollbarGeometry.  Why do we need to do it here as well?  Can
we just call updateScrollbars(scrollOffsetDouble())?

https://codereview.chromium.org/702913002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/702913002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:214: if (isDocumentElement() || isBody()) {
Have you tested this scenario with an overflow:scroll div?  We probably need
similar code in RenderLayerScrollableArea.

[MuVen, 2014-11-07 09:46:59]

PTAL.addressed review comments.

Thanks,
~MuVen.

https://codereview.chromium.org/702913002/diff/60001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-changing-style.html (right):

https://codereview.chromium.org/702913002/diff/60001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:19: var sheet =
document.head.appendChild(style).sheet;
On 2014/11/07 00:13:59, skobes wrote:
> I think this would be easier to understand if the calls to createElement and
> appendChild were moved above the addRule initializer, e.g.:
> 
> var styleElement = document.createElement("style");
> document.head.appendChild(styleElement);
> var addRule = function(selector, properties) {
>     ...
> };

Done.

https://codereview.chromium.org/702913002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/702913002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:336:
m_horizontalScrollbar->setFrameRect(adjustScrollbarRectForResizer(hBarRect,
m_horizontalScrollbar.get()));
instead of updateScrollbar, updateScrollbarGeometry should be find, as
updateScrollbar will be called in layout function after this.

https://codereview.chromium.org/702913002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/702913002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:214: if (isDocumentElement() || isBody()) {
this part of code is not needed in RenderLayerScrollbableArea as in
updateAfterStyleChange API 

    if (m_hBar)
        m_hBar->styleChanged();
    if (m_vBar)
        m_vBar->styleChanged(); is being called.

[rune, 2014-11-07 10:58:08]

https://codereview.chromium.org/702913002/diff/80001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-changing-style.html (right):

https://codereview.chromium.org/702913002/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:32:
setTimeout(function() {addstyle();}, 1);
I still think it would be more robust if you put the script below the div, did a
doument.body.offsetTop before calling addStyle and skipped both the
waitUntilDone and the setTimeout.

https://codereview.chromium.org/702913002/diff/80001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html
(right):

https://codereview.chromium.org/702913002/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html:20:
<ul>
Could you just use a tall div here instead, like you do in the other test?

[MuVen, 2014-11-07 11:33:55]

PTAL.

https://codereview.chromium.org/702913002/diff/80001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-changing-style.html (right):

https://codereview.chromium.org/702913002/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:32:
setTimeout(function() {addstyle();}, 1);
On 2014/11/07 10:58:08, rune wrote:
> I still think it would be more robust if you put the script below the div, did
a
> doument.body.offsetTop before calling addStyle and skipped both the
> waitUntilDone and the setTimeout.

Done.

https://codereview.chromium.org/702913002/diff/80001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html
(right):

https://codereview.chromium.org/702913002/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html:20:
<ul>
On 2014/11/07 10:58:08, rune wrote:
> Could you just use a tall div here instead, like you do in the other test?

Done.

[MuVen, 2014-11-07 14:09:47]

Patchset #7 (id:120001) has been deleted

[MuVen, 2014-11-07 15:53:27]

Patchset #6 (id:100001) has been deleted

[MuVen, 2014-11-07 15:55:46]

@skobes,@rune,

PTAL. provide your inputs to purse further.

Thanks,
MuVen.

[skobes, 2014-11-07 17:54:41]

https://codereview.chromium.org/702913002/diff/130001/LayoutTests/scrollbars/...
File LayoutTests/scrollbars/custom-scrollbar-changing-style.html (right):

https://codereview.chromium.org/702913002/diff/130001/LayoutTests/scrollbars/...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:8: background:
rgba(255,0,0,0.8);
Fix indentation.

https://codereview.chromium.org/702913002/diff/130001/LayoutTests/scrollbars/...
File LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html
(right):

https://codereview.chromium.org/702913002/diff/130001/LayoutTests/scrollbars/...
LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html:8:
background: rgba(255,0,0,0.8);
Fix indentation.

https://codereview.chromium.org/702913002/diff/130001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/702913002/diff/130001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:328: if (hasCustomScrollbars()) {
You don't need this check since you are checking isCustomScrollbar() below.

https://codereview.chromium.org/702913002/diff/130001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:329: bool didStyleRecalculated = false;
Call this "didStyleChange".

[MuVen, 2014-11-07 18:30:08]

Patchset #7 (id:140001) has been deleted

[MuVen, 2014-11-07 18:31:19]

Done. PTAL.

https://codereview.chromium.org/702913002/diff/130001/LayoutTests/scrollbars/...
File LayoutTests/scrollbars/custom-scrollbar-changing-style.html (right):

https://codereview.chromium.org/702913002/diff/130001/LayoutTests/scrollbars/...
LayoutTests/scrollbars/custom-scrollbar-changing-style.html:8: background:
rgba(255,0,0,0.8);
On 2014/11/07 17:54:41, skobes wrote:
> Fix indentation.

Done.

https://codereview.chromium.org/702913002/diff/130001/LayoutTests/scrollbars/...
File LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html
(right):

https://codereview.chromium.org/702913002/diff/130001/LayoutTests/scrollbars/...
LayoutTests/scrollbars/custom-scrollbar-thickness-change-on-zoom-crash.html:8:
background: rgba(255,0,0,0.8);
On 2014/11/07 17:54:41, skobes wrote:
> Fix indentation.

Done.

https://codereview.chromium.org/702913002/diff/130001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/702913002/diff/130001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:328: if (hasCustomScrollbars()) {
On 2014/11/07 17:54:41, skobes wrote:
> You don't need this check since you are checking isCustomScrollbar() below.

Done.

https://codereview.chromium.org/702913002/diff/130001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:328: if (hasCustomScrollbars()) {
On 2014/11/07 17:54:41, skobes wrote:
> You don't need this check since you are checking isCustomScrollbar() below.

Done.

https://codereview.chromium.org/702913002/diff/130001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:329: bool didStyleRecalculated = false;
On 2014/11/07 17:54:41, skobes wrote:
> Call this "didStyleChange".

Done.

[skobes, 2014-11-07 18:39:08]

lgtm

[MuVen, 2014-11-07 18:42:20]

On 2014/11/07 18:39:08, skobes wrote:
> lgtm

Thanks for the review. Special thanks to pdr :)

[MuVen, 2014-11-07 18:42:27]

The CQ bit was checked by sataya.m@samsung.com

[commit-bot: I haz the power, 2014-11-07 18:43:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/702913002/160001

[MuVen, 2014-11-07 18:46:17]

@esprehn, 

This might require your LGTM even i guess. PTAL.

Thanks.

[commit-bot: I haz the power, 2014-11-07 18:47:09]

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

[commit-bot: I haz the power, 2014-11-07 18:47:11]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[pdr., 2014-11-07 18:54:37]

pdr@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2014-11-07 18:54:37]

The CQ bit was checked by pdr@chromium.org

[pdr., 2014-11-07 18:54:38]

lgtm

[pdr., 2014-11-07 18:54:48]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2014-11-07 18:54:52]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-11-07 18:55:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/702913002/160001

[commit-bot: I haz the power, 2014-11-07 19:49:28]

Committed patchset #7 (id:160001) as 184986