Trigger style recalc when the window's active state change

Trigger style recalc when the window's active state change.

Trigger style recalc when the window's active state change. This is 
needed to correctly apply :window-inactive styling to custom scrollbars.

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

[MuVen, 2014-11-26 14:42:28]

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

[MuVen, 2014-11-26 14:43:14]

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

[MuVen, 2014-11-26 14:43:16]

Hi Skobes,

PTAL.

Thanks,
MuVen.

[MuVen, 2014-11-26 14:43:24]

Hi Skobes/Enne,

PTAL.

Thanks,
MuVen.

[enne (OOO), 2014-11-26 18:10:44]

enne@chromium.org changed reviewers:
- enne@chromium.org

[MuVen, 2014-11-28 09:41:59]

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

[MuVen, 2014-11-28 16:50:55]

friendly ping !!!

[skobes, 2014-12-02 02:10:52]

I see there is already a test for custom scrollbars with :window-inactive
styling:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

So I'd like to understand two things:

(1) Assuming that test passes, and your new test doesn't, how are they
different?

(2) When :window-inactive styling is correctly applied, what codepath triggers
the style invalidation?

[MuVen, 2014-12-02 11:23:21]

Thanks for mentioning the test-case, it helped me in learning new thing.

1. Test-cases passes as another tab is opened with the help of js.

var win = window.open('about:blank');
win.focus();

due to which Document::implicitClose() is triggered which calls
FrameView::Layout,
which internally triggers RenderScrollbar styleChange due to which we see the
test
case as been passed, here is the callstack.

blink::RenderScrollbar::styleChanged()
blink::FrameView::setHasHorizontalScrollbar()
blink::FrameView::adjustScrollbarExistence()
blink::FrameView::updateScrollbars()
blink::FrameView::setScrollbarModes()
blink::FrameView::layout()
blink::Document::implicitClose()


2. Where as in the test upload by me im setting the focus of the current tab
false,
//window.testRunner.setWindowIsKey(false);If you click on the URL bar (omni bar)

window will be in inactive state which is calling
FrameSelection::focusedOrActiveStateChanged
but there is no style Invalidation, as updateRenderTreeIfNeeded call is not
triggering
any styleRecalc, as no setNeedsStyleRecalc is set.

[skobes, 2014-12-02 17:43:22]

Thanks for the explanation.

The wording of the change description is not very clear.  I would suggest:

"Trigger style recalc when the window's active state changes.

This is needed to correctly apply :window-inactive styling to custom
scrollbars."

https://codereview.chromium.org/748513006/diff/20001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html (right):

https://codereview.chromium.org/748513006/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html:23:
Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac
turpis egestas. Vestibulum tortor quam, feugiat vitae, ultricies eget, tempor
sit amet, ante. Donec eu libero sit amet quam egestas semper. Aenean ultricies
mi vitae est. Mauris placerat eleifend leo. Quisque sit amet est et sapien
ullamcorper pharetra. Vestibulum erat wisi, condimentum sed, commodo vitae,
ornare sit amet, wisi. Aenean fermentum, elit eget tincidunt condimentum, eros
ipsum rutrum orci, sagittis tempus lacus enim ac dui. Donec non enim in turpis
pulvinar facilisis. Ut felis. Praesent dapibus, neque id cursus faucibus, tortor
neque egestas augue, eu vulputate magna eros eu erat. Aliquam erat volutpat. Nam
dui mi, tincidunt quis, accumsan porttitor, facilisis luctus, metus
Instead of all this dummy text, can you put in an element with an explicit
height to trigger the scrollbar?

https://codereview.chromium.org/748513006/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html:44:
window.testRunner.setWindowIsKey(false);
Add a comment explaining what this does.

[MuVen, 2014-12-02 18:40:35]

Done, Addressed review comments. PTAL.

https://codereview.chromium.org/748513006/diff/20001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html (right):

https://codereview.chromium.org/748513006/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html:23:
Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac
turpis egestas. Vestibulum tortor quam, feugiat vitae, ultricies eget, tempor
sit amet, ante. Donec eu libero sit amet quam egestas semper. Aenean ultricies
mi vitae est. Mauris placerat eleifend leo. Quisque sit amet est et sapien
ullamcorper pharetra. Vestibulum erat wisi, condimentum sed, commodo vitae,
ornare sit amet, wisi. Aenean fermentum, elit eget tincidunt condimentum, eros
ipsum rutrum orci, sagittis tempus lacus enim ac dui. Donec non enim in turpis
pulvinar facilisis. Ut felis. Praesent dapibus, neque id cursus faucibus, tortor
neque egestas augue, eu vulputate magna eros eu erat. Aliquam erat volutpat. Nam
dui mi, tincidunt quis, accumsan porttitor, facilisis luctus, metus
On 2014/12/02 17:43:22, skobes wrote:
> Instead of all this dummy text, can you put in an element with an explicit
> height to trigger the scrollbar?

Done.

https://codereview.chromium.org/748513006/diff/20001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html:44:
window.testRunner.setWindowIsKey(false);
On 2014/12/02 17:43:22, skobes wrote:
> Add a comment explaining what this does.

Done.

[skobes, 2014-12-02 18:43:19]

lgtm

[MuVen, 2014-12-02 18:53:22]

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

[commit-bot: I haz the power, 2014-12-02 18:53:53]

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

[commit-bot: I haz the power, 2014-12-02 19:03:21]

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

[commit-bot: I haz the power, 2014-12-02 19:03:22]

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

[MuVen, 2014-12-03 13:42:51]

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

[commit-bot: I haz the power, 2014-12-03 13:43:26]

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

[MuVen, 2014-12-03 13:43:55]

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

[MuVen, 2014-12-03 14:40:37]

Hi Skobes, 

I have modified the patch, i tried to set setNeedsStyleRecalc in FrameSelection
itself. PTAL.

Thanks,
~MuVen.

[skobes, 2014-12-03 16:40:26]

still lgtm

[MuVen, 2014-12-03 16:43:27]

@pdr, PTAL. Need owners approval.

[pdr., 2014-12-03 19:36:12]

On 2014/12/03 at 16:43:27, sataya.m wrote:
> @pdr, PTAL. Need owners approval.

Does this patch mean clicking the window or clicking away will cause a style
recalc in order to support custom scrollbars?

[MuVen, 2014-12-04 03:13:16]

On 2014/12/03 19:36:12, pdr wrote:
> On 2014/12/03 at 16:43:27, sataya.m wrote:
> > @pdr, PTAL. Need owners approval.
> 
> Does this patch mean clicking the window or clicking away will cause a style
> recalc in order to support custom scrollbars?

Yes, to support custom scrollbar on window inactive we need style recalc

[pdr., 2014-12-04 03:50:42]

On 2014/12/04 at 03:13:16, sataya.m wrote:
> On 2014/12/03 19:36:12, pdr wrote:
> > On 2014/12/03 at 16:43:27, sataya.m wrote:
> > > @pdr, PTAL. Need owners approval.
> > 
> > Does this patch mean clicking the window or clicking away will cause a style
> > recalc in order to support custom scrollbars?
> 
> Yes, to support custom scrollbar on window inactive we need style recalc

I don't think we should land this change because it introduces an expensive full
style recalc for all pages even though custom scrollbars are incredibly rare.
There are ways to avoid this work on pages without custom scrollbars but I don't
think it would be worth the complexity for such a rare feature.

[MuVen, 2014-12-04 04:03:37]

On 2014/12/04 03:50:42, pdr wrote:
> On 2014/12/04 at 03:13:16, sataya.m wrote:
> > On 2014/12/03 19:36:12, pdr wrote:
> > > On 2014/12/03 at 16:43:27, sataya.m wrote:
> > > > @pdr, PTAL. Need owners approval.
> > > 
> > > Does this patch mean clicking the window or clicking away will cause a
style
> > > recalc in order to support custom scrollbars?
> > 
> > Yes, to support custom scrollbar on window inactive we need style recalc
> 
> I don't think we should land this change because it introduces an expensive
full
> style recalc for all pages even though custom scrollbars are incredibly rare.
> There are ways to avoid this work on pages without custom scrollbars but I
don't
> think it would be worth the complexity for such a rare feature.

to avoid full style recalc, can we do just like textselection ? store all the
custom
scrollbar created in one datastructure and call style invalidation on
focusactivechanged ?

[pdr., 2014-12-04 04:09:15]

On 2014/12/04 at 04:03:37, sataya.m wrote:
> On 2014/12/04 03:50:42, pdr wrote:
> > On 2014/12/04 at 03:13:16, sataya.m wrote:
> > > On 2014/12/03 19:36:12, pdr wrote:
> > > > On 2014/12/03 at 16:43:27, sataya.m wrote:
> > > > > @pdr, PTAL. Need owners approval.
> > > > 
> > > > Does this patch mean clicking the window or clicking away will cause a
style
> > > > recalc in order to support custom scrollbars?
> > > 
> > > Yes, to support custom scrollbar on window inactive we need style recalc
> > 
> > I don't think we should land this change because it introduces an expensive
full
> > style recalc for all pages even though custom scrollbars are incredibly
rare.
> > There are ways to avoid this work on pages without custom scrollbars but I
don't
> > think it would be worth the complexity for such a rare feature.
> 
> to avoid full style recalc, can we do just like textselection ? store all the
custom
> scrollbar created in one datastructure and call style invalidation on
focusactivechanged ?

That sounds like an excellent approach. I'm still wary of adding a bunch of
complexity for this feature though. I'll defer to @skobes who is more familiar
with this area.

[MuVen, 2014-12-04 04:13:05]

On 2014/12/04 04:09:15, pdr wrote:
> On 2014/12/04 at 04:03:37, sataya.m wrote:
> > On 2014/12/04 03:50:42, pdr wrote:
> > > On 2014/12/04 at 03:13:16, sataya.m wrote:
> > > > On 2014/12/03 19:36:12, pdr wrote:
> > > > > On 2014/12/03 at 16:43:27, sataya.m wrote:
> > > > > > @pdr, PTAL. Need owners approval.
> > > > > 
> > > > > Does this patch mean clicking the window or clicking away will cause a
> style
> > > > > recalc in order to support custom scrollbars?
> > > > 
> > > > Yes, to support custom scrollbar on window inactive we need style recalc
> > > 
> > > I don't think we should land this change because it introduces an
expensive
> full
> > > style recalc for all pages even though custom scrollbars are incredibly
> rare.
> > > There are ways to avoid this work on pages without custom scrollbars but I
> don't
> > > think it would be worth the complexity for such a rare feature.
> > 
> > to avoid full style recalc, can we do just like textselection ? store all
the
> custom
> > scrollbar created in one datastructure and call style invalidation on
> focusactivechanged ?
> 
> That sounds like an excellent approach. I'm still wary of adding a bunch of
> complexity for this feature though. I'll defer to @skobes who is more familiar
> with this area.

Sure, I remember this was working on safari thats
why i raised this bug. Thanks for your input pdr. 
i shall check with skobes on this.

[pdr., 2014-12-04 04:24:43]

On 2014/12/04 at 04:13:05, sataya.m wrote:
> On 2014/12/04 04:09:15, pdr wrote:
> > On 2014/12/04 at 04:03:37, sataya.m wrote:
> > > On 2014/12/04 03:50:42, pdr wrote:
> > > > On 2014/12/04 at 03:13:16, sataya.m wrote:
> > > > > On 2014/12/03 19:36:12, pdr wrote:
> > > > > > On 2014/12/03 at 16:43:27, sataya.m wrote:
> > > > > > > @pdr, PTAL. Need owners approval.
> > > > > > 
> > > > > > Does this patch mean clicking the window or clicking away will cause
a
> > style
> > > > > > recalc in order to support custom scrollbars?
> > > > > 
> > > > > Yes, to support custom scrollbar on window inactive we need style
recalc
> > > > 
> > > > I don't think we should land this change because it introduces an
expensive
> > full
> > > > style recalc for all pages even though custom scrollbars are incredibly
> > rare.
> > > > There are ways to avoid this work on pages without custom scrollbars but
I
> > don't
> > > > think it would be worth the complexity for such a rare feature.
> > > 
> > > to avoid full style recalc, can we do just like textselection ? store all
the
> > custom
> > > scrollbar created in one datastructure and call style invalidation on
> > focusactivechanged ?
> > 
> > That sounds like an excellent approach. I'm still wary of adding a bunch of
> > complexity for this feature though. I'll defer to @skobes who is more
familiar
> > with this area.
> 
> Sure, I remember this was working on safari thats
> why i raised this bug. Thanks for your input pdr. 
> i shall check with skobes on this.

Interesting, I didn't realize that. If you end up pursuing this change, it may
be worth investigating how this works in Safari/Webkit because we still share a
lot of code in this area:
http://trac.webkit.org/browser/trunk/Source/WebCore/editing/FrameSelection.cpp
If this used to work in blink but regressed, there's a tool for bisecting builds
(/tools/bisect-builds.py) that you could use to find what change broke this
without rebuilding.

[skobes, 2014-12-04 07:02:55]

+rune@opera.com

I agree with pdr that we should try not to add complexity here.

I think Rune is the expert on style invalidation, maybe he can advise on the
best solution here?

[MuVen, 2014-12-04 07:13:48]

sure, it should even support multiple-customscrollbar-inactive.html testcase
attached in the bug. Share your inputs to solve this without much complexity.

[rune, 2014-12-04 08:13:54]

On 2014/12/04 at 07:02:55, skobes wrote:
> +rune@opera.com
> 
> I agree with pdr that we should try not to add complexity here.
> 
> I think Rune is the expert on style invalidation, maybe he can advise on the
best solution here?

How about storing the presence of a :window-inactive in the rule feature data
and only invalidate style in the presence of the pseudo class? That should avoid
recalc for most pages?

It's possible to make descendant invalidation sets have a bit saying that
descendants with scrollbars should be invalidated, and that the presence of a
:window-inactive rule will create an invalidation set: {
invalidateScrollableContainers:1 } that we schedule for the root element, and
let the StyleInvalidator invalidate elements with scrollbars as part of the
invalidation pass.

As a side-note, I've thought about bringing scrollbar pseudos as invalidation
set features in some way as they're quite often universal rules (at least in the
rightmost compound selector). For instance for gmail.com/google-inbox there are
a lot of scrollbar pseudo rules that are considered for every element in the
page while matching, most of them skipped by the bloom filters after being
collected, though.

[MuVen, 2014-12-05 14:25:18]

PTAL. I have made minimal changes to invalidate scrollbar on window
focus changed.

[rune, 2014-12-05 15:13:48]

On 2014/12/05 at 14:25:18, sataya.m wrote:
> PTAL. I have made minimal changes to invalidate scrollbar on window
> focus changed.

I still think this should be a no-op when there's no :window-inactive pseudo
class present.

[MuVen, 2014-12-05 15:20:55]

true, implemented this API, just by following the TextSelection 
window-inactive(invalidatePaintForSelection).

[MuVen, 2014-12-05 15:26:43]

@rune, as done for webkit-scrollbar @ https://codereview.chromium.org/653423003 
for window-active also we need to do ? is that fine ?

[MuVen, 2014-12-05 15:37:34]

On 2014/12/05 15:26:43, muven wrote:
> @rune, as done for webkit-scrollbar @
https://codereview.chromium.org/653423003 
> for window-active also we need to do ? is that fine ?

In the rulefeature PseudoWindowInactive is been added by you @
https://codereview.chromium.org/669873003.

May be invalidateTextSelection/invalidateAllCustomScrollbarsOnActiveChanged
needs to triggered only on window-inActive ?

[rune, 2014-12-06 00:04:00]

On 2014/12/05 at 15:26:43, sataya.m wrote:
> @rune, as done for webkit-scrollbar @
https://codereview.chromium.org/653423003 
> for window-active also we need to do ? is that fine ?

No, I was thinking about doing the same as for usesFirstLineRules in
RuleFeatureSet::FeatureMetadata and StyleEngine and not do anything if that is
false when changing active state.

[pdr., 2014-12-06 04:17:53]

A few more comments. I think the approach you've taken is good.

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo-expected.html
(right):

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo-expected.html:32:
<iframe src="resources/scrollable-iframe.html"></iframe>
Are we actually testing custom scrollbars in iframes with these? Don't we need
something like resources/scrollable-iframe-active.html for the expected result?

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html (right):

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html:25: width:
1000px;
Nit: Please fix the indentation in the tests. 8 or 4 or 2 spaces are fine as
long as we're consistent in a file.

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html:49:
setTimeout(function(){ changeFocus(); }, 100);
This timeout doesn't seem right. Was this added so that the iframes have time to
load? If so, that'll unfortunately lead to flakiness on our very slow bots
(win-dbg) and slow tests on our fast bots (linux-rel). Something like this
should work without settimeout:
window.onload = function() {
  if (window.testRunner) {
    testRunner.setWindowsIsKey(false);
  }
}

https://codereview.chromium.org/748513006/diff/80001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/748513006/diff/80001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:1472:
view->invalidateAllCustomScrollbarsOnActiveChanged();
Should this be here or in pageActivationChanged?

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

https://codereview.chromium.org/748513006/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:350:
toFrameView(widget)->recalculateCustomScrollbarStyle();
Just to make sure I understand: we don't need to call
toFrameView(widget)->invalidateAllCustomScrollbarsOnActiveChanged() here because
focusOrActiveStateChanged will already get called on sub-frameviews?

[MuVen, 2014-12-06 12:51:08]

Patchset #8 (id:140001) has been deleted

[MuVen, 2014-12-06 12:51:28]

Patchset #6 (id:100001) has been deleted

[MuVen, 2014-12-06 12:51:37]

Patchset #7 (id:160001) has been deleted

[MuVen, 2014-12-06 12:51:45]

Patchset #6 (id:120001) has been deleted

[MuVen, 2014-12-06 12:53:58]

Patchset #6 (id:180001) has been deleted

[MuVen, 2014-12-06 13:22:53]

Patchset #6 (id:200001) has been deleted

[MuVen, 2014-12-06 13:27:03]

PTAL. 

Thanks,
~MuVen.

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo-expected.html
(right):

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo-expected.html:32:
<iframe src="resources/scrollable-iframe.html"></iframe>
On 2014/12/06 04:17:53, pdr wrote:
> Are we actually testing custom scrollbars in iframes with these? Don't we need
> something like resources/scrollable-iframe-active.html for the expected
result?

Done.

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
File LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html (right):

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html:25: width:
1000px;
On 2014/12/06 04:17:53, pdr wrote:
> Nit: Please fix the indentation in the tests. 8 or 4 or 2 spaces are fine as
> long as we're consistent in a file.

Done.

https://codereview.chromium.org/748513006/diff/80001/LayoutTests/scrollbars/c...
LayoutTests/scrollbars/custom-scrollbarthumb-inactive-pseudo.html:49:
setTimeout(function(){ changeFocus(); }, 100);
On 2014/12/06 04:17:53, pdr wrote:
> This timeout doesn't seem right. Was this added so that the iframes have time
to
> load? If so, that'll unfortunately lead to flakiness on our very slow bots
> (win-dbg) and slow tests on our fast bots (linux-rel). Something like this
> should work without settimeout:
> window.onload = function() {
>   if (window.testRunner) {
>     testRunner.setWindowsIsKey(false);
>   }
> }

Done.

https://codereview.chromium.org/748513006/diff/80001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/748513006/diff/80001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:1472:
view->invalidateAllCustomScrollbarsOnActiveChanged();
On 2014/12/06 04:17:53, pdr wrote:
> Should this be here or in pageActivationChanged?

I think its better to be here, as all window-inactive 
invalidations can be at one place.
(TextSelection on window-inactive: invalidatePaintForSelection.)
let me know if you want this to be place at another place.

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

https://codereview.chromium.org/748513006/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:350:
toFrameView(widget)->recalculateCustomScrollbarStyle();
Good Catch !!! thanks, we have to call
invalidateAllCustomScrollbarsOnActiveChanged() here too to invalidate
sub-frameviews. Updated the testcase with iframe, inside iframe.

[MuVen, 2014-12-06 15:29:34]

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

[commit-bot: I haz the power, 2014-12-06 15:30:31]

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

[commit-bot: I haz the power, 2014-12-06 15:34:08]

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

[commit-bot: I haz the power, 2014-12-06 15:34:09]

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

[MuVen, 2014-12-08 16:38:20]

@pdr, PTAL, in #46 comments i have mentioned about and addressed 
all the review comments.

[pdr., 2014-12-08 18:41:14]

On 2014/12/08 at 16:38:20, sataya.m wrote:
> @pdr, PTAL, in #46 comments i have mentioned about and addressed 
> all the review comments.

All my concerns have been addressed, thank you.

@skobes, does this patch look good to you?

[skobes, 2014-12-08 21:48:04]

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

https://codereview.chromium.org/748513006/diff/240001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:351:
toFrameView(widget)->invalidateAllCustomScrollbarsOnActiveChanged();
Sorry to revisit this but I am still confused by the recursive descent.  Doesn't
the iframe have its own FrameSelection object?  Is there some reason we would
get FrameSelection::focusedOrActiveStateChanged() only for the topmost frame?

[rune, 2014-12-08 23:14:25]

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

[rune, 2014-12-08 23:14:26]

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1467:
document->updateRenderTreeIfNeeded();
Updating the render tree synchronously immediately before invalidating the
custom scrollbars looks wrong.

Why do we need a synchronous render tree build here at all? It looks like it's
for RenderView::invalidatePaintForSelection below.

What happens if you have a scrollbar pseudo rule that sets the scrollbar to a
different width for :window-inactive? Will the selection invalidation be correct
if the selected text is reflowed at a different width due to the wider/narrower
scrollbar?

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1472:
view->invalidateAllCustomScrollbarsOnActiveChanged();
This is still called unconditionally even when there is no :window-inactive
selector present. Did we end up saying this was cheap enough? This can be a lot
of scrollbar parts in the worst case, right?

[MuVen, 2014-12-09 09:33:07]

PTAL.

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1467:
document->updateRenderTreeIfNeeded();
the text reflow has been solved earlier for the custom-scrollbar on width
changed. When there is change in CS thickness the RenderBox is set for layout
and the width available for the children is changed is mentioned during
RenderBlock width calculation. 

PTAL @ https://codereview.chromium.org/708283002/.

I have added custom-scrollbar-thumb-width-changed-on-inactive-pseudo.html
testcase for the same.

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1472:
view->invalidateAllCustomScrollbarsOnActiveChanged();
True. I would like to optimise this in my next patch if its ok for you ?

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

https://codereview.chromium.org/748513006/diff/240001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:351:
toFrameView(widget)->invalidateAllCustomScrollbarsOnActiveChanged();
Actually as i see there is an issue in the FrameSelection even. I have raised
issue for the same @ crbug.com/440313. Only focused element shouldnt be
invalidated. window-inactive css needs to be applied for all the elements with
in the document which has window-inactive css.

[rune, 2014-12-09 10:36:33]

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1467:
document->updateRenderTreeIfNeeded();
On 2014/12/09 at 09:33:07, muven wrote:
> the text reflow has been solved earlier for the custom-scrollbar on width
changed. When there is change in CS thickness the RenderBox is set for layout
and the width available for the children is changed is mentioned during
RenderBlock width calculation. 
> 
> PTAL @ https://codereview.chromium.org/708283002/.
> 
> I have added custom-scrollbar-thumb-width-changed-on-inactive-pseudo.html
testcase for the same.

But this still doesn't look right. You do:

  updateRenderTreeIfNeeded()
  invalidateAllCustomScrollbarsOnActiveChanged()
  invalidatePaintForSelection()

invalidateAllCustomScrollbarsOnActiveChanged may change the render tree, so why
not do this?:

  invalidateAllCustomScrollbarsOnActiveChanged()
  updateRenderTreeIfNeeded()
  invalidatePaintForSelection()

invalidateAllCustomScrollbarsOnActiveChanged may also require a layout that
708283002 introduces. Also since invalidatePaintForSelection() wants to work on
an up-to-date layout, I wonder why we just do an updateRenderTreeIfNeeded and
not an updateLayout. Ultimately, I think we want to do render tree changes and
layout for scrollbars and selection at the next frame display.

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1472:
view->invalidateAllCustomScrollbarsOnActiveChanged();
On 2014/12/09 at 09:33:07, muven wrote:
> True. I would like to optimise this in my next patch if its ok for you ?

Sure.

[MuVen, 2014-12-09 10:43:03]

Patchset #9 (id:280001) has been deleted

[MuVen, 2014-12-09 10:44:20]

@rune, PTAL.

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/748513006/diff/240001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1467:
document->updateRenderTreeIfNeeded();
On 2014/12/09 10:36:33, rune wrote:
> On 2014/12/09 at 09:33:07, muven wrote:
> > the text reflow has been solved earlier for the custom-scrollbar on width
> changed. When there is change in CS thickness the RenderBox is set for layout
> and the width available for the children is changed is mentioned during
> RenderBlock width calculation. 
> > 
> > PTAL @ https://codereview.chromium.org/708283002/.
> > 
> > I have added custom-scrollbar-thumb-width-changed-on-inactive-pseudo.html
> testcase for the same.
> 
> But this still doesn't look right. You do:
> 
>   updateRenderTreeIfNeeded()
>   invalidateAllCustomScrollbarsOnActiveChanged()
>   invalidatePaintForSelection()
> 
> invalidateAllCustomScrollbarsOnActiveChanged may change the render tree, so
why
> not do this?:
> 
>   invalidateAllCustomScrollbarsOnActiveChanged()
>   updateRenderTreeIfNeeded()
>   invalidatePaintForSelection()
> 
> invalidateAllCustomScrollbarsOnActiveChanged may also require a layout that
> 708283002 introduces. Also since invalidatePaintForSelection() wants to work
on
> an up-to-date layout, I wonder why we just do an updateRenderTreeIfNeeded and
> not an updateLayout. Ultimately, I think we want to do render tree changes and
> layout for scrollbars and selection at the next frame display.

true. It makes sense for me, Ideally updateRenderTreeIfNeeded should be after
invalidating CustomScrollbar.

[rune, 2014-12-09 10:51:54]

On 2014/12/09 at 10:44:20, sataya.m wrote:
> @rune, PTAL.

My issues are resolved now. Did you address skobes@ recursive descent comment?

[MuVen, 2014-12-09 10:53:42]

On 2014/12/09 10:51:54, rune wrote:
> On 2014/12/09 at 10:44:20, sataya.m wrote:
> > @rune, PTAL.
> 
> My issues are resolved now. Did you address skobes@ recursive descent comment?

yes, Actually as i see there is an issue in the FrameSelection even. I have
raised
issue for the same @ crbug.com/440313. Only focused element shouldnt be
invalidated. window-inactive css needs to be applied for all the elements with
in the document which has window-inactive css. For this i see there is need
for recursive call.

[MuVen, 2014-12-09 11:29:48]

PTAL.

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

https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:354: updateScrollbarGeometry();
This snippet was missed. If RootFrameView has CustomScrollbar and its style has
been changed then ScrollbarGeometry needs to be updated. I missed out that.

[rune, 2014-12-09 11:41:34]

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

https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:354: updateScrollbarGeometry();
On 2014/12/09 at 11:29:48, muven wrote:
> This snippet was missed. If RootFrameView has CustomScrollbar and its style
has been changed then ScrollbarGeometry needs to be updated. I missed out that.

I'm not an expert on scrollbar widgets, but doesn't this code call
updateScrollbarGeometry on the FrameView for every scrollbar in every overflow
container in that FrameView as well?

[MuVen, 2014-12-09 14:46:48]

On 2014/12/09 11:41:34, rune wrote:
>
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> Source/core/frame/FrameView.cpp:354: updateScrollbarGeometry();
> On 2014/12/09 at 11:29:48, muven wrote:
> > This snippet was missed. If RootFrameView has CustomScrollbar and its style
> has been changed then ScrollbarGeometry needs to be updated. I missed out
that.
> 
> I'm not an expert on scrollbar widgets, but doesn't this code call
> updateScrollbarGeometry on the FrameView for every scrollbar in every overflow
> container in that FrameView as well?

No, only for the RootFrameView scrollbar's this shall be called.

[rune, 2014-12-09 15:30:30]

On 2014/12/09 at 14:46:48, sataya.m wrote:
> On 2014/12/09 11:41:34, rune wrote:
> >
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> > File Source/core/frame/FrameView.cpp (right):
> > 
> >
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> > Source/core/frame/FrameView.cpp:354: updateScrollbarGeometry();
> > On 2014/12/09 at 11:29:48, muven wrote:
> > > This snippet was missed. If RootFrameView has CustomScrollbar and its
style
> > has been changed then ScrollbarGeometry needs to be updated. I missed out
that.
> > 
> > I'm not an expert on scrollbar widgets, but doesn't this code call
> > updateScrollbarGeometry on the FrameView for every scrollbar in every
overflow
> > container in that FrameView as well?
> 
> No, only for the RootFrameView scrollbar's this shall be called.

But is it?

[MuVen, 2014-12-09 15:35:51]

On 2014/12/09 15:30:30, rune wrote:
> On 2014/12/09 at 14:46:48, sataya.m wrote:
> > On 2014/12/09 11:41:34, rune wrote:
> > >
>
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> > > File Source/core/frame/FrameView.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> > > Source/core/frame/FrameView.cpp:354: updateScrollbarGeometry();
> > > On 2014/12/09 at 11:29:48, muven wrote:
> > > > This snippet was missed. If RootFrameView has CustomScrollbar and its
> style
> > > has been changed then ScrollbarGeometry needs to be updated. I missed out
> that.
> > > 
> > > I'm not an expert on scrollbar widgets, but doesn't this code call
> > > updateScrollbarGeometry on the FrameView for every scrollbar in every
> overflow
> > > container in that FrameView as well?
> > 
> > No, only for the RootFrameView scrollbar's this shall be called.
> 
> But is it?

yes, if iframe has custom-scrollbars in that case  if (widget->isFrameView())
will
be true and the recalculateCustomScrollbarStyle shall take care of
updatingScrollbarGeometry.

When it comes to individual scrollbar of FrameView then the scrollbar needs to
adjusted.

[rune, 2014-12-09 15:59:09]

On 2014/12/09 at 15:35:51, sataya.m wrote:
> On 2014/12/09 15:30:30, rune wrote:
> > On 2014/12/09 at 14:46:48, sataya.m wrote:
> > > On 2014/12/09 11:41:34, rune wrote:
> > > >
> >
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> > > > File Source/core/frame/FrameView.cpp (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> > > > Source/core/frame/FrameView.cpp:354: updateScrollbarGeometry();
> > > > On 2014/12/09 at 11:29:48, muven wrote:
> > > > > This snippet was missed. If RootFrameView has CustomScrollbar and its
> > style
> > > > has been changed then ScrollbarGeometry needs to be updated. I missed
out
> > that.
> > > > 
> > > > I'm not an expert on scrollbar widgets, but doesn't this code call
> > > > updateScrollbarGeometry on the FrameView for every scrollbar in every
> > overflow
> > > > container in that FrameView as well?
> > > 
> > > No, only for the RootFrameView scrollbar's this shall be called.
> > 
> > But is it?
> 
> yes, if iframe has custom-scrollbars in that case  if (widget->isFrameView())
will
> be true and the recalculateCustomScrollbarStyle shall take care of
updatingScrollbarGeometry.
> 
> When it comes to individual scrollbar of FrameView then the scrollbar needs to
adjusted.

But updateScrollbarGeometry() is called for every single scrollbar in every
single frame. Why does it have to been inside the loop? If it's for the root
view, why can't you just do it after adjusting the scrollbars of the root frame
view?

Can you change the recursiveness to handle the root frame view as any other
frame view and let recalculateCustomScrollbarStyle() handle
updateScrollbarGeometry for that as well?

[MuVen, 2014-12-09 16:11:26]

On 2014/12/09 15:59:09, rune wrote:
> On 2014/12/09 at 15:35:51, sataya.m wrote:
> > On 2014/12/09 15:30:30, rune wrote:
> > > On 2014/12/09 at 14:46:48, sataya.m wrote:
> > > > On 2014/12/09 11:41:34, rune wrote:
> > > > >
> > >
>
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> > > > > File Source/core/frame/FrameView.cpp (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/748513006/diff/320001/Source/core/frame/Frame...
> > > > > Source/core/frame/FrameView.cpp:354: updateScrollbarGeometry();
> > > > > On 2014/12/09 at 11:29:48, muven wrote:
> > > > > > This snippet was missed. If RootFrameView has CustomScrollbar and
its
> > > style
> > > > > has been changed then ScrollbarGeometry needs to be updated. I missed
> out
> > > that.
> > > > > 
> > > > > I'm not an expert on scrollbar widgets, but doesn't this code call
> > > > > updateScrollbarGeometry on the FrameView for every scrollbar in every
> > > overflow
> > > > > container in that FrameView as well?
> > > > 
> > > > No, only for the RootFrameView scrollbar's this shall be called.
> > > 
> > > But is it?
> > 
> > yes, if iframe has custom-scrollbars in that case  if
(widget->isFrameView())
> will
> > be true and the recalculateCustomScrollbarStyle shall take care of
> updatingScrollbarGeometry.
> > 
> > When it comes to individual scrollbar of FrameView then the scrollbar needs
to
> adjusted.
> 
> But updateScrollbarGeometry() is called for every single scrollbar in every
> single frame. Why does it have to been inside the loop? If it's for the root
> view, why can't you just do it after adjusting the scrollbars of the root
frame
> view?
> 
> Can you change the recursiveness to handle the root frame view as any other
> frame view and let recalculateCustomScrollbarStyle() handle
> updateScrollbarGeometry for that as well?

Sorry if i had confused you. For example if i frame has a i frame and a 2  child
scrollbars, 
In that case iframe frameview calculation is taken care by first if  conditiin
loop and the child  shall 
Scrollbars  style is changed but needs its geometry updated. In that  case this
shall be helpful
To avoid regressions.

[rune, 2014-12-09 18:02:35]

On 2014/12/09 at 16:11:26, sataya.m wrote:
> On 2014/12/09 15:59:09, rune wrote:

> > But updateScrollbarGeometry() is called for every single scrollbar in every
> > single frame. Why does it have to been inside the loop? If it's for the root
> > view, why can't you just do it after adjusting the scrollbars of the root
frame
> > view?
> > 
> > Can you change the recursiveness to handle the root frame view as any other
> > frame view and let recalculateCustomScrollbarStyle() handle
> > updateScrollbarGeometry for that as well?
> 
> Sorry if i had confused you. For example if i frame has a i frame and a 2 
child scrollbars, 
> In that case iframe frameview calculation is taken care by first if  conditiin
loop and the child  shall 
> Scrollbars  style is changed but needs its geometry updated. In that  case
this shall be helpful
> To avoid regressions.

I don't think I'm confused. I applied patch set 10 here and a single-frame
document with 30 overflow:scroll containers calls your added
updateScrollbarGeometry() on the top frame 171 times for a single focus change.
That's not intentional, right?

[MuVen, 2014-12-09 18:18:28]

On 2014/12/09 18:02:35, rune wrote:
> On 2014/12/09 at 16:11:26, sataya.m wrote:
> > On 2014/12/09 15:59:09, rune wrote:
> 
> > > But updateScrollbarGeometry() is called for every single scrollbar in
every
> > > single frame. Why does it have to been inside the loop? If it's for the
root
> > > view, why can't you just do it after adjusting the scrollbars of the root
> frame
> > > view?
> > > 
> > > Can you change the recursiveness to handle the root frame view as any
other
> > > frame view and let recalculateCustomScrollbarStyle() handle
> > > updateScrollbarGeometry for that as well?
> > 
> > Sorry if i had confused you. For example if i frame has a i frame and a 2 
> child scrollbars, 
> > In that case iframe frameview calculation is taken care by first if 
conditiin
> loop and the child  shall 
> > Scrollbars  style is changed but needs its geometry updated. In that  case
> this shall be helpful
> > To avoid regressions.
> 
> I don't think I'm confused. I applied patch set 10 here and a single-frame
> document with 30 overflow:scroll containers calls your added
> updateScrollbarGeometry() on the top frame 171 times for a single focus
change.
> That's not intentional, right?

true, it makes sense if its pulled out of for loop.

[MuVen, 2014-12-10 06:05:21]

Hi Rune/Skobes,

I have added custom-scrollbar-adjust-on-inactive-pseudo.html testcase and added
proper checks to call recursive API's.

PTAL.

Thanks,
~MuVen.

[rune, 2014-12-10 08:29:32]

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

https://codereview.chromium.org/748513006/diff/330001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:352: if
(toFrameView(widget)->hasCustomScrollbars())
hasCustomScrollbars() is traversing the whole widget sub-tree in the case where
you have no custom scrollbars. In the case where you find a custom scrollbar and
return from hasCustomScrollbars(), you'll enter and traverse the same widgets
again. This if-check only makes this code more expensive.


How about doing this instead? (I haven't tested that it works):

void FrameView::invalidateAllCustomScrollbarsOnActiveChanged()
{
    const ChildrenWidgetSet* viewChildren = children();
    for (const RefPtrWillBeMember<Widget>& child : *viewChildren) {
        Widget* widget = child.get();
        if (widget->isFrameView())
            toFrameView(widget)->invalidateAllCustomScrollbarsOnActiveChanged();
        else if (widget->isScrollbar())
            toScrollbar(widget)->styleChanged();
    }

    recalculateCustomScrollbarStyle();
}

[MuVen, 2014-12-10 08:44:33]

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

https://codereview.chromium.org/748513006/diff/330001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:352: if
(toFrameView(widget)->hasCustomScrollbars())
yes this is much simpler, I have verified all the test, its passing. PTAL of
updated patch with these changes.

[rune, 2014-12-10 08:51:38]

lgtm if you do tabs -> spaces in your tests.

Didn't the check-webkit-style script choke on those tabs?

https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
File LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html
(right):

https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:6:
height: 12px;
Something funny with the indentation.

https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:25:
height: 150px;
Indentation

https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:29:
-webkit-border-radius: 1px;
Indentation.

https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:34:
height: 1000px;
Indentation

https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:39:
height: 1500px;
Indentation

[MuVen, 2014-12-10 09:02:02]

On 2014/12/10 08:51:38, rune wrote:
> lgtm if you do tabs -> spaces in your tests.
> 
> Didn't the check-webkit-style script choke on those tabs?
> 
>
https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
> File LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html
> (right):
> 
>
https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
> LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:6:
> height: 12px;
> Something funny with the indentation.
> 
>
https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
> LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:25:
> height: 150px;
> Indentation
> 
>
https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
> LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:29:
> -webkit-border-radius: 1px;
> Indentation.
> 
>
https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
> LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:34:
> height: 1000px;
> Indentation
> 
>
https://codereview.chromium.org/748513006/diff/350001/LayoutTests/scrollbars/...
> LayoutTests/scrollbars/custom-scrollbar-adjust-on-inactive-pseudo.html:39:
> height: 1500px;
> Indentation

thanks a lot for your patient review.

[MuVen, 2014-12-10 09:02:10]

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

[commit-bot: I haz the power, 2014-12-10 09:03:05]

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

[commit-bot: I haz the power, 2014-12-10 10:27:40]

Committed patchset #14 (id:390001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186842