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

Make requestPersistent() return a boolean #7

Closed
annevk opened this issue Jul 27, 2015 · 9 comments
Closed

Make requestPersistent() return a boolean #7

annevk opened this issue Jul 27, 2015 · 9 comments

Comments

@annevk
Copy link
Member

annevk commented Jul 27, 2015

Let's just make this a method that returns a promise for a boolean, that tells you whether the default box is persistent or not.

@inexorabletash @davidsgrogan I'm guessing we never want to give synchronous access to boxes, correct?

@annevk
Copy link
Member Author

annevk commented Jul 27, 2015

(By the way, I'm raising this as an issue so you folks can object. But please also tell me if you agree so I can actually fix this.)

@annevk
Copy link
Member Author

annevk commented Jul 27, 2015

Actually, I suspect we need to keep persistentPermission() so you can avoid prompting for the "denied" case. But we should also have persistent().

@davidsgrogan
Copy link

Actually, I suspect we need to keep persistentPermission() so you can avoid prompting for the "denied" case.

My thinking was that we should keep persistentPermission() so that sites can know to prompt when they would be autogranted the permission without bothering the user. We are planning on using an "engagement score" for such a purpose: when a user uses a site enough and that site requests to be exempt from disk cleanup (aka the persistent permission), the exemption will be granted without prompting the user. But the site must request the exemption to get it.

But we should also have persistent().

This would be a convenience method that would return the equivalent of something like persistentPermission(defaultBox) == "granted"?

I'm guessing we never want to give synchronous access to boxes, correct?

You mean adding a function to the API that takes a box id/reference as a parameter and returns synchronously? I would think not, for the classic problem where that info is on disk and we don't want to block the main thread on disk access.

@davidsgrogan
Copy link

I should also clarify that we are neither committed yet nor opposed yet to implementing the generic boxes concept you outlined on the old wiki page. But we've been thinking of our storage in terms of just having the default box and that seems to work ok, at least for the time being.

@annevk
Copy link
Member Author

annevk commented Jul 31, 2015

I think there's three things we care about:

  1. Ask the user and change the default box.
  2. Check whether you have permission.
  3. Check whether the default box is changed.

Having thought about it some more, my proposal is to change the return value of requestPersistent() to be a boolean, representing whether or not the default box is persistent.

This means:

  • If permission is "denied", requestPersistent() will return false.
  • If permission is "default", requestPersistent() will ask the user, maybe change the default storage to persistent, and then return whether or not the default box is persistent.
  • If permission is "granted", requestPersistent() will maybe change the default storage to persistent (if that hasn't happened already), and then return true.
  • persistentPermission() will always return the permission value.

I think this API satisfies all the criteria and is not awful.

@davidsgrogan
Copy link

Are you opposed to adding a fourth permission that's something like wouldBeGranted? With your proposal a site would have to call requestPersistent() even after permission has been granted, which seems weird.

@annevk
Copy link
Member Author

annevk commented Aug 1, 2015

I don't understand how wouldBeGranted changes anything. What requestPersistent() does is change the default box. Just like requestFullscreen() makes you go fullscreen. Sometimes that might require upfront permission, sometimes not.

@davidsgrogan
Copy link

Sorry for confusing this issue with wouldBeGranted. I'll open a new issue for that.

Your proposal in #7 (comment) looks fine to me/us.

Just curious, what was your original motivation for making requestPersistent() return a boolean instead of the permission string? We surmised it's because web devs only really care about the boolean and even though they can derive the boolean from the permission, we can do it for them. But if they really want to query for the permission string they can do that with persistentPermission. Is that about right?

@annevk
Copy link
Member Author

annevk commented Aug 8, 2015

The idea behind it is that persistentPermission() is about querying the permission and requestPersistent() is about changing the default box. The boolean it returns basically reflects the state of the default box. I think that because of #6 I messed up the return value in the initial round.

@annevk annevk changed the title Change persistentPermission() to persistent() Make persistentPermission() return a boolean Aug 10, 2015
@annevk annevk changed the title Make persistentPermission() return a boolean Make requestPersistent() return a boolean Aug 10, 2015
annevk added a commit that referenced this issue Aug 10, 2015
@annevk annevk closed this as completed Aug 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants