In ScrollView::updateScrollbars, it called visibleContentsResized when scrollbars are shown or hidden. but, frameFlattening option can block creation of scrollbar. so we don't need to call visibleContentsResized.
Created attachment 70030 [details] Patch
Comment on attachment 70030 [details] Patch Seems reasonable to me. Did you see any perf gain?
(In reply to comment #2) > (From update of attachment 70030 [details]) > Seems reasonable to me. Did you see any perf gain? Sorry about late answer. yes, I saw sluggish scrolling at our mobile browser using frameflattening option. Those sites contains frameset or iframe. And this patch scrolling this kinds of sites make very faster than before.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 70030 [details] [details]) > > Seems reasonable to me. Did you see any perf gain? > > Sorry about late answer. > > yes, I saw sluggish scrolling at our mobile browser using frameflattening option. > Those sites contains frameset or iframe. > And this patch scrolling this kinds of sites make very faster than before. Strange, only relayout could affect scrolling, so are these sites for some reason relayouting the whole time? Maybe it is better to look into this? or optimize the frame flattening layout code. Also, frame flattening is needed for a nice touch interaction model. That is way more important than the performance.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 70030 [details] [details] [details]) > > > Seems reasonable to me. Did you see any perf gain? > > > > Sorry about late answer. > > > > yes, I saw sluggish scrolling at our mobile browser using frameflattening option. > > Those sites contains frameset or iframe. > > And this patch scrolling this kinds of sites make very faster than before. > > Strange, only relayout could affect scrolling, so are these sites for some reason relayouting the whole time? Maybe it is better to look into this? or optimize the frame flattening layout code. > you're right. this patch want not to relayout if scrollbar wasn't changed. If not, ScrollView calls contentsResized and visiblieContentsResized although scrollbar wasn't created because of frameflattening. And my test site is http://postech.edu/ > Also, frame flattening is needed for a nice touch interaction model. That is way more important than the performance. Yes, so we want to use frame flattening. But I found that it's too sluggish case and I believe that this patch remove it without any side effect.
Ah, I understood now, so with this patch scrolling performance with frame flattening is improved. Hyatt, can you have a look at this one, please?
Generally, a user-feel improvement is valid, but there are many more accurate way to measure a performance improvement, e.g. frames per second ...
(In reply to comment #7) > Generally, a user-feel improvement is valid, but there are many more accurate way to measure a performance improvement, e.g. frames per second ... Hi, I tested using our mobile browser implemented by EFL. I don't have idea which browser(not EFL) can test this. I use ECORE_EVAS_FPS_DEBUG to get below data without any changes. without our patch. FRAME: 454, FPS: 28.0, RTIME 65% FRAME: 469, FPS: 28.7, RTIME 64% FRAME: 484, FPS: 28.3, RTIME 65% FRAME: 499, FPS: 28.9, RTIME 65% FRAME: 514, FPS: 28.4, RTIME 64% FRAME: 529, FPS: 28.4, RTIME 64% FRAME: 544, FPS: 28.4, RTIME 66% FRAME: 559, FPS: 28.1, RTIME 66% FRAME: 574, FPS: 29.3, RTIME 65% FRAME: 589, FPS: 28.5, RTIME 67% FRAME: 604, FPS: 28.4, RTIME 65% FRAME: 619, FPS: 28.6, RTIME 65% FRAME: 634, FPS: 28.8, RTIME 64% FRAME: 649, FPS: 29.0, RTIME 68% FRAME: 664, FPS: 28.7, RTIME 65% FRAME: 679, FPS: 28.6, RTIME 64% FRAME: 694, FPS: 28.6, RTIME 66% FRAME: 708, FPS: 27.8, RTIME 64% FRAME: 723, FPS: 29.0, RTIME 68% FRAME: 738, FPS: 28.4, RTIME 66% FRAME: 753, FPS: 28.7, RTIME 65% FRAME: 768, FPS: 28.6, RTIME 66% FRAME: 784, FPS: 31.3, RTIME 69% FRAME: 800, FPS: 31.4, RTIME 69% FRAME: 817, FPS: 33.4, RTIME 63% with our patch FRAME: 374, FPS: 23.8, RTIME 33% FRAME: 404, FPS: 59.4, RTIME 77% FRAME: 435, FPS: 62.0, RTIME 77% FRAME: 465, FPS: 58.8, RTIME 76% FRAME: 495, FPS: 59.4, RTIME 76% FRAME: 526, FPS: 60.3, RTIME 75% FRAME: 556, FPS: 59.9, RTIME 75% FRAME: 588, FPS: 63.1, RTIME 79% FRAME: 619, FPS: 62.0, RTIME 77% FRAME: 651, FPS: 62.9, RTIME 78% FRAME: 682, FPS: 61.5, RTIME 77% FRAME: 714, FPS: 61.9, RTIME 76% FRAME: 745, FPS: 61.8, RTIME 78% FRAME: 777, FPS: 63.2, RTIME 77% FRAME: 809, FPS: 63.1, RTIME 77% FRAME: 840, FPS: 61.0, RTIME 79% FRAME: 877, FPS: 73.8, RTIME 83%
Comment on attachment 70030 [details] Patch Should the caller just check avoidScrollbarCreation? Or maybe sendContentResizedNotification should just be set to the return value of the setHasHorizontalScrollbar call.
Comment on attachment 70030 [details] Patch Ryan, I know your intentions and it generally makes sense. But I think doing as Eric suggested it clearer. It is strange that a setter returns something.
(In reply to comment #9) > (From update of attachment 70030 [details]) > Should the caller just check avoidScrollbarCreation? Or maybe sendContentResizedNotification should just be set to the return value of the setHasHorizontalScrollbar call. Thank you for your comment. I realized my mistake. (In reply to comment #10) > (From update of attachment 70030 [details]) > Ryan, I know your intentions and it generally makes sense. But I think doing as Eric suggested it clearer. > > It is strange that a setter returns something. I'll make newer like Eric mentioned. Thanks.
Created attachment 76465 [details] Patch
Comment on attachment 76465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76465&action=review > WebCore/ChangeLog:15 > + You have to verify that you do not break the frame flattening tests with this change. Are you running the layout tests?
Comment on attachment 76465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76465&action=review Also as kenneth said, Qt Gtk and Mac DRT test the flame flattening feature. Please, make sure there is no regression. > WebCore/platform/ScrollView.cpp:102 > void ScrollView::setHasVerticalScrollbar(bool hasBar) > { > - if (hasBar && avoidScrollbarCreation()) > + if (hasBar) Well, if we go for it, I would also suggest to add an assert here: ASSERT(avoidScrollbarCreation()) That way we ensure that no one is calling setHasVerticalScrollbar() without previously calling avoidScrollbarCreation()
> > ASSERT(avoidScrollbarCreation()) Of course I meant ASSERT(!avoidScrollbarCreation())
(In reply to comment #13) > (From update of attachment 76465 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76465&action=review > > > WebCore/ChangeLog:15 > > + > > You have to verify that you do not break the frame flattening tests with this change. Are you running the layout tests? Sorry. With layout tests, I got failed. I think that I should investigate more. (In reply to comment #14) > (From update of attachment 76465 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76465&action=review > > Also as kenneth said, Qt Gtk and Mac DRT test the flame flattening feature. Please, make sure there is no regression. > > > WebCore/platform/ScrollView.cpp:102 > > void ScrollView::setHasVerticalScrollbar(bool hasBar) > > { > > - if (hasBar && avoidScrollbarCreation()) > > + if (hasBar) > > Well, if we go for it, I would also suggest to add an assert here: > > ASSERT(avoidScrollbarCreation()) > > That way we ensure that no one is calling setHasVerticalScrollbar() without previously calling avoidScrollbarCreation() Sorry, This patch was wrong. I'll prepare new patch after those testing.
Created attachment 78365 [details] Patch
Comment on attachment 78365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78365&action=review > Source/WebCore/platform/ScrollView.cpp:89 > + ASSERT(!(hasBar && avoidScrollbarCreation())); I don't understand why the "hasBar && avoidScrollbarCreation()" test no longer returns. You assert if this case is true -- do you want to execute the code under this condition? > Source/WebCore/platform/ScrollView.cpp:105 > + ASSERT(!(hasBar && avoidScrollbarCreation())); Ditto
Created attachment 89577 [details] Patch
(In reply to comment #18) > (From update of attachment 78365 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78365&action=review > > > Source/WebCore/platform/ScrollView.cpp:89 > > + ASSERT(!(hasBar && avoidScrollbarCreation())); > > I don't understand why the "hasBar && avoidScrollbarCreation()" test no longer returns. You assert if this case is true -- do you want to execute the code under this condition? > > > Source/WebCore/platform/ScrollView.cpp:105 > > + ASSERT(!(hasBar && avoidScrollbarCreation())); > > Ditto Sorry about late answer. This patch is old and I can't assure any side effects. So I simplified my patch to explain what I really want. It will be fine, but check hasBar && avoidScrollbarCreation two times.
James should see this go by.
Comment on attachment 89577 [details] Patch Seems reasonable to me. But I know very little about the scrollbar code.
James, Hyatt, Peter? Someone who's worked in scrollbars?
Comment on attachment 89577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89577&action=review In theory this seems OK, but I'm not a WebKit reviewer. > Source/WebCore/platform/ScrollView.cpp:500 > if (hasHorizontalScrollbar != newHasHorizontalScrollbar) { Nit: I think it might be clearer to write this block as follows: if (hasHorizontalScrollbar != newHasHorizontalScrollbar && (hasHorizontalScrollbar || !avoidScrollbarCreation())) { ... original block contents here... } ...and the same for the case below. This makes it a bit clearer that we don't want to do anything in the case where we're supposed to avoid scrollbar creation. I would also like to see setHasXXXScrollbar() converted from conditional-checking (hasBar && avoidScrollbarCreation()) into asserting that that combination is false. Either this is safe because these are the only places which call these functions, or else your patch should probably be expanded to the other callers thereof. In either case, making this expectation part of those functions' contracts seems clearer and safer long-term to me.
Created attachment 99804 [details] Patch
(In reply to comment #24) > (From update of attachment 89577 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89577&action=review > > In theory this seems OK, but I'm not a WebKit reviewer. > > > Source/WebCore/platform/ScrollView.cpp:500 > > if (hasHorizontalScrollbar != newHasHorizontalScrollbar) { > > Nit: I think it might be clearer to write this block as follows: > > if (hasHorizontalScrollbar != newHasHorizontalScrollbar && (hasHorizontalScrollbar || !avoidScrollbarCreation())) { > ... original block contents here... > } > > ...and the same for the case below. This makes it a bit clearer that we don't want to do anything in the case where we're supposed to avoid scrollbar creation. > > I would also like to see setHasXXXScrollbar() converted from conditional-checking (hasBar && avoidScrollbarCreation()) into asserting that that combination is false. Either this is safe because these are the only places which call these functions, or else your patch should probably be expanded to the other callers thereof. In either case, making this expectation part of those functions' contracts seems clearer and safer long-term to me. Thank you for your comment. I updated like you mentioned.
Comment on attachment 99804 [details] Patch Informal r+ from me; you'll need a real reviewer to do the formal r+.
Comment on attachment 99804 [details] Patch Thanks Peter and Ryuan.
Comment on attachment 99804 [details] Patch Clearing flags on attachment: 99804 Committed r90478: <http://trac.webkit.org/changeset/90478>
All reviewed patches have been landed. Closing bug.