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

beginFrameSubscription bugfix and improvement #6164

Merged
merged 8 commits into from Jun 26, 2016

Conversation

Projects
None yet
3 participants
@brenca
Member

brenca commented Jun 21, 2016

With the current implementation, every time we were notified about a presentation event we asked for a copy of the BackingStore, which sadly caused another presentation event, which caused a landslide of presentation events. To fix this, we now check for the damaged_rect, which is empty if we ask for a copy of the BackingStore.

I've also added another option to the subscriber that allows the user to only get the damaged area in the frameBuffer, potentially increasing performance. Specifying this is oprional, so it shouldn't break currently functioning apps using the API.

const FrameCaptureCallback& callback)
: isolate_(isolate), view_(view), callback_(callback), weak_factory_(this) {
const FrameCaptureCallback& callback,
const bool& only_damaged)

This comment has been minimized.

@zcbenz

zcbenz Jun 21, 2016

Contributor

For primitive types, bool is preferred than const bool&.

@@ -27,21 +30,27 @@ bool FrameSubscriber::ShouldCaptureFrame(
if (!view_ || !host)
return false;
const auto size = view_->GetVisibleViewportSize();
if (damage_rect.width() == 0 || damage_rect.height() == 0)

This comment has been minimized.

@zcbenz

zcbenz Jun 21, 2016

Contributor

We can use damage_rect.IsEmpty().

@@ -917,19 +917,26 @@ For the `mouseWheel` event, the `event` object also have following properties:
* `hasPreciseScrollingDeltas` Boolean
* `canScroll` Boolean
### `webContents.beginFrameSubscription(callback)`
### `webContents.beginFrameSubscription(callback, onlyDamaged)`

This comment has been minimized.

@zcbenz

zcbenz Jun 21, 2016

Contributor

Current convention for APIs is to put callback as last parameter, can you make the signature of this API look like below?

beginFrameSubscription([onlyDamaged, ]callback)
@miniak

This comment has been minimized.

Contributor

miniak commented Jun 22, 2016

I would consider using a better name than onlyDamaged, affected areas are usually referred to as dirty not damaged.

@brenca

This comment has been minimized.

Member

brenca commented Jun 25, 2016

@miniak Agreed, dirty area is a more common name for the affected areas, but I originally decided to go with how chromium named things in their code: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_browsertest.cc?rcl=1466839340&l=257

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Jun 26, 2016

Thanks!

@zcbenz zcbenz merged commit 3d2ad00 into electron:master Jun 26, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment