Bug 38418

Created attachment 54864 [details] Fix This probably needs another set of eyes because it affects non-AC code paths as well.

Created attachment 54865 [details] Re-upload

It looks like this only covers the case for the main frame, but not if the same happens within a child frame. AFAICS in the mac code that's done per frame. I wonder if the right fix is cross-platform inside WebCore instead of going through the WebKit layer and then back into WebCore?

I think it should be platform specific - there's no way for WebCore to know when the platform actually scrolled.

(In reply to comment #4) > I think it should be platform specific - there's no way for WebCore to know > when the platform actually scrolled. Why not? In your patch you're modifying ChromeClient(Qt)::scroll, which _is_ cross-platform and the place where WebCore tells the platform to scroll. The other way around, when the upper application layer wants to scroll the WebCore::ScrollView, then it calls functions like scrollBy() inside WebCore. I must be missing something :)

hmm, I guess you're right. But wouldn't that mess with the Apple implementation?

I researched further: this has to be platform specific, as other platforms might choose to implement scrolling in an async way. scrollPoitionChanged() should be called after the async scroll operation actually took place. In Qt we do sync scrolling, so it happens that we can call scrollPositionChanged() directly - but that behavior is platform specific. Therefore, I can't think of a differrent/more appropriate way to do this than ChromeClientQt :)

(In reply to comment #7) > I researched further: this has to be platform specific, as other platforms might choose to implement scrolling in an async way. scrollPoitionChanged() should be called after the async scroll operation actually took place. In Qt we do sync scrolling, so it happens that we can call scrollPositionChanged() directly - but that behavior is platform specific. > Therefore, I can't think of a differrent/more appropriate way to do this than ChromeClientQt :) You're right. Ok, what about the main frame vs. child frames point? :)

Created attachment 57707 [details] Different approach How about this approach then (see new attachment). It's not for review yet, just discussion.

Comment on attachment 57707 [details] Different approach I like this approach :) WebCore/platform/HostWindow.h:60 + // Returns true if The docs should give some more help, so that the other ports have an idea what to do here ;)

Comment on attachment 54865 [details] Re-upload r- because of lack of frame handling, but the new patch looks promising :)

What's missing for this to handle frames? It's implemented entirely in ScrollView so it should work for anything.

Also, can we add it for the common use case and test frames later? I don't get the logic of "Let's reject a bug that fixes FOO because it doesn't fix BAR" :) No'am

I think your third party is good, I like the approach. I r-'ed the second patch, because your third patch replaces it. Please add a changelog to the third one and put it up for review :)

ah, I see. my misunderstanding. New patch coming up :)

This was apparently fixed, see bug 33150. *** This bug has been marked as a duplicate of bug 33150 ***

(In reply to comment #16) > This was apparently fixed, see bug 33150. > > *** This bug has been marked as a duplicate of bug 33150 *** I have not checked the bug but it looks unrelated to 33150. On scroll, the layers are updated via FrameView::scrollPositionChanged() See https://bugs.webkit.org/show_bug.cgi?id=36783 && https://bugs.webkit.org/show_bug.cgi?id=38286 https://bugs.webkit.org/show_bug.cgi?id=33150 improves performance but don't really change the behavior.

It could have been 39918 then. The point is the bug is no longer there.