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

session: add setPermissionRequestHandler api #4223

Merged
merged 9 commits into from
Feb 1, 2016

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 force-pushed the notification_disable_patch branch 2 times, most recently from 9d3802b to ffaf9a9 Compare January 25, 2016 16:46
zcbenz added a commit that referenced this pull request Jan 27, 2016
@deepak1556 deepak1556 changed the title webview: add permission-request event [WIP] webview: add permission-request event Jan 29, 2016
@xywz
Copy link

xywz commented Jan 30, 2016

Will you be adding the other permission types?

@deepak1556 deepak1556 force-pushed the notification_disable_patch branch from ffaf9a9 to 2a278ce Compare January 30, 2016 11:19
@deepak1556 deepak1556 changed the title [WIP] webview: add permission-request event webview: add permission-request event Jan 30, 2016
@deepak1556 deepak1556 force-pushed the notification_disable_patch branch from 85b68aa to f7556de Compare January 30, 2016 13:41
@deepak1556
Copy link
Member Author

@xywz currently supports media, geolocation, notifications, midiSysex. Is there anyother type you are interested in ?

@zcbenz this PR is ready for review.

@@ -133,6 +133,10 @@ class WebContents : public mate::TrackableObject<WebContents>,
void SetAllowTransparency(bool allow);
bool IsGuest() const;

// Handler for permission requests.
void SetPermissionRequestHandler(v8::Local<v8::Value> val,
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be a method of Session since permission manager is session wide.

Copy link
Member Author

Choose a reason for hiding this comment

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

if we are providing this method with session module, can the permission-request event be removed ? the event way currently breaks permission being granted by default. With the callback,

session.setPermissionRequestHandler((webContents, permission, callback) => {
  // logic
  callback(); // "granted/denied"
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me to remove permission-request event if we have session.setPermissionRequestHandler API.

@deepak1556 deepak1556 force-pushed the notification_disable_patch branch from 31fcf4f to db26dca Compare February 1, 2016 06:54
@deepak1556
Copy link
Member Author

@zcbenz have made the changes, thanks!

@xywz
Copy link

xywz commented Feb 1, 2016

@deepak1556 pointerLock and fullscreen would be great.

* `callback` Function - Allow or deny the permission.

Sets the handler which can be used to respond to permission requests for the `session`.
Calling `callback('granted')` will allow the permission and `callback('denied')` will reject it.
Copy link

Choose a reason for hiding this comment

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

Maybe use a boolean instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@deepak1556
Copy link
Member Author

Have added pointerLock and fullscreen types.

@deepak1556 deepak1556 force-pushed the notification_disable_patch branch from c088172 to b575cd0 Compare February 1, 2016 10:52
@@ -397,6 +399,18 @@ void Session::SetCertVerifyProc(v8::Local<v8::Value> val,
browser_context_->cert_verifier()->SetVerifyProc(proc);
}

void Session::SetPermissionRequestHandler(v8::Local<v8::Value> val,
mate::Arguments* args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unaligned indention.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@deepak1556 deepak1556 changed the title webview: add permission-request event session: add setPermissionRequestHandler api Feb 1, 2016
@zcbenz
Copy link
Contributor

zcbenz commented Feb 1, 2016

👍

zcbenz added a commit that referenced this pull request Feb 1, 2016
session: add setPermissionRequestHandler api
@zcbenz zcbenz merged commit 69f93a7 into electron:master Feb 1, 2016
@xywz
Copy link

xywz commented Feb 1, 2016

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants