Mark IFrame support in PaymentRequest experimental

Mark IFrame support in PaymentRequest experimental

IFrame support in PaymentRequest may be better served through
FeaturePolicy. This patch marks the current IFrame support
experimental, so it's disabled by default, but developers still can
experiment with it. Once the FeaturePolicy question is resolved, the
current implementation of the IFrame support will be either shipped or
replaced by FeaturePolicy.

BUG=652148
Committed: https://crrev.com/d2c4379ec3783f7f70b14f6fda92f13cd496bf66
Cr-Commit-Position: refs/heads/master@{#434257}

[please use gerrit instead, 2016-11-22 23:34:33]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 23:35:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2016-11-22 23:37:09]

rouslan@chromium.org changed reviewers:
+ rbyers@chromium.org

[please use gerrit instead, 2016-11-22 23:37:10]

Rick, ptal.

Alex, fyi. The change is almost a one-liner in production code. The deleted
files are tests for the feature being removed.

[Rick Byers, 2016-11-23 01:10:31]

On 2016/11/22 23:37:10, rouslan wrote:
> Rick, ptal.

This disables the feature but still leaves the API exposed (eg. a typical
feature-detect like "'allowpaymentrequest' in HTMLIFrameElement.prototype" will
still return true).  

Perhaps the simplest solution is to add a new status=experimental
RuntimeEnabledFeature and change the RuntimeEnabled in
HTMLIFrameElementPayments.idl?  Then you can leave the C++ code and tests 100%
intact (and developers can still experiment with it).

[commit-bot: I haz the power, 2016-11-23 01:23:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 01:23:16]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2016-11-23 15:19:54]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 15:20:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2016-11-23 15:21:55]

Rick, ptal patch 3. 

Good point about feature detection of 'allowpaymentrequest'. I've marked it
experimental now. However, my tests show that marking that feature experimental
does not disable the iframe logic in PaymentRequest.cpp. Therefore, I would
still like to remove that logic. Now 'allowpaymentrequest' tag can be turned one
via chrome://flags/#enable-experimental-web-platform-features, but it has no
effect. If we decided to ship this feature, enabling it is as easy as reverting
this patch.

[please use gerrit instead, 2016-11-23 15:40:30]

rouslan@chromium.org changed reviewers:
+ haraken@chromium.org

[please use gerrit instead, 2016-11-23 15:40:31]

haraken, owners ptal RuntimeEnabledFeatures.in

[haraken, 2016-11-23 15:44:28]

On 2016/11/23 15:40:31, rouslan wrote:
> haraken, owners ptal RuntimeEnabledFeatures.in

(API-related changes should go to an API owner -- Rick :-)

[please use gerrit instead, 2016-11-23 16:26:42]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 16:27:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2016-11-23 16:27:38]

Rick, ptal patch 4.

[Rick Byers, 2016-11-23 16:54:55]

LGTM

I suggest updating the CL title/description to now say that it's just demoted to
experimental, not disabled.  Also IMHO it's not really "API owner approval" that
matters here but the outstanding debate about the API shape:
https://github.com/w3c/browser-payment-api/issues/311

https://codereview.chromium.org/2525813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2525813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:523: if
(!RuntimeEnabledFeatures::paymentRequestIFrameEnabled())
nit: this check is now redundant with the check in allowPaymentRequest(), right?
 Maybe remove it or replace by a CHECK somewhere?

[Rick Byers, 2016-11-23 16:56:43]

https://codereview.chromium.org/2525813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/HTMLIFrameElementPayments.cpp
(right):

https://codereview.chromium.org/2525813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/HTMLIFrameElementPayments.cpp:52:
return RuntimeEnabledFeatures::paymentRequestIFrameEnabled() &&
It seems like it would be nice if the bindings generator generated a method like
this automatically (so we don't have to remember to add an additional
RuntimeEnabledFeatures check), but putting the check here explicitly is nice and
simple and definitely the right place to have it.  Thanks!

[please use gerrit instead, 2016-11-23 17:27:37]

Sending to cq.

https://codereview.chromium.org/2525813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/HTMLIFrameElementPayments.cpp
(right):

https://codereview.chromium.org/2525813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/HTMLIFrameElementPayments.cpp:52:
return RuntimeEnabledFeatures::paymentRequestIFrameEnabled() &&
On 2016/11/23 16:56:43, Rick Byers wrote:
> It seems like it would be nice if the bindings generator generated a method
like
> this automatically (so we don't have to remember to add an additional
> RuntimeEnabledFeatures check), but putting the check here explicitly is nice
and
> simple and definitely the right place to have it.  Thanks!

Acknowledged.

https://codereview.chromium.org/2525813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2525813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:523: if
(!RuntimeEnabledFeatures::paymentRequestIFrameEnabled())
On 2016/11/23 16:54:55, Rick Byers wrote:
> nit: this check is now redundant with the check in allowPaymentRequest(),
right?
>  Maybe remove it or replace by a CHECK somewhere?

Removed.

[please use gerrit instead, 2016-11-23 17:27:40]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-11-23 17:27:40]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2525813003/#ps80001
(title: "Remove duplicate check")

[commit-bot: I haz the power, 2016-11-23 17:28:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2016-11-23 17:50:11]

The CQ bit was unchecked by rouslan@chromium.org

[please use gerrit instead, 2016-11-23 17:52:56]

Description was changed from

==========
Disable IFrame support in PaymentRequest

IFrame support in PaymentRequest never received approval to ship from
API owners. Need to get the approval before shipping.

BUG=652148
==========

to

==========
Make IFrame support in PaymentRequest experimental

IFrame support in PaymentRequest may be better served through
FeaturePolicy. This patch marks the current IFrame support experimental,
so it's disabled by default, but developers still can experiment with
it. Once the FeaturePolicy question is resolved, the current
implementation of the IFrame support will be either shipped or replaced
by FeaturePolicy.

BUG=652148
==========

[please use gerrit instead, 2016-11-23 17:53:31]

Description was changed from

==========
Make IFrame support in PaymentRequest experimental

IFrame support in PaymentRequest may be better served through
FeaturePolicy. This patch marks the current IFrame support experimental,
so it's disabled by default, but developers still can experiment with
it. Once the FeaturePolicy question is resolved, the current
implementation of the IFrame support will be either shipped or replaced
by FeaturePolicy.

BUG=652148
==========

to

==========
Mark IFrame support in PaymentRequest experimental

IFrame support in PaymentRequest may be better served through
FeaturePolicy. This patch marks the current IFrame support
experimental, so it's disabled by default, but developers still can
experiment with it. Once the FeaturePolicy question is resolved, the
current implementation of the IFrame support will be either shipped or
replaced by FeaturePolicy.

BUG=652148
==========

[please use gerrit instead, 2016-11-23 17:53:36]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2016-11-23 17:54:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 21:46:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479923616816140,
"parent_rev": "5a8583b949fd401e97655f7535531d7379919c4d", "commit_rev":
"560018cc15d5577f7aed8a0952488c3bc3ae03c4"}

[commit-bot: I haz the power, 2016-11-23 21:46:35]

Description was changed from

==========
Mark IFrame support in PaymentRequest experimental

IFrame support in PaymentRequest may be better served through
FeaturePolicy. This patch marks the current IFrame support
experimental, so it's disabled by default, but developers still can
experiment with it. Once the FeaturePolicy question is resolved, the
current implementation of the IFrame support will be either shipped or
replaced by FeaturePolicy.

BUG=652148
==========

to

==========
Mark IFrame support in PaymentRequest experimental

IFrame support in PaymentRequest may be better served through
FeaturePolicy. This patch marks the current IFrame support
experimental, so it's disabled by default, but developers still can
experiment with it. Once the FeaturePolicy question is resolved, the
current implementation of the IFrame support will be either shipped or
replaced by FeaturePolicy.

BUG=652148
==========

[commit-bot: I haz the power, 2016-11-23 21:46:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-23 21:48:28]

Description was changed from

==========
Mark IFrame support in PaymentRequest experimental

IFrame support in PaymentRequest may be better served through
FeaturePolicy. This patch marks the current IFrame support
experimental, so it's disabled by default, but developers still can
experiment with it. Once the FeaturePolicy question is resolved, the
current implementation of the IFrame support will be either shipped or
replaced by FeaturePolicy.

BUG=652148
==========

to

==========
Mark IFrame support in PaymentRequest experimental

IFrame support in PaymentRequest may be better served through
FeaturePolicy. This patch marks the current IFrame support
experimental, so it's disabled by default, but developers still can
experiment with it. Once the FeaturePolicy question is resolved, the
current implementation of the IFrame support will be either shipped or
replaced by FeaturePolicy.

BUG=652148

Committed: https://crrev.com/d2c4379ec3783f7f70b14f6fda92f13cd496bf66
Cr-Commit-Position: refs/heads/master@{#434257}
==========

[commit-bot: I haz the power, 2016-11-23 21:48:29]

Patchset 5 (id:??) landed as
https://crrev.com/d2c4379ec3783f7f70b14f6fda92f13cd496bf66
Cr-Commit-Position: refs/heads/master@{#434257}