Add app indexing reporter public class.

Add app indexing reporter public class.

BUG=693633
Review-Url: https://codereview.chromium.org/2622313002
Cr-Commit-Position: refs/heads/master@{#451818}
Committed: https://chromium.googlesource.com/chromium/src/+/47722dd6d68e52093d7f75f83d36a9fa011ede88

[dproctor, 2017-01-24 23:49:46]

Description was changed from

==========
Add app indexing reporter public class.

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

to

==========
Add app indexing reporter public class.

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

[wychen, 2017-02-13 22:08:06]

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

[wychen, 2017-02-13 22:08:06]

https://codereview.chromium.org/2622313002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/raw/parse.js (right):

https://codereview.chromium.org/2622313002/diff/20001/chrome/android/java/res...
chrome/android/java/res/raw/parse.js:9: var
a=document.querySelectorAll("script[type=\"application/ld+json\"]");if(a){ var
b=[];a.forEach(function(s) {b.push(JSON.parse(s.innerHTML)) });b; }
A few corner cases we might want to handle:

{
  "@graph": [
    { ... your first JSON-LD block ... },
    { ... your second JSON-LD block ... }
  ]
}
Ref:
http://stackoverflow.com/questions/32841383/multiple-json-ld-tag-groups-in-on...

[
  {
    ...first JSON-LD
  },
  {
    ...second JSON-LD
  }
]
Ref:
http://stackoverflow.com/questions/30723531/best-json-ld-practices-using-mult...

Do you know how popular these are, compared with the typical case this CL can
handle?

[dproctor, 2017-02-13 23:11:57]

https://codereview.chromium.org/2622313002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/raw/parse.js (right):

https://codereview.chromium.org/2622313002/diff/20001/chrome/android/java/res...
chrome/android/java/res/raw/parse.js:9: var
a=document.querySelectorAll("script[type=\"application/ld+json\"]");if(a){ var
b=[];a.forEach(function(s) {b.push(JSON.parse(s.innerHTML)) });b; }
On 2017/02/13 22:08:06, wychen (APAC time zone) wrote:
> A few corner cases we might want to handle:
> 
> {
>   "@graph": [
>     { ... your first JSON-LD block ... },
>     { ... your second JSON-LD block ... }
>   ]
> }
> Ref:
>
http://stackoverflow.com/questions/32841383/multiple-json-ld-tag-groups-in-on...
> 
> [
>   {
>     ...first JSON-LD
>   },
>   {
>     ...second JSON-LD
>   }
> ]
> Ref:
>
http://stackoverflow.com/questions/30723531/best-json-ld-practices-using-mult...
> 
> Do you know how popular these are, compared with the typical case this CL can
> handle?

Hey, thanks for raising the issue. I didn't see any examples of these as I was
looking through head domains, I'm not aware of any easy way to query this
(sporedash/ doesn't cover it). However, I definitely think that we should
support both cases.

I'll add logic to the Java class the handle the @graph case. As for the second
case, what do you think about flattening the arrays, so, if you have 

<script>
[
  { JSON1},
  { JSON2},
]
</script>
<script>
[
  { JSON3},
  { JSON4},
]
</script>

we return [ {JSON1}, {JSON2}, {JSON3}, {JSON4} ]

?

[wychen, 2017-02-13 23:44:31]

https://codereview.chromium.org/2622313002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/raw/parse.js (right):

https://codereview.chromium.org/2622313002/diff/20001/chrome/android/java/res...
chrome/android/java/res/raw/parse.js:9: var
a=document.querySelectorAll("script[type=\"application/ld+json\"]");if(a){ var
b=[];a.forEach(function(s) {b.push(JSON.parse(s.innerHTML)) });b; }
On 2017/02/13 23:11:57, dproctor wrote:
> On 2017/02/13 22:08:06, wychen (APAC time zone) wrote:
> > A few corner cases we might want to handle:
> > 
> > {
> >   "@graph": [
> >     { ... your first JSON-LD block ... },
> >     { ... your second JSON-LD block ... }
> >   ]
> > }
> > Ref:
> >
>
http://stackoverflow.com/questions/32841383/multiple-json-ld-tag-groups-in-on...
> > 
> > [
> >   {
> >     ...first JSON-LD
> >   },
> >   {
> >     ...second JSON-LD
> >   }
> > ]
> > Ref:
> >
>
http://stackoverflow.com/questions/30723531/best-json-ld-practices-using-mult...
> > 
> > Do you know how popular these are, compared with the typical case this CL
can
> > handle?
> 
> Hey, thanks for raising the issue. I didn't see any examples of these as I was
> looking through head domains, I'm not aware of any easy way to query this
> (sporedash/ doesn't cover it). However, I definitely think that we should
> support both cases.
> 
> I'll add logic to the Java class the handle the @graph case. As for the second
> case, what do you think about flattening the arrays, so, if you have 
> 
> <script>
> [
>   { JSON1},
>   { JSON2},
> ]
> </script>
> <script>
> [
>   { JSON3},
>   { JSON4},
> ]
> </script>
> 
> we return [ {JSON1}, {JSON2}, {JSON3}, {JSON4} ]
> 
> ?

I think flattening sounds nice, but then it would mean unpacking and packing the
JSON in Blink as well. If we keep using plain text manipulation, the output
would be [[ {JSON1}, {JSON2}], [{JSON3}, {JSON4} ]]. It might make sense to
consolidate the JSON handling logic to Java side. WDYT?

[dproctor, 2017-02-14 00:37:39]

On 2017/02/13 23:44:31, wychen wrote:
>
https://codereview.chromium.org/2622313002/diff/20001/chrome/android/java/res...
> File chrome/android/java/res/raw/parse.js (right):
> 
>
https://codereview.chromium.org/2622313002/diff/20001/chrome/android/java/res...
> chrome/android/java/res/raw/parse.js:9: var
> a=document.querySelectorAll("script[type=\"application/ld+json\"]");if(a){ var
> b=[];a.forEach(function(s) {b.push(JSON.parse(s.innerHTML)) });b; }
> On 2017/02/13 23:11:57, dproctor wrote:
> > On 2017/02/13 22:08:06, wychen (APAC time zone) wrote:
> > > A few corner cases we might want to handle:
> > > 
> > > {
> > >   "@graph": [
> > >     { ... your first JSON-LD block ... },
> > >     { ... your second JSON-LD block ... }
> > >   ]
> > > }
> > > Ref:
> > >
> >
>
http://stackoverflow.com/questions/32841383/multiple-json-ld-tag-groups-in-on...
> > > 
> > > [
> > >   {
> > >     ...first JSON-LD
> > >   },
> > >   {
> > >     ...second JSON-LD
> > >   }
> > > ]
> > > Ref:
> > >
> >
>
http://stackoverflow.com/questions/30723531/best-json-ld-practices-using-mult...
> > > 
> > > Do you know how popular these are, compared with the typical case this CL
> can
> > > handle?
> > 
> > Hey, thanks for raising the issue. I didn't see any examples of these as I
was
> > looking through head domains, I'm not aware of any easy way to query this
> > (sporedash/ doesn't cover it). However, I definitely think that we should
> > support both cases.
> > 
> > I'll add logic to the Java class the handle the @graph case. As for the
second
> > case, what do you think about flattening the arrays, so, if you have 
> > 
> > <script>
> > [
> >   { JSON1},
> >   { JSON2},
> > ]
> > </script>
> > <script>
> > [
> >   { JSON3},
> >   { JSON4},
> > ]
> > </script>
> > 
> > we return [ {JSON1}, {JSON2}, {JSON3}, {JSON4} ]
> > 
> > ?
> 
> I think flattening sounds nice, but then it would mean unpacking and packing
the
> JSON in Blink as well. If we keep using plain text manipulation, the output
> would be [[ {JSON1}, {JSON2}], [{JSON3}, {JSON4} ]]. It might make sense to
> consolidate the JSON handling logic to Java side. WDYT?

If it simplifies the blink code, then let's go with that. Since we're already
parsing JSON in Java, it's easy enough to handle there.

[wychen, 2017-02-14 00:42:30]

On 2017/02/14 00:37:39, dproctor wrote:
> If it simplifies the blink code, then let's go with that. Since we're already
> parsing JSON in Java, it's easy enough to handle there.

Awesome. Then let's do it this way.

[dproctor, 2017-02-14 17:44:36]

On 2017/02/14 00:42:30, wychen wrote:
> On 2017/02/14 00:37:39, dproctor wrote:
> > If it simplifies the blink code, then let's go with that. Since we're
already
> > parsing JSON in Java, it's easy enough to handle there.
> 
> Awesome. Then let's do it this way.

Done. I added the new behavior in the internal change (with tests), but left the
js change here as is, since it'll be replaced soon anyways.

[wychen, 2017-02-14 17:54:53]

On 2017/02/14 17:44:36, dproctor wrote:
> On 2017/02/14 00:42:30, wychen wrote:
> > On 2017/02/14 00:37:39, dproctor wrote:
> > > If it simplifies the blink code, then let's go with that. Since we're
> already
> > > parsing JSON in Java, it's easy enough to handle there.
> > 
> > Awesome. Then let's do it this way.
> 
> Done. I added the new behavior in the internal change (with tests), but left
the
> js change here as is, since it'll be replaced soon anyways.

Thanks!

[dproctor, 2017-02-15 07:21:43]

dproctor@google.com changed reviewers:
+ tedchoc@chromium.org

[wychen, 2017-02-15 21:40:59]

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

[wychen, 2017-02-16 01:16:24]

Description was changed from

==========
Add app indexing reporter public class.

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

to

==========
Add app indexing reporter public class.

BUG=682077
==========

[Maria, 2017-02-16 05:51:48]

On 2017/02/14 17:54:53, wychen wrote:
> On 2017/02/14 17:44:36, dproctor wrote:
> > On 2017/02/14 00:42:30, wychen wrote:
> > > On 2017/02/14 00:37:39, dproctor wrote:
> > > > If it simplifies the blink code, then let's go with that. Since we're
> > already
> > > > parsing JSON in Java, it's easy enough to handle there.
> > > 
> > > Awesome. Then let's do it this way.
> > 
> > Done. I added the new behavior in the internal change (with tests), but left
> the
> > js change here as is, since it'll be replaced soon anyways.
> 
> Thanks!

I don't see any callers of this code in this review. Is that intentional?

[dproctor, 2017-02-16 07:52:41]

On 2017/02/16 05:51:48, Maria wrote:
> On 2017/02/14 17:54:53, wychen wrote:
> > On 2017/02/14 17:44:36, dproctor wrote:
> > > On 2017/02/14 00:42:30, wychen wrote:
> > > > On 2017/02/14 00:37:39, dproctor wrote:
> > > > > If it simplifies the blink code, then let's go with that. Since we're
> > > already
> > > > > parsing JSON in Java, it's easy enough to handle there.
> > > > 
> > > > Awesome. Then let's do it this way.
> > > 
> > > Done. I added the new behavior in the internal change (with tests), but
left
> > the
> > > js change here as is, since it'll be replaced soon anyways.
> > 
> > Thanks!
> 
> I don't see any callers of this code in this review. Is that intentional?

Yeah, it's intentional. wychen@ is currently working on the cl that will call
this method, so that'll be submitted in a subsequent cl.

[wychen, 2017-02-17 18:10:09]

Description was changed from

==========
Add app indexing reporter public class.

BUG=682077
==========

to

==========
Add app indexing reporter public class.

BUG=693633
==========

[Maria, 2017-02-17 22:51:48]

lgtm

[wychen, 2017-02-17 23:59:48]

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

[commit-bot: I haz the power, 2017-02-18 00:00:57]

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-02-18 02:04:34]

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

[commit-bot: I haz the power, 2017-02-18 02:04:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[wychen, 2017-02-18 03:59:18]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2017-02-18 03:59:45]

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-02-18 07:38:29]

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

[commit-bot: I haz the power, 2017-02-18 07:38:31]

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

[wychen, 2017-02-21 17:05:48]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2017-02-21 17:06:30]

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-02-21 19:35:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487696748741950,
"parent_rev": "d13bd4b7858208f2fb37cfea437e06b8a95902bc", "commit_rev":
"47722dd6d68e52093d7f75f83d36a9fa011ede88"}

[commit-bot: I haz the power, 2017-02-21 19:36:27]

Description was changed from

==========
Add app indexing reporter public class.

BUG=693633
==========

to

==========
Add app indexing reporter public class.

BUG=693633

Review-Url: https://codereview.chromium.org/2622313002
Cr-Commit-Position: refs/heads/master@{#451818}
Committed:
https://chromium.googlesource.com/chromium/src/+/47722dd6d68e52093d7f75f83d36...
==========

[commit-bot: I haz the power, 2017-02-21 19:36:28]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/47722dd6d68e52093d7f75f83d36...