Make payment manifest download/parse cross-platform

Make payment manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) GN build target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

Manifest download is currently used only on Android after enabling the
chrome://flags/#android-payment-apps flag.

BUG=683329
Review-Url: https://codereview.chromium.org/2759283002
Cr-Commit-Position: refs/heads/master@{#460141}
Committed: https://chromium.googlesource.com/chromium/src/+/bb7522e6433d4aa05dd81026c42559f9ca2011e6

[please use gerrit instead, 2017-03-20 20:46:23]

Description was changed from

==========
Make manifest download/parse cross-platform

This patch moves downloader and parser utility into cross-platform
directory. The Android wrapper for the parser utility has been split
into a cross-platform host and an Android wrapper. Reason for the patch
is an upcoming change to download/parse the payment manifest on desktop
in addition to Android.

BUG=683329
==========

to

==========
Make manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) Target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

BUG=683329
==========

[please use gerrit instead, 2017-03-20 20:58:14]

Description was changed from

==========
Make manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) Target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

BUG=683329
==========

to

==========
Make manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) GN build target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

BUG=683329
==========

[please use gerrit instead, 2017-03-20 20:58:30]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2017-03-20 21:01:52]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2017-03-20 21:04:26]

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

[commit-bot: I haz the power, 2017-03-20 21:04:56]

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, 2017-03-20 21:05:49]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2017-03-20 21:06:30]

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

[please use gerrit instead, 2017-03-20 21:06:31]

Mathieu, ptal overall.

[commit-bot: I haz the power, 2017-03-20 21:11:17]

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

[commit-bot: I haz the power, 2017-03-20 21:11:18]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[please use gerrit instead, 2017-03-21 16:36:10]

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

[commit-bot: I haz the power, 2017-03-21 16:36:33]

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, 2017-03-21 16:38:41]

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

[commit-bot: I haz the power, 2017-03-21 16:38:58]

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-03-21 18:27:18]

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

[commit-bot: I haz the power, 2017-03-21 18:27:24]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-03-21 20:18:12]

Pretty great! lgtm

https://codereview.chromium.org/2759283002/diff/100001/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/2759283002/diff/100001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:290:
registry->AddInterface(base::Bind(&payments::PaymentManifestParser::Create));
are we not binding it yet on other platforms? Is there a crbug to implement this
soon?

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
File components/payments/content/payment_manifest_parser_host.cc (right):

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
components/payments/content/payment_manifest_parser_host.cc:38:
std::move(callback).Run(std::vector<mojom::PaymentManifestSectionPtr>());
std::move(callback).Run() is an interesting construct. I'm curious if it's used
a lot in Chrome?

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
File components/payments/content/payment_manifest_parser_host.h (right):

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
components/payments/content/payment_manifest_parser_host.h:30:
PaymentManifestParserHost();
Does this need to be here?

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
components/payments/content/payment_manifest_parser_host.h:35: // Starts the
utility process.
Mention that it may have long startup time and should be done as soon as
possible?

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
components/payments/content/payment_manifest_parser_host.h:52:
std::vector<Callback> pending_callbacks_;
Have you looked into base/callback_list.h?

https://cs.chromium.org/chromium/src/base/callback_list.h?q=CallbackList&sq=p...

https://codereview.chromium.org/2759283002/diff/100001/components/payments_st...
File components/payments_strings.grdp (right):

https://codereview.chromium.org/2759283002/diff/100001/components/payments_st...
components/payments_strings.grdp:253: Manifest Parser
Payment Manifest Parser? If I saw "Manifest Parser" in the process list I'd be
kind of lost

[please use gerrit instead, 2017-03-21 20:34:52]

https://codereview.chromium.org/2759283002/diff/100001/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/2759283002/diff/100001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:290:
registry->AddInterface(base::Bind(&payments::PaymentManifestParser::Create));
On 2017/03/21 20:18:12, Mathieu Perreault wrote:
> are we not binding it yet on other platforms? Is there a crbug to implement
this
> soon?

Oops, my mistake. I need to register this on all platforms in order to make
manifest parsing cross-platform.

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
File components/payments/content/payment_manifest_parser_host.cc (right):

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
components/payments/content/payment_manifest_parser_host.cc:38:
std::move(callback).Run(std::vector<mojom::PaymentManifestSectionPtr>());
On 2017/03/21 20:18:12, Mathieu Perreault wrote:
> std::move(callback).Run() is an interesting construct. I'm curious if it's
used
> a lot in Chrome? 

That's how you run base::OnceCallback:

https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md

"In new code, prefer OnceCallback<> where possible."

"Note that OnceCallback::Run consumes the callback object and can only be
invoked on a callback rvalue."

"""
// |Baz| takes the ownership of |cb| and consumes |cb| by Run().
void Baz(OnceCallback<void(int)> cb) {
  std::move(cb).Run(42);
}
"""

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
File components/payments/content/payment_manifest_parser_host.h (right):

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
components/payments/content/payment_manifest_parser_host.h:30:
PaymentManifestParserHost();
On 2017/03/21 20:18:12, Mathieu Perreault wrote:
> Does this need to be here?

Where do you propose to put it? This place seems sensible :-P

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
components/payments/content/payment_manifest_parser_host.h:35: // Starts the
utility process.
On 2017/03/21 20:18:12, Mathieu Perreault wrote:
> Mention that it may have long startup time and should be done as soon as
> possible?

Done.

https://codereview.chromium.org/2759283002/diff/100001/components/payments/co...
components/payments/content/payment_manifest_parser_host.h:52:
std::vector<Callback> pending_callbacks_;
On 2017/03/21 20:18:12, Mathieu Perreault wrote:
> Have you looked into base/callback_list.h?
> 
>
https://cs.chromium.org/chromium/src/base/callback_list.h?q=CallbackList&sq=p...

CallbackList works with base::Callback, which is currently aliased to the
copyable base::RepeatingCallback, whereas I'm using the move-only
base::OnceCallback here.

https://codereview.chromium.org/2759283002/diff/100001/components/payments_st...
File components/payments_strings.grdp (right):

https://codereview.chromium.org/2759283002/diff/100001/components/payments_st...
components/payments_strings.grdp:253: Manifest Parser
On 2017/03/21 20:18:12, Mathieu Perreault wrote:
> Payment Manifest Parser? If I saw "Manifest Parser" in the process list I'd be
> kind of lost

Done.

[please use gerrit instead, 2017-03-21 20:35:05]

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

[commit-bot: I haz the power, 2017-03-21 20:35:43]

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, 2017-03-21 20:45:33]

rouslan@chromium.org changed reviewers:
+ agrieve@chromium.org, haraken@chromium.org, jochen@chromium.org,
palmer@chromium.org, thakis@chromium.org

[please use gerrit instead, 2017-03-21 20:45:34]

agrieve, ptal chrome/android/BUILD.gn in patch 4.

thakis, ptal chrome/utility/* in patch 4.

jochen, ptal components/BUILD.gn and content/* in patch 4.

palmer, ptal the move of
components/payments/content/android/payment_manifest_parser.mojom into
components/payments/content/payment_manifest_parser.mojom in patch 4.

haraken, ptal third_party/WebKit/Source/modules/BUILD.gn in patch 4.

[Nico, 2017-03-21 20:52:22]

chrome/utility lgtm

In the CL description, please prepend the first line with some context, else
it's not clear which manifests this is about.

[please use gerrit instead, 2017-03-21 20:55:20]

Description was changed from

==========
Make manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) GN build target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

BUG=683329
==========

to

==========
Make payment manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) GN build target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

Manifest download is currently used only on Android after enabling the
chrome://flags/#android-payment-apps flag.

BUG=683329
==========

[please use gerrit instead, 2017-03-21 20:56:19]

On 2017/03/21 20:52:22, Nico wrote:
> In the CL description, please prepend the first line with some context, else
> it's not clear which manifests this is about.

Changed "manifest" to "payment manifest" in the CL description. Also noted that
this is currently behind a flag.

[commit-bot: I haz the power, 2017-03-21 21:01:09]

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

[commit-bot: I haz the power, 2017-03-21 21:01:10]

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

[agrieve, 2017-03-22 00:21:08]

On 2017/03/21 20:56:19, ಠ_ಠ wrote:
> On 2017/03/21 20:52:22, Nico wrote:
> > In the CL description, please prepend the first line with some context, else
> > it's not clear which manifests this is about.
> 
> Changed "manifest" to "payment manifest" in the CL description. Also noted
that
> this is currently behind a flag.

BUILD lgtm

[jochen (gone - plz use gerrit), 2017-03-22 10:05:25]

content/ and components/BUILD.gn lgtm

[haraken, 2017-03-27 13:50:56]

LGTM

[please use gerrit instead, 2017-03-27 14:11:22]

Description was changed from

==========
Make payment manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) GN build target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

Manifest download is currently used only on Android after enabling the
chrome://flags/#android-payment-apps flag.

BUG=683329
==========

to

==========
Make payment manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) GN build target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

Manifest download is currently used only on Android after enabling the
chrome://flags/#android-payment-apps flag.

BUG=683329
==========

[palmer, 2017-03-27 23:58:13]

lgtm

[please use gerrit instead, 2017-03-28 14:08:32]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2017-03-28 14:08:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2759283002/#ps120001
(title: "Address comments")

[commit-bot: I haz the power, 2017-03-28 14:08:55]

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-03-28 14:15:53]

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

[commit-bot: I haz the power, 2017-03-28 14:15:55]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[please use gerrit instead, 2017-03-28 14:44:57]

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

[please use gerrit instead, 2017-03-28 14:44:58]

jkarlin, net owners ptal for the +net dep.

** Presubmit ERRORS **
You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
    '+net',

[jkarlin, 2017-03-28 14:50:42]

+net in components/payments/content/DEPS lgtm

[please use gerrit instead, 2017-03-28 14:53:38]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2017-03-28 14:53:54]

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-03-28 17:06:24]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490712818203510,
"parent_rev": "1b47fda711331d9d39563ce0749c5f515b186169", "commit_rev":
"bb7522e6433d4aa05dd81026c42559f9ca2011e6"}

[commit-bot: I haz the power, 2017-03-28 17:07:10]

Description was changed from

==========
Make payment manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) GN build target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

Manifest download is currently used only on Android after enabling the
chrome://flags/#android-payment-apps flag.

BUG=683329
==========

to

==========
Make payment manifest download/parse cross-platform

Reason for the refactor is an upcoming change to download/parse the
payment manifest on desktop in addition to Android.

1) This patch moves downloader and parser utility into cross-platform
   directory //components/payments/content.

2) The cross-platform PaymentManifestParserHost has been separated from
   its Android wrapper PaymentManifestParserAndroid.

3) GN build target renames:
   a) Payments mojom targets have been renamed to include the string
      "mojom" for clarity.
   b) The main target of "//components/payments/content" has been
      renamed to "content" for brevity.
   c) The "//components/payments/content:payment_validators" target has
      been renamed to "//components/payments/content:utils", because it
      now contains manifest downloader and parser.

Manifest download is currently used only on Android after enabling the
chrome://flags/#android-payment-apps flag.

BUG=683329

Review-Url: https://codereview.chromium.org/2759283002
Cr-Commit-Position: refs/heads/master@{#460141}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb7522e6433d4aa05dd81026c425...
==========

[commit-bot: I haz the power, 2017-03-28 17:07:11]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/bb7522e6433d4aa05dd81026c425...

[wychen, 2017-03-28 19:18:00]

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

[wychen, 2017-03-28 19:18:01]

clank build with ToT was broken.

ERROR Unresolved dependencies.
//clank/java:clank_java__build_config(//build/toolchain/android:android_clang_arm)
  needs
//components/payments/content:payment_request_java__build_config(//build/toolchain/android:android_clang_arm)
//clank/java:clank_java__compile_java__javac(//build/toolchain/android:android_clang_arm)
  needs
//components/payments/content:payment_request_java(//build/toolchain/android:android_clang_arm)
//clank/java:clank_java__compile_java__process_prebuilt__assert(//build/toolchain/android:android_clang_arm)
  needs
//components/payments/content:payment_request_java(//build/toolchain/android:android_clang_arm)
//clank/java:clank_java__compile_java__process_prebuilt__copy(//build/toolchain/android:android_clang_arm)
  needs
//components/payments/content:payment_request_java(//build/toolchain/android:android_clang_arm)
//clank/java:clank_java__compile_java__process_prebuilt__filter(//build/toolchain/android:android_clang_arm)
  needs
//components/payments/content:payment_request_java(//build/toolchain/android:android_clang_arm)
//clank/java:clank_java__lint(//build/toolchain/android:android_clang_arm)
  needs
//components/payments/content:payment_request_java(//build/toolchain/android:android_clang_arm)