Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popup menus #57

Closed
wants to merge 3 commits into from
Closed

Conversation

JPEWdev
Copy link
Contributor

@JPEWdev JPEWdev commented Aug 27, 2019

Adds support for WebKit to allocate popup menus.

Moving the input handling to a separate object that wpe_view_backend
contains allows other objects to also be treated as input objects.
The platform backend interface allows WebKit core to call into platform
specific backend API, which will eventually be used to implement popup
menu support.
Adds support for allocating popup menus from then platform backend,
allocating buffers, and attaching them to the popup menu.
@JPEWdev JPEWdev changed the title Popup menu Popup menus Aug 27, 2019
Copy link
Contributor

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of doing this, and funneling things through libwpe?

The WebKitWebView:show-option-menu can be used directly by an embedding application to implement popups in any way it may need already. For example this is done in Igalia/cog#190 using Cairo to paint the menus—but it could have used anything else.

Unless there is some use case that this solves and cannot be taken care of handling the show-option-menu signal, I would rather not merge this 🤔

@JPEWdev
Copy link
Contributor Author

JPEWdev commented Oct 28, 2020

I think this is unnecessary and I just forgot to close it. Thanks :)

@JPEWdev JPEWdev closed this Oct 28, 2020
Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently I have a pending review on this PR from over a year ago that I hadn't submitted, oops. :P Anyway, just a warning there are ABI issues here:


struct wpe_view_platform_interface {
bool (*create_popup)(void*, struct wpe_popup*, int32_t, int32_t, int32_t, int32_t, int32_t, int32_t);
bool (*alloc_buffer)(void*, struct wpe_buffer*, uint32_t, uint32_t, uint32_t);

void (*_wpe_reserved0)(void);
void (*_wpe_reserved1)(void);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to remove the last two bytes of padding here to avoid breaking ABI.

struct wpe_popup_interface {
void (*destroy)(void*);
void (*attach_buffer)(void*, void*);
void (*get_info)(void*, struct wpe_popup_info*);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, you'll want to add several padding bytes here so that you can expand this struct in the future. I guess four bytes is standard for libwpe.


struct wpe_buffer_interface {
void (*destroy)(void*);
void (*get_info)(void*, struct wpe_buffer_info* info);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@mcatanzaro
Copy link
Contributor

Not that it matters anymore. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants