Bug 197750 – Do not wait until requestPermission() is called to fire deviceorientation events if permission was already granted

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Do not wait until requestPermission() is called to fire deviceorientation events if permission was already granted for this origin during this browsing session.

[Chris Dumez]

Created attachment 369513 [details]
Patch

[EWS Watchlist]

Attachment 369513 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/DeviceOrientation.mm:262:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 369517 [details]
Patch

[EWS Watchlist]

Attachment 369517 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/DeviceOrientation.mm:262:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 369517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369517&action=review

> Source/WebKit/ChangeLog:9
> +        session. However, the WebContent process was not aware of previous decisions and would therefore not fire any

IIUC, as long as UIProcess is running, once permission is granted for one page, permission is granted for all current and future pages of the same origin.
Is that correct?

As a user, I would probably have expected that reloading the page would clear that permission (I haven't looked at the prompt message though).
This can for instance help in case user denies by mistake.
In case of user granting, is there a way for users to clear that permission bit, or maybe a timer that reset this non-persistent permission after some time?

Related to this patch, I wonder what the problem is with the current behavior.
It seems that, currently, device orientation events would only flow for pages that receive a user gesture.
This seems ok to me.

[Chris Dumez]

(In reply to youenn fablet from comment #5)
> Comment on attachment 369517 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369517&action=review
> 
> > Source/WebKit/ChangeLog:9
> > +        session. However, the WebContent process was not aware of previous decisions and would therefore not fire any
> 
> IIUC, as long as UIProcess is running, once permission is granted for one
> page, permission is granted for all current and future pages of the same
> origin.
> Is that correct?

Correct and as per HI design.

> 
> As a user, I would probably have expected that reloading the page would
> clear that permission (I haven't looked at the prompt message though).
> This can for instance help in case user denies by mistake.
> In case of user granting, is there a way for users to clear that permission
> bit, or maybe a timer that reset this non-persistent permission after some
> time?

Again, this is per HI design. I believe such a change would need to be cleared by HI. I personally like the current behavior. If the user made a mistake, they can always restart Safari.

> 
> Related to this patch, I wonder what the problem is with the current
> behavior.
> It seems that, currently, device orientation events would only flow for
> pages that receive a user gesture.
> This seems ok to me.

This is not accurate. A user gesture is not required since permission was already granted. A user gesture is only needed to prompt the user, which would not happen in this case. It seems silly to require the JS to request for permission when they already have it, simply to start receiving the events. When I wrote my demo today, the current web behavior bothered me so I fixed it in this patch.

[Geoffrey Garen]

Comment on attachment 369517 [details]
Patch

r=me

I do remember the discussion around caching the Granted state for the current session, and I think that makes sense. (The privacy problem only really happens when permission is implicitly granted across websites, which doesn't happen in this case.)

I don't specifically remember a discussion around caching the Denied state. I wonder if that's the right choice. I do worry that a user could deny -- by accident or on purpose -- and then not know how to change that decision.

[youenn fablet]

(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 369517 [details]
> Patch
> 
> r=me
> 
> I do remember the discussion around caching the Granted state for the
> current session, and I think that makes sense. (The privacy problem only
> really happens when permission is implicitly granted across websites, which
> doesn't happen in this case.)

If we are not firing these events for background pages, this seems ok to me.
Are we preventing firing these events for background pages?

[WebKit Commit Bot]

Comment on attachment 369517 [details]
Patch

Clearing flags on attachment: 369517

Committed r245185: <https://trac.webkit.org/changeset/245185>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50671568>

[Chris Dumez]

(In reply to youenn fablet from comment #8)
> (In reply to Geoffrey Garen from comment #7)
> > Comment on attachment 369517 [details]
> > Patch
> > 
> > r=me
> > 
> > I do remember the discussion around caching the Granted state for the
> > current session, and I think that makes sense. (The privacy problem only
> > really happens when permission is implicitly granted across websites, which
> > doesn't happen in this case.)
> 
> If we are not firing these events for background pages, this seems ok to me.
> Are we preventing firing these events for background pages?

Why wouldn't your background page be suspended?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 369517 [details]
> Patch
> 
> r=me
> 
> I do remember the discussion around caching the Granted state for the
> current session, and I think that makes sense. (The privacy problem only
> really happens when permission is implicitly granted across websites, which
> doesn't happen in this case.)
> 
> I don't specifically remember a discussion around caching the Denied state.
> I wonder if that's the right choice. I do worry that a user could deny -- by
> accident or on purpose -- and then not know how to change that decision.

Oh, good point. I assume the caching decision applied to both Granted and Denied. I do not specifically remember if that's what was agreed upon.

[youenn fablet]

> > > I do remember the discussion around caching the Granted state for the
> > > current session, and I think that makes sense. (The privacy problem only
> > > really happens when permission is implicitly granted across websites, which
> > > doesn't happen in this case.)
> > 
> > If we are not firing these events for background pages, this seems ok to me.
> > Are we preventing firing these events for background pages?
> 
> Why wouldn't your background page be suspended?

It seems a bit orthogonal to me, this API is not designed specifically to an architecture for which background pages are to be suspended.

Looking at https://w3c.github.io/deviceorientation/#security-and-privacy, the spec recommends that events be fired for visible documents.
That would be good for battery life as well.

[Chris Dumez]

(In reply to youenn fablet from comment #14)
> > > > I do remember the discussion around caching the Granted state for the
> > > > current session, and I think that makes sense. (The privacy problem only
> > > > really happens when permission is implicitly granted across websites, which
> > > > doesn't happen in this case.)
> > > 
> > > If we are not firing these events for background pages, this seems ok to me.
> > > Are we preventing firing these events for background pages?
> > 
> > Why wouldn't your background page be suspended?
> 
> It seems a bit orthogonal to me, this API is not designed specifically to an
> architecture for which background pages are to be suspended.

But this is the only architecture where we support it.

> 
> Looking at https://w3c.github.io/deviceorientation/#security-and-privacy,
> the spec recommends that events be fired for visible documents.
> That would be good for battery life as well.

I haven't checked our code to see if we would in theory fire events for non visible documents. I would imagine that we would but this would be a theoretical and pre-existing bug, not related to this patch.

I say it is not related to this page because if a page is somehow running in the background then it could already ask for permission and receive the events.