Bug 84933 – [EFL] scrolling with fixed position does not work correctly when using ewk_view_tiled.

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Ryuan Choi]

For now, DRT/Efl can not pass compositing/geometry/fixed-position.html.

It's because ewk_view_tiled does not know whether scrolled when scrolling as slow path.

[Ryuan Choi]

IIRC, tomasz already fixed this issue in a local.

[Tomasz Morawski]

Created attachment 140686 [details]
Fix for issue

Patch for discussion.

[Ryuan Choi]

Comment on attachment 140686 [details]
Fix for issue

View in context: https://bugs.webkit.org/attachment.cgi?id=140686&action=review

Thanks for the patch.

> Source/WebKit/efl/ChangeLog:9
> +        Sets backing store's full offeset when ChromeClientEfl::invalidateContentsForSlowScroll is
> +        called.

Could you explain more?

And I think that this patch can fix compositing/geometry/fixed-position.html now.

Could you rebase it and add some comment about it?

> Source/WebKit/efl/ewk/ewk_view.h:145
> +    void (*backing_store_offset_update)(Ewk_View_Smart_Data *sd);

Are there any use case that any applications want to override this?

IMO, it is better not to touch public structure if not.

If you want to use it only for ewk_view_tiled,
What do you think about just adding EWK_VIEW_TYPE_CHECK_OR_RETURN in ewk_view_scroll_offset_update
like ewk_view_frame_view_creation_notify?

[Tomasz Morawski]

(In reply to comment #3)
> If you want to use it only for ewk_view_tiled,
> What do you think about just adding EWK_VIEW_TYPE_CHECK_OR_RETURN in ewk_view_scroll_offset_update
> like ewk_view_frame_view_creation_notify?
Yes, I agree that it should not be in public API. But current architecture doesn't allow solve this issue in proper way. Moreover, IMO any implementation that is in ewk_view.cpp file should be generic for all view type. I think it should be not allowed to put specific single/tiled view implementation in this file. For example it is not good to have ewk_view_frame_view_creation_notify implementation in ewk_view file and IMO it should be removed from there. I know the way that you proposed is simple but we need something what will simulate "protected" scope from C++ language. For example pointers to function that should not be in public API (not in _Ewk_View_Smart_Class) should be placed in new _Ewk_View_Private_Smart_Class structure in ewk_view_private.h file. This will solve this and similar issue (like ewk_view_frame_view_creation_notify).

[Ryuan Choi]

(In reply to comment #4)
> (In reply to comment #3)
> > If you want to use it only for ewk_view_tiled,
> > What do you think about just adding EWK_VIEW_TYPE_CHECK_OR_RETURN in ewk_view_scroll_offset_update
> > like ewk_view_frame_view_creation_notify?
> Yes, I agree that it should not be in public API. But current architecture doesn't allow solve this issue in proper way. Moreover, IMO any implementation that is in ewk_view.cpp file should be generic for all view type. I think it should be not allowed to put specific single/tiled view implementation in this file. For example it is not good to have ewk_view_frame_view_creation_notify implementation in ewk_view file and IMO it should be removed from there. I know the way that you proposed is simple but we need something what will simulate "protected" scope from C++ language. For example pointers to function that should not be in public API (not in _Ewk_View_Smart_Class) should be placed in new _Ewk_View_Private_Smart_Class structure in ewk_view_private.h file. This will solve this and similar issue (like ewk_view_frame_view_creation_notify)

I also agree that it is not cool.
But, I am now thinking that changing API for the internal requirements is worse
and introducing additional structure for few function of ewk_view_tiled is over-implementation.

I want to listen more opinion for this.

Thanks.

[KwangHyuk]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > If you want to use it only for ewk_view_tiled,
> > > What do you think about just adding EWK_VIEW_TYPE_CHECK_OR_RETURN in ewk_view_scroll_offset_update
> > > like ewk_view_frame_view_creation_notify?
> > Yes, I agree that it should not be in public API. But current architecture doesn't allow solve this issue in proper way. Moreover, IMO any implementation that is in ewk_view.cpp file should be generic for all view type. I think it should be not allowed to put specific single/tiled view implementation in this file. For example it is not good to have ewk_view_frame_view_creation_notify implementation in ewk_view file and IMO it should be removed from there. I know the way that you proposed is simple but we need something what will simulate "protected" scope from C++ language. For example pointers to function that should not be in public API (not in _Ewk_View_Smart_Class) should be placed in new _Ewk_View_Private_Smart_Class structure in ewk_view_private.h file. This will solve this and similar issue (like ewk_view_frame_view_creation_notify)
> 
> I also agree that it is not cool.
> But, I am now thinking that changing API for the internal requirements is worse
> and introducing additional structure for few function of ewk_view_tiled is over-implementation.
> 
> I want to listen more opinion for this.
> 
> Thanks.

IMO, using smart change api for the object would be possible. :)

[Rafael Antognolli]

This makes sense to me.

But please, just make sure that it won't break anything else, since you are introducing an offset change in the middle of a repaint request, and the order that scrolls and repaints are processed used to make difference (I don't remember if in the end we fixed them to the order don't matter anymore).

I guess that the layout tests should take care of checking for any regression regarding this.

Other than that, the patch looks good.

[Ryuan Choi]

(In reply to comment #7)
> This makes sense to me.
> 
> But please, just make sure that it won't break anything else, since you are introducing an offset change in the middle of a repaint request, and the order that scrolls and repaints are processed used to make difference (I don't remember if in the end we fixed them to the order don't matter anymore).
> 
> I guess that the layout tests should take care of checking for any regression regarding this.
> 
> Other than that, the patch looks good.

If the major like current patch, I will follow it.

Tomasz, could you rebase the patch ?

[Tomasz Morawski]

Comment on attachment 140686 [details]
Fix for issue

Patch doesn't fix the issue now.