Closed Bug 1286717 Opened 8 years ago Closed 7 years ago

Expose persist/persisted method to StorageManager

Categories

(Core :: Storage: Quota Manager, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [storage-v1])

Attachments

(4 files, 69 obsolete files)

25.28 KB, patch
janv
: review+
baku
: review+
Details | Diff | Splinter Review
8.10 KB, patch
janv
: review+
Details | Diff | Splinter Review
1.04 KB, patch
janv
: review+
Details | Diff | Splinter Review
28.40 KB, patch
janv
: review+
Details | Diff | Splinter Review
Expose persist() method to StorageManager
Shawn, per my understanding this is the next step for the Storage API so I set this to P2 (something b/w urgent and backlog). Please feel free to adjust this if you have a different opinion. Thanks!
Priority: -- → P2
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Shawn, per my understanding this is the next step for the Storage API so I
> set this to P2 (something b/w urgent and backlog). Please feel free to
> adjust this if you have a different opinion. Thanks!

Yeah, this is our next steps.
Assignee: nobody → shuang
https://storage.spec.whatwg.org/#dom-storagemanager-persist
"If permission is "prompt", then determine through asking the user or using heuristics whether origin is allowed to use persistent storage. If that returns a positive answer, then set permission to "granted". If that returns a negative answer, then set permission to "denied". If that returns no answer, then do not alter permission."

I guess we can implement it in QuotaManagerService, so we don't need to take care ipc in StorageManager.
One thing we should consider to expose API for frond-end to revoke permission and change it back to non-persistence mode.
Depends on: 1298329
Attached patch (WIP)bug-1286717.patch (obsolete) — Splinter Review
Now I can show permission dialog now.
The persistent storage permission prompt UI is enabled:

- 0001-WIP-Add-persistent-storage-prompt.patch requires (WIP)bug-1286717.patch applied first

- Observed PersistentStoragePermissionRequest::Allow is called and promise is resolved with "granted" if allow storing

- Observed PersistentStoragePermissionRequest::Cancel is called and promise is resolved with "prompt" if not allow storing

- The UI style is using current design and will update afterwards
(In reply to Fischer [:Fischer] from comment #8)
> Created attachment 8794088 [details] [diff] [review]
> 0001-WIP-Add-persistent-storage-prompt.patch
> 
> The persistent storage permission prompt UI is enabled:
> 
> - 0001-WIP-Add-persistent-storage-prompt.patch requires
> (WIP)bug-1286717.patch applied first
> 
> - Observed PersistentStoragePermissionRequest::Allow is called and promise
> is resolved with "granted" if allow storing
> 
> - Observed PersistentStoragePermissionRequest::Cancel is called and promise
> is resolved with "prompt" if not allow storing
> 
> - The UI style is using current design and will update afterwards

Thanks! Fisher! Let me give it a try :)
(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> (In reply to Fischer [:Fischer] from comment #8)
> > Created attachment 8794088 [details] [diff] [review]
> > 
> > - The UI style is using current design and will update afterwards
> 
> Thanks! Fisher! Let me give it a try :)

Hey! I can manage to show storage permission prompt and it works! Hooray! Nice!
Comment on attachment 8796066 [details] [diff] [review]
(WIP) bug-1286717.patch

I'm still working on worker part but I would like to know if this is the right direction.
Attachment #8796066 - Flags: feedback?(jvarga)
The permission notification on the central has been updated so 0001-WIP-Add-persistent-storage-prompt.patch becomes obsolete. 

To work with the current central:
- Remove 0001-WIP-Add-persistent-storage-prompt.patch
- Rebase to the up-to-date central
- Apply 0001-WIP-Add-PermissionUI.PersistentStoragePermissionProm.patch
Attachment #8794088 - Attachment is obsolete: true
I'll take a look at the patch today.
Comment on attachment 8796066 [details] [diff] [review]
(WIP) bug-1286717.patch

Review of attachment 8796066 [details] [diff] [review]:
-----------------------------------------------------------------

I see a lot of code duplication here, much of persist() c++ code is very similar to estimate() c++ code.

::: dom/indexedDB/test/unit/test_storage_manager_persist.js
@@ +6,5 @@
> +	       "test_storage_manager_estimate.js";
> +  const objectStoreName = "storagesManager";
> +  const arraySize = 1e6;
> +
> +  ok('estimate' in navigator.storage, 'Has estimate function');

I guess you plan to update this test more

::: dom/quota/StorageManager.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_StorageManager_h
>  #define mozilla_dom_StorageManager_h
>  
> +#include "mozilla/dom/StorageManagerBinding.h"

why is this needed here ? and in .cpp too ?

@@ +39,5 @@
>    already_AddRefed<Promise>
>    Estimate(ErrorResult& aRv);
>  
> +  already_AddRefed<Promise>
> +  Persist(ErrorResult& aRv);

Nit: this method is declared before estimate() in webidl
Comment on attachment 8796066 [details] [diff] [review]
(WIP) bug-1286717.patch

Review of attachment 8796066 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/StorageManager.cpp
@@ +108,5 @@
>    MainThreadRun() override;
>  };
>  
> +// This class is used to get quota callback.
> +class PersistResolver final

very similar to EstimateResolver

@@ +151,5 @@
> +  { }
> +};
> +
> +// This class is used to return promise on worker thread.
> +class PersistResolver::FinishWorkerRunnable final

dtto

@@ +169,5 @@
> +  virtual bool
> +  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override;
> +};
> +
> +class PersistWorkerMainThreadRunnable

dtto

@@ +410,5 @@
> +NS_IMPL_ISUPPORTS(PersistResolver, nsIQuotaCallback)
> +
> +void
> +PersistResolver::ResolveOrReject(Promise* aPromise)
> +{

dtto

@@ +422,5 @@
> +}
> +
> +bool
> +PersistResolver::
> +FinishWorkerRunnable::WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)

dtto

@@ +441,5 @@
> +  return true;
> +}
> +
> +NS_IMETHODIMP
> +PersistResolver::OnComplete(nsIQuotaRequest *aRequest)

dtto

@@ +480,5 @@
> +  return NS_OK;
> +}
> +
> +bool
> +PersistWorkerMainThreadRunnable::MainThreadRun()

dtto

You really need to create a base class or something and share as much code as you can. Imagine the case when we need to add another similar method. Would you copy and paste this again ?

I'll review the rest once you remove the code duplication.
Attachment #8796066 - Flags: feedback?(jvarga)
It seems those classes are not used at all, I know you requested just a feedback, but I would expect that you send something which is a bit more polished.
Comment on attachment 8796066 [details] [diff] [review]
(WIP) bug-1286717.patch

Review of attachment 8796066 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/StorageManager.cpp
@@ +636,5 @@
> +  RefPtr<nsIQuotaRequest> request;
> +
> +  return PersistStorageForPrincipal(mPrincipal,
> +                                    resolver,
> +                                    getter_AddRefs(request));

|return ...| and then |return NS_OK|

I would expect that you do some kind of self review before submitting a patch.

@@ +653,5 @@
> +
> +  return NS_OK;
> +}
> +
> +inline nsresult

why |inline|, do you know ?
or it's just copy and paste ?
(In reply to Jan Varga [:janv] from comment #17)
> It seems those classes are not used at all, I know you requested just a
> feedback, but I would expect that you send something which is a bit more
> polished.

Oh I see, PersistResolver is used, so what I said in comment 16.
You need a base class for PersistResolver and EstimateResolver.
(In reply to Jan Varga [:janv] from comment #19)
> (In reply to Jan Varga [:janv] from comment #17)
> > It seems those classes are not used at all, I know you requested just a
> > feedback, but I would expect that you send something which is a bit more
> > polished.
> 
> Oh I see, PersistResolver is used, so what I said in comment 16.
> You need a base class for PersistResolver and EstimateResolver.

Yes, sure, i will do it.
(In reply to Jan Varga [:janv] from comment #18)
> Comment on attachment 8796066 [details] [diff] [review]
> (WIP) bug-1286717.patch
> 
> Review of attachment 8796066 [details] [diff] [review]:
> |return ...| and then |return NS_OK|
> 
> I would expect that you do some kind of self review before submitting a
> patch.
My apologies. It won't happen again.
> > +
> > +inline nsresult
> 
> why |inline|, do you know ?
> or it's just copy and paste ?
Copy from: http://searchfox.org/mozilla-central/source/dom/notification/Notification.cpp#629
I can remove inline, if you prefer.
I know it was just a feedback, if you want me to just verify some parts of the patch, then you need to tell me or put a comment somewhere - "this needs more work" for stuff that I shouldn't look at yet.
(In reply to Jan Varga [:janv] from comment #22)
> I know it was just a feedback, if you want me to just verify some parts of
> the patch, then you need to tell me or put a comment somewhere - "this needs
> more work" for stuff that I shouldn't look at yet.
Sorry, Jan. I did not explain my intention carefully in the beginning.
My original intention is to verify the permission dialog part via ContentPermissionRequest instead of NotifyObserver directly, like PermissionRequestBase[1][2]. It looks very specific for indexeddb prompt only. Because when we discuss previously, you mentioned we should use the same way like IndexedDB permission dialog (or even re-use the same code and merge together).
But later I found other APIs using permission api uses ContentPermissionRequest, not like what IndexedDB PermissionRequest did.
Shall we still follow the way IndexedDB PermissionRequest implemented? Or it's fine to use ContentPermissionRequest? It's critical for me that's why I try to ask for feedback very early.:(

[1] http://searchfox.org/mozilla-central/source/dom/indexedDB/PermissionRequestBase.cpp#155
[2] http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#6129
Ok, now I know what you actually want to know. I'll take a look today.
Blocks: 1309123
Comment on attachment 8796066 [details] [diff] [review]
(WIP) bug-1286717.patch

Review of attachment 8796066 [details] [diff] [review]:
-----------------------------------------------------------------

Despite this is a rough patch, I think nsIContentPermissionRequest is the right direction.
However, I'd like to see how it works when it's initialized from a DOM worker.

f=me on using nsIContentPermissionRequest

Shawn++ on finding this generic helper instead of handling the permission directly
(like IndexedDB impl currently does)

It seems to me, once persist() is done and we remove the support for explicit persistent
storage in IndexedDB, we can remove the prompt from IndexedDB which is quite a bit of code.

::: dom/quota/StorageManager.cpp
@@ +512,5 @@
> +/*******************************************************************************
> + * PersistentStoragePermissionRequest
> + ******************************************************************************/
> +
> +class PersistentStoragePermissionRequest

Ok, so this was copied from Notification.cpp.
It seems using nsIContentPermissionRequest and nsContentPermissionUtils::AskPermission() is the way to go. The latter also handles the child process case, so it sends stuff across IPC.

However I'm a bit worried that we don't need everything from Notification.cpp
Do we really need the cycle collection support ?
All those new prefs, do we need them ?

I would start with a minimal nsIContentPermissionRequest implementation and add stuff in steps as it's needed.

There are many nsIContentPermissionRequest implementations in dom/
You can check some of them to get more ideas how to handle it properly.

::: dom/webidl/StorageManager.webidl
@@ +12,5 @@
>  interface StorageManager {
>    // [Throws]
>    // Promise<boolean> persisted();
> +  [Throws]
> +  Promise<boolean> persist();

Nit: I would place an empty line between these method declarations.
Attachment #8796066 - Flags: feedback+
(In reply to Jan Varga [:janv] from comment #25)
> Comment on attachment 8796066 [details] [diff] [review]
Thank you for your patience and tolerance since this patch is full of mistakes.
I will improve the patch quality next time before I ask for feedback.
> However, I'd like to see how it works when it's initialized from a DOM
> worker.
This is something I'm going to do for workers next.
> It seems to me, once persist() is done and we remove the support for
> explicit persistent
> storage in IndexedDB, we can remove the prompt from IndexedDB which is quite
> a bit of code.
I can file an another bug for IndexedDB prompt clean up.
> 
> ::: dom/quota/StorageManager.cpp
> @@ +512,5 @@
> > +/*******************************************************************************
> > + * PersistentStoragePermissionRequest
> > + ******************************************************************************/
> > +
> > +class PersistentStoragePermissionRequest
> 
> Ok, so this was copied from Notification.cpp.
> It seems using nsIContentPermissionRequest and
> nsContentPermissionUtils::AskPermission() is the way to go. The latter also
> handles the child process case, so it sends stuff across IPC.
> 
> However I'm a bit worried that we don't need everything from Notification.cpp
> Do we really need the cycle collection support ?
You're correct. I should remove it. There is no reason to keep CC here.
> All those new prefs, do we need them ?
Originally I thought I can also use prefs to do more tests, but it's not complete, I should remove it.
> I would start with a minimal nsIContentPermissionRequest implementation and
> add stuff in steps as it's needed.
> There are many nsIContentPermissionRequest implementations in dom/
> You can check some of them to get more ideas how to handle it properly.
Now I know this is the right direction, and I will deliver a better patch.
I will clean up and start refresh again.
Whiteboard: storage-v1
Blocks: 1312347
No longer blocks: 1312347
Blocks: 1313313
No longer blocks: 1313313
I found Notification API Notification.requestPermission() used to have discussion on permission [1]. It seems to me Storage Standard may have the same issue. Since StorageManager exposed to both window, and worker. Now I found persist() exposed to window only, this makes things a lot easier. 
[1] https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Jul/0178.html
[2] https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Jul/0179.html
Summary: Expose persist() method to StorageManager → Expose persist/persisted method to StorageManager
Comment on attachment 8814056 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager

Review of attachment 8814056 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Bevis,
This patch exposes persist/persisted method to StorageManager, I based on the previous Jan's feedback.
Since Jan is still reviewing Tom's patch, I'm wondering if you can take a look like we did for estimate() first.

1. persist() is exposed to Window only, so we don't need to consider worker issue for permission request, persisted() is exposed to both Window/Worker like estimate().
2. ExecuteOpOnMainOrWorkerThread() is extracted from the original Estimate(), so Estimate() and Persisted() share with the same code.
3. Test cases only cover interface level, internal QuotaManager test cases cover by bug 1298329.
Attachment #8814056 - Flags: feedback?(btseng)
Comment on attachment 8814056 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager

Review of attachment 8814056 [details] [diff] [review]:
-----------------------------------------------------------------

There is still something we can improve!
Let's resume the review again after the following comments are addressed. :)

::: dom/quota/StorageManager.cpp
@@ +29,5 @@
> +{
> +  Estimate,
> +  Persist,
> +  Persisted
> +};

Move this enum into class EstimatePersistedResolver and rename it as enum Type.
Then, we'll access it, for example, as RequestResolver::Type::Estimate.

@@ +32,5 @@
> +  Persisted
> +};
> +
> +// This class is used to get quota usage, and check persisted status callback.
> +class EstimatePersistedResolver final

s/EstimatePersistedResolver/RequestResolver

@@ +39,5 @@
>  {
>    class FinishWorkerRunnable;
>  
> +  bool mPersisted;
> +  CallbackType mCallbackType;

s/CallbackType/RequestType

@@ +96,1 @@
>    : public WorkerRunnable

ditto

@@ +252,5 @@
> +
> +already_AddRefed<Promise>
> +ExecuteOpOnMainOrWorkerThread(nsIGlobalObject* aGlobal,
> +                              ErrorResult& aRv,
> +                              bool aIsPersisted)

Can we just accept a parameter RequestResolver::Type aRequestType here?
Then, all the implementations ofStorageManager::Estimate(), StorageManager::Persist(), StorageManager::Persisted() will all looks the same as:
{
  MOZ_ASSERT(mOwner);

  return ExecuteOpOnMainOrWorkerThread(mOwner, aRv, RequestResolver::Type::Xxxxx);
}

@@ +254,5 @@
> +ExecuteOpOnMainOrWorkerThread(nsIGlobalObject* aGlobal,
> +                              ErrorResult& aRv,
> +                              bool aIsPersisted)
> +{
> +  MOZ_ASSERT(aGlobal);

All we need to do for Type::Persist can be specified inside this function and make an MOZ_ASSERT_IF(aRequestType == RequestResolver::Type::Persist, NS_IsMainThread()) here.

@@ +278,5 @@
> +    nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();
> +    MOZ_ASSERT(principal);
> +
> +    nsresult rv;
> +    if (aIsPersisted) {

switch (aRequestType) {
  case RequestResolver::Type::Estimate: {
    ...
  }
  break;
  case RequestResolver::Type::Persist: {
    ...
  }
  break;
  case RequestResolver::Type::Persisted: {
    ...
  }
  break;
  default:
    MOZ_CRASH("Invalid aRequestType!");
}

@@ +311,5 @@
> +  if (NS_WARN_IF(!promiseProxy)) {
> +    return nullptr;
> +  }
> +
> +  if (aIsPersisted) {

switch (aRequestType) {
  case RequestResolver::Type::Estimate: {
    ...
  }
  break;
  case RequestResolver::Type::Persisted: {
    ...
  }
  break;
  default:
    MOZ_CRASH("Invalid aRequestType!");
}

@@ +359,5 @@
> +  case nsIPermissionManager::DENY_ACTION:
> +    return PersistentStoragePermission::Denied;
> +  default:
> +    return PersistentStoragePermission::Prompt;
> +  }

You can reused ActionToPermissionState() in PermissionUtils.h

@@ +370,5 @@
>   ******************************************************************************/
>  
>  void
> +EstimatePersistedResolver::ResolveOrReject(Promise* aPromise,
> +                                           bool aIsPersistType)

RequestResolver::Type aRequestType

@@ +375,4 @@
>  {
>    MOZ_ASSERT(aPromise);
>  
> +  if (aIsPersistType) {

if (aRequestType == RequestResolver::Type::Persisted) {

@@ +431,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aRequest);
> +
> +  // Identify request: check if the callback is for persist or persisted

// Retrieve the result according to the RequestResolverType

::: dom/webidl/StorageManager.webidl
@@ +26,5 @@
>    unsigned long long usage;
>    unsigned long long quota;
>  };
> +
> +enum PersistentStoragePermission { "prompt", "denied", "granted" };

storage API refers the PersistentState defined in https://w3c.github.io/permissions/#idl-index
So we could just refer to http://searchfox.org/mozilla-central/source/dom/webidl/PermissionStatus.webidl#10 by exposing the existing PermissionUtils.h in dom/permission/moz.build which includes PermissionStatusBinding.h you need and include it in your implementation.
In addition, the ActionToPermissionState in this utility is also useful to you!
Attachment #8814056 - Flags: feedback?(btseng)
Bevis,
Thanks for your immediate feedback response. I will fix issues in the next version, what your comments addressed are all meaningful.
Comment on attachment 8814360 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted to StorageManager

Hi Bevis,
Thanks for feedback, i've revised code that you addressed in the comments.
Attachment #8814360 - Flags: feedback?(btseng)
Attachment #8814360 - Attachment is obsolete: true
Attachment #8814360 - Flags: feedback?(btseng)
Bevis,
I found did not handle prompt testing flag correct. So i modified PersistentStoragePermissionRequest::Run().
Attachment #8814786 - Flags: feedback?(btseng)
Comment on attachment 8814786 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager

Review of attachment 8814786 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to revisist again after all the comment are addressed.
Note: the wpt test cases are not related to indexedDB. please also rename these test cases properly.

Thanks!

::: dom/quota/StorageManager.cpp
@@ +5,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "StorageManager.h"
>  
> +#include "mozilla/dom/PermissionStatusBinding.h"

No need for this #include directive:
PermissionStatusBinding.h will be included included in PermissionUtils.h

@@ +13,5 @@
>  #include "mozilla/dom/StorageManagerBinding.h"
>  #include "mozilla/dom/WorkerPrivate.h"
>  #include "mozilla/ErrorResult.h"
> +#include "nsContentPermissionHelper.h"
> +#include "nsIContentPermissionPrompt.h"

nsIContentPermissionPrompt.h has been included already in nsContentPermssionHelper.h

@@ +35,3 @@
>  {
> +public:
> +  enum RequestType

nit: RequestResolver::Type shall be good enough and that's why we make it as inner enum in side this classs to make the access of these types shorter with class namespace RequestResolver::Type::Xxxx

@@ +45,3 @@
>    class FinishWorkerRunnable;
>  
> +  bool mPersisted;

move bool mPersisted to the line above:
  StorageEstimate mStorageEstimate;
to list possible result types together.

@@ +45,4 @@
>    class FinishWorkerRunnable;
>  
> +  bool mPersisted;
> +  RequestType mRequestType;

nit: Type mRequestType;
and make it a const type:
const Type mResultType;

@@ +78,5 @@
>      MOZ_ASSERT(aProxy);
>    }
>  
>    void
> +  ResolveOrReject(Promise* aPromise, RequestType aRequestType);

We will use the mRequestType & mPromise/mPromiseProxy instead of add these 2 parameters.
See the implementation for the detailed information.

@@ +96,1 @@
>    { }

// Ensure ResolveOrReject() to be run in the owning thread.
NS_DECL_OWNINGTHREAD

@@ +101,3 @@
>    : public WorkerRunnable
>  {
> +  bool mIsPersisted;

No need for this boolean.
You should should utilize mResolver->GetRequestType() instead.

@@ +105,4 @@
>  
>  public:
> +  explicit FinishWorkerRunnable(RequestResolver* aResolver,
> +                                bool aIsPersisted)

ditto

@@ +109,2 @@
>      : WorkerRunnable(aResolver->mProxy->GetWorkerPrivate())
> +    , mIsPersisted(aIsPersisted)

ditto

@@ +165,5 @@
> + ******************************************************************************/
> +
> +class PersistentStoragePermissionRequest
> +  : public nsIContentPermissionRequest,
> +    public nsIRunnable

It looks unnecessary to me to have this class as a runnable while all the procedure can be done by a sequence of function calls in main thread directly:
1. In ExecuteOpOnMainOrWorkerThread(), you can define a ::Start() function to do what you have done in ::Run() and replace the NS_DispatchToMainThread(request) with the invocation of this ::Start() function.
2. You don't need ::DispatchResolvePromise() while all the call sites are in main thread. You can just call ResolvePromise() directly and also put a MOZ_ASSERT(NS_IsMainThread()) at the beginning of ::ResolvePromise() instead. In addition, the resolve/reject callback of the promise will always be invoked asynchronously in Stable State of the event queue, so the dispatching from main thread to main thread looks unnecessary to me.

@@ +187,5 @@
> +protected:
> +  virtual ~PersistentStoragePermissionRequest() {}
> +
> +  nsresult ResolvePromise();
> +  nsresult DispatchResolvePromise();

Replace the call sites of DispatchResolvePromise() to ResolvePromise() and remove ::DispatchResolvePromise()

@@ +214,5 @@
>  
> +  nsresult rv = qms->GetUsageForPrincipal(aPrincipal,
> +                                          aCallback,
> +                                          true,
> +                                          aRequest);

unnecessary change that increases the depth of hg log for blaming.

@@ +342,5 @@
> +          new PersistentStoragePermissionRequest(principal,
> +                                                 window,
> +                                                 promise);
> +
> +        NS_DispatchToMainThread(request);

Don't dispatch it. Just start the request:
rv = request->Start();

@@ +407,5 @@
> +
> +PermissionState
> +TestPermission(nsIPrincipal* aPrincipal)
> +{
> +  AssertIsOnMainThread();

This is a wrapper only available in Workers.h. Please replace it with the generic one:
nit: MOZ_ASSERT(NS_IsMainThread();

@@ +436,5 @@
>   ******************************************************************************/
>  
>  void
> +RequestResolver::ResolveOrReject(Promise* aPromise,
> +                                 RequestType aRequestType)

the mResultCode was accessed by both types and mRequestType and mPromise/mPromiseProxy is also available, so we supposed to rewrite this function as followed:
RequestResolver::ResolveOrReject()
{
  NS_ASSERT_OWNINGTHREAD(RequestResolver);

  RefPtr<Promise> promise = mPromise;
  RefPtr<PromiseWorkerProxy> proxy;
  if (!promise) {
    proxy = mProxy;
    promise = proxy->WorkerPromise();
    MOZ_ASSERT(promise);
  }

  class MOZ_STACK_CLASS CleanupProxy {
    public:
      CleanupProxy(PromiseWorkerProxy* aProxy);
    private:
      ~CleanupProxy() {
        if (mProxy) {
          proxy->CleanUp();
        }
      }
      RefPtr<PromiseWorkerProxy> mProxy;
  } autoCleanUp(proxy);

  if (NS_WARN_IF(NS_FAILED(rv))) {
    aPromise->MaybeReject(mResultCode);
    return;
  }

  if (mRequestType == Type::Persisted) {
    aPromise->MaybeResolve(mPersisted);
    return;
  }

  MOZ_ASSERT(aRequestType == Type::Estimate);
  aPromise->MaybeReject(mResultCode);
}

@@ +495,5 @@
> +NS_IMETHODIMP
> +RequestResolver::OnComplete(nsIQuotaRequest *aRequest)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aRequest);

MOZ_ASSERT(mResultCode == NS_OK);

@@ +499,5 @@
> +  MOZ_ASSERT(aRequest);
> +
> +  // Retrieve the result according to the RequestResolverType
> +  nsIQuotaCallback* callback;
> +  nsresult rv = aRequest->GetCallback(&callback);

MOZ_ASSERT(NS_SUCCEEDED(rv), "Invalid callback!");

@@ +533,5 @@
> +    case RequestType::Persist: {
> +      rv = aRequest->GetResultCode(&mResultCode);
> +
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        mPersisted = false;

Just assign the rv to mResultCode and keep mPersisted unchanged. We will reject the promise later according to the mResultCode;

@@ +538,5 @@
> +        mResultCode = rv;
> +      } else if (NS_SUCCEEDED(mResultCode)) {
> +        // PersistStorageForPrincipal doesn't return any value
> +        mPersisted = true;
> +        mResultCode = rv;

Keep mResultCode unchange.

@@ +558,5 @@
> +    return NS_OK;
> +  }
> +
> +  // In a worker thread request.
> +  MutexAutoLock lock(mProxy->Lock());

create a block here to acquire the Lock only if this block is entered:
{
  MutexAutoLock lock(mProxy->Lock());

  if (NS_WARN_IF(mProxy->CleanedUp())) {
    return NS_ERROR_FAILURE;
  }

  RefPtr<FinishWorkerRunnable> runnable = new FinishWorkerRunnable(this, false);
  if (NS_WARN_IF(!runnable->Dispatch())) {
    return NS_ERROR_FAILURE;
  }
}

return NS_OK;

@@ +580,2 @@
>  FinishWorkerRunnable::WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
>  {

Just rewrite this entire function as followed:
{
  MOZ_ASSERT(aWorkerPrivate);
  aWorkerPrivate->AssertIsOnWorkerThread();

  mResolver->ResolveOrReject();

  return true;
}

@@ +662,5 @@
> +}
> +
> +
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::Run()

rename ::Run() to ::Start()

@@ +663,5 @@
> +
> +
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::Run()
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +680,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::GetPrincipal(nsIPrincipal** aRequestingPrincipal)
> +{

MOZ_ASSERT(NS_IsMainThread());
NS_ENSURE_ARG_POINTER(aRequestingPrincipal);

@@ +687,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::GetWindow(mozIDOMWindow** aRequestingWindow)
> +{

MOZ_ASSERT(NS_IsMainThread());
NS_ENSURE_ARG_POINTER(aRequestingWindow);

@@ +694,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::GetElement(nsIDOMElement** aElement)
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +702,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::Cancel()
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +732,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::GetRequester(nsIContentPermissionRequester** aRequester)
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +742,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +PersistentStoragePermissionRequest::DispatchResolvePromise()

Remove this function.

@@ +764,5 @@
> +  if (mPermission == PermissionState::Granted) {
> +    mPromise->MaybeResolve(true);
> +  } else {
> +    mPromise->MaybeResolve(false);
> +  }

replace if-else with:
// The Promise will always be resolved in both cases with boolean value return:
// https://storage.spec.whatwg.org/#dom-storagemanager-persist
mPromise->MaybeResolve(mPermission == PermissionState::Granted);

::: modules/libpref/init/all.js
@@ +5524,5 @@
>  // Disable Storage api in release builds.
>  #ifdef NIGHTLY_BUILD
>  pref("dom.storageManager.enabled", true);
> +pref("dom.storageManager.prompt.testing", false);
> +pref("dom.storageManager.prompt.testing.allow", false);

Why don't we define these 2 without #ifdef-else-endif if the values are the same?
Attachment #8814786 - Flags: feedback?(btseng)
Comment on attachment 8814786 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager

Review of attachment 8814786 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/StorageManager.cpp
@@ +436,5 @@
>   ******************************************************************************/
>  
>  void
> +RequestResolver::ResolveOrReject(Promise* aPromise,
> +                                 RequestType aRequestType)

After checking the spec,
The resolve/reject  of the promise for these request types shall be summarized as follow:

if (mRequestType == Type::Estimate) {
  aPromise->MaybeResolve(mStorageEstimate);
  return;
}

MOZ_ASSERT(aRequestType == Type::Persist || aRequestType == Type::Persisted);
aPromise->MaybeResolve(mPersisted);
> @@ +96,1 @@
> >    { }
> 
> // Ensure ResolveOrReject() to be run in the owning thread.
> NS_DECL_OWNINGTHREAD
> 
We don't need to add NS_DECL_OWNINGTHREAD explicitly, NS_DECL_THREADSAFE_ISUPPORTS already includes NS_DECL_OWNINGTHREAD.
Revised patch based on the previous comments addressed.
Update the test patch handling persistent-storage permission request.
Attachment #8796495 - Attachment is obsolete: true
Comment on attachment 8814866 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager

Revised comments addressed.
Attachment #8814866 - Flags: feedback?(btseng)
Comment on attachment 8814866 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager

Review of attachment 8814866 [details] [diff] [review]:
-----------------------------------------------------------------

f=me after the following comments are addressed.

Thanks!

::: dom/quota/StorageManager.cpp
@@ +48,5 @@
>    // Otherwise mProxy must be non-null.
>    RefPtr<Promise> mPromise;
>    RefPtr<PromiseWorkerProxy> mProxy;
>  
>    nsresult mResultCode;

we can remove mResultCode now if the promise will never be rejected.
Please also check the sample code of the change in RequestResolver::OnUsageResult() & RequestResolver::OnComplete()

@@ +178,5 @@
> +
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> +
> +protected:

There seems no subclass of this class.
We could mark it as final and put these data members below as private instead of protected.

@@ +485,3 @@
>        mResultCode = rv;
>      }
>    }

This can be simplified after mResultCode is removed:
  nsresult resultCode;
  if (!NS_WARN_IF(NS_FAILED(aRequest->GetResultCode(&resultCode))) &&
      !NS_WARN_IF(NS_FAILED(resultCode))) {
    Unused << NS_WARN_IF(GetStorageEstimate(aRequest, mStorageEstimate));
  }

@@ +525,5 @@
> +    static_cast<RequestResolver*>(callback);
> +
> +  Type type = result->GetType();
> +
> +  switch (type) {

We could simplify this switch case as followed because the mResultCode is useless now:
  nsresult resultCode;
  switch (type) {
    case Type::Persisted:
      if (!NS_WARN_IF(NS_FAILED(aRequest->GetResultCode(&resultCode))) &&
          !NS_WARN_IF(NS_FAILED(resultCode))) {
        nsCOMPtr<nsIVariant> result;
        if (!NS_WARN_IF(NS_FAILED(
              aRequest->GetResult(getter_AddRefs(result)))) && 
            result) {
          Unused << NS_WARN_IF(result->GetAsBool(&mPersisted));
        }
      }
      break;

    case Type::Persist:
      nsresult resultCode;
      mPersisted = 
        !NS_WARN_IF(NS_FAILED(aRequest->GetResultCode(&resultCode))) && 
        !NS_WARN_IF(NS_FAILED(resultCode));
      break;
    
    case Type::Estimate:
    default:
      MOZ_CRASH("Invalid callback type!");
      break;
  }

::: testing/web-platform/tests/storage/persist-allow.https.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <meta charset="utf-8">
> +    <title>StorageManager: Allow prompt after calling persist() for indexeddb</title>

remove " for indexeddb"

::: testing/web-platform/tests/storage/persist-deny.https.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <meta charset="utf-8">
> +    <title>StorageManager: Deny prompt after calling persist() for indexeddb</title>

ditto

::: testing/web-platform/tests/storage/persisted-worker.https.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <meta charset="utf-8">
> +    <title>StorageManager: persisted() for indexeddb from worker</title>

ditto

::: testing/web-platform/tests/storage/persisted.https.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <meta charset="utf-8">
> +    <title>StorageManager: persisted() for indexeddb</title>

ditto
Attachment #8814866 - Flags: feedback?(btseng) → feedback+
Per off-line discussion with Bevis, it's better to test persist() using dom.storageManager.prompt.testing in mochitests instead of wpt test cases, but leave persist() manual test case in wpt. Since testing flags can not be used on other browsers.
(In reply to Fischer [:Fischer] from comment #41)
> Created attachment 8815146 [details] [diff] [review]
> 0001-WIP-Handle-Persistent-Storage-Permission-Request.patch
> 
> Update the test patch handling persistent-storage permission request.

I'm a bit curious the wording and UI. I know this is just an experimental patch. The prompt shows "Always allow to store data" and "Never allow to store data" but in the permission tab, i can see the default value is "Always Ask".
Flags: needinfo?(fliu)
I will check it out with the latest central, thanks.
Flags: needinfo?(fliu)
Update the test patch
Attachment #8815146 - Attachment is obsolete: true
(In reply to Bevis Tseng[:bevistseng][:btseng](vacation Dec 12-16) from comment #36)
> Comment on attachment 8814786 [details] [diff] [review]
> Bug 1286717 - Expose persist/persisted method to StorageManager
> 
> Review of attachment 8814786 [details] [diff] [review]:
> -----------------------------------------------------------------

> RequestResolver::ResolveOrReject()
> {
>   NS_ASSERT_OWNINGTHREAD(RequestResolver);
Per off-line discussion, I will delete this line. This won't work on worker version.
(In reply to Bevis Tseng[:bevistseng][:btseng](vacation Dec 12-16) from comment #43)
> Comment on attachment 8814866 [details] [diff] [review]
> Bug 1286717 - Expose persist/persisted method to StorageManager
> 
> Review of attachment 8814866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f=me after the following comments are addressed.
> 
> Thanks!
Hi Bevis,
So the specification had been changed today.
https://github.com/whatwg/storage/commit/4062af57975908346d220597f5a2e420ab100790

If there was an internal error of some kind while obtaining usage and quota, or when allocating dictionary, then reject promise with a TypeError.
This is not expected to happen normally. Otherwise, resolve promise with dictionary.
Comment on attachment 8817332 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager

Hi Jan,
* Storage specification changed a bit:
1. estimate() rejects a promise with TypeError
2. persist() is exposed to window only, so we don't need to take care of complicated situation.

*  StorageManager.cpp: 
   ExecuteOpOnMainOrWorkerThread added to reduce duplicated code.
*  Test cases divide into mochitest and wpt test cases. I cannot use pref to test persist() in wpt test cases, so I moved auto test cases to mochitests.

Do you mind reviewing before bug 1298329 got r+?
If you have any concern, feel free to cancel the review request.
Attachment #8817332 - Flags: review?(jvarga)
Comment on attachment 8815991 [details] [diff] [review]
0002-WIP-Handle-Persistent-Storage-Permission-Request.patch

Review of attachment 8815991 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/SitePermissions.jsm
@@ +266,5 @@
> +  "indexedDB": {},
> +
> +  "persistent-storage": {
> +    exactHostMatch: true
> +  }

Hi Fisher,
I just found try-server test case failure after applying your patch.

Since you added persistent-storage in gPermissionObject, make sure you modify the xpcshell test case: browser/modules/test/xpcshell/test_SitePermissions.js.

http://searchfox.org/mozilla-central/source/browser/modules/test/xpcshell/test_SitePermissions.js#11

Add persistent-storage into array.
(In reply to Shawn Huang [:shawnjohnjr] from comment #55)
> Hi Fisher,
> I just found try-server test case failure after applying your patch.
> 
> Since you added persistent-storage in gPermissionObject, make sure you
> modify the xpcshell test case:
> browser/modules/test/xpcshell/test_SitePermissions.js.
> 
> http://searchfox.org/mozilla-central/source/browser/modules/test/xpcshell/
> test_SitePermissions.js#11
> 
> Add persistent-storage into array.

Thanks for reminding this. I will update the test_SitePermissions.js test in the formal patch.
Flags: needinfo?(fliu)
Comment on attachment 8817332 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager

Review of attachment 8817332 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this looks much better, here are some nits, more to come.

::: dom/quota/StorageManager.cpp
@@ +43,3 @@
>    class FinishWorkerRunnable;
>  
> +  const Type mType;

This should be coupled with similarly sized members like mPersisted which usually go last.

@@ +49,5 @@
>    RefPtr<Promise> mPromise;
>    RefPtr<PromiseWorkerProxy> mProxy;
>  
>    nsresult mResultCode;
> +  bool mPersisted;

A bool takes 1 byte, StorageEstimate is a struct, so mPersisted should go last.

@@ +54,4 @@
>    StorageEstimate mStorageEstimate;
>  
>  public:
> +  explicit RequestResolver(Promise* aPromise,

Doesn't need to be explicit anymore (2 args)

@@ +65,5 @@
>      MOZ_ASSERT(aPromise);
>    }
>  
> +  explicit RequestResolver(PromiseWorkerProxy* aProxy,
> +                           Type aType)

dtto

@@ +79,5 @@
>    void
> +  ResolveOrReject();
> +
> +  Type
> +  GetType()

const

@@ +252,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsresult rv =
> +    qms->PersistStorageForPrincipal(aPrincipal, aCallback, aRequest);

No need for this empty line.

@@ +274,5 @@
> +  nsCOMPtr<nsIQuotaManagerService> qms = QuotaManagerService::GetOrCreate();
> +  if (NS_WARN_IF(!qms)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +

dtto

@@ +286,5 @@
> +};
> +
> +already_AddRefed<Promise>
> +ExecuteOpOnMainOrWorkerThread(nsIGlobalObject* aGlobal,
> +                              ErrorResult& aRv,

It would be nicer to put aRv after aType.
(In reply to Jan Varga [:janv] from comment #57)
> Comment on attachment 8817332 [details] [diff] [review]
> Bug 1286717 - Expose persist/persisted method to StorageManager
> 
> Review of attachment 8817332 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, this looks much better, here are some nits, more to come.
> 
> @@ +49,5 @@
> >    RefPtr<Promise> mPromise;
> >    RefPtr<PromiseWorkerProxy> mProxy;
> >  
> >    nsresult mResultCode;
> > +  bool mPersisted;
> 
> A bool takes 1 byte, StorageEstimate is a struct, so mPersisted should go
> last.
I see. So we should sort a bit based on size.
> @@ +286,5 @@
> > +};
> > +
> > +already_AddRefed<Promise>
> > +ExecuteOpOnMainOrWorkerThread(nsIGlobalObject* aGlobal,
> > +                              ErrorResult& aRv,
> 
> It would be nicer to put aRv after aType.
Okay.
Since bug 1298329 changed persist interface again. I will revise this patch again after this round.
Rebase: testing/web-platform/meta/MANIFEST.json, modules/libpref/init/all.js again.
Hi Jan,
Do you still want to review v1 patch? Since persist/persisted api already changed in bug 1298329. Shall I cancel the review request and replace it with v2?
Flags: needinfo?(jvarga)
I guess v2 is just better version of v1 and it's based on latest tom's patches.
So I think there's no need to review v1 anymore.
Flags: needinfo?(jvarga)
Attachment #8817332 - Attachment is obsolete: true
Attachment #8817332 - Flags: review?(jvarga)
Comment on attachment 8829764 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager, v2

Review of attachment 8829764 [details] [diff] [review]:
-----------------------------------------------------------------

It seems that all test_storage_manager_persist* files don't need IndexedDB at all. I created dom/quota/test/unit in bug 1311057. Do you think you can move these files to dom/quota/test ?
I can help you with creating helpers.js, etc. if you want.

I will send comments for StorageManager.cpp separately.

::: dom/quota/StorageManager.h
@@ +45,5 @@
> +  already_AddRefed<Promise>
> +  Persist(ErrorResult& aRv);
> +
> +  already_AddRefed<Promise>
> +  Persisted(ErrorResult& aRv);

I think Persisted() and then Persist() should go before Estimate(), here and in the .cpp too.

::: dom/webidl/StorageManager.webidl
@@ +17,5 @@
> +  [Exposed=Window, Throws]
> +  Promise<boolean> persist();
> +
> +  [Throws]
> +  Promise<boolean> persisted();

Why this order of methods ?

See https://storage.spec.whatwg.org/#storagemanager

::: modules/libpref/init/all.js
@@ +5545,5 @@
>  // Disable Storage api in release builds.
>  #ifdef NIGHTLY_BUILD
>  pref("dom.storageManager.enabled", true);
> +pref("dom.storageManager.prompt.testing", false);
> +pref("dom.storageManager.prompt.testing.allow", false);

Both branches contain the same setting, do you want to put it after the ifdef ?
Attachment #8829764 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #67)
> Comment on attachment 8829764 [details] [diff] [review]
> Bug 1286717 - Expose persist/persisted method to StorageManager, v2
> 
> Review of attachment 8829764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems that all test_storage_manager_persist* files don't need IndexedDB
> at all. I created dom/quota/test/unit in bug 1311057. Do you think you can
> move these files to dom/quota/test ?
> I can help you with creating helpers.js, etc. if you want.
I thought we don't add test folders in dom/quota on purpose. It will be great if we can move test cases to dom/quota/test/mochitest, can we leverage dom/indexedDB/test/helpers.js?
> ::: dom/webidl/StorageManager.webidl
> @@ +17,5 @@
> > +  [Exposed=Window, Throws]
> > +  Promise<boolean> persist();
> > +
> > +  [Throws]
> > +  Promise<boolean> persisted();
> 
> Why this order of methods ?
> 
> See https://storage.spec.whatwg.org/#storagemanager
I though it's better to put functions in an alphabet order. I changed the order following by webidl.

> ::: modules/libpref/init/all.js
> @@ +5545,5 @@
> >  // Disable Storage api in release builds.
> >  #ifdef NIGHTLY_BUILD
> >  pref("dom.storageManager.enabled", true);
> > +pref("dom.storageManager.prompt.testing", false);
> > +pref("dom.storageManager.prompt.testing.allow", false);
> 
> Both branches contain the same setting, do you want to put it after the
> ifdef ?
I thought about it. But is it ok to expose preferences which are hidden from NIGHTLY_BUILD flag? If you think that's better to remove duplicate logic, I will move prefs after the ifdef.
(In reply to Shawn Huang [:shawnjohnjr] from comment #68)
> (In reply to Jan Varga [:janv] from comment #67)
> > Comment on attachment 8829764 [details] [diff] [review]
> > Bug 1286717 - Expose persist/persisted method to StorageManager, v2
> > 
> > Review of attachment 8829764 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It seems that all test_storage_manager_persist* files don't need IndexedDB
> > at all. I created dom/quota/test/unit in bug 1311057. Do you think you can
> > move these files to dom/quota/test ?
> > I can help you with creating helpers.js, etc. if you want.
> I thought we don't add test folders in dom/quota on purpose. It will be
> great if we can move test cases to dom/quota/test/mochitest, can we leverage
> dom/indexedDB/test/helpers.js?

I think quota manager and storage manager has got bigger enough to create a separate test suites.
Anyway, don't create dom/quota/test/mochitest, dom/quota/test is enough.
And also, I don't think we want to share dom/indexedDB/test/helpers.js, there are many functions optimized for IndexedDB. Just copy (maybe also adjust/optimize) necessary functions to new helpers.js.

> > ::: dom/webidl/StorageManager.webidl
> > @@ +17,5 @@
> > > +  [Exposed=Window, Throws]
> > > +  Promise<boolean> persist();
> > > +
> > > +  [Throws]
> > > +  Promise<boolean> persisted();
> > 
> > Why this order of methods ?
> > 
> > See https://storage.spec.whatwg.org/#storagemanager
> I though it's better to put functions in an alphabet order. I changed the
> order following by webidl.

Alphabet order is not always desired, these interfaces should match the spec for easier comparison, etc.

> 
> > ::: modules/libpref/init/all.js
> > @@ +5545,5 @@
> > >  // Disable Storage api in release builds.
> > >  #ifdef NIGHTLY_BUILD
> > >  pref("dom.storageManager.enabled", true);
> > > +pref("dom.storageManager.prompt.testing", false);
> > > +pref("dom.storageManager.prompt.testing.allow", false);
> > 
> > Both branches contain the same setting, do you want to put it after the
> > ifdef ?
> I thought about it. But is it ok to expose preferences which are hidden from
> NIGHTLY_BUILD flag? If you think that's better to remove duplicate logic, I
> will move prefs after the ifdef.

Hm, so you are adding #ifdef NIGHTLY_BUILD new_prefs #else the_same_new_prefs.
How are the new prefs hidden from NIGHTLY_BUILD ?
(In reply to Jan Varga [:janv] from comment #69)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #68)
> > (In reply to Jan Varga [:janv] from comment #67)
> > > Comment on attachment 8829764 [details] [diff] [review]
> > > Bug 1286717 - Expose persist/persisted method to StorageManager, v2
> > > 
> > > Review of attachment 8829764 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> I think quota manager and storage manager has got bigger enough to create a
> separate test suites.
> Anyway, don't create dom/quota/test/mochitest, dom/quota/test is enough.
> And also, I don't think we want to share dom/indexedDB/test/helpers.js,
> there are many functions optimized for IndexedDB. Just copy (maybe also
> adjust/optimize) necessary functions to new helpers.js.
I see. I will add mochitest test cases under dom/quota/test and add necessary helper.js functions.
> Hm, so you are adding #ifdef NIGHTLY_BUILD new_prefs #else
> the_same_new_prefs.
> How are the new prefs hidden from NIGHTLY_BUILD ?
Sorry, I'm wrong here.
Comment on attachment 8829764 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager, v2

Review of attachment 8829764 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/test/unit/test_storage_manager_persist_allow.js
@@ +1,3 @@
> +var disableWorkerTest = "Persist doesn't work in workers";
> +var testGenerator = testSteps();
> +

Ok, so this test verifies that persist() works if we set the testing prefs which actually bypasses the prompt. That's fine, but we will also need a test that verifies the real prompt. Something like dom/indexedDB/test/browser_permissionsPromptAllow.js

@@ +1,4 @@
> +var disableWorkerTest = "Persist doesn't work in workers";
> +var testGenerator = testSteps();
> +
> +function testSteps()

convert to |function* testSteps()|, all the test have been converted

@@ +5,5 @@
> +{
> +  navigator.storage.persist().then(result => {
> +    testGenerator.send(result);
> +  });
> +  let persistResult  = yield undefined;

Nit: add an empty line here

@@ +13,5 @@
> +    testGenerator.send(result);
> +  });
> +  let persistedResult  = yield undefined;
> +
> +  is(persistResult, persistedResult, "persist/persisted results shall be consistent");

Nit: add an empty line here

@@ +15,5 @@
> +  let persistedResult  = yield undefined;
> +
> +  is(persistResult, persistedResult, "persist/persisted results shall be consistent");
> +  finishTest();
> +  yield undefined;

this final yield is not needed anymore

@@ +24,5 @@
> +  SpecialPowers.pushPrefEnv({
> +    "set": [["dom.storageManager.enabled", true],
> +            ["dom.storageManager.prompt.testing", true],
> +            ["dom.storageManager.prompt.testing.allow", true]]
> +  },  runTest);

You can replace |runTest| with |continueToNextStep| and move this pushPrefEnv() call to testSteps()
Then you can remove setup().

I somehow missed this when I was reviewing test_storage_manager_estimate.js

Almost all these comments apply to test_storage_manager_persist_deny.js too.
(In reply to Jan Varga [:janv] from comment #71)
> Comment on attachment 8829764 [details] [diff] [review]
> Bug 1286717 - Expose persist/persisted method to StorageManager, v2
> 
> Review of attachment 8829764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/test/unit/test_storage_manager_persist_allow.js
> @@ +1,3 @@
> > +var disableWorkerTest = "Persist doesn't work in workers";
> > +var testGenerator = testSteps();
> > +
> 
> Ok, so this test verifies that persist() works if we set the testing prefs
> which actually bypasses the prompt. That's fine, but we will also need a
> test that verifies the real prompt. Something like
> dom/indexedDB/test/browser_permissionsPromptAllow.js
I see, I indeed missed this test. I will add it under folder "dom/quota/test".
Thanks for pointing out.
(In reply to Shawn Huang [:shawnjohnjr] from comment #72)
> (In reply to Jan Varga [:janv] from comment #71)
> > Comment on attachment 8829764 [details] [diff] [review]
> > Bug 1286717 - Expose persist/persisted method to StorageManager, v2
> > 
> > Review of attachment 8829764 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/indexedDB/test/unit/test_storage_manager_persist_allow.js
> > @@ +1,3 @@
> > > +var disableWorkerTest = "Persist doesn't work in workers";
> > > +var testGenerator = testSteps();
> > > +
> > 
> > Ok, so this test verifies that persist() works if we set the testing prefs
> > which actually bypasses the prompt. That's fine, but we will also need a
> > test that verifies the real prompt. Something like
> > dom/indexedDB/test/browser_permissionsPromptAllow.js
> I see, I indeed missed this test. I will add it under folder
> "dom/quota/test".
> Thanks for pointing out.
Hi All,

The real prompt is being implemented in Bug 1309123 currently.
However, this bug here and Bug 1309123 kind of depends on each other.
We are requested to test the real permission prompt with the real Web API of `navigator.storage.persist()` in Bug 1309123, see the comment [1].
And the real Web API is being implemented by this bug here.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1309123#c26
(In reply to Fischer [:Fischer] from comment #73)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #72)
> > (In reply to Jan Varga [:janv] from comment #71)
> > > Comment on attachment 8829764 [details] [diff] [review]
> > > Bug 1286717 - Expose persist/persisted method to StorageManager, v2
> > > 
> > > Review of attachment 8829764 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/indexedDB/test/unit/test_storage_manager_persist_allow.js
> > > @@ +1,3 @@
> > > > +var disableWorkerTest = "Persist doesn't work in workers";
> > > > +var testGenerator = testSteps();
> > > > +
> > > 
> > > Ok, so this test verifies that persist() works if we set the testing prefs
> > > which actually bypasses the prompt. That's fine, but we will also need a
> > > test that verifies the real prompt. Something like
> > > dom/indexedDB/test/browser_permissionsPromptAllow.js
> > I see, I indeed missed this test. I will add it under folder
> > "dom/quota/test".
> > Thanks for pointing out.
> Hi All,
> 
> The real prompt is being implemented in Bug 1309123 currently.
> However, this bug here and Bug 1309123 kind of depends on each other.
> We are requested to test the real permission prompt with the real Web API of
> `navigator.storage.persist()` in Bug 1309123, see the comment [1].
> And the real Web API is being implemented by this bug here.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1309123#c26

So if we need to add the simliar 'browser' test like browser_permissionsPromptAllow.js, we shall wait for Bug 1309123 landed, without landing bug 1309123 we can't verify the real prompt here. Jan, do you mind if I can open an another follow bug to add browser tests after this bug and bug 1309123 both landed?
Flags: needinfo?(jvarga)
I don't see why that's a problem. If they depend on each other, they have to land together, right ?
So the test can land together with them too, no ?
Or you planned to land this bug first (w/o having the prompt implementation) ?
Flags: needinfo?(jvarga)
(In reply to Jan Varga [:janv] from comment #75)
> I don't see why that's a problem. If they depend on each other, they have to
> land together, right ?
> So the test can land together with them too, no ?
> Or you planned to land this bug first (w/o having the prompt implementation)
> ?
Hi Jan,
I did not know we can merge different patches in different bugs and land them together.
I think we will land this bug and bug 1309123 together as you suggested. Thanks.
You don't have to merge anything, just land patches from both bugs at once.
Summary:
1. Move tests from indexedDB to quota folder
2. Add config: browser.ini, mochitest.ini, 
3. Add browserHelpers.js, browser_permissionsPrompt.html, browser_permissionsPromptAllow.js, browser_permissionsPromptDeny.js,
head.js , helpers.js
4. |promiseMessage()| verifies the resolve with boolean value of |persist()| call,
sent from |finishTest()| in browser_permissionsPrompt.html.
Comment on attachment 8835473 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager, v3

Hi Jan,

This patch changed:
1. Move tests from indexedDB to quota folder
2. Add config: browser.ini, mochitest.ini, 
3. Add browserHelpers.js, browser_permissionsPrompt.html, browser_permissionsPromptAllow.js, browser_permissionsPromptDeny.js,
head.js , helpers.js
4. |promiseMessage()| verifies the resolve with boolean value of |persist()| call,
sent from |finishTest()| in browser_permissionsPrompt.html.
5. Kepp clearAllDatabases in helpers.js, it is still required
Attachment #8835473 - Flags: review?(jvarga)
Comment on attachment 8835473 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager, v3

Review of attachment 8835473 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/test/browser_permissionsPromptAllow.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +const testPageURL = "http://mochi.test:8888/browser/" +

When I was working on bug 1268804, I found this should be loaded over https. I will modify to |const testPageURL = "https://example.com/browser/"| based on [1].
I will change it next around since I've already requested for review.
[1] https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests#Support-files.
Whiteboard: storage-v1 → [storage-v1]
Component: DOM → DOM: Quota Manager
Comment on attachment 8835473 [details] [diff] [review]
Bug 1286717 - Expose persist/persisted method to StorageManager, v3

Review of attachment 8835473 [details] [diff] [review]:
-----------------------------------------------------------------

Some initial comments for StorageManager.cpp

::: dom/quota/StorageManager.cpp
@@ +54,3 @@
>  
>  public:
> +  RequestResolver(Promise* aPromise, Type aType)

I think "Type aType, Promise* aPromise" makes more sense.

@@ +62,5 @@
>      MOZ_ASSERT(NS_IsMainThread());
>      MOZ_ASSERT(aPromise);
>    }
>  
> +  RequestResolver(PromiseWorkerProxy* aProxy, Type aType)

"Type aType, PromiseWorkerProxy* aProxy"

@@ +76,5 @@
>    void
> +  ResolveOrReject();
> +
> +  Type
> +  GetType() const

getters usually go first (before ResolveOrReject in this case)

@@ +148,5 @@
> +    MOZ_ASSERT(aProxy);
> +  }
> +
> +  virtual bool
> +  MainThreadRun() override;

The style has changed in the meantime, we don't need "virtual" if there's "override" already.

@@ +164,5 @@
> +                                     nsPIDOMWindowInner* aWindow,
> +                                     Promise* aPromise)
> +    : mPrincipal(aPrincipal), mWindow(aWindow),
> +      mPermission(PermissionState::Prompt),
> +      mPromise(aPromise)

: mPrincipal(aPrincipal)
, mWindow(aWindow)
, mPermission(PermissionState::Prompt)
, mPromise(aPromise)

@@ +166,5 @@
> +    : mPrincipal(aPrincipal), mWindow(aWindow),
> +      mPermission(PermissionState::Prompt),
> +      mPromise(aPromise)
> +  {
> +    MOZ_ASSERT(aPromise);

What about aPrincipal and aWindow, I think they must be non-null too.

@@ +170,5 @@
> +    MOZ_ASSERT(aPromise);
> +    mRequester = new nsContentPermissionRequester(mWindow);
> +  }
> +
> +  NS_IMETHODIMP

NS_IMETHODIMP or NS_IMETHOD or just nsresult ?

@@ +179,5 @@
> +
> +private:
> +  virtual ~PersistentStoragePermissionRequest() {}
> +
> +  nsresult ResolvePromise();

return type on a separate line please

@@ +180,5 @@
> +private:
> +  virtual ~PersistentStoragePermissionRequest() {}
> +
> +  nsresult ResolvePromise();
> +  nsCOMPtr<nsIPrincipal> mPrincipal;

all these members can go to the top, before "public:"

@@ +182,5 @@
> +
> +  nsresult ResolvePromise();
> +  nsCOMPtr<nsIPrincipal> mPrincipal;
> +  nsCOMPtr<nsPIDOMWindowInner> mWindow;
> +  PermissionState mPermission;

PermissionState is an enum and should take just one byte, so I would move it to the end.

@@ +235,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +PersistStorageForPrincipal(nsIPrincipal* aPrincipal,

I don't know, we changed the method name from PersistStorageForPrincipal to just Persist() in nsIQuotaManagerService. I think we might want to do the same here too.

@@ +244,5 @@
> +
> +  nsCOMPtr<nsIQuotaManagerService> qms = QuotaManagerService::GetOrCreate();
> +  if (NS_WARN_IF(!qms)) {
> +    return NS_ERROR_FAILURE;
> +  }

Nit: add a blank line here

@@ +254,5 @@
> +  return NS_OK;
> +};
> +
> +nsresult
> +CheckIsPersistedForPrincipal(nsIPrincipal* aPrincipal,

Again, the method name changed.

@@ +263,5 @@
> +
> +  nsCOMPtr<nsIQuotaManagerService> qms = QuotaManagerService::GetOrCreate();
> +  if (NS_WARN_IF(!qms)) {
> +    return NS_ERROR_FAILURE;
> +  }

Nit: add a blank line here

@@ +533,5 @@
> +        !NS_WARN_IF(NS_FAILED(resultCode));
> +      break;
> +    case Type::Estimate:
> +    default:
> +      MOZ_CRASH("Invalid callback type!");

The "break" is not needed for two reasons: "break" is not needed for "default"/last case, break/return is not needed after MOZ_CRASH()

@@ +628,5 @@
> +    new RequestResolver(mProxy, RequestResolver::Type::Persisted);
> +
> +  RefPtr<nsIQuotaRequest> request;
> +  nsresult rv = CheckIsPersistedForPrincipal(principal,
> +                                              getter_AddRefs(request));

Nit: indentation

@@ +631,5 @@
> +  nsresult rv = CheckIsPersistedForPrincipal(principal,
> +                                              getter_AddRefs(request));
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return false;
> +  }

Nit: add a blank line here

@@ +665,5 @@
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::GetPrincipal(nsIPrincipal** aRequestingPrincipal)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  NS_ENSURE_ARG_POINTER(aRequestingPrincipal);

this can be just a MOZ_ASSERT()
you can also assert mPrincipal, since you call NS_ADDREF() below.

@@ +675,5 @@
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::GetWindow(mozIDOMWindow** aRequestingWindow)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  NS_ENSURE_ARG_POINTER(aRequestingWindow);

just assert
and assert mWindow too

@@ +686,5 @@
> +PersistentStoragePermissionRequest::GetElement(nsIDOMElement** aElement)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  NS_ENSURE_ARG_POINTER(aElement);

just assert

@@ +758,5 @@
> +PersistentStoragePermissionRequest::GetTypes(nsIArray** aTypes)
> +{
> +  nsTArray<nsString> emptyOptions;
> +
> +  return nsContentPermissionUtils::CreatePermissionArray(NS_LITERAL_CSTRING("persistent-storage"),

Does this maintain 80 chars per line ?
Attachment #8835473 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #87)
> Comment on attachment 8835473 [details] [diff] [review]
> Bug 1286717 - Expose persist/persisted method to StorageManager, v3
> 
> Review of attachment 8835473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some initial comments for StorageManager.cpp
Thanks for your comments, I will fix as soon as possible.

> ::: dom/quota/StorageManager.cpp
> : mPrincipal(aPrincipal)
> , mWindow(aWindow)
> , mPermission(PermissionState::Prompt)
> , mPromise(aPromise)
> 
> @@ +166,5 @@
> > +    : mPrincipal(aPrincipal), mWindow(aWindow),
> > +      mPermission(PermissionState::Prompt),
> > +      mPromise(aPromise)
> > +  {
> > +    MOZ_ASSERT(aPromise);
> 
> What about aPrincipal and aWindow, I think they must be non-null too.
Okay I will add it.

> 
> @@ +170,5 @@
> > +    MOZ_ASSERT(aPromise);
> > +    mRequester = new nsContentPermissionRequester(mWindow);
> > +  }
> > +
> > +  NS_IMETHODIMP
> 
> NS_IMETHODIMP or NS_IMETHOD or just nsresult ?
Should be nsresult, I did not catch this problem after renaming the function name.

> @@ +758,5 @@
> > +PersistentStoragePermissionRequest::GetTypes(nsIArray** aTypes)
> > +{
> > +  nsTArray<nsString> emptyOptions;
> > +
> > +  return nsContentPermissionUtils::CreatePermissionArray(NS_LITERAL_CSTRING("persistent-storage"),
> 
> Does this maintain 80 chars per line ?
No, it doesn't. I will fix it.
Attachment #8845231 - Attachment description: Bug 1286717 - Expose persist/persisted method to StorageManager, v4 → Bug 1286717 - Part 1: Expose persist/persisted to StorageManager, v4
Attached patch BUG-1309123-p2.patch (front-end) (obsolete) — Splinter Review
Attached patches in bug-1309123. Running test cases of Part2 patch requires theses patches.
Attachment #8845307 - Attachment description: Bug 1286717 - Expose persist/persisted method to StorageManager, v4 → Bug 1286717 - Part 1: Expose persist/persisted method to StorageManager, v4
Hi Jan,
You've created head.js under test/unit folder for xpcshell tests in Bug 1339081.
I wonder you have any objection if I can create the separate helper.js for mochitest.
Or I shall reuse unit/head.js as much as I can? I want to know this in advance.

A part of code is redundant for sure between mochitest/xpcshell helpers.
So I'm trying to reuse unit/head.js, but it seems a part of code doesn't expose to mochitest and only for xpcshell.

2 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/test_storage_manager_persist_allow.html | uncaught exception - TypeError: Cr is undefined at @https://example.com/tests/dom/quota/test/unit/head.js:8:7
Flags: needinfo?(jvarga)
The test suites have their specifics, so I'm ok with having separate helpers for them. We might try to unify them one day, but for now it's ok keep them separate.
Flags: needinfo?(jvarga)
Comment on attachment 8845427 [details] [diff] [review]
Bug 1286717 - Part 3: Add wpt test cases for persist/persisted functions

Review of attachment 8845427 [details] [diff] [review]:
-----------------------------------------------------------------

How did you update MANIFEST.json file ?
There was a post in dev-platform about this:
"Please do NOT hand-edit web platform test MANIFEST.json files"

Maybe you did the right thing, just double checking.
(In reply to Jan Varga [:janv] from comment #105)
> Comment on attachment 8845427 [details] [diff] [review]
> Bug 1286717 - Part 3: Add wpt test cases for persist/persisted functions
> 
> Review of attachment 8845427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How did you update MANIFEST.json file ?
> There was a post in dev-platform about this:
> "Please do NOT hand-edit web platform test MANIFEST.json files"
> 
> Maybe you did the right thing, just double checking.
Yeah, I'm aware of that.
Running 'mach web-platform-tests --manifest-update' to update the MANIFEST.json. 
This MANIFEST.json requires to update every revision and sometimes '--manifest-update' option touches unrelated changes (here we might have some uuid changes unrelated to 'storage'). AFAIK, try-server will run 'manifest-update' to check that MANIFEST.json exactly matches auto-generated modification.
Blocks: 1349152
Comment on attachment 8845373 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted method to StorageManager, v4

Review of attachment 8845373 [details] [diff] [review]:
-----------------------------------------------------------------

Doing another round of review, here are some comments.

::: dom/quota/StorageManager.cpp
@@ +683,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PersistentStoragePermissionRequest::GetPrincipal(
> +                                            nsIPrincipal** aRequestingPrincipal)

Nit: I think this can be just aPrincipal

@@ +701,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aRequestingWindow);
> +
> +  NS_ADDREF(*aRequestingWindow = mWindow);
> +  MOZ_ASSERT(mWindow);

Ok, so I suggested to also assert mWindow, but ...
Assertions are usually only useful when you assert variables before you actually use them in real code. Here in this specific case, if mWindow is null, you assign it to *aRequestingWindow and then it's addrefed by calling *aRequestingWindow->AddRef()
So you end up dereferencing a null pointer which can potentially cause a security problem.
If you put the assert before the addref, the assertion will catch the null pointer and crash the app in a secure manner, only in the debug mode, but that should be enough to catch possible problems.
(In reply to Jan Varga [:janv] from comment #107)
> Comment on attachment 8845373 [details] [diff] [review]
> Bug 1286717 - Part 1: Expose persist/persisted method to StorageManager, v4
> 
> Review of attachment 8845373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Doing another round of review, here are some comments.
> 
> ::: dom/quota/StorageManager.cpp
> @@ +683,5 @@
> > +}
> > +
> > +NS_IMETHODIMP
> > +PersistentStoragePermissionRequest::GetPrincipal(
> > +                                            nsIPrincipal** aRequestingPrincipal)
> 
> Nit: I think this can be just aPrincipal
Okay, I will rename it.
> 
> @@ +701,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aRequestingWindow);
> > +
> > +  NS_ADDREF(*aRequestingWindow = mWindow);
> > +  MOZ_ASSERT(mWindow);
> 
> Ok, so I suggested to also assert mWindow, but ...
> Assertions are usually only useful when you assert variables before you
> actually use them in real code. Here in this specific case, if mWindow is
> null, you assign it to *aRequestingWindow and then it's addrefed by calling
> *aRequestingWindow->AddRef()
> So you end up dereferencing a null pointer which can potentially cause a
> security problem.
> If you put the assert before the addref, the assertion will catch the null
> pointer and crash the app in a secure manner, only in the debug mode, but
> that should be enough to catch possible problems.
This is my bad. I will fix it.
bug 1348660 landed, so I will rebase the patch again.
Remove meta file for interfaces.https.ini. After bug 1286717 landed, this test case can also pass.
Hi Jan,
Do you have any chance to send any comments this week?
I have to know if 4/17 is the reasonable date to land all MVP patches.
Please let me know if you have any big concern regarding Part 1 patch.
Yes, I will send it following days.
Comment on attachment 8845373 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted method to StorageManager, v4

Review of attachment 8845373 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there, I need to see an updated patch before final r+

::: dom/quota/StorageManager.cpp
@@ +246,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +Persist(nsIPrincipal* aPrincipal, nsIQuotaRequest** aRequest)

Nit: This method has only one caller (persist is only exposed to Window), so you can call stuff directly in PersistentStoragePermissionRequest::Allow().

@@ +265,5 @@
> +  return NS_OK;
> +};
> +
> +nsresult
> +Persisted(nsIPrincipal* aPrincipal, nsIQuotaRequest** aRequest)

This could also take |nsIQuotaUsageCallback* aCallback|, just like in GetUsageForPrincipal()
Then you can call SetCallback() in this method.

@@ +313,5 @@
> +
> +    nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();
> +    MOZ_ASSERT(principal);
> +
> +    nsresult rv = NS_OK;

This |rv| might not be needed.

@@ +321,5 @@
> +        RefPtr<RequestResolver> resolver =
> +          new RequestResolver(RequestResolver::Type::Persisted, promise);
> +
> +        RefPtr<nsIQuotaRequest> request;
> +        rv = Persisted(principal, getter_AddRefs(request));

Nit: I think you can assign to aRv directly here and below

@@ +323,5 @@
> +
> +        RefPtr<nsIQuotaRequest> request;
> +        rv = Persisted(principal, getter_AddRefs(request));
> +        if (NS_SUCCEEDED(rv)) {
> +          request->SetCallback(resolver);

You can do this in Persisted()

@@ +352,5 @@
> +      default:
> +        MOZ_CRASH("Invalid aRequest type!");
> +    }
> +
> +    if (NS_WARN_IF(NS_FAILED(rv))) {

if (NS_WARN_IF(aRv.Failed())) {

@@ +353,5 @@
> +        MOZ_CRASH("Invalid aRequest type!");
> +    }
> +
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      aRv.Throw(rv);

This is then not needed

@@ +418,5 @@
> +    permissionManager->TestExactPermissionFromPrincipal(aPrincipal,
> +                                                        "persistent-storage",
> +                                                        &permission);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return PermissionState::Denied;

So what's the difference between failing to get the permission manager and failing to test the permission ? The former resolves as |Prompt| and the latter as |Denied|
I think it would be better to propagate the error result here:
nsresult
TestPermission(nsIPrincipal* aPrincipal,
               PermissionState* aPermission)

@@ +421,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return PermissionState::Denied;
> +  }
> +
> +  return ActionToPermissionState(permission);

I don't see why we have to use PermissionState here instead of PermissionValue, as far as I know, the value is only used internally here, it's not exposed to DOM.

@@ +458,5 @@
> +      if (mProxy) {
> +        mProxy->CleanUp();
> +      }
> +    }
> +  } autoCleanUp(proxy);

This auto cleanup stuff is only needed for the worker case. You can use Maybe<...> to only initialize it for the worker case.

@@ -169,3 @@
>  }
>  
> -NS_IMPL_ISUPPORTS(EstimateResolver, nsIQuotaUsageCallback)

This could stay here.
Just move |NS_IMPL_ISUPPORTS(RequestResolver, nsIQuotaUsageCallback, nsIQuotaCallback)| here.

I'm just trying to preserve the order declared by:
NS_DECL_THREADSAFE_ISUPPORTS
NS_DECL_NSIQUOTACALLBACK
NS_DECL_NSIQUOTAUSAGECALLBACK

@@ +515,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +RequestResolver::OnComplete(nsIQuotaRequest *aRequest)

Nit: this should go before OnUsageResult()

@@ +527,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  RequestResolver* result = static_cast<RequestResolver*>(callback.get());

This can be:
auto result = static_cast<RequestResolver*>(callback.get());

@@ +555,5 @@
> +    default:
> +      MOZ_CRASH("Invalid callback type!");
> +  }
> +
> +  // In a main thread request.

I think following block can be shared by OnUsageResult and OnComplete:
  // In a main thread request.
  if (!mProxy) {
    MOZ_ASSERT(mPromise);

    ResolveOrReject();
    return NS_OK;
  }

  {
    // In a worker thread request.
    MutexAutoLock lock(mProxy->Lock());

    if (NS_WARN_IF(mProxy->CleanedUp())) {
      return NS_ERROR_FAILURE;
    }

    RefPtr<FinishWorkerRunnable> runnable = new FinishWorkerRunnable(this);
    if (NS_WARN_IF(!runnable->Dispatch())) {
      return NS_ERROR_FAILURE;
    }
  }

@@ +655,5 @@
> +
> +  // All the methods in nsIQuotaManagerService shouldn't synchronously fire
> +  // any callbacks when they are being executed. Even when a result is ready,
> +  // a new runnable should be dispatched to current thread to fire the callback
> +  // asynchronously.

I would add something like: ... so it's safe to set the callback after we call Persisted()

@@ +656,5 @@
> +  // All the methods in nsIQuotaManagerService shouldn't synchronously fire
> +  // any callbacks when they are being executed. Even when a result is ready,
> +  // a new runnable should be dispatched to current thread to fire the callback
> +  // asynchronously.
> +  request->SetCallback(resolver);

You don't check rv here, but SetCallback() can't fail, so just wrap it in MOZ_ALWAYS_SUCCEEDS(...)

@@ +670,5 @@
> +  // Grant permission if pref'ed on.
> +  if (Preferences::GetBool("dom.storageManager.prompt.testing", false)) {
> +    if (Preferences::GetBool("dom.storageManager.prompt.testing.allow",
> +                             false)) {
> +      mPermission = PermissionState::Granted;

Allow() also sets mPermission to Granted.

@@ +675,5 @@
> +      return Allow(JS::UndefinedHandleValue);
> +    }
> +
> +    mPermission = PermissionState::Denied;
> +    return ResolvePromise();

See my comment in ResolvePromise(). I think you can just call Cancel() here.

@@ +724,5 @@
> +
> +  // `Cancel` is called if the user denied permission or dismissed the
> +  // permission request. To distinguish between the two, we set the
> +  // permission to "prompt" and query the permission manager in
> +  // `ResolvePromise`.

Do we actually need to distinguish between the two ?

@@ +727,5 @@
> +  // permission to "prompt" and query the permission manager in
> +  // `ResolvePromise`.
> +  mPermission = PermissionState::Prompt;
> +
> +  return ResolvePromise();

See my comment in ResolvePromise().
Can we just call:
mPromise->MaybeResolve(false);
here ?

@@ +742,5 @@
> +
> +  RefPtr<nsIQuotaRequest> request;
> +
> +  nsresult rv = Persist(mPrincipal, getter_AddRefs(request));
> +  if (NS_SUCCEEDED(rv)) {

if (NS_WARN_IF(NS_FAILED(...
  return rv;

@@ +743,5 @@
> +  RefPtr<nsIQuotaRequest> request;
> +
> +  nsresult rv = Persist(mPrincipal, getter_AddRefs(request));
> +  if (NS_SUCCEEDED(rv)) {
> +    request->SetCallback(resolver);

MOZ_ALWAYS_SUCCEEDS(...)

@@ +764,5 @@
> +}
> +
> +nsresult
> +PersistentStoragePermissionRequest::ResolvePromise()
> +{

Ok, so I investigated the prompt flow a bit and I think we don't need ResolvePromise() method at all. It seems we don't need TestPermission() either.
mPermission is then also useless I think.

All in all, it seems this was mostly copied from Notification.cpp, but it needs to be optimized for our case a bit more.
Attachment #8845373 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #114)
> Comment on attachment 8845373 [details] [diff] [review]
> Bug 1286717 - Part 1: Expose persist/persisted method to StorageManager, v4
> 
> Review of attachment 8845373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost there, I need to see an updated patch before final r+
> 
> ::: dom/quota/StorageManager.cpp
> @@ +246,5 @@
> >    return NS_OK;
> >  }
> >  
> > +nsresult
> > +Persist(nsIPrincipal* aPrincipal, nsIQuotaRequest** aRequest)
> 
> Nit: This method has only one caller (persist is only exposed to Window), so
> you can call stuff directly in PersistentStoragePermissionRequest::Allow().
Sure.
> 
> @@ +265,5 @@
> > +  return NS_OK;
> > +};
> > +
> > +nsresult
> > +Persisted(nsIPrincipal* aPrincipal, nsIQuotaRequest** aRequest)
> 
> This could also take |nsIQuotaUsageCallback* aCallback|, just like in
> GetUsageForPrincipal()
> Then you can call SetCallback() in this method.
> 
Sure, I will move SetCallback in this method.
> @@ +313,5 @@
> > +
> > +    nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();
> > +    MOZ_ASSERT(principal);
> > +
> > +    nsresult rv = NS_OK;
> 
> This |rv| might not be needed.
> 
> @@ +321,5 @@
> > +        RefPtr<RequestResolver> resolver =
> > +          new RequestResolver(RequestResolver::Type::Persisted, promise);
> > +
> > +        RefPtr<nsIQuotaRequest> request;
> > +        rv = Persisted(principal, getter_AddRefs(request));
> 
> Nit: I think you can assign to aRv directly here and below
> 
> @@ +323,5 @@
> > +
> > +        RefPtr<nsIQuotaRequest> request;
> > +        rv = Persisted(principal, getter_AddRefs(request));
> > +        if (NS_SUCCEEDED(rv)) {
> > +          request->SetCallback(resolver);
> 
> You can do this in Persisted()
> 
Okay.
> @@ +352,5 @@
> > +      default:
> > +        MOZ_CRASH("Invalid aRequest type!");
> > +    }
> > +
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> 
> if (NS_WARN_IF(aRv.Failed())) {
> 
> @@ +353,5 @@
> > +        MOZ_CRASH("Invalid aRequest type!");
> > +    }
> > +
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +      aRv.Throw(rv);
> 
> This is then not needed
> 
> @@ +418,5 @@
> > +    permissionManager->TestExactPermissionFromPrincipal(aPrincipal,
> > +                                                        "persistent-storage",
> > +                                                        &permission);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return PermissionState::Denied;
> 
> So what's the difference between failing to get the permission manager and
> failing to test the permission ? The former resolves as |Prompt| and the
> latter as |Denied|
> I think it would be better to propagate the error result here:
> nsresult
> TestPermission(nsIPrincipal* aPrincipal,
>                PermissionState* aPermission)
> 
Yes, it makes sense than what I did.
> @@ +421,5 @@
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return PermissionState::Denied;
> > +  }
> > +
> > +  return ActionToPermissionState(permission);
> 
> I don't see why we have to use PermissionState here instead of
> PermissionValue, as far as I know, the value is only used internally here,
> it's not exposed to DOM.
Do you mean PermissionValue in indexeddb?
Or you mean I should directly use enum in nsIPermissionManager.

PermissionState is exposed to dom but mPermission which we don't need it.
http://searchfox.org/mozilla-central/source/dom/webidl/PermissionStatus.webidl#10

Since we don't need mPermission, I will use enum in nsIPermissionManager here. 
> 
> @@ +458,5 @@
> > +      if (mProxy) {
> > +        mProxy->CleanUp();
> > +      }
> > +    }
> > +  } autoCleanUp(proxy);
> 
> This auto cleanup stuff is only needed for the worker case. You can use
> Maybe<...> to only initialize it for the worker case.
Thanks for catching this. I indeed forgot to check only for worker case.

> 
> @@ -169,3 @@
> >  }
> >  
> > -NS_IMPL_ISUPPORTS(EstimateResolver, nsIQuotaUsageCallback)
> 
> This could stay here.
> Just move |NS_IMPL_ISUPPORTS(RequestResolver, nsIQuotaUsageCallback,
> nsIQuotaCallback)| here.
Okay, I should catch this when doing self-review :(
> 
> I'm just trying to preserve the order declared by:
> NS_DECL_THREADSAFE_ISUPPORTS
> NS_DECL_NSIQUOTACALLBACK
> NS_DECL_NSIQUOTAUSAGECALLBACK
> 
> @@ +515,5 @@
> >    return NS_OK;
> >  }
> >  
> > +NS_IMETHODIMP
> > +RequestResolver::OnComplete(nsIQuotaRequest *aRequest)
> 
> Nit: this should go before OnUsageResult()
> 
Okay, I should catch this when doing self-review :(
> @@ +527,5 @@
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return rv;
> > +  }
> > +
> > +  RequestResolver* result = static_cast<RequestResolver*>(callback.get());
> 
> This can be:
> auto result = static_cast<RequestResolver*>(callback.get());
Okay, I forgot to use auto here.
> 
> @@ +555,5 @@
> > +    default:
> > +      MOZ_CRASH("Invalid callback type!");
> > +  }
> > +
> > +  // In a main thread request.
> 
> I think following block can be shared by OnUsageResult and OnComplete:
>   // In a main thread request.
>   if (!mProxy) {
>     MOZ_ASSERT(mPromise);
> 
>     ResolveOrReject();
>     return NS_OK;
>   }
> 
>   {
>     // In a worker thread request.
>     MutexAutoLock lock(mProxy->Lock());
> 
>     if (NS_WARN_IF(mProxy->CleanedUp())) {
>       return NS_ERROR_FAILURE;
>     }
> 
>     RefPtr<FinishWorkerRunnable> runnable = new FinishWorkerRunnable(this);
>     if (NS_WARN_IF(!runnable->Dispatch())) {
>       return NS_ERROR_FAILURE;
>     }
>   }
Okay, this is again I did not catch in the self-review.
> @@ +655,5 @@
> > +
> > +  // All the methods in nsIQuotaManagerService shouldn't synchronously fire
> > +  // any callbacks when they are being executed. Even when a result is ready,
> > +  // a new runnable should be dispatched to current thread to fire the callback
> > +  // asynchronously.
> 
> I would add something like: ... so it's safe to set the callback after we
> call Persisted()
> 
I will add it.
> @@ +656,5 @@
> > +  // All the methods in nsIQuotaManagerService shouldn't synchronously fire
> > +  // any callbacks when they are being executed. Even when a result is ready,
> > +  // a new runnable should be dispatched to current thread to fire the callback
> > +  // asynchronously.
> > +  request->SetCallback(resolver);
> 
> You don't check rv here, but SetCallback() can't fail, so just wrap it in
> MOZ_ALWAYS_SUCCEEDS(...)
Ok, and I will move this to Persisted().
> 
> @@ +670,5 @@
> > +  // Grant permission if pref'ed on.
> > +  if (Preferences::GetBool("dom.storageManager.prompt.testing", false)) {
> > +    if (Preferences::GetBool("dom.storageManager.prompt.testing.allow",
> > +                             false)) {
> > +      mPermission = PermissionState::Granted;
> 
> Allow() also sets mPermission to Granted.
> 
Okay. I will replace it.
> @@ +675,5 @@
> > +      return Allow(JS::UndefinedHandleValue);
> > +    }
> > +
> > +    mPermission = PermissionState::Denied;
> > +    return ResolvePromise();
> 
> See my comment in ResolvePromise(). I think you can just call Cancel() here.
> 
Okay.
> @@ +724,5 @@
> > +
> > +  // `Cancel` is called if the user denied permission or dismissed the
> > +  // permission request. To distinguish between the two, we set the
> > +  // permission to "prompt" and query the permission manager in
> > +  // `ResolvePromise`.
> 
> Do we actually need to distinguish between the two ?
We don't care about mPermission, so I will remove it.
> 
> @@ +727,5 @@
> > +  // permission to "prompt" and query the permission manager in
> > +  // `ResolvePromise`.
> > +  mPermission = PermissionState::Prompt;
> > +
> > +  return ResolvePromise();
> 
> See my comment in ResolvePromise().
> Can we just call:
> mPromise->MaybeResolve(false);
> here ?
Sure.
> 
> @@ +742,5 @@
> > +
> > +  RefPtr<nsIQuotaRequest> request;
> > +
> > +  nsresult rv = Persist(mPrincipal, getter_AddRefs(request));
> > +  if (NS_SUCCEEDED(rv)) {
> 
> if (NS_WARN_IF(NS_FAILED(...
>   return rv;
Okay.
> 
> @@ +743,5 @@
> > +  RefPtr<nsIQuotaRequest> request;
> > +
> > +  nsresult rv = Persist(mPrincipal, getter_AddRefs(request));
> > +  if (NS_SUCCEEDED(rv)) {
> > +    request->SetCallback(resolver);
> 
> MOZ_ALWAYS_SUCCEEDS(...)
Okay.
> 
> @@ +764,5 @@
> > +}
> > +
> > +nsresult
> > +PersistentStoragePermissionRequest::ResolvePromise()
> > +{
> 
> Ok, so I investigated the prompt flow a bit and I think we don't need
> ResolvePromise() method at all. It seems we don't need TestPermission()
> either.
> mPermission is then also useless I think.
> 
> All in all, it seems this was mostly copied from Notification.cpp, but it
> needs to be optimized for our case a bit more.
I will remove them. I don't need to get mPermission in our case.
Comment on attachment 8852880 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted method to StorageManager, v5

Review of attachment 8852880 [details] [diff] [review]:
-----------------------------------------------------------------

Fix issues addressed in Comment 114. interdiff also attached.
Attachment #8852880 - Flags: review?(jvarga)
Comment on attachment 8852880 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted method to StorageManager, v5

Review of attachment 8852880 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/StorageManager.cpp
@@ +11,5 @@
>  #include "mozilla/dom/StorageManagerBinding.h"
>  #include "mozilla/dom/WorkerPrivate.h"
>  #include "mozilla/ErrorResult.h"
> +#include "nsContentPermissionHelper.h"
> +#include "nsIPermissionManager.h"

Is nsIPermissionManager.h still needed ?

@@ +90,4 @@
>    { }
> +
> +  nsresult
> +  HandleRequest();

Nit: RequestFinished() sounds a bit better

@@ +192,5 @@
> +  { }
> +};
> +
> +NS_IMPL_ISUPPORTS(PersistentStoragePermissionRequest,
> +                  nsIContentPermissionRequest)

Nit: This should go after:
nsresult
PersistentStoragePermissionRequest::Start()
{
  ...
}

@@ +267,5 @@
> +  if (NS_WARN_IF(!qms)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsresult rv = qms->Persisted(aPrincipal, aRequest);

This is not a big deal, but usually I prefer to not set "out" arguments before we are sure that everything succeeds. So I would add a new variable here, nsCOMPtr<nsIQuotaRequest> request and then do request.forget(aRequest) before final return NS_OK

@@ +276,5 @@
> +  // All the methods in nsIQuotaManagerService shouldn't synchronously fire
> +  // any callbacks when they are being executed. Even when a result is ready,
> +  // a new runnable should be dispatched to current thread to fire the callback
> +  // asynchronously. It's safe to set the callback after we call Persisted().
> +  MOZ_ALWAYS_SUCCEEDS((*aRequest)->SetCallback(aCallback));

Once you use the local "request" variable, you can use it here too.

@@ +405,1 @@
>  {

I didn't test this, but I believe all this can be done with fewer variable assignments and less "if" statements.

What about something like this:
  class MOZ_STACK_CLASS AutoCleanup final
  {
    RefPtr<PromiseWorkerProxy> mProxy;

  public:
    explicit AutoCleanup(PromiseWorkerProxy* aProxy)
      : mProxy(aProxy)
    {
      MOZ_ASSERT(aProxy);
    }

    ~AutoCleanup()
    {
      MOZ_ASSERT(mProxy);

      mProxy->CleanUp();
    }
  };

  RefPtr<Promise> promise;
  Maybe<AutoCleanup> autoCleanup;

  if (mPromise) {
    promise = mPromise;
  } else {
    MOZ_ASSERT(mProxy);
    promise = mProxy->WorkerPromise();

    // Only clean up for worker case.
    autoCleanup.emplace(mProxy);
  }

  if (mType == Type::Estimate) {

  ...

@@ +500,5 @@
> +  nsresult resultCode;
> +
> +  switch (type) {
> +    case Type::Persisted:
> +      if (!NS_WARN_IF(NS_FAILED(aRequest->GetResultCode(&resultCode))) &&

I need to take a look at these !NS_WARN_IF() one more time, but that's it. We are really close to final r+

@@ +640,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aPrincipal);
> +
> +  MOZ_ASSERT(mPrincipal);

Nit: move this right after MOZ_ASSERT(aPrincipal) and add a blank line after it. Just like in GetWindow() below.
Attachment #8852880 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #123)
> Comment on attachment 8852880 [details] [diff] [review]
> Bug 1286717 - Part 1: Expose persist/persisted method to StorageManager, v5
> 
> Review of attachment 8852880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/StorageManager.cpp
> @@ +11,5 @@
> >  #include "mozilla/dom/StorageManagerBinding.h"
> >  #include "mozilla/dom/WorkerPrivate.h"
> >  #include "mozilla/ErrorResult.h"
> > +#include "nsContentPermissionHelper.h"
> > +#include "nsIPermissionManager.h"
> 
> Is nsIPermissionManager.h still needed ?
No. I will remove it.
> 
> @@ +90,4 @@
> >    { }
> > +
> > +  nsresult
> > +  HandleRequest();
> 
> Nit: RequestFinished() sounds a bit better
Sure. Indeed it's a better name.
> 
> @@ +192,5 @@
> > +  { }
> > +};
> > +
> > +NS_IMPL_ISUPPORTS(PersistentStoragePermissionRequest,
> > +                  nsIContentPermissionRequest)
> 
> Nit: This should go after:
Ok.
> nsresult
> PersistentStoragePermissionRequest::Start()
> {
>   ...
> }
> 
> @@ +267,5 @@
> > +  if (NS_WARN_IF(!qms)) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  nsresult rv = qms->Persisted(aPrincipal, aRequest);
> 
> This is not a big deal, but usually I prefer to not set "out" arguments
> before we are sure that everything succeeds. So I would add a new variable
> here, nsCOMPtr<nsIQuotaRequest> request and then do request.forget(aRequest)
> before final return NS_OK
This would be a better practice in gecko, thanks for correcting me.
I guess we should try to avoid using raw pointer.

> @@ +276,5 @@
> > +  // All the methods in nsIQuotaManagerService shouldn't synchronously fire
> > +  // any callbacks when they are being executed. Even when a result is ready,
> > +  // a new runnable should be dispatched to current thread to fire the callback
> > +  // asynchronously. It's safe to set the callback after we call Persisted().
> > +  MOZ_ALWAYS_SUCCEEDS((*aRequest)->SetCallback(aCallback));
> 
> Once you use the local "request" variable, you can use it here too.
> 
> @@ +405,1 @@
> >  {
> 
> I didn't test this, but I believe all this can be done with fewer variable
> assignments and less "if" statements.
> 
Thanks for reminders. I should do a self-review for code structure after modification.
> @@ +640,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aPrincipal);
> > +
> > +  MOZ_ASSERT(mPrincipal);
> 
> Nit: move this right after MOZ_ASSERT(aPrincipal) and add a blank line after
> it. Just like in GetWindow() below.
Sure.
Comment on attachment 8853315 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted to StorageManager

I've fixed issues addressed in Comment 123. interdiff also attached.
Attachment #8853315 - Flags: review?(jvarga)
Comment on attachment 8853315 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted to StorageManager

Review of attachment 8853315 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks great, but I found something in RequestResolver::OnComplete()
Let me test it a bit.
Attachment #8853315 - Flags: review?(jvarga) → review+
Anne, shouldn't persist() and persisted() reject promise if there's an internal error, just like we do for estimate() ?

persist() needs to store a new state on disk while estimate() is just a readonly operation, so if we reject promise for estimate() we should logically also do it for persist() I think.

Is there any practical reason for not doing it ?
Flags: needinfo?(annevk)
(In reply to Jan Varga [:janv] from comment #129)
> Anne, shouldn't persist() and persisted() reject promise if there's an
> internal error, just like we do for estimate() ?
> 
> persist() needs to store a new state on disk while estimate() is just a
> readonly operation, so if we reject promise for estimate() we should
> logically also do it for persist() I think.
> 
> Is there any practical reason for not doing it ?

I remember Anne said persist() would return false if there is an internal error or whatever happened.
I filed https://github.com/whatwg/storage/issues/44. Returning false still seems reasonable since unexpected rejections are somewhat hard to deal with, but if there's a good reason to expose the distinction I'd be okay with using a TypeError instead.
Flags: needinfo?(annevk)
(In reply to Anne (:annevk) from comment #131)
> I filed https://github.com/whatwg/storage/issues/44. Returning false still
> seems reasonable since unexpected rejections are somewhat hard to deal with,
> but if there's a good reason to expose the distinction I'd be okay with
> using a TypeError instead.

Ok, I'll see if it's really needed.

One more thing, if I'm reading the spec correctly, for persist() if the "persistent-storage" permission is not granted we should get current "persisted" state and resolve the promise with it.
However, current implementation just resolves the promise with "false", see PersistentStoragePermissionRequest::Cancel().
So if an origin is already "persisted" for some reason we won't return correct value.
(In reply to Jan Varga [:janv] from comment #132)
> (In reply to Anne (:annevk) from comment #131)
> > I filed https://github.com/whatwg/storage/issues/44. Returning false still
> > seems reasonable since unexpected rejections are somewhat hard to deal with,
> > but if there's a good reason to expose the distinction I'd be okay with
> > using a TypeError instead.
> 
> Ok, I'll see if it's really needed.
> 
> One more thing, if I'm reading the spec correctly, for persist() if the
> "persistent-storage" permission is not granted we should get current
> "persisted" state and resolve the promise with it.
> However, current implementation just resolves the promise with "false", see
> PersistentStoragePermissionRequest::Cancel().
> So if an origin is already "persisted" for some reason we won't return
> correct value.

Okay, I guess I misunderstood the specification. I thought without persistent-storage permission, we shall never let people call persist method. So without permission, people still can use persist() like persisted() to query the current status.
Is this the intention?
Flags: needinfo?(annevk)
Yeah. The result of persist() basically tells you whether your data is persisted (since that is the goal of the operation). Returning false when it's actually persisted seems a little weird.
Flags: needinfo?(annevk)
Comment on attachment 8853315 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted to StorageManager

Review of attachment 8853315 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks great, but I found something in RequestResolver::OnComplete()
Let me test it a bit.

::: dom/quota/StorageManager.cpp
@@ +489,5 @@
> +  MOZ_ASSERT(aRequest);
> +
> +  // Retrieve the result according to the RequestResolverType.
> +  nsCOMPtr<nsIQuotaCallback> callback;
> +  nsresult rv = aRequest->GetCallback(getter_AddRefs(callback));

Hm, why can't we just check mType directly here, instead of getting callback, doing the static cast, etc. ?
Getting callback would make sense if there was just one RequestResolver, no ?

@@ +502,5 @@
> +
> +  switch (type) {
> +    case Type::Persisted:
> +      if (!NS_WARN_IF(NS_FAILED(aRequest->GetResultCode(&resultCode))) &&
> +          !NS_WARN_IF(NS_FAILED(resultCode))) {

My intention in OnUsageResult was to not warn if resultCode is not NS_OK.
See |} else if (NS_SUCCEEDED(mResultCode)) {|

We already warned in the parent if something failed and resultCode in this context is just a normal value.

@@ +505,5 @@
> +      if (!NS_WARN_IF(NS_FAILED(aRequest->GetResultCode(&resultCode))) &&
> +          !NS_WARN_IF(NS_FAILED(resultCode))) {
> +        nsCOMPtr<nsIVariant> result;
> +        if (!NS_WARN_IF(NS_FAILED(aRequest->GetResult(getter_AddRefs(result))))
> +                        && result) {

|&& result| is not aligned correctly and it's actually not needed. If GetResult() succeeds, result must be non-null. We can add an assertion for that, but it's not a big deal.

I rewrote this stuff locally, I'll send you a patch, but let me test it a bit.
(In reply to Shawn Huang [:shawnjohnjr] from comment #133)
> (In reply to Jan Varga [:janv] from comment #132)
> > (In reply to Anne (:annevk) from comment #131)
> > > I filed https://github.com/whatwg/storage/issues/44. Returning false still
> > > seems reasonable since unexpected rejections are somewhat hard to deal with,
> > > but if there's a good reason to expose the distinction I'd be okay with
> > > using a TypeError instead.
> > 
> > Ok, I'll see if it's really needed.
> > 
> > One more thing, if I'm reading the spec correctly, for persist() if the
> > "persistent-storage" permission is not granted we should get current
> > "persisted" state and resolve the promise with it.
> > However, current implementation just resolves the promise with "false", see
> > PersistentStoragePermissionRequest::Cancel().
> > So if an origin is already "persisted" for some reason we won't return
> > correct value.
> 
> Okay, I guess I misunderstood the specification. I thought without
> persistent-storage permission, we shall never let people call persist
> method. So without permission, people still can use persist() like
> persisted() to query the current status.
> Is this the intention?

I would rather interpret that as falling back to persisted() if the permission is not granted.
So, people won't be able to call persist() nor using persist() to query the current status.
We can just call Persisted() in Cancel() to get current status.
Attached patch My changes for Part1 (obsolete) — Splinter Review
(In reply to Jan Varga [:janv] from comment #136)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #133)
> > (In reply to Jan Varga [:janv] from comment #132)
> > > (In reply to Anne (:annevk) from comment #131)
> > > > I filed https://github.com/whatwg/storage/issues/44. Returning false still
> > > > seems reasonable since unexpected rejections are somewhat hard to deal with,
> > > > but if there's a good reason to expose the distinction I'd be okay with
> > > > using a TypeError instead.
> > > 
> > > Ok, I'll see if it's really needed.
> > > 
> > > One more thing, if I'm reading the spec correctly, for persist() if the
> > > "persistent-storage" permission is not granted we should get current
> > > "persisted" state and resolve the promise with it.
> > > However, current implementation just resolves the promise with "false", see
> > > PersistentStoragePermissionRequest::Cancel().
> > > So if an origin is already "persisted" for some reason we won't return
> > > correct value.
> > 
> > Okay, I guess I misunderstood the specification. I thought without
> > persistent-storage permission, we shall never let people call persist
> > method. So without permission, people still can use persist() like
> > persisted() to query the current status.
> > Is this the intention?
> 
> I would rather interpret that as falling back to persisted() if the
> permission is not granted.
> So, people won't be able to call persist() nor using persist() to query the
> current status.
> We can just call Persisted() in Cancel() to get current status.
Yes, I will fix this issue today.
(In reply to Jan Varga [:janv] from comment #137)
> Created attachment 8854360 [details] [diff] [review]
> My changes for Part1

Thank you, Jan, I will merge it with my patch. The code is cleaner than I did before.
Comment on attachment 8845411 [details] [diff] [review]
Bug 1286717 - Part 2: Add mochitests for persist/persisted functions

Review of attachment 8845411 [details] [diff] [review]:
-----------------------------------------------------------------

Some initial comments, more to come.

::: dom/quota/test/test_storage_manager_persist_allow.js
@@ +12,5 @@
> +
> +  navigator.storage.persist().then(result => {
> +    testGenerator.next(result);
> +  });
> +  let persistResult  = yield undefined;

Nit: Extra whitespace here and below.

@@ +14,5 @@
> +    testGenerator.next(result);
> +  });
> +  let persistResult  = yield undefined;
> +
> +  is(persistResult, true, "Persist successfully");

"Persist succeeded" ?

::: dom/quota/test/test_storage_manager_persist_deny.js
@@ +10,5 @@
> +  }, continueToNextStep);
> +  yield undefined;
> +
> +  navigator.storage.persist().then(result => {
> +    testGenerator.next(result);

Nit: I created a helper for this, grabArgAndContinueHandler in unit/head.js, you might want to add the same helper to helpers.js
So you could just do: navigator.storage.persist().then(grabArgAndContinueHandler)
here and below and also in the _allow.js

@@ +12,5 @@
> +
> +  navigator.storage.persist().then(result => {
> +    testGenerator.next(result);
> +  });
> +  let persistResult  = yield undefined;

Nit: There's extra whitespace here.

@@ +19,5 @@
> +
> +  navigator.storage.persisted().then(result => {
> +    testGenerator.next(result);
> +  });
> +  let persistedResult  = yield undefined;

Nit: Extra whitespace.
(In reply to Jan Varga [:janv] from comment #136)
> > Okay, I guess I misunderstood the specification. I thought without
> > persistent-storage permission, we shall never let people call persist
> > method. So without permission, people still can use persist() like
> > persisted() to query the current status.
> > Is this the intention?
> 
> I would rather interpret that as falling back to persisted() if the
> permission is not granted.
> So, people won't be able to call persist() nor using persist() to query the
> current status.
> We can just call Persisted() in Cancel() to get current status.
In addition, I will add one test condition to test this situation in Part2 chrome browser test case.
Somehow, I can't find any method to test this in wpt test case. After checking 'Permission API', it only supports query.

Anne, do you think we can test this case in web-platform-test? I mean to revoke 'persistent-storage' permission then call persist() again. If there is no facility (remove permission) to do so, I will only add it in chrome browser test.
Flags: needinfo?(annevk)
No, we don't have cross-browser APIs for permission management yet unfortunately. A Mozilla-only test is best, we can see about upstreaming those once web-platform-tests gains the necessary APIs.
Flags: needinfo?(annevk)
Comment on attachment 8854360 [details] [diff] [review]
My changes for Part1

Review of attachment 8854360 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/StorageManager.cpp
@@ +474,5 @@
> +  MOZ_ASSERT(aResult);
> +  MOZ_ASSERT(mType == Type::Persist || mType == Type::Persisted);
> +
> +#ifdef DEBUG
> +  uint16_t dataType;

Hi Jan,
If I read it carefully, this might cause build failure in non-debug build. Shall we remove ifdef or add ifdef below?
(In reply to Jan Varga [:janv] from comment #140)
> Comment on attachment 8845411 [details] [diff] [review]
> Bug 1286717 - Part 2: Add mochitests for persist/persisted functions
> 
> Review of attachment 8845411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some initial comments, more to come.
> 
> ::: dom/quota/test/test_storage_manager_persist_allow.js
> @@ +14,5 @@
> > +    testGenerator.next(result);
> > +  });
> > +  let persistResult  = yield undefined;
> > +
> > +  is(persistResult, true, "Persist successfully");
> 
> "Persist succeeded" ?
Okay
> 
> ::: dom/quota/test/test_storage_manager_persist_deny.js
> @@ +10,5 @@
> > +  }, continueToNextStep);
> > +  yield undefined;
> > +
> > +  navigator.storage.persist().then(result => {
> > +    testGenerator.next(result);
> 
> Nit: I created a helper for this, grabArgAndContinueHandler in unit/head.js,
> you might want to add the same helper to helpers.js
> So you could just do:
> navigator.storage.persist().then(grabArgAndContinueHandler)
> here and below and also in the _allow.js
Originally I added it, then I thought it might not be necessary so I removed.
1. Applied Jan's patch
2. Call Persisted() in PersistentStoragePermissionRequest::Cancel()
3. Add ifdef debug in RequestResolver::GetPersisted.
(In reply to Shawn Huang [:shawnjohnjr] from comment #143)
> Hi Jan,
> If I read it carefully, this might cause build failure in non-debug build.
> Shall we remove ifdef or add ifdef below?

MOZ_ASSERT in non-debug builds is just ignored, no ?
I don't think you need to add ifdef around it, did you try it in a non-debug build ?
(In reply to Jan Varga [:janv] from comment #146)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #143)
> > Hi Jan,
> > If I read it carefully, this might cause build failure in non-debug build.
> > Shall we remove ifdef or add ifdef below?
> 
> MOZ_ASSERT in non-debug builds is just ignored, no ?
> I don't think you need to add ifdef around it, did you try it in a non-debug
> build ?

Sorry. This is embarrassing, I will remove it.
Comment on attachment 8854499 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted to StorageManager, r=janv

Review of attachment 8854499 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect!
Attachment #8854499 - Flags: review+
Comment on attachment 8845411 [details] [diff] [review]
Bug 1286717 - Part 2: Add mochitests for persist/persisted functions

Review of attachment 8845411 [details] [diff] [review]:
-----------------------------------------------------------------

A few notes for mochitests in this patch:
- I think helpers.js can be reduced even more.
- I also think that .js files should go to unit/
- Persisted() is not tested in a worker.

I'll send a patch for that.

I'll take a look at browser tests after that.
Attached patch My changes for Part2 (obsolete) — Splinter Review
Where are the .js files ?
Attachment #8845411 - Attachment is obsolete: true
Attachment #8845411 - Flags: review?(jvarga)
Attachment #8854833 - Flags: review?(jvarga)
Attachment #8854360 - Attachment is obsolete: true
Attachment #8854737 - Attachment is obsolete: true
I will send a patch for browser tests, nothing big, just a correctness stuff.
(In reply to Shawn Huang [:shawnjohnjr] from comment #155)
> Created attachment 8854833 [details] [diff] [review]
> Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v3)

Sorry, I made a mistake here. For worker version, helpers.js will not be accessible here.
I will change it back.


diff --git a/dom/quota/test/unit/test_storage_manager_persisted.js b/dom/quota/test/unit/test_storage_manager_persisted.js
--- a/dom/quota/test/unit/test_storage_manager_persisted.js
+++ b/dom/quota/test/unit/test_storage_manager_persisted.js
@@ -1,11 +1,13 @@
 var testGenerator = testSteps();
 
 function* testSteps()
 {
-  navigator.storage.persisted().then(grabArgAndContinueHandler);
+  navigator.storage.persisted().then(result => {
+   testGenerator.next(result);
+  });
   let persistedResult = yield undefined;
 
   is(persistedResult, false, "Persisted returns false");
 
   finishTest();
 }
Attachment #8854833 - Attachment is obsolete: true
Attachment #8854833 - Flags: review?(jvarga)
(In reply to Shawn Huang [:shawnjohnjr] from comment #110)
> Created attachment 8850473 [details] [diff] [review]
> Bug 1286717 - Part 3: Add wpt test cases for persist/persisted functions
> 
> Remove meta file for interfaces.https.ini. After bug 1286717 landed, this
> test case can also pass.

Hmm based on Bug 1350732, I guess it's better to give up using __dir__.ini. And set pref one by one.
(In reply to Shawn Huang [:shawnjohnjr] from comment #157)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #155)
> > Created attachment 8854833 [details] [diff] [review]
> > Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v3)
> 
> Sorry, I made a mistake here. For worker version, helpers.js will not be
> accessible here.
> I will change it back.
> 
> 
> diff --git a/dom/quota/test/unit/test_storage_manager_persisted.js
> b/dom/quota/test/unit/test_storage_manager_persisted.js
> --- a/dom/quota/test/unit/test_storage_manager_persisted.js
> +++ b/dom/quota/test/unit/test_storage_manager_persisted.js
> @@ -1,11 +1,13 @@
>  var testGenerator = testSteps();
>  
>  function* testSteps()
>  {
> -  navigator.storage.persisted().then(grabArgAndContinueHandler);
> +  navigator.storage.persisted().then(result => {
> +   testGenerator.next(result);
> +  });
>    let persistedResult = yield undefined;
>  
>    is(persistedResult, false, "Persisted returns false");
>  
>    finishTest();
>  }

Why don't you add grabArgAndContinueHandler version for workers ?
See workerScript() function.
Attached patch My changes for Part2 (obsolete) — Splinter Review
Additional style and correctness fixes for Part 2.
Comment on attachment 8856450 [details] [diff] [review]
Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v3)

Review of attachment 8856450 [details] [diff] [review]:
-----------------------------------------------------------------

See the style and correctness issues below.
I also found a more serious problems in the browser tests, a patch and additional comments coming later today.

::: dom/quota/test/browserHelpers.js
@@ +5,5 @@
> +
> +var testGenerator = testSteps();
> +
> +var testResult;
> +var testException;

unused

@@ +18,5 @@
> +}
> +
> +function runTest()
> +{
> +  testGenerator.next();

This could clear storage first.

@@ +23,5 @@
> +}
> +
> +function continueToNextStep()
> +{
> +  SimpleTest.executeSoon(function() {

We can use a timeout here, SimpleTest is not included.

@@ +37,5 @@
> +  }
> +}
> +
> +function finishTest()
> +{

This could clear storage first.

::: dom/quota/test/browser_permissionsPrompt.html
@@ +5,5 @@
> +<html>
> +  <head>
> +    <meta charset=UTF-8>
> +    <title>Persistent-Storage Permission Prompt Test</title>
> +    <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>

I don't remember exact reason, but SimpleTest.js shouldn't be included in these html files.

@@ +7,5 @@
> +    <meta charset=UTF-8>
> +    <title>Persistent-Storage Permission Prompt Test</title>
> +    <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +
> +    <script type="text/javascript;version=1.7">

we don't need "version=1.7" anymore

@@ +17,5 @@
> +                  ["dom.storageManager.prompt.testing.allow", false]]
> +        }, continueToNextStep);
> +        yield undefined;
> +
> +        clearAllDatabases(continueToNextStep);

This can be done automatically by runTest().

@@ +25,5 @@
> +          testGenerator.next(result);
> +        });
> +        testResult = yield undefined;
> +
> +        info("Persist() result: " + testResult);

info() can't be used here

@@ +27,5 @@
> +        testResult = yield undefined;
> +
> +        info("Persist() result: " + testResult);
> +
> +        navigator.storage.persisted().then(result => {

The consistency is checked elsewhere, we could do it here again, but it would make the return value harder to check.

@@ +34,5 @@
> +        let persistedResult = yield undefined;
> +
> +        info("Persisted() result: " + persistedResult);
> +
> +        is(testResult, persistedResult, "Persist/persisted results are consistent");

is() can't be used here

::: dom/quota/test/browser_permissionsPromptAllow.js
@@ +5,5 @@
> +
> +const testPageURL =
> +  "https://example.com/browser/dom/quota/test/browser_permissionsPrompt.html";
> +
> +add_task(function* testPermissionAllow() {

We should remove the permission here first, just to be sure the prompt will show up.

::: dom/quota/test/head.js
@@ +63,5 @@
> +    EventUtils.synthesizeKey("VK_ESCAPE", {});
> +  });
> +}
> +
> +function setPermission(url, permission)

This is not used.

@@ +113,5 @@
> +{
> +  return ContentTask.spawn(browser.selectedBrowser, aMessage, function* (aMessage) {
> +    yield new Promise((resolve, reject) => {
> +      content.addEventListener("message", function messageListener(event) {
> +        content.removeEventListener("message", messageListener);

You can use {once: true} instead of calling removeEventListener manually.

::: dom/quota/test/helpers.js
@@ +16,5 @@
> +
> +var testHarnessGenerator = testHarnessSteps();
> +testHarnessGenerator.next();
> +
> +function testHarnessSteps()

If you change this to |function* testHarnessSteps()| then you don't need to declare "version=1.7" in all the places.
Attachment #8856450 - Flags: review?(jvarga)
Attached patch Patch for testing (obsolete) — Splinter Review
Comment on attachment 8856450 [details] [diff] [review]
Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v3)

Review of attachment 8856450 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/test/browserHelpers.js
@@ +5,5 @@
> +
> +var testGenerator = testSteps();
> +
> +var testResult;
> +var testException;

unused

@@ +18,5 @@
> +}
> +
> +function runTest()
> +{
> +  testGenerator.next();

This could clear storage first.

@@ +23,5 @@
> +}
> +
> +function continueToNextStep()
> +{
> +  SimpleTest.executeSoon(function() {

We can use a timeout here, SimpleTest is not included.

@@ +37,5 @@
> +  }
> +}
> +
> +function finishTest()
> +{

This could clear storage first.

::: dom/quota/test/browser_permissionsPrompt.html
@@ +5,5 @@
> +<html>
> +  <head>
> +    <meta charset=UTF-8>
> +    <title>Persistent-Storage Permission Prompt Test</title>
> +    <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>

I don't remember exact reason, but SimpleTest.js shouldn't be included in these html files.

@@ +7,5 @@
> +    <meta charset=UTF-8>
> +    <title>Persistent-Storage Permission Prompt Test</title>
> +    <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +
> +    <script type="text/javascript;version=1.7">

we don't need "version=1.7" anymore

@@ +17,5 @@
> +                  ["dom.storageManager.prompt.testing.allow", false]]
> +        }, continueToNextStep);
> +        yield undefined;
> +
> +        clearAllDatabases(continueToNextStep);

This can be done automatically by runTest().

@@ +25,5 @@
> +          testGenerator.next(result);
> +        });
> +        testResult = yield undefined;
> +
> +        info("Persist() result: " + testResult);

info() can't be used here

@@ +27,5 @@
> +        testResult = yield undefined;
> +
> +        info("Persist() result: " + testResult);
> +
> +        navigator.storage.persisted().then(result => {

The consistency is checked elsewhere, we could do it here again, but it would make the return value harder to check.

@@ +34,5 @@
> +        let persistedResult = yield undefined;
> +
> +        info("Persisted() result: " + persistedResult);
> +
> +        is(testResult, persistedResult, "Persist/persisted results are consistent");

is() can't be used here

::: dom/quota/test/browser_permissionsPromptAllow.js
@@ +5,5 @@
> +
> +const testPageURL =
> +  "https://example.com/browser/dom/quota/test/browser_permissionsPrompt.html";
> +
> +add_task(function* testPermissionAllow() {

We should remove the permission here first, just to be sure the prompt will show up.

::: dom/quota/test/browser_permissionsPromptDeny.js
@@ +45,5 @@
> +  win.gBrowser.selectedBrowser.loadURI(testPageURL);
> +  yield BrowserTestUtils.browserLoaded(win.gBrowser.selectedBrowser);
> +
> +  // PermissionManager is not supposed to have any knowledge of private browsing,
> +  // Prompt shall not be displayed.

Ok, so the prompt is not displayed here, but the reason is different.
It's simple, the permission was already set to DENY_ACTION in the previous test!
That's why it's not shown. When you apply my patch for testing, you will see that these
registerPopupEventHandler calls are wrong (they are wrong in dom/indexedDB/test too)

Here's the original test:
https://hg.mozilla.org/mozilla-central/diff/882aba67b5d4/dom/indexedDB/test/browser_permissionsPromptDeny.js
So the second test there checks that the prompt is not shown and there's no private browsing stuff.

Here's the changeset that (wrongly) added private browsing stuff:
https://hg.mozilla.org/mozilla-central/diff/0729990b395c/dom/indexedDB/test/browser_permissionsPromptDeny.js

When you want to test the prompt in private browsing mode, you must remove the permission first!

You also need to check browser_permissionsPromptAllow.js

::: dom/quota/test/head.js
@@ +63,5 @@
> +    EventUtils.synthesizeKey("VK_ESCAPE", {});
> +  });
> +}
> +
> +function setPermission(url, permission)

This is not used.

@@ +113,5 @@
> +{
> +  return ContentTask.spawn(browser.selectedBrowser, aMessage, function* (aMessage) {
> +    yield new Promise((resolve, reject) => {
> +      content.addEventListener("message", function messageListener(event) {
> +        content.removeEventListener("message", messageListener);

You can use {once: true} instead of calling removeEventListener manually.

::: dom/quota/test/helpers.js
@@ +16,5 @@
> +
> +var testHarnessGenerator = testHarnessSteps();
> +testHarnessGenerator.next();
> +
> +function testHarnessSteps()

If you change this to |function* testHarnessSteps()| then you don't need to declare "version=1.7" in all the places.
Sorry for those duplicate comments, the new one is: "Ok, so the prompt is not displayed here ..."
It seems to me that you need to add C++ code for persist() to not show the prompt in private browsing mode and just resolve the promise to false ?
Comment on attachment 8856955 [details] [diff] [review]
My changes for Part2

Review of attachment 8856955 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the correctness. I think removePermission should be persistent-storage.

::: dom/quota/test/browser_permissionsPromptAllow.js
@@ +6,5 @@
>  const testPageURL =
>    "https://example.com/browser/dom/quota/test/browser_permissionsPrompt.html";
>  
>  add_task(function* testPermissionAllow() {
> +  removePermission(testPageURL, "indexedDB");

I will change it to 'persistent-storage'.

::: dom/quota/test/browser_permissionsPromptDeny.js
@@ +6,5 @@
>  const testPageURL =
>    "https://example.com/browser/dom/quota/test/browser_permissionsPrompt.html";
>  
>  add_task(function* testPermissionDenied() {
> +  removePermission(testPageURL, "indexedDB");

I will change it to 'persistent-storage'.
(In reply to Jan Varga [:janv] from comment #165)
> Comment on attachment 8856450 [details] [diff] [review]
> Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v3)
> 
> Review of attachment 8856450 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/quota/test/browser_permissionsPromptDeny.js
> @@ +45,5 @@
> > +  win.gBrowser.selectedBrowser.loadURI(testPageURL);
> > +  yield BrowserTestUtils.browserLoaded(win.gBrowser.selectedBrowser);
> > +
> > +  // PermissionManager is not supposed to have any knowledge of private browsing,
> > +  // Prompt shall not be displayed.
> 
> Ok, so the prompt is not displayed here, but the reason is different.
> It's simple, the permission was already set to DENY_ACTION in the previous
> test!
> That's why it's not shown. When you apply my patch for testing, you will see
> that these
> registerPopupEventHandler calls are wrong (they are wrong in
> dom/indexedDB/test too)
> 
> Here's the original test:
> https://hg.mozilla.org/mozilla-central/diff/882aba67b5d4/dom/indexedDB/test/
> browser_permissionsPromptDeny.js
> So the second test there checks that the prompt is not shown and there's no
> private browsing stuff.
> 
> Here's the changeset that (wrongly) added private browsing stuff:
> https://hg.mozilla.org/mozilla-central/diff/0729990b395c/dom/indexedDB/test/
> browser_permissionsPromptDeny.js
> 
> When you want to test the prompt in private browsing mode, you must remove
> the permission first!
> 

When I added this, I have the same doubts. But i saw the discussion in Bug 1269361, I thought that's on purpose to keep permission to test that permission still the same in private browsing mode. I misunderstood the code.
(In reply to Jan Varga [:janv] from comment #167)
> It seems to me that you need to add C++ code for persist() to not show the
> prompt in private browsing mode and just resolve the promise to false ?

Okay. Can I add an another patch for this? I want to keep Part 1 as it is.
(In reply to Shawn Huang [:shawnjohnjr] from comment #170)
> (In reply to Jan Varga [:janv] from comment #167)
> > It seems to me that you need to add C++ code for persist() to not show the
> > prompt in private browsing mode and just resolve the promise to false ?
> 
> Okay. Can I add an another patch for this? I want to keep Part 1 as it is.

Sure
(In reply to Shawn Huang [:shawnjohnjr] from comment #168)
> Comment on attachment 8856955 [details] [diff] [review]
> My changes for Part2
> 
> Review of attachment 8856955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the correctness. I think removePermission should be
> persistent-storage.

ah right, sorry
(In reply to Jan Varga [:janv] from comment #161)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #157)
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #155)
> > > Created attachment 8854833 [details] [diff] [review]
> > > Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v3)
> Why don't you add grabArgAndContinueHandler version for workers ?
> See workerScript() function.
Okay, originally I thought grabArgAndContinueHandler was only called once in worker.
I will add it for consistency.
Comment on attachment 8856866 [details] [diff] [review]
Bug 1286717 - Part 3: Add wpt test cases for persist/persisted functions

Review of attachment 8856866 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/storage/storage-persisted.js
@@ +9,5 @@
> +}, 'persisted() method exists and returns a Promise');
> +
> +promise_test(function(t) {
> +  return navigator.storage.persisted().then(function(result) {
> +    assert_equals(typeof result, 'boolean');

Don't we need to check actual value here ?
Like "false" by default ?
Attachment #8856866 - Flags: review?(jvarga) → review+
(In reply to Jan Varga [:janv] from comment #174)
> Comment on attachment 8856866 [details] [diff] [review]
> Bug 1286717 - Part 3: Add wpt test cases for persist/persisted functions
> 
> Review of attachment 8856866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/web-platform/tests/storage/storage-persisted.js
> @@ +9,5 @@
> > +}, 'persisted() method exists and returns a Promise');
> > +
> > +promise_test(function(t) {
> > +  return navigator.storage.persisted().then(function(result) {
> > +    assert_equals(typeof result, 'boolean');
> 
> Don't we need to check actual value here ?
> Like "false" by default ?

That makes sense I will add it.
Comment on attachment 8857383 [details] [diff] [review]
Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v4)

Review of attachment 8857383 [details] [diff] [review]:
-----------------------------------------------------------------

So, did you actually try my patch for "testing" ?

::: dom/quota/test/browser_permissionsPromptDeny.js
@@ +35,5 @@
> +  gBrowser.removeCurrentTab();
> +  unregisterAllPopupEventHandlers();
> +});
> +
> +add_task(function* testPermissionDeniedInPrivateWindow() {

I thought you would add another test here that checks that the prompt is not shown if the user already made a decision, so the permission is either ALLOW or DENY. That is needed to make sure that we don't ask the same thing multiple times.

@@ +48,5 @@
> +  info("Loading test page: " + testPageURL);
> +  win.gBrowser.selectedBrowser.loadURI(testPageURL);
> +  yield BrowserTestUtils.browserLoaded(win.gBrowser.selectedBrowser);
> +
> +  registerPopupEventHandler("popupshowing", function () {

So this call is still wrong, right ?
You created a new window, you need to register event handlers in that window.
Someone also needs to rebase all the patches, frontend patches don't apply well anymore.
Comment on attachment 8857385 [details] [diff] [review]
Bug 1286717 - Part 4: Resolve promise with false for persist() in private browsing mode

As we discussed, I will try to fallback to persisted().
Attachment #8857385 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #179)
> Someone also needs to rebase all the patches, frontend patches don't apply
> well anymore.

Sorry. I will update it.
(In reply to Shawn Huang [:shawnjohnjr] from comment #181)
> (In reply to Jan Varga [:janv] from comment #179)
> > Someone also needs to rebase all the patches, frontend patches don't apply
> > well anymore.
> 
> Sorry. I will update it.

Hi Jan,
Patch already got landed.
https://hg.mozilla.org/mozilla-central/rev/4f90a5f5fb16
Bug 1309123 - Show persistent-storage permission request notification, r=florian
Comment on attachment 8856955 [details] [diff] [review]
My changes for Part2

Review of attachment 8856955 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/test/browser_permissionsPromptAllow.js
@@ +65,5 @@
>    is(getPermission(testPageURL, "persistent-storage"),
>       Components.interfaces.nsIPermissionManager.ALLOW_ACTION,
>       "Correct permission set");
> +  win.gBrowser.removeCurrentTab();
> +  win.close();

What's the reason to do win.close() first, then unregisterAllPopupEventHandlers? Since PopupNotifications is the attribute of window. When win.close() call first, window will be gone.
Hm, well, there's still a reference to the window. win.close() just hides the window and starts destruction of the window.
Anyway, feel free to put the unregister call before removeCurrentTab() here and other places too.
Comment on attachment 8857858 [details] [diff] [review]
Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v4)

Review of attachment 8857858 [details] [diff] [review]:
-----------------------------------------------------------------

Fix issues in Comment 161, Comment 165, Comment 178.
And further adjustment for PopupNotifications in different windows.
I went quickly over these patches and they look good, but I'll take a closer look later day. Thanks!
(In reply to Jan Varga [:janv] from comment #188)
> I went quickly over these patches and they look good, but I'll take a closer
> look later day. Thanks!

Thank you for catching serious issues for private window. I've learned new lesson this time.
I think you can push the patches to try, if you didn't already.
(In reply to Shawn Huang [:shawnjohnjr] from comment #189)
> Thank you for catching serious issues for private window. I've learned new
> lesson this time.

Sure any time! :)
Comment on attachment 8854499 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted to StorageManager, r=janv

This patch has already reviewed by Jan, but it also touches webidl. Would you mind reviewing webidl part? Thank you.
Attachment #8854499 - Flags: review?(amarchesini)
Comment on attachment 8857861 [details] [diff] [review]
Bug 1286717 - Part 4: Resolve a promise for persist() in private browsing mode

Review of attachment 8857861 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/StorageManager.cpp
@@ +304,5 @@
> +          RefPtr<RequestResolver> resolver =
> +            new RequestResolver(RequestResolver::Type::Persisted, promise);
> +
> +          RefPtr<nsIQuotaRequest> request;
> +          aRv = Persisted(principal, resolver, getter_AddRefs(request));

This is basically the same code as in PersistentStoragePermissionRequest::Cancel().

Maybe we could just do this:
if (nsContentUtils::IsInPrivateBrowsing(doc)) {
  aRv = request->Cancel();
} else {
  aRv = request->Start();
}
Attachment #8857861 - Flags: review?(jvarga) → review+
(In reply to Jan Varga [:janv] from comment #196)
> Comment on attachment 8857861 [details] [diff] [review]
> Bug 1286717 - Part 4: Resolve a promise for persist() in private browsing
> mode
> 
> Review of attachment 8857861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/StorageManager.cpp
> @@ +304,5 @@
> > +          RefPtr<RequestResolver> resolver =
> > +            new RequestResolver(RequestResolver::Type::Persisted, promise);
> > +
> > +          RefPtr<nsIQuotaRequest> request;
> > +          aRv = Persisted(principal, resolver, getter_AddRefs(request));
> 
> This is basically the same code as in
> PersistentStoragePermissionRequest::Cancel().
> 
> Maybe we could just do this:
> if (nsContentUtils::IsInPrivateBrowsing(doc)) {
>   aRv = request->Cancel();
> } else {
>   aRv = request->Start();
> }

Originally we thought it's better to resolve as early as I can. I will change it. Thank you.
(In reply to Shawn Huang [:shawnjohnjr] from comment #197)
> Originally we thought it's better to resolve as early as I can. I will
> change it. Thank you.

It's still synchronous, right ?
There's just some extra object creation. We can live with that, given it's just for private browsing.
Attachment #8858268 - Flags: review+
Attachment #8857893 - Flags: review+
Comment on attachment 8857858 [details] [diff] [review]
Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v4)

Review of attachment 8857858 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/test/browser_permissionsPromptAllow.js
@@ +64,5 @@
> +  removePermission(testPageURL, "persistent-storage");
> +});
> +
> +add_task(function* testPermissionAllowInPrivateWindow() {
> +  removePermission(testPageURL, "persistent-storage");

Nit: The previous test removed the permission.

::: dom/quota/test/browser_permissionsPromptDeny.js
@@ +64,5 @@
> +  gBrowser.removeCurrentTab();
> +  removePermission(testPageURL, "persistent-storage");
> +});
> +
> +add_task(function* testPermissionDeniedInPrivateWindow() {

Nit: testPermissionUnknownInPrivateWindow

Hm, I just found out that this actually doesn't fit into this test.
testPermissionDeniedInPrivateWindow and testPermissionAllowInPrivateWindow contains exactly the same code, so I think it should be a separate browser test
Sorry about that, it's not your fault that the dom/indexedDB browser test that was taken as a template for this, is actually broken.

@@ +65,5 @@
> +  removePermission(testPageURL, "persistent-storage");
> +});
> +
> +add_task(function* testPermissionDeniedInPrivateWindow() {
> +  removePermission(testPageURL, "persistent-storage");

Nit: The previous test already removed the permission

::: dom/quota/test/head.js
@@ +1,5 @@
> +/**
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +

All this event handler registration won't work correctly if someone decides to use it with multiple windows.
I don't think we need to fix it right now, but maybe we should add a comment here that it only works with one window.
Attachment #8857858 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #200)
> Comment on attachment 8857858 [details] [diff] [review]
> Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v4)
> 
> Review of attachment 8857858 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/test/browser_permissionsPromptAllow.js
> @@ +64,5 @@
> > +  removePermission(testPageURL, "persistent-storage");
> > +});
> > +
> > +add_task(function* testPermissionAllowInPrivateWindow() {
> > +  removePermission(testPageURL, "persistent-storage");
> 
> Nit: The previous test removed the permission.
I will remove it. I added it because I remember months ago Kris told me in bug 1268804, each test task shall be considered separately as much as we can.

> 
> ::: dom/quota/test/browser_permissionsPromptDeny.js
> @@ +64,5 @@
> > +  gBrowser.removeCurrentTab();
> > +  removePermission(testPageURL, "persistent-storage");
> > +});
> > +
> > +add_task(function* testPermissionDeniedInPrivateWindow() {
> 
> Nit: testPermissionUnknownInPrivateWindow
> 
> Hm, I just found out that this actually doesn't fit into this test.
> testPermissionDeniedInPrivateWindow and testPermissionAllowInPrivateWindow
> contains exactly the same code, so I think it should be a separate browser
> test
> Sorry about that, it's not your fault that the dom/indexedDB browser test
> that was taken as a template for this, is actually broken.

I will remove testPermissionAllowInPrivateWindow, then only keep testPermissionDeniedInPrivateWindow.

> 
> ::: dom/quota/test/head.js
> @@ +1,5 @@
> > +/**
> > + * Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/
> > + */
> > +
> 
> All this event handler registration won't work correctly if someone decides
> to use it with multiple windows.
> I don't think we need to fix it right now, but maybe we should add a comment
> here that it only works with one window.

Sure. I will add comments for it to avoid misleading.
(In reply to Shawn Huang [:shawnjohnjr] from comment #201)
> I will remove it. I added it because I remember months ago Kris told me in
> bug 1268804, each test task shall be considered separately as much as we can.

That's quite hard to achieve in this case, given the comment in the test:
// Keep persistent-storage permission for the next test.

However, if you want to follow Kris's suggestion, shouldn't you add removePermission() to testPermissionDeniedDismiss() too ?

Either way, the test should be consistent in this regard.

> I will remove testPermissionAllowInPrivateWindow, then only keep
> testPermissionDeniedInPrivateWindow.

It's not Denied, it's Unknown and I just think that this was wrongly added to dom/indexedDB/test/browser_permissionsPromptDeny.js
It should live in its own browser test, for example browser_permissionsPromptUnknown.js
(In reply to Jan Varga [:janv] from comment #203)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #201)
> > I will remove it. I added it because I remember months ago Kris told me in
> > bug 1268804, each test task shall be considered separately as much as we can.
> 
> That's quite hard to achieve in this case, given the comment in the test:
> // Keep persistent-storage permission for the next test.
> 
> However, if you want to follow Kris's suggestion, shouldn't you add
> removePermission() to testPermissionDeniedDismiss() too ?
> 
> Either way, the test should be consistent in this regard.
Okay. I see.
> 
> > I will remove testPermissionAllowInPrivateWindow, then only keep
> > testPermissionDeniedInPrivateWindow.
> 
> It's not Denied, it's Unknown and I just think that this was wrongly added
> to dom/indexedDB/test/browser_permissionsPromptDeny.js
> It should live in its own browser test, for example
> browser_permissionsPromptUnknown.js
Oh. Sorry. I misunderstood your comments. I will add it into another file.
Attachment #8858274 - Attachment is obsolete: true
Attachment #8858274 - Flags: review?(jvarga)
Move private browsing test into browser_permissionsPromptUnknown.js.

I keep removePermission in the beginning of testPermissionUnknownInPrivateWindow, for consistency. I also removed re-test in the normal mode in browser_permissionsPromptAllow.js, since private browsing test was removed.
Attachment #8858279 - Flags: review?(jvarga)
Comment on attachment 8858279 [details] [diff] [review]
Bug 1286717 - Part 2: Add mochitests for persist/persisted functions (v5)

Review of attachment 8858279 [details] [diff] [review]:
-----------------------------------------------------------------

Ok

::: dom/quota/test/browser_permissionsPromptUnknown.js
@@ +33,5 @@
> +  is(getPermission(testPageURL, "persistent-storage"),
> +     Components.interfaces.nsIPermissionManager.UNKNOWN_ACTION,
> +     "Correct permission set");
> +  unregisterAllPopupEventHandlers(win);
> +  removePermission(testPageURL, "persistent-storage");

Nit: move removePermission() after win.close()

::: dom/quota/test/head.js
@@ +5,5 @@
> +
> +var gActiveListeners = {};
> +
> +// These event (un)registration handlers only work for one window, DONOT use
> +// them with multiple windows.

Nit: Add a blank line here since the comment is not just for this funtion.
Attachment #8858279 - Flags: review?(jvarga) → review+
Comment on attachment 8854499 [details] [diff] [review]
Bug 1286717 - Part 1: Expose persist/persisted to StorageManager, r=janv

Review of attachment 8854499 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/StorageManager.cpp
@@ +85,4 @@
>    NS_DECL_NSIQUOTAUSAGECALLBACK
>  
>  private:
> +  ~RequestResolver()

= default
Attachment #8854499 - Flags: review?(amarchesini) → review+
Comment on attachment 8858289 [details] [diff] [review]
Bug 1286717 - Part 2: Add mochitests for persist/persisted functions, r=janv

Review of attachment 8858289 [details] [diff] [review]:
-----------------------------------------------------------------

Mission complete :)
Attachment #8858289 - Flags: review+
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cbdf0c95214
Part 1: Expose persist/persisted to StorageManager, r=janv, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3972f05c4d41
Part 2: Add mochitests for persist/persisted functions, r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/3845c01a7d8c
Part 3: Add wpt test cases for persist/persisted functions, r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe9127b627b8
Part 4: Resolve a promise for persist() in private browsing mode, r=janv
Hmm, I just saw wpt tests added in upstream 9 days ago.
https://github.com/w3c/web-platform-tests/commit/3d5db8631c592ba41a7e5f7aac1bc2c1600b497c

So does that mean we should only take upstream test cases?
Depends on: 1357975
Could this have caused bug 1357872? (just asking)
It is guaranteed that PersistentStoragePermissionRequest is deleted very soon after creation?
It keeps strong references to objects which may keep worlds alive.
Hm, maybe we need to make PersistentStoragePermissionRequest a cycle collected object.

I just checked a similar object NotificationPermissionRequest in Notification.cpp and it traverses/unlinks mWindow
We should probably do the same for PersistentStoragePermissionRequest.
Sorry that I didn't catch this during review, but I'm surprised that the automatic leak analysis is ok with it.
(In reply to Jan Varga [:janv] from comment #217)
> Hm, maybe we need to make PersistentStoragePermissionRequest a cycle
> collected object.
> 
> I just checked a similar object NotificationPermissionRequest in
> Notification.cpp and it traverses/unlinks mWindow
> We should probably do the same for PersistentStoragePermissionRequest.
> Sorry that I didn't catch this during review, but I'm surprised that the
> automatic leak analysis is ok with it.

I've filed Bug 1358767, sorry about messing up :(
Hmm. Are the sites actually call persist/persisted() methods in bug 1357872?
I thought the storage api is not so popular yet.
I don't know whether this bug is causing bug 1357872. It is just that this is in the regression range.
I'm still trying to find the actual bug which caused bug 1357872.
I've checked, and these methods looked to have been documented already:

https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/persist
https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/persisted

I've added a note to the Fx55 release notes about it:

https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM

Let me know if that's OK. Also, can you check and let me know where the note in the browser support is still accurate:

https://developer.mozilla.org/en-US/docs/Web/API/StorageManager

This is still only enabled by default in Nightly, right? It's just that now it's exposed to Window ?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: