Invalidate CustomScrollbars on having window-inactive selector.

Invalidate CustomScrollbars on having window-inactive selector.

Invalidate CustomScrollbars on having window-inactive selector, if 
window-inactive selector is not present then dont invalidate all 
custom scrollbars on active changed.

BUG=436865
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188803

[MuVen, 2015-01-20 13:02:04]

sataya.m@samsung.com changed reviewers:
+ rune@opera.com

[MuVen, 2015-01-20 13:03:19]

Rune,

PTAL.

Thanks,
MuVen

[rune, 2015-01-20 13:42:34]

https://codereview.chromium.org/861553003/diff/1/Source/core/css/RuleFeature.h
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/861553003/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.h:64: bool usesWindowInActiveSelector() const {
return m_metadata.usesWindowInActiveSelector; }
It's "inactive", not "in active", so you should call all methods/members
"Inactive" instead of "InActive".

https://codereview.chromium.org/861553003/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/861553003/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:344: return;
Even if this document doesn't have a :window-inactive selector, it doesn't mean
that the child iframes may not have any. You can skip the styleChanged() on the
scrollbar children, and the recalculateCustomScrollbarStyle, but  not the
recursive call into invalidateAllCustomScrollbarsOnActiveChanged.

[rune, 2015-01-20 13:44:29]

On 2015/01/20 13:42:34, rune wrote:

> Even if this document doesn't have a :window-inactive selector, it doesn't
mean
> that the child iframes may not have any. You can skip the styleChanged() on
the
> scrollbar children, and the recalculateCustomScrollbarStyle, but  not the
> recursive call into invalidateAllCustomScrollbarsOnActiveChanged.

Also, you need tests. In particular the case where you have iframes with
:window-inactive inside documents without :window-inactive.

[MuVen, 2015-01-20 14:50:11]

PTAL. Addressed review comments.

https://codereview.chromium.org/861553003/diff/1/Source/core/css/RuleFeature.h
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/861553003/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.h:64: bool usesWindowInActiveSelector() const {
return m_metadata.usesWindowInActiveSelector; }
On 2015/01/20 13:42:34, rune wrote:
> It's "inactive", not "in active", so you should call all methods/members
> "Inactive" instead of "InActive".

Done.

https://codereview.chromium.org/861553003/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/861553003/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:344: return;
On 2015/01/20 13:42:34, rune wrote:
> Even if this document doesn't have a :window-inactive selector, it doesn't
mean
> that the child iframes may not have any. You can skip the styleChanged() on
the
> scrollbar children, and the recalculateCustomScrollbarStyle, but  not the
> recursive call into invalidateAllCustomScrollbarsOnActiveChanged.

Done.

[rune, 2015-01-20 15:48:13]

Since these tests are all new, could you upload next patch set with
--no-find-copies?

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
File
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html
(left):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html:13:
-webkit-border-radius: 10px;
Do you need things like border-radius, box-shadow and opacity here? Wouldn't it
be enough to use different background-color for :window-inactive?

Try make the test as simple as possible.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html:27:
}
When using "overflow: scroll" on html, do you really need width/height on body?

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html:34:
<body>
Drop <body>

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
File
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html
(right):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html:34:
<div><p style="width:200px;height:200px"></p></div>
overflow:scroll in the stylesheet should make the <p> elements above
unnecessary.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
File
LayoutTests/scrollbars/resources/scrollable-iframe-customscrollbar-windowinactive.html
(right):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-iframe-customscrollbar-windowinactive.html:1:
<html>
<!DOCTYPE html> and no <html>, <head>, <body>.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-iframe-customscrollbar-windowinactive.html:30:
<div id="fillDIV"
style="background-color:lightblue;width:400px;height:400px;overflow:hidden;">
</div>
What's this div for?

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
File
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive-expected.html
(right):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive-expected.html:2:
<head>
- Use <!DOCTYPE html> for strict mode
- Don't include <html>, <head>, and <body>

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
File LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html
(right):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html:1:
<html>
<!DOCTYPE html> and no <html>, <head>, <body>.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html:22:
<div id="fillDIV"
style="background-color:lightblue;width:400px;height:400px;overflow:hidden;">
</div>
What's this div for?

[MuVen, 2015-01-21 06:14:10]

PTAL. I have minimized the testcase.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
File
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html
(left):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html:13:
-webkit-border-radius: 10px;
On 2015/01/20 15:48:12, rune wrote:
> Do you need things like border-radius, box-shadow and opacity here? Wouldn't
it
> be enough to use different background-color for :window-inactive?
> 
> Try make the test as simple as possible.

Done.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html:27:
}
On 2015/01/20 15:48:12, rune wrote:
> When using "overflow: scroll" on html, do you really need width/height on
body?

Yes to display custom scrollbars on the main frameview i need to set the body
width and height.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html:34:
<body>
On 2015/01/20 15:48:12, rune wrote:
> Drop <body>

Done.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
File
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html
(right):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-only-on-windowinactive-selector.html:34:
<div><p style="width:200px;height:200px"></p></div>
On 2015/01/20 15:48:12, rune wrote:
> overflow:scroll in the stylesheet should make the <p> elements above
> unnecessary.

I have replaced p element with div element, just to check
if(usesWindowInactiveSelector && widget->isScrollbar() &&
toScrollbar(widget)->isCustomScrollbar())
    toScrollbar(widget)->styleChanged();
condition.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
File
LayoutTests/scrollbars/resources/scrollable-iframe-customscrollbar-windowinactive.html
(right):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-iframe-customscrollbar-windowinactive.html:1:
<html>
On 2015/01/20 15:48:12, rune wrote:
> <!DOCTYPE html> and no <html>, <head>, <body>.

Done.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-iframe-customscrollbar-windowinactive.html:30:
<div id="fillDIV"
style="background-color:lightblue;width:400px;height:400px;overflow:hidden;">
</div>
On 2015/01/20 15:48:12, rune wrote:
> What's this div for?

Removed, Adjusted iframe height.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
File
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive-expected.html
(right):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive-expected.html:2:
<head>
On 2015/01/20 15:48:13, rune wrote:
> - Use <!DOCTYPE html> for strict mode
> - Don't include <html>, <head>, and <body>

Done.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
File LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html
(right):

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html:1:
<html>
On 2015/01/20 15:48:13, rune wrote:
> <!DOCTYPE html> and no <html>, <head>, <body>.

Done.

https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html:22:
<div id="fillDIV"
style="background-color:lightblue;width:400px;height:400px;overflow:hidden;">
</div>
On 2015/01/20 15:48:13, rune wrote:
> What's this div for?

just to display customscrollbar on iframe, im overflowing the content.

[rune, 2015-01-21 08:26:48]

On 2015/01/21 06:14:10, MuVen(Back) wrote:
> PTAL. I have minimized the testcase.

>
https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
>
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html:22:
> <div id="fillDIV"
> style="background-color:lightblue;width:400px;height:400px;overflow:hidden;">
> </div>
> On 2015/01/20 15:48:13, rune wrote:
> > What's this div for?
> 
> just to display customscrollbar on iframe, im overflowing the content.

In general for this test, if you style background-color on ::-webkit-scrollbar
instead of ::-webkit-scrollbar-thumb, you can make the test simpler by having
overflow:scroll and no extra content to cause overflow should be necessary.
Unless it's important to test thumb styling, but that's not the case here,
right?

[MuVen, 2015-01-22 05:59:40]

On 2015/01/21 08:26:48, rune wrote:
> On 2015/01/21 06:14:10, MuVen(Back) wrote:
> > PTAL. I have minimized the testcase.
> 
> >
>
https://codereview.chromium.org/861553003/diff/20001/LayoutTests/scrollbars/r...
> >
>
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html:22:
> > <div id="fillDIV"
> >
style="background-color:lightblue;width:400px;height:400px;overflow:hidden;">
> > </div>
> > On 2015/01/20 15:48:13, rune wrote:
> > > What's this div for?
> > 
> > just to display customscrollbar on iframe, im overflowing the content.
> 
> In general for this test, if you style background-color on ::-webkit-scrollbar
> instead of ::-webkit-scrollbar-thumb, you can make the test simpler by having
> overflow:scroll and no extra content to cause overflow should be necessary.
> Unless it's important to test thumb styling, but that's not the case here,
> right?

True, I have uploaded testcase picking up the old one.
Now I have modified test case as you have suggested.PTAL.

[rune, 2015-01-22 09:21:23]

https://codereview.chromium.org/861553003/diff/60001/LayoutTests/scrollbars/c...
File
LayoutTests/scrollbars/custom-scrollbar-inactive-only-on-windowinactive-selector-expected.html
(right):

https://codereview.chromium.org/861553003/diff/60001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbar-inactive-only-on-windowinactive-selector-expected.html:6:
background: rgba(255,0,0,0.8);
Wrong indentation?

https://codereview.chromium.org/861553003/diff/60001/LayoutTests/scrollbars/r...
File
LayoutTests/scrollbars/resources/scrollable-iframe-customscrollbar-windowinactive-expected.html
(right):

https://codereview.chromium.org/861553003/diff/60001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-iframe-customscrollbar-windowinactive-expected.html:6:
background: rgba(255,150,250,0.8);
Wrong indentation?

https://codereview.chromium.org/861553003/diff/60001/LayoutTests/scrollbars/r...
File
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive-expected.html
(right):

https://codereview.chromium.org/861553003/diff/60001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive-expected.html:6:
background: rgba(255,0,0,0.8);
Indentation

https://codereview.chromium.org/861553003/diff/60001/LayoutTests/scrollbars/r...
File LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html
(right):

https://codereview.chromium.org/861553003/diff/60001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html:6:
background: rgba(255,0,0,0.8);
Wrong indentation

https://codereview.chromium.org/861553003/diff/60001/LayoutTests/scrollbars/r...
LayoutTests/scrollbars/resources/scrollable-subiframe-nowindowinactive.html:9:
<div style="width:400px;height:400px;overflow:hidden;"> </div>
Instead of this div you could've used "html { overflow: scroll }" in the
stylesheet?

[MuVen, 2015-01-22 09:31:51]

Sorry for the wrong indentation. 

PTAL. modified test case as mentioned.


Thanks,

[rune, 2015-01-22 09:49:45]

lgtm

[MuVen, 2015-01-22 09:50:38]

On 2015/01/22 09:49:45, rune wrote:
> lgtm

Thanks for your patient review.

[MuVen, 2015-01-22 09:50:40]

On 2015/01/22 09:49:45, rune wrote:
> lgtm

Thanks for your patient review.

[MuVen, 2015-01-22 09:50:52]

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

[commit-bot: I haz the power, 2015-01-22 09:51:58]

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

[commit-bot: I haz the power, 2015-01-22 11:05:04]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188803