Skip to content

Add fullscreen API#86

Merged
aperezdc merged 1 commit intoWebPlatformForEmbedded:masterfrom
MortimerGoro:mortimer/fullscreen
Aug 4, 2021
Merged

Add fullscreen API#86
aperezdc merged 1 commit intoWebPlatformForEmbedded:masterfrom
MortimerGoro:mortimer/fullscreen

Conversation

@MortimerGoro
Copy link
Contributor

@MortimerGoro MortimerGoro commented Jul 14, 2021

  • Receive DOM fullscreen state via wpe_view_backend_set_fullscreen
  • Dispatch exit/enter fullscreem from libwpe to the client (e.g. WPEView in Webkit)

Corresponding WPE WebKit patch: https://bugs.webkit.org/show_bug.cgi?id=227951

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.

Functionality-wise I think the patch looks fine, though it was a bit hard to follow what's going on and how the API is to be used without looking at the accompanying patch for WebKit and the PR for Cog. Would it be possible to add some reference documentation?

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.

This is starting to look ready for merging, thanks for working on it! 💪🏼

I have left a few comments with suggestions, and it would be great if we could have @zdobersek and/or @carlosgcampos check this and rubber-stamp that they are okay with the new API.

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.

This looks almost ready, thanks! It will only need a couple of touch ups in the documentation comments.

In the meantime, I will try to get someone else to do a review as well, I would prefer to have one more person agree on new API before merging.

As a side note, I noticed a bug in HotDoc while trying to build the documentation added by this MR locally, and I have reported it: hotdoc/hotdoc#230 🙃

@MortimerGoro MortimerGoro force-pushed the mortimer/fullscreen branch from 7115f40 to 6e8fccf Compare July 30, 2021 09:33
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.

This looks mergeable to me. Waiting to have a second opinion on the new API, hopefully @zdobersek and/or @carlosgcampos can take a look soon 👍🏼

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.

Thanks @carlosgcampos for commenting, and @MortimerGoro for updating the PR once again! I think we can consider this now approved, right?

@aperezdc aperezdc added this to the 1.12.0 milestone Aug 4, 2021
@aperezdc
Copy link
Contributor

aperezdc commented Aug 4, 2021

Let's merge this, I would like to make a development release including this feature, and it looks like @carlosgcampos should be happy now with the function name changes applied. If there are any other tweaks on top of this we may want before 1.12, we can use follow-up PRs.

@aperezdc aperezdc merged commit 1f36ea8 into WebPlatformForEmbedded:master Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants