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

Update draggable regions when changing BrowserView #12348

Merged
merged 5 commits into from Mar 20, 2018

Conversation

Projects
None yet
7 participants
@poiru
Member

poiru commented Mar 19, 2018

This PR is best reviewed by looking at each individual commit.

This should be cherry-picked to 2-0-x, but it will require a separate PR due to conflicts.

Fixes #12150.

@poiru poiru requested review from felixrieseberg and MarshallOfSound Mar 19, 2018

@poiru poiru requested review from electron/browserview as code owners Mar 19, 2018

@poiru poiru changed the title from Update draggable regions after changing BrowserView to Update draggable regions when changing BrowserView Mar 19, 2018

NativeBrowserView::~NativeBrowserView() {}
brightray::InspectableWebContentsView*
NativeBrowserView::GetInspectableWebContentsView() {
return inspectable_web_contents_->GetView();

This comment has been minimized.

@deepak1556

deepak1556 Mar 20, 2018

Member

Can you check the availability of inspectable_web_contents_ before using it. Its possible for the webContents to get destroyed before the view.

@deepak1556

deepak1556 Mar 20, 2018

Member

Can you check the availability of inspectable_web_contents_ before using it. Its possible for the webContents to get destroyed before the view.

This comment has been minimized.

@poiru

poiru Mar 20, 2018

Member

I think the existing code already had similar issues. I'll work on fixing it in a separate PR, but for the purposes of fixing the draggable region bug, I think we should go ahead and merge this as-is.

@poiru

poiru Mar 20, 2018

Member

I think the existing code already had similar issues. I'll work on fixing it in a separate PR, but for the purposes of fixing the draggable region bug, I think we should go ahead and merge this as-is.

system_drag_exclude_areas.begin();
iter != system_drag_exclude_areas.end();
++iter) {
for (const auto& rect : drag_exclude_rects) {

This comment has been minimized.

@felixrieseberg

felixrieseberg Mar 20, 2018

Member

I love how much simpler this is

@felixrieseberg

felixrieseberg Mar 20, 2018

Member

I love how much simpler this is

@felixrieseberg

Gave it a good read. Agree with @deepak1556 that we should check if stuff is still there, but this looks like it's overall simpler and more accessible (to read and understand).

@poiru poiru added the target/1-8-x label Mar 20, 2018

@jkleinsc jkleinsc merged commit 060b592 into master Mar 20, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins: arm64/pr-head This commit looks good
Details
jenkins: macOS/pr-head This commit looks good
Details
@trop

This comment has been minimized.

Show comment
Hide comment
@trop

trop bot Mar 20, 2018

An error occurred while attempting to backport this PR to "1-8-x", you will need to perform this backport manually

trop bot commented Mar 20, 2018

An error occurred while attempting to backport this PR to "1-8-x", you will need to perform this backport manually

@trop

This comment has been minimized.

Show comment
Hide comment
@trop

trop bot Mar 20, 2018

An error occurred while attempting to backport this PR to "2-0-x", you will need to perform this backport manually

trop bot commented Mar 20, 2018

An error occurred while attempting to backport this PR to "2-0-x", you will need to perform this backport manually

@jkleinsc jkleinsc deleted the fix-browser-view-draggable-region branch Mar 20, 2018

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Mar 20, 2018

Member

I don't understand why this was merged with new possible nullptr dereferences, especially since we intend to backport this to other branches. Saying "let's merge this as-is and fix those in another PR" just means we now have two branches that will need manual backporting

Member

ckerr commented Mar 20, 2018

I don't understand why this was merged with new possible nullptr dereferences, especially since we intend to backport this to other branches. Saying "let's merge this as-is and fix those in another PR" just means we now have two branches that will need manual backporting

@jkleinsc

This comment has been minimized.

Show comment
Hide comment
@jkleinsc

jkleinsc Mar 20, 2018

Contributor

@ckerr I merged it because folks approved it. If it wasn't ready to go, I would prefer folks ask for changes vs approving.

Contributor

jkleinsc commented Mar 20, 2018

@ckerr I merged it because folks approved it. If it wasn't ready to go, I would prefer folks ask for changes vs approving.

@poiru

This comment has been minimized.

Show comment
Hide comment
@poiru

poiru Mar 20, 2018

Member

@ckerr The old code would have crashed as well with a different call stack. I agree that it should be fixed, but I don't think that bit necessarily needs to be backported.

Member

poiru commented Mar 20, 2018

@ckerr The old code would have crashed as well with a different call stack. I agree that it should be fixed, but I don't think that bit necessarily needs to be backported.

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Mar 20, 2018

Member

@jkleinsc that's fair, I should have said "I don't understand why this is approved". Completely agree that approved code passing CI should be merged without being second-guessed. 👍

@poiru I agree with you about the old code being unsafe too, we're on the same page there. My only point is I don't see why we need a separate PR for the nullptr guards. Is there more safety cleanup to be done in addition to that, and you want to group it all together?

Member

ckerr commented Mar 20, 2018

@jkleinsc that's fair, I should have said "I don't understand why this is approved". Completely agree that approved code passing CI should be merged without being second-guessed. 👍

@poiru I agree with you about the old code being unsafe too, we're on the same page there. My only point is I don't see why we need a separate PR for the nullptr guards. Is there more safety cleanup to be done in addition to that, and you want to group it all together?

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Mar 20, 2018

Member

@poiru also, nice job getting the backports PRed so quickly 😸

Member

ckerr commented Mar 20, 2018

@poiru also, nice job getting the backports PRed so quickly 😸

@poiru

This comment has been minimized.

Show comment
Hide comment
@poiru

poiru Mar 20, 2018

Member

@ckerr Ah, I see! Unfortunately it's not as simple as adding nullptr checks, we need to refactor some code to make it safe.

Member

poiru commented Mar 20, 2018

@ckerr Ah, I see! Unfortunately it's not as simple as adding nullptr checks, we need to refactor some code to make it safe.

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