[Payments] Implement web payment app manifest

Implement web payment app manifest

The web page registering web payment app needs to add 
<link rel="manifest" href="manifest.json"> 
in the page to provide the web app manifest.

BUG=735063
Review-Url: https://codereview.chromium.org/2958333002
Cr-Commit-Position: refs/heads/master@{#484193}
Committed: https://chromium.googlesource.com/chromium/src/+/70b9dcb8dae67c7c101a1c4c8f17da2adbcf98ab

[gogerald1, 2017-06-29 13:29:41]

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

[commit-bot: I haz the power, 2017-06-29 13:29:55]

Dry run: 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, 2017-06-29 13:41:34]

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

[commit-bot: I haz the power, 2017-06-29 13:41:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gogerald1, 2017-06-29 15:21:31]

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

[commit-bot: I haz the power, 2017-06-29 15:21:45]

Dry run: 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, 2017-06-29 15:32:50]

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

[commit-bot: I haz the power, 2017-06-29 15:32:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gogerald1, 2017-06-29 17:14:28]

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

[commit-bot: I haz the power, 2017-06-29 17:14:38]

Dry run: CQ is trying da patch.

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

[gogerald1, 2017-06-29 17:14:45]

Patchset #1 (id:1) has been deleted

[gogerald1, 2017-06-29 17:14:53]

Patchset #1 (id:20001) has been deleted

[gogerald1, 2017-06-29 19:07:51]

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

[commit-bot: I haz the power, 2017-06-29 19:08:01]

Dry run: CQ is trying da patch.

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

[gogerald1, 2017-06-29 19:20:44]

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

[gogerald1, 2017-06-29 19:20:57]

Patchset #1 (id:40001) has been deleted

[gogerald1, 2017-06-29 19:21:03]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-06-29 19:21:22]

Dry run: CQ is trying da patch.

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

[gogerald1, 2017-06-29 19:25:23]

Description was changed from

==========
Implement web payment handler manifest

BUG=
==========

to

==========
Implement web payment handler manifest

The web page registering web payment handler needs to add <link rel="manifest"
href="manifest.json"> in its page.

BUG=735063
==========

[gogerald1, 2017-06-29 19:26:00]

Description was changed from

==========
Implement web payment handler manifest

The web page registering web payment handler needs to add <link rel="manifest"
href="manifest.json"> in its page.

BUG=735063
==========

to

==========
Implement web payment app manifest

The web page registering web payment handler needs to add <link rel="manifest"
href="manifest.json"> in its page.

BUG=735063
==========

[gogerald1, 2017-06-29 19:27:01]

Description was changed from

==========
Implement web payment app manifest

The web page registering web payment handler needs to add <link rel="manifest"
href="manifest.json"> in its page.

BUG=735063
==========

to

==========
Implement web payment app manifest

The web page registering web payment handler needs to add <link rel="manifest"
href="manifest.json"> in its page to provide the web app manifest.

BUG=735063
==========

[gogerald1, 2017-06-29 19:27:12]

Description was changed from

==========
Implement web payment app manifest

The web page registering web payment handler needs to add <link rel="manifest"
href="manifest.json"> in its page to provide the web app manifest.

BUG=735063
==========

to

==========
Implement web payment app manifest

The web page registering web payment handler needs to add <link rel="manifest"
href="manifest.json"> 
in its page to provide the web app manifest.

BUG=735063
==========

[gogerald1, 2017-06-29 19:27:47]

Description was changed from

==========
Implement web payment app manifest

The web page registering web payment handler needs to add <link rel="manifest"
href="manifest.json"> 
in its page to provide the web app manifest.

BUG=735063
==========

to

==========
Implement web payment app manifest

The web page registering web payment app needs to add <link rel="manifest"
href="manifest.json"> 
in its page to provide the web app manifest.

BUG=735063
==========

[gogerald1, 2017-06-29 19:28:36]

Description was changed from

==========
Implement web payment app manifest

The web page registering web payment app needs to add <link rel="manifest"
href="manifest.json"> 
in its page to provide the web app manifest.

BUG=735063
==========

to

==========
Implement web payment app manifest

The web page registering web payment app needs to add <link rel="manifest"
href="manifest.json"> 
in the page to provide the web app manifest.

BUG=735063
==========

[gogerald1, 2017-06-29 19:30:36]

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

[gogerald1, 2017-06-29 19:30:37]

Hi rouslan@, ptal,

[please use gerrit instead, 2017-06-29 21:15:39]

Please make sure that a failure to retrieve payment app's label and icon does
not disqualify it from being displayed. If that happens, please print a
developer console warning message and use the app's origin as the label. 

If you do find the label, the origin should be the sublabel.

https://codereview.chromium.org/2958333002/diff/80001/content/browser/payment...
File content/browser/payments/payment_app_info_fetcher.cc (right):

https://codereview.chromium.org/2958333002/diff/80001/content/browser/payment...
content/browser/payments/payment_app_info_fetcher.cc:20: #define
PAYMENT_APP_MINIMUM_ICON_SIZE 0
Use constants instead of #define.

https://codereview.chromium.org/2958333002/diff/80001/content/test/data/payme...
File content/test/data/payments/payment_app_invocation.html (right):

https://codereview.chromium.org/2958333002/diff/80001/content/test/data/payme...
content/test/data/payments/payment_app_invocation.html:5: </head>>
Remove the duplicate ">".

[commit-bot: I haz the power, 2017-06-29 21:20:14]

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

[commit-bot: I haz the power, 2017-06-29 21:20:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[falken, 2017-06-30 01:14:35]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2017-06-30 01:14:36]

https://codereview.chromium.org/2958333002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2958333002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.h:250:
ServiceWorkerContextCore* context();
This represents a big shift in this class's role. This class is supposed to be a
barrier against using ContextCore directly since bad things happened when code
outside content/browser/serviceworker/ had direct access:
https://codereview.chromium.org/1079923002

I'm not totally sure whether the reasons given in that CL are still valid, but I
think for this patch we should try to keep the same model and add a helper
function to ContextWrapper for what you need instead of exposing ContextCore.

[gogerald1, 2017-06-30 02:33:14]

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

[commit-bot: I haz the power, 2017-06-30 02:33:23]

Dry run: CQ is trying da patch.

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

[gogerald1, 2017-06-30 02:55:23]

Thanks, another look?

https://codereview.chromium.org/2958333002/diff/80001/content/browser/payment...
File content/browser/payments/payment_app_info_fetcher.cc (right):

https://codereview.chromium.org/2958333002/diff/80001/content/browser/payment...
content/browser/payments/payment_app_info_fetcher.cc:20: #define
PAYMENT_APP_MINIMUM_ICON_SIZE 0
On 2017/06/29 21:15:39, rouslan wrote:
> Use constants instead of #define.

Done.

https://codereview.chromium.org/2958333002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2958333002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.h:250:
ServiceWorkerContextCore* context();
On 2017/06/30 01:14:36, falken wrote:
> This represents a big shift in this class's role. This class is supposed to be
a
> barrier against using ContextCore directly since bad things happened when code
> outside content/browser/serviceworker/ had direct access:
> https://codereview.chromium.org/1079923002
> 
> I'm not totally sure whether the reasons given in that CL are still valid, but
I
> think for this patch we should try to keep the same model and add a helper
> function to ContextWrapper for what you need instead of exposing ContextCore.

Done.

https://codereview.chromium.org/2958333002/diff/80001/content/test/data/payme...
File content/test/data/payments/payment_app_invocation.html (right):

https://codereview.chromium.org/2958333002/diff/80001/content/test/data/payme...
content/test/data/payments/payment_app_invocation.html:5: </head>>
On 2017/06/29 21:15:39, rouslan wrote:
> Remove the duplicate ">".

Done.

[commit-bot: I haz the power, 2017-06-30 03:13:29]

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

[commit-bot: I haz the power, 2017-06-30 03:13:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gogerald1, 2017-06-30 03:43:44]

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

[commit-bot: I haz the power, 2017-06-30 03:44:02]

Dry run: 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, 2017-06-30 04:27:52]

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

[commit-bot: I haz the power, 2017-06-30 04:27:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gogerald1, 2017-06-30 13:40:06]

Patchset #3 (id:120001) has been deleted

[gogerald1, 2017-06-30 13:40:42]

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

[commit-bot: I haz the power, 2017-06-30 13:40:46]

Dry run: 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, 2017-06-30 15:39:18]

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

[commit-bot: I haz the power, 2017-06-30 15:39:19]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-06-30 15:43:31]

LGTM % comments.

Please reformat the CL description to be at most 72 characters wide.

https://codereview.chromium.org/2958333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right):

https://codereview.chromium.org/2958333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:61: "field in
the manifest) failed."));
A couple of comments:

1) Payment app is not the terminology used by W3C. They call it "payment
handler". Let's use that language as well.

2) Let the web developer know what is the impact of this error.

Here's the text that I propose:

"Unable to fetch payment handler's name and icon from its web app manifest. User
may not recognize this payment handler in UI, because it will be labeled only by
its origin."

[gogerald1, 2017-06-30 16:08:02]

Description was changed from

==========
Implement web payment app manifest

The web page registering web payment app needs to add <link rel="manifest"
href="manifest.json"> 
in the page to provide the web app manifest.

BUG=735063
==========

to

==========
Implement web payment app manifest

The web page registering web payment app needs to add 
<link rel="manifest" href="manifest.json"> 
in the page to provide the web app manifest.

BUG=735063
==========

[gogerald1, 2017-06-30 16:18:26]

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

[commit-bot: I haz the power, 2017-06-30 16:18:35]

Dry run: CQ is trying da patch.

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

[gogerald1, 2017-06-30 16:25:08]

gogerald@chromium.org changed reviewers:
+ nasko@chromium.org

[gogerald1, 2017-06-30 16:25:09]

Thanks,

Hi nasko@, please review changes in
content/public/* and content/browser/BUILD.gn

https://codereview.chromium.org/2958333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right):

https://codereview.chromium.org/2958333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:61: "field in
the manifest) failed."));
On 2017/06/30 15:43:30, rouslan wrote:
> A couple of comments:
> 
> 1) Payment app is not the terminology used by W3C. They call it "payment
> handler". Let's use that language as well.
> 
> 2) Let the web developer know what is the impact of this error.
> 
> Here's the text that I propose:
> 
> "Unable to fetch payment handler's name and icon from its web app manifest.
User
> may not recognize this payment handler in UI, because it will be labeled only
by
> its origin."

Done.

[nasko, 2017-06-30 17:32:47]

https://codereview.chromium.org/2958333002/diff/160001/content/public/browser...
File content/public/browser/stored_payment_app.h (right):

https://codereview.chromium.org/2958333002/diff/160001/content/public/browser...
content/public/browser/stored_payment_app.h:28: GURL origin;
Why not url::Origin? Using GURLs to represent origins is not ideal, which is why
url::Origin exists and should be used.

[gogerald1, 2017-06-30 18:16:35]

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

[commit-bot: I haz the power, 2017-06-30 18:16:54]

Dry run: CQ is trying da patch.

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

[gogerald1, 2017-06-30 18:18:51]

Hi nasko@, another look?

https://codereview.chromium.org/2958333002/diff/160001/content/public/browser...
File content/public/browser/stored_payment_app.h (right):

https://codereview.chromium.org/2958333002/diff/160001/content/public/browser...
content/public/browser/stored_payment_app.h:28: GURL origin;
On 2017/06/30 17:32:47, nasko (out until Jul 17th) wrote:
> Why not url::Origin? Using GURLs to represent origins is not ideal, which is
why
> url::Origin exists and should be used.

Done. used url::Origin in this structure, we could clean other files in a
separate CL if necessary,

[commit-bot: I haz the power, 2017-06-30 20:55:17]

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

[commit-bot: I haz the power, 2017-06-30 20:55:18]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-06-30 21:02:34]

IPC LGTM

https://codereview.chromium.org/2958333002/diff/160001/content/public/browser...
File content/public/browser/stored_payment_app.h (right):

https://codereview.chromium.org/2958333002/diff/160001/content/public/browser...
content/public/browser/stored_payment_app.h:28: GURL origin;
On 2017/06/30 18:18:51, gogerald1 wrote:
> On 2017/06/30 17:32:47, nasko (out until Jul 17th) wrote:
> > Why not url::Origin? Using GURLs to represent origins is not ideal, which is
> why
> > url::Origin exists and should be used.
> 
> Done. used url::Origin in this structure, we could clean other files in a
> separate CL if necessary,

It will be best to move over to url::Origin, so please do follow up.
Thanks!

[gogerald1, 2017-07-05 01:34:04]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2017-07-05 01:34:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2958333002/#ps180001
(title: "use url::Origin")

[commit-bot: I haz the power, 2017-07-05 01:34:12]

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, 2017-07-05 01:36:21]

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

[commit-bot: I haz the power, 2017-07-05 01:36:22]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[falken, 2017-07-05 01:55:05]

https://codereview.chromium.org/2958333002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2958333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:583:
render_frames->push_back(
This isn't necessarily a render_frame. I would call |render_frames| something
like |provider_host_ids|.

https://codereview.chromium.org/2958333002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2958333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:135: // Gets
provider hosts render process and frame Ids for the given |origin|.
// Returns all render process ids and frame ids for the given |origin|.

Please document this return value like this:
<int /* render process id */, int /* frame id */>

FYI this will also return a lot of frame_id == MSG_ROUTING_NONE, for
ServiceWorkerProviderHosts that are not hosting documents.

[gogerald1, 2017-07-05 02:09:58]

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

[commit-bot: I haz the power, 2017-07-05 02:10:11]

Dry run: CQ is trying da patch.

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

[gogerald1, 2017-07-05 02:20:21]

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

[commit-bot: I haz the power, 2017-07-05 02:20:35]

Dry run: CQ is trying da patch.

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

[gogerald1, 2017-07-05 02:26:04]

Thanks, addressed your comments,

https://codereview.chromium.org/2958333002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2958333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:583:
render_frames->push_back(
On 2017/07/05 01:55:05, falken wrote:
> This isn't necessarily a render_frame. I would call |render_frames| something
> like |provider_host_ids|.

Done.

https://codereview.chromium.org/2958333002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2958333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:135: // Gets
provider hosts render process and frame Ids for the given |origin|.
On 2017/07/05 01:55:05, falken wrote:
> // Returns all render process ids and frame ids for the given |origin|.
> 
> Please document this return value like this:
> <int /* render process id */, int /* frame id */>
> 
> FYI this will also return a lot of frame_id == MSG_ROUTING_NONE, for
> ServiceWorkerProviderHosts that are not hosting documents.
> 

Done.

[falken, 2017-07-05 02:27:43]

Thanks, service worker lgtm

[gogerald1, 2017-07-05 03:32:31]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2017-07-05 03:32:33]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2017-07-05 03:32:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2958333002/#ps220001
(title: "rename and comments")

[commit-bot: I haz the power, 2017-07-05 03:32:42]

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, 2017-07-05 05:28:51]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1499225553365790,
"parent_rev": "b4260c0fe1992fc54993e41df0b3f904dee63c7b", "commit_rev":
"70b9dcb8dae67c7c101a1c4c8f17da2adbcf98ab"}

[commit-bot: I haz the power, 2017-07-05 05:29:05]

Description was changed from

==========
Implement web payment app manifest

The web page registering web payment app needs to add 
<link rel="manifest" href="manifest.json"> 
in the page to provide the web app manifest.

BUG=735063
==========

to

==========
Implement web payment app manifest

The web page registering web payment app needs to add 
<link rel="manifest" href="manifest.json"> 
in the page to provide the web app manifest.

BUG=735063

Review-Url: https://codereview.chromium.org/2958333002
Cr-Commit-Position: refs/heads/master@{#484193}
Committed:
https://chromium.googlesource.com/chromium/src/+/70b9dcb8dae67c7c101a1c4c8f17...
==========

[commit-bot: I haz the power, 2017-07-05 05:29:07]

Committed patchset #7 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/70b9dcb8dae67c7c101a1c4c8f17...