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

Refactor NativeWindow (Part 1): Remove WebContentsObserver methods #12008

Merged
merged 15 commits into from Feb 23, 2018

Conversation

Projects
None yet
3 participants
@zcbenz
Contributor

zcbenz commented Feb 22, 2018

This PR relies on #12004, and should be reviewed and merged after it.

This is part of the changes to decouple NativeWindow from WebContents, so it would be possible for us to implement new APIs based on NativeWindow, e.g. providing APIs to create native GUI elements without using WebContents.

This PR moves WebContentsObserver methods of NativeWindow into api::BrowserWindow. Commits have been organized to make it easier to review.

@zcbenz zcbenz requested a review from electron/reviewers as a code owner Feb 22, 2018

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Feb 22, 2018

It is weird the last commit is showing in the middle of the list, anyway the CI is correctly running for the last commit.

@@ -129,6 +136,8 @@ void BrowserWindow::Init(v8::Isolate* isolate,
mate::Handle<class WebContents> web_contents) {
web_contents_.Reset(isolate, web_contents.ToV8());
api_web_contents_ = web_contents.get();
Observe(web_contents->web_contents());

This comment has been minimized.

@deepak1556

deepak1556 Feb 22, 2018

Member

maybe api_web_contents_->web_contents() for clarity.

}
void BrowserWindow::NotifyWindowUnresponsive() {
window_unresposive_closure_.Cancel();

This comment has been minimized.

@deepak1556

deepak1556 Feb 22, 2018

Member

Is it necessary to Cancel the closure once the callback is run ? Also might be a good chance to fix window_unresposive_closure_ => window_unresponsive_closure_.

This comment has been minimized.

@zcbenz

zcbenz Feb 23, 2018

Contributor

There is code checking whether the callback is cancelled before scheduling a new unresponsive event, so this is necessary.

@zcbenz zcbenz merged commit 15ce235 into master Feb 23, 2018

7 of 9 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
ci/circleci: electron-linux-x64 CircleCI is running your tests
Details
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
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@zcbenz zcbenz deleted the window-refactor-1 branch Feb 23, 2018

@zcbenz zcbenz referenced this pull request Feb 23, 2018

Merged

Upgrade to Chromium 62 #11230

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Feb 23, 2018

It is weird the last commit is showing in the middle of the list, anyway the CI is correctly running for the last commit.

For some reason GitHub shows commits sorted by date, not in "true" order.

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