1498013 - Add a boolean attribute to nsIPaymentRequest to indicate whether it's from a private window

Status: NEW

(Core :: DOM: Web Payments, enhancement, P3)

Description

[Matthew N. [:MattN]]

The PaymentRequest UI has different behaviour in private browsing but we don't currently have a foolproof way to determine if the request is coming from private browsing. Adding a boolean on nsIPaymentRequest would fix this.

The current front-end code is:
https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/browser/components/payments/content/paymentDialogWrapper.js#456

That would then be changed so `isPrivate` is set to `this.request.inPrivateBrowsing` if the attribute is named `inPrivateBrowsing`. Please make the change to the front-end code when implementing this.

Examples of other comparable attribute names: https://dxr.mozilla.org/mozilla-central/search?q=attribute+private+ext%3Aidl&redirect=false
I don't think we should use one that includes the word "window" since it may not always be a separate window in the future.

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: Part 1: Adding an attribute nsIPaymentRequest::InPrivateBrowsing to indicate the PaymentRequest is from PrivateBrowsing mode.

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Part 2 - Using nsIPaymentRequest::InPrivateBrowsing in Payment UI.

Comment 3

[Eden Chuang[:edenchuang]]

Try result with patches.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b557c5177b0e9d0cf9e847eaad524a7dd34bba&selectedJob=212025821

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 9025657 [details] [diff] [review]
Part 2 - Using nsIPaymentRequest::InPrivateBrowsing in Payment UI.

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

This is the minimal necessary change but ideally all references to `state.isPrivate` would have been updated to use `state.request.inPrivateBrowsing`. If you don't want to do that then please file a front-end follow-up to do the cleanup.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 9025656 [details] [diff] [review]
Part 1: Adding an attribute nsIPaymentRequest::InPrivateBrowsing to indicate the PaymentRequest is from PrivateBrowsing mode.

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

::: dom/payments/PaymentRequest.cpp
@@ +673,5 @@
>    MOZ_ASSERT(aWindow);
>    RegisterActivityObserver();
> +  nsIDocument* doc = aWindow->GetExtantDoc();
> +  if (doc) {
> +    mInPrivateBrowsing = nsContentUtils::IsInPrivateBrowsing(doc);

Defaulting to false with this `doc` check is a bit concerning to me. When is doc ever null here?

::: dom/payments/PaymentRequest.h
@@ +215,4 @@
>    // front end about it.
>    bool mDeferredShow;
>  
> +  // Ture if the payment is created in the private browsing context.

Typo: "ture"

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 9025656 [details] [diff] [review]
Part 1: Adding an attribute nsIPaymentRequest::InPrivateBrowsing to indicate the PaymentRequest is from PrivateBrowsing mode.

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

::: dom/interfaces/payments/nsIPaymentRequest.idl
@@ +83,4 @@
>  {
>    readonly attribute uint64_t topOuterWindowId;
>    readonly attribute nsIPrincipal topLevelPrincipal;
> +  readonly attribute bool inPrivateBrowsing;

you can use the toplevelPrincipal.originAttributes.
why do you need this extra boolean?

Comment 7

[Matthew N. [:MattN]]

(In reply to Andrea Marchesini [:baku] from comment #6)
> Comment on attachment 9025656 [details] [diff] [review]
> Part 1: Adding an attribute nsIPaymentRequest::InPrivateBrowsing to indicate
> the PaymentRequest is from PrivateBrowsing mode.
> 
> ::: dom/interfaces/payments/nsIPaymentRequest.idl
> @@ +83,4 @@
> >  {
> >    readonly attribute uint64_t topOuterWindowId;
> >    readonly attribute nsIPrincipal topLevelPrincipal;
> > +  readonly attribute bool inPrivateBrowsing;
> 
> you can use the toplevelPrincipal.originAttributes.
> why do you need this extra boolean?

a) It seemed a bit gross to depend on `privateBrowsingId > 0` for a private browsing check.
b) topLevelPrincipal isn't the principal of the document doing the request so in theory `privateBrowsingId` could be different for them. In practice they're currently the same since we only support per-window PB now but I could see this changing with tracking protection and such.