Move classes to //components/ntp_tiles.

Move classes to //components/ntp_tiles.

Move MostVisitedSites and PopularSites; leave behind ChromePopularSites,
which depends on Chrome paths.

In the short term, there are no changes introduced by this CL; the same
code builds as before against the same dependencies. In the future,
multiple platforms' versions of the new tab page will share this part of
their implementation. (Android's NTP currently is all native code; on
other platforms we use HTML)

BUG=603026
Committed: https://crrev.com/3ff01c0dd7b7b6955dafda763cbc120718dc00ef
Cr-Commit-Position: refs/heads/master@{#399455}

[sfiera, 2016-05-17 11:21:15]

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

[commit-bot: I haz the power, 2016-05-17 11:21:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/1

[commit-bot: I haz the power, 2016-05-17 11:24:24]

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

[commit-bot: I haz the power, 2016-05-17 11:24:25]

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

[sfiera, 2016-05-17 11:53:23]

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

[commit-bot: I haz the power, 2016-05-17 11:53:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/40001

[commit-bot: I haz the power, 2016-05-17 11:59:36]

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

[commit-bot: I haz the power, 2016-05-17 11:59:37]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[sfiera, 2016-05-17 14:08:52]

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

[commit-bot: I haz the power, 2016-05-17 14:09:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/60001

[sfiera, 2016-05-17 15:09:58]

sfiera@chromium.org changed reviewers:
+ treib@chromium.org

[sfiera, 2016-05-17 15:09:59]

We'll need a bunch of approvals too.

[sfiera, 2016-05-17 15:30:26]

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

[commit-bot: I haz the power, 2016-05-17 15:30:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/80001

[commit-bot: I haz the power, 2016-05-17 15:43:07]

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

[commit-bot: I haz the power, 2016-05-17 15:43:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Marc Treib, 2016-05-17 15:50:25]

lgtm

https://codereview.chromium.org/1983063002/diff/80001/chrome/browser/android/...
File chrome/browser/android/ntp/popular_sites.h (right):

https://codereview.chromium.org/1983063002/diff/80001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:5: #ifndef
CHROME_BROWSER_ANDROID_NTP_POPULAR_SITES_H_
Rename this to chrome_popular_sites.h/cc now?

https://codereview.chromium.org/1983063002/diff/80001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1983063002/diff/80001/components/components_t...
components/components_tests.gyp:413: 'ntp_tiles_unittest_sources': [
This should go after ntp_snippets

https://codereview.chromium.org/1983063002/diff/80001/components/ntp_tiles/BU...
File components/ntp_tiles/BUILD.gn (right):

https://codereview.chromium.org/1983063002/diff/80001/components/ntp_tiles/BU...
components/ntp_tiles/BUILD.gn:5: assert(!is_ios)
Do we want to keep the assert here? If so, then I think it deserves a comment.

https://codereview.chromium.org/1983063002/diff/80001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/1983063002/diff/80001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:42: // Shim interface for
SupervisedUserService.
Everything here should go into an ntp_tiles namespace now (but it's fine to do
that in another CL).

[sfiera, 2016-05-18 08:42:02]

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

[commit-bot: I haz the power, 2016-05-18 08:42:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/140001

[sfiera, 2016-05-18 08:42:14]

The CQ bit was unchecked by sfiera@chromium.org

[sfiera, 2016-05-18 08:43:28]

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

[commit-bot: I haz the power, 2016-05-18 08:43:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/160001

[sfiera, 2016-05-18 08:43:42]

https://codereview.chromium.org/1983063002/diff/80001/chrome/browser/android/...
File chrome/browser/android/ntp/popular_sites.h (right):

https://codereview.chromium.org/1983063002/diff/80001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:5: #ifndef
CHROME_BROWSER_ANDROID_NTP_POPULAR_SITES_H_
On 2016/05/17 15:50:25, Marc Treib wrote:
> Rename this to chrome_popular_sites.h/cc now?

Next CL OK? webui references this file, and I wanted to avoid touching another
directory in this CL.

https://codereview.chromium.org/1983063002/diff/80001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1983063002/diff/80001/components/components_t...
components/components_tests.gyp:413: 'ntp_tiles_unittest_sources': [
On 2016/05/17 15:50:25, Marc Treib wrote:
> This should go after ntp_snippets

Done. Praise our new gn overlords and their `gn format`.

https://codereview.chromium.org/1983063002/diff/80001/components/ntp_tiles/BU...
File components/ntp_tiles/BUILD.gn (right):

https://codereview.chromium.org/1983063002/diff/80001/components/ntp_tiles/BU...
components/ntp_tiles/BUILD.gn:5: assert(!is_ios)
On 2016/05/17 15:50:25, Marc Treib wrote:
> Do we want to keep the assert here? If so, then I think it deserves a comment.

Figuring out why ios broke was enough of annoyance that I want to keep it :)
(now with a comment)

https://codereview.chromium.org/1983063002/diff/80001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/1983063002/diff/80001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:42: // Shim interface for
SupervisedUserService.
On 2016/05/17 15:50:25, Marc Treib wrote:
> Everything here should go into an ntp_tiles namespace now (but it's fine to do
> that in another CL).

Ack, next CL.

[Marc Treib, 2016-05-18 08:55:41]

lgtm

https://codereview.chromium.org/1983063002/diff/80001/chrome/browser/android/...
File chrome/browser/android/ntp/popular_sites.h (right):

https://codereview.chromium.org/1983063002/diff/80001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:5: #ifndef
CHROME_BROWSER_ANDROID_NTP_POPULAR_SITES_H_
On 2016/05/18 08:43:42, sfiera wrote:
> On 2016/05/17 15:50:25, Marc Treib wrote:
> > Rename this to chrome_popular_sites.h/cc now?
> 
> Next CL OK? webui references this file, and I wanted to avoid touching another
> directory in this CL.

Yup, that's okay.

[commit-bot: I haz the power, 2016-05-18 09:11:22]

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

[commit-bot: I haz the power, 2016-05-18 09:11:23]

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

[sfiera, 2016-05-18 09:23:14]

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

[commit-bot: I haz the power, 2016-05-18 09:23:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/180001

[commit-bot: I haz the power, 2016-05-18 09:43:19]

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

[commit-bot: I haz the power, 2016-05-18 09:43:20]

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

[sfiera, 2016-05-18 09:49:07]

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

[commit-bot: I haz the power, 2016-05-18 09:49:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/200001

[commit-bot: I haz the power, 2016-05-18 10:52:26]

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

[commit-bot: I haz the power, 2016-05-18 10:52:27]

Dry run: This issue passed the CQ dry run.

[sfiera, 2016-05-18 12:07:43]

sfiera@chromium.org changed reviewers:
+ jochen@chromium.org

[sfiera, 2016-05-18 12:07:44]

+jochen for components/BUILD.gn and chrome/chrome.gyp

This also unintentionally covers some other owners, such as
chrome/android/BUILD.gn and added DEPS, so we may want to chase down more
specific approvals too.

[sfiera, 2016-05-23 13:24:49]

sfiera@chromium.org changed reviewers:
+ bauerb@chromium.org, sdefresne@chromium.org, sky@chromium.org
- jochen@chromium.org

[sfiera, 2016-05-23 13:24:50]

-jochen (OOO)
+bauerb for //chrome/android/... (Java enum moved into components)
+sky for //chrome/*.{gyp,gypi} (files moved into components)
+sdefresne for //components/BUILD.gn (added test)

[sky, 2016-05-23 17:24:26]

Gyp(i) files in chrome LGTM

[sdefresne, 2016-05-24 06:36:32]

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/DEPS
File components/ntp_tiles/DEPS (right):

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/D...
components/ntp_tiles/DEPS:10: "+content/public/browser",
Is there plan to use this component on iOS? If yes, I would recommend removing
the dependency on //content/public/browser before moving this code into the
component. It looks like the only dependency is on content::BrowserThread which
is easy to abstract by injecting the threads into the object at creation (see
all the other layered components).

If no, what are the different embedders that will use this component?

[Bernhard Bauer, 2016-05-24 08:14:51]

LGTM

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/B...
File components/ntp_tiles/BUILD.gn (right):

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/B...
components/ntp_tiles/BUILD.gn:53: java_cpp_enum("most_visited_sites_enums_java")
{
This target generates all of the enums for the component, right? (Hence "enums"
in plural.) So it should be ntp_tiles_enums_java.

[sfiera, 2016-05-24 12:40:04]

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

[sfiera, 2016-05-24 12:40:15]

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/B...
File components/ntp_tiles/BUILD.gn (right):

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/B...
components/ntp_tiles/BUILD.gn:53: java_cpp_enum("most_visited_sites_enums_java")
{
On 2016/05/24 08:14:51, Bernhard Bauer wrote:
> This target generates all of the enums for the component, right? (Hence
"enums"
> in plural.) So it should be ntp_tiles_enums_java.

Yes, it generates all the enums. Although there's only one :)

Renamed here and in gyp files.

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/DEPS
File components/ntp_tiles/DEPS (right):

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/D...
components/ntp_tiles/DEPS:10: "+content/public/browser",
On 2016/05/24 06:36:32, sdefresne wrote:
> Is there plan to use this component on iOS? If yes, I would recommend removing
> the dependency on //content/public/browser before moving this code into the
> component. It looks like the only dependency is on content::BrowserThread
which
> is easy to abstract by injecting the threads into the object at creation (see
> all the other layered components).
> 
> If no, what are the different embedders that will use this component?

Our immediate plan is to use it in the desktop new tab page; we haven't made any
plans to use it on iOS. We might eventually want to, but it would be a few
quarters out at least.

[commit-bot: I haz the power, 2016-05-24 12:40:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/220001

[commit-bot: I haz the power, 2016-05-24 12:41:59]

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

[commit-bot: I haz the power, 2016-05-24 12:42:01]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Marc Treib, 2016-05-24 12:46:35]

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/DEPS
File components/ntp_tiles/DEPS (right):

https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/D...
components/ntp_tiles/DEPS:10: "+content/public/browser",
On 2016/05/24 12:40:15, sfiera wrote:
> On 2016/05/24 06:36:32, sdefresne wrote:
> > Is there plan to use this component on iOS? If yes, I would recommend
removing
> > the dependency on //content/public/browser before moving this code into the
> > component. It looks like the only dependency is on content::BrowserThread
> which
> > is easy to abstract by injecting the threads into the object at creation
(see
> > all the other layered components).
> > 
> > If no, what are the different embedders that will use this component?
> 
> Our immediate plan is to use it in the desktop new tab page; we haven't made
any
> plans to use it on iOS. We might eventually want to, but it would be a few
> quarters out at least.

I guess the main blocker is that nobody on our team has experience with iOS. If
somebody from the iOS team wants to do the integration work, then I'm sure we
can make that happen in less than a few quarters :)
Not in this CL though.

[blundell, 2016-05-24 12:52:03]

On 2016/05/24 12:46:35, Marc Treib wrote:
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/DEPS
> File components/ntp_tiles/DEPS (right):
> 
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/D...
> components/ntp_tiles/DEPS:10: "+content/public/browser",
> On 2016/05/24 12:40:15, sfiera wrote:
> > On 2016/05/24 06:36:32, sdefresne wrote:
> > > Is there plan to use this component on iOS? If yes, I would recommend
> removing
> > > the dependency on //content/public/browser before moving this code into
the
> > > component. It looks like the only dependency is on content::BrowserThread
> > which
> > > is easy to abstract by injecting the threads into the object at creation
> (see
> > > all the other layered components).
> > > 
> > > If no, what are the different embedders that will use this component?
> > 
> > Our immediate plan is to use it in the desktop new tab page; we haven't made
> any
> > plans to use it on iOS. We might eventually want to, but it would be a few
> > quarters out at least.
> 
> I guess the main blocker is that nobody on our team has experience with iOS.
If
> somebody from the iOS team wants to do the integration work, then I'm sure we
> can make that happen in less than a few quarters :)
> Not in this CL though.

drive-by: Why move it to a component as opposed to e.g. //chrome/browser/ui if
you're only looking to expand its usage from Chrome on Android to Chrome on
{Android, desktop}?

[sfiera, 2016-05-24 13:14:30]

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

[commit-bot: I haz the power, 2016-05-24 13:14:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/240001

[sfiera, 2016-05-24 13:22:19]

On 2016/05/24 12:52:03, blundell wrote:
> On 2016/05/24 12:46:35, Marc Treib wrote:
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/DEPS
> > File components/ntp_tiles/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/D...
> > components/ntp_tiles/DEPS:10: "+content/public/browser",
> > On 2016/05/24 12:40:15, sfiera wrote:
> > > On 2016/05/24 06:36:32, sdefresne wrote:
> > > > Is there plan to use this component on iOS? If yes, I would recommend
> > removing
> > > > the dependency on //content/public/browser before moving this code into
> the
> > > > component. It looks like the only dependency is on
content::BrowserThread
> > > which
> > > > is easy to abstract by injecting the threads into the object at creation
> > (see
> > > > all the other layered components).
> > > > 
> > > > If no, what are the different embedders that will use this component?
> > > 
> > > Our immediate plan is to use it in the desktop new tab page; we haven't
made
> > any
> > > plans to use it on iOS. We might eventually want to, but it would be a few
> > > quarters out at least.
> > 
> > I guess the main blocker is that nobody on our team has experience with iOS.
> If
> > somebody from the iOS team wants to do the integration work, then I'm sure
we
> > can make that happen in less than a few quarters :)
> > Not in this CL though.
> 
> drive-by: Why move it to a component as opposed to e.g. //chrome/browser/ui if
> you're only looking to expand its usage from Chrome on Android to Chrome on
> {Android, desktop}?

Well, the short answer is that componentizing the NTP tiles code is the task I
was given, and I'm not yet familiar enough with Chrome to ask clarifying
questions like that. Marc, is there a long answer? :)

[blundell, 2016-05-24 13:25:42]

On 2016/05/24 13:22:19, sfiera wrote:
> On 2016/05/24 12:52:03, blundell wrote:
> > On 2016/05/24 12:46:35, Marc Treib wrote:
> > >
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/DEPS
> > > File components/ntp_tiles/DEPS (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/D...
> > > components/ntp_tiles/DEPS:10: "+content/public/browser",
> > > On 2016/05/24 12:40:15, sfiera wrote:
> > > > On 2016/05/24 06:36:32, sdefresne wrote:
> > > > > Is there plan to use this component on iOS? If yes, I would recommend
> > > removing
> > > > > the dependency on //content/public/browser before moving this code
into
> > the
> > > > > component. It looks like the only dependency is on
> content::BrowserThread
> > > > which
> > > > > is easy to abstract by injecting the threads into the object at
creation
> > > (see
> > > > > all the other layered components).
> > > > > 
> > > > > If no, what are the different embedders that will use this component?
> > > > 
> > > > Our immediate plan is to use it in the desktop new tab page; we haven't
> made
> > > any
> > > > plans to use it on iOS. We might eventually want to, but it would be a
few
> > > > quarters out at least.
> > > 
> > > I guess the main blocker is that nobody on our team has experience with
iOS.
> > If
> > > somebody from the iOS team wants to do the integration work, then I'm sure
> we
> > > can make that happen in less than a few quarters :)
> > > Not in this CL though.
> > 
> > drive-by: Why move it to a component as opposed to e.g. //chrome/browser/ui
if
> > you're only looking to expand its usage from Chrome on Android to Chrome on
> > {Android, desktop}?
> 
> Well, the short answer is that componentizing the NTP tiles code is the task I
> was given, and I'm not yet familiar enough with Chrome to ask clarifying
> questions like that. Marc, is there a long answer? :)

Looking at the bug, it says that //components/ntp_snippets will shortly be a
consumer of //components/ntp_tiles. If this is the case, then this means that
//components/ntp_tiles effectively is shared on iOS, as
//components/ntp_snippets is shared on iOS. So that would be a clear reason for
moving it into //components, but would also suggest following sdefresne's advice
on getting rid of the //content dependency.

[blundell, 2016-05-24 13:29:52]

On 2016/05/24 13:25:42, blundell wrote:
> On 2016/05/24 13:22:19, sfiera wrote:
> > On 2016/05/24 12:52:03, blundell wrote:
> > > On 2016/05/24 12:46:35, Marc Treib wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/DEPS
> > > > File components/ntp_tiles/DEPS (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/D...
> > > > components/ntp_tiles/DEPS:10: "+content/public/browser",
> > > > On 2016/05/24 12:40:15, sfiera wrote:
> > > > > On 2016/05/24 06:36:32, sdefresne wrote:
> > > > > > Is there plan to use this component on iOS? If yes, I would
recommend
> > > > removing
> > > > > > the dependency on //content/public/browser before moving this code
> into
> > > the
> > > > > > component. It looks like the only dependency is on
> > content::BrowserThread
> > > > > which
> > > > > > is easy to abstract by injecting the threads into the object at
> creation
> > > > (see
> > > > > > all the other layered components).
> > > > > > 
> > > > > > If no, what are the different embedders that will use this
component?
> > > > > 
> > > > > Our immediate plan is to use it in the desktop new tab page; we
haven't
> > made
> > > > any
> > > > > plans to use it on iOS. We might eventually want to, but it would be a
> few
> > > > > quarters out at least.
> > > > 
> > > > I guess the main blocker is that nobody on our team has experience with
> iOS.
> > > If
> > > > somebody from the iOS team wants to do the integration work, then I'm
sure
> > we
> > > > can make that happen in less than a few quarters :)
> > > > Not in this CL though.
> > > 
> > > drive-by: Why move it to a component as opposed to e.g.
//chrome/browser/ui
> if
> > > you're only looking to expand its usage from Chrome on Android to Chrome
on
> > > {Android, desktop}?
> > 
> > Well, the short answer is that componentizing the NTP tiles code is the task
I
> > was given, and I'm not yet familiar enough with Chrome to ask clarifying
> > questions like that. Marc, is there a long answer? :)
> 
> Looking at the bug, it says that //components/ntp_snippets will shortly be a
> consumer of //components/ntp_tiles. If this is the case, then this means that
> //components/ntp_tiles effectively is shared on iOS, as
> //components/ntp_snippets is shared on iOS. So that would be a clear reason
for
> moving it into //components, but would also suggest following sdefresne's
advice
> on getting rid of the //content dependency.

To be concrete, it looks like you should be able to follow the example of e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
in order to eliminate this dependency: just have it take in the threads as the
//base-level primitives. The content::BrowserThread operations all map trivially
down to the corresponding //base-level operations.

[Marc Treib, 2016-05-24 13:36:00]

On 2016/05/24 13:25:42, blundell wrote:
> On 2016/05/24 13:22:19, sfiera wrote:
> > On 2016/05/24 12:52:03, blundell wrote:
> > > On 2016/05/24 12:46:35, Marc Treib wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/DEPS
> > > > File components/ntp_tiles/DEPS (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/D...
> > > > components/ntp_tiles/DEPS:10: "+content/public/browser",
> > > > On 2016/05/24 12:40:15, sfiera wrote:
> > > > > On 2016/05/24 06:36:32, sdefresne wrote:
> > > > > > Is there plan to use this component on iOS? If yes, I would
recommend
> > > > removing
> > > > > > the dependency on //content/public/browser before moving this code
> into
> > > the
> > > > > > component. It looks like the only dependency is on
> > content::BrowserThread
> > > > > which
> > > > > > is easy to abstract by injecting the threads into the object at
> creation
> > > > (see
> > > > > > all the other layered components).
> > > > > > 
> > > > > > If no, what are the different embedders that will use this
component?
> > > > > 
> > > > > Our immediate plan is to use it in the desktop new tab page; we
haven't
> > made
> > > > any
> > > > > plans to use it on iOS. We might eventually want to, but it would be a
> few
> > > > > quarters out at least.
> > > > 
> > > > I guess the main blocker is that nobody on our team has experience with
> iOS.
> > > If
> > > > somebody from the iOS team wants to do the integration work, then I'm
sure
> > we
> > > > can make that happen in less than a few quarters :)
> > > > Not in this CL though.
> > > 
> > > drive-by: Why move it to a component as opposed to e.g.
//chrome/browser/ui
> if
> > > you're only looking to expand its usage from Chrome on Android to Chrome
on
> > > {Android, desktop}?
> > 
> > Well, the short answer is that componentizing the NTP tiles code is the task
I
> > was given, and I'm not yet familiar enough with Chrome to ask clarifying
> > questions like that. Marc, is there a long answer? :)
> 
> Looking at the bug, it says that //components/ntp_snippets will shortly be a
> consumer of //components/ntp_tiles. If this is the case, then this means that
> //components/ntp_tiles effectively is shared on iOS, as
> //components/ntp_snippets is shared on iOS. So that would be a clear reason
for
> moving it into //components, but would also suggest following sdefresne's
advice
> on getting rid of the //content dependency.

Err, right, that's a good point. ntp_snippets is compiled on iOS, though it's
not actually active there yet (which is why I didn't really think of that).
So yes, we should remove the dependency on content/, especially if it's as
straightforward as suggested. I'd be more comfortable with a separate CL for
that though.

[sdefresne, 2016-05-24 13:41:06]

On 2016/05/24 13:36:00, Marc Treib wrote:
> On 2016/05/24 13:25:42, blundell wrote:
> > On 2016/05/24 13:22:19, sfiera wrote:
> > > On 2016/05/24 12:52:03, blundell wrote:
> > > > On 2016/05/24 12:46:35, Marc Treib wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/DEPS
> > > > > File components/ntp_tiles/DEPS (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1983063002/diff/200001/components/ntp_tiles/D...
> > > > > components/ntp_tiles/DEPS:10: "+content/public/browser",
> > > > > On 2016/05/24 12:40:15, sfiera wrote:
> > > > > > On 2016/05/24 06:36:32, sdefresne wrote:
> > > > > > > Is there plan to use this component on iOS? If yes, I would
> recommend
> > > > > removing
> > > > > > > the dependency on //content/public/browser before moving this code
> > into
> > > > the
> > > > > > > component. It looks like the only dependency is on
> > > content::BrowserThread
> > > > > > which
> > > > > > > is easy to abstract by injecting the threads into the object at
> > creation
> > > > > (see
> > > > > > > all the other layered components).
> > > > > > > 
> > > > > > > If no, what are the different embedders that will use this
> component?
> > > > > > 
> > > > > > Our immediate plan is to use it in the desktop new tab page; we
> haven't
> > > made
> > > > > any
> > > > > > plans to use it on iOS. We might eventually want to, but it would be
a
> > few
> > > > > > quarters out at least.
> > > > > 
> > > > > I guess the main blocker is that nobody on our team has experience
with
> > iOS.
> > > > If
> > > > > somebody from the iOS team wants to do the integration work, then I'm
> sure
> > > we
> > > > > can make that happen in less than a few quarters :)
> > > > > Not in this CL though.
> > > > 
> > > > drive-by: Why move it to a component as opposed to e.g.
> //chrome/browser/ui
> > if
> > > > you're only looking to expand its usage from Chrome on Android to Chrome
> on
> > > > {Android, desktop}?
> > > 
> > > Well, the short answer is that componentizing the NTP tiles code is the
task
> I
> > > was given, and I'm not yet familiar enough with Chrome to ask clarifying
> > > questions like that. Marc, is there a long answer? :)
> > 
> > Looking at the bug, it says that //components/ntp_snippets will shortly be a
> > consumer of //components/ntp_tiles. If this is the case, then this means
that
> > //components/ntp_tiles effectively is shared on iOS, as
> > //components/ntp_snippets is shared on iOS. So that would be a clear reason
> for
> > moving it into //components, but would also suggest following sdefresne's
> advice
> > on getting rid of the //content dependency.
> 
> Err, right, that's a good point. ntp_snippets is compiled on iOS, though it's
> not actually active there yet (which is why I didn't really think of that).
> So yes, we should remove the dependency on content/, especially if it's as
> straightforward as suggested. I'd be more comfortable with a separate CL for
> that though.

I found in the past that it was better to remove dependency on //content before
moving the code to //components. Can you do a separate CL to remove the
dependency and then update this one on top of it?

[commit-bot: I haz the power, 2016-05-24 14:37:26]

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

[commit-bot: I haz the power, 2016-05-24 14:37:28]

Dry run: This issue passed the CQ dry run.

[sfiera, 2016-05-24 15:06:25]

Sent crrev.com/2012473002 to remove content dependency.

[sfiera, 2016-05-27 13:08:37]

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

[commit-bot: I haz the power, 2016-05-27 13:08:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/260001

[sfiera, 2016-05-27 13:09:27]

Updated with no content following crrev.com/2012473002. PTAL.

[blundell, 2016-05-27 13:12:52]

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

[blundell, 2016-05-27 13:12:54]

https://codereview.chromium.org/1983063002/diff/260001/components/components_...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1983063002/diff/260001/components/components_...
components/components_tests.gyp:1264: '<@(ntp_tiles_unittest_sources)',
you need an addition to components.gyp too

https://codereview.chromium.org/1983063002/diff/260001/components/ntp_tiles.gypi
File components/ntp_tiles.gypi (right):

https://codereview.chromium.org/1983063002/diff/260001/components/ntp_tiles.g...
components/ntp_tiles.gypi:7: ['OS != "ios"', {
You might as well build this on iOS now that it's buildable on iOS :).

https://codereview.chromium.org/1983063002/diff/260001/components/ntp_tiles.g...
components/ntp_tiles.gypi:18: '../content/content.gyp:content_browser',
kill this now

[sfiera, 2016-05-27 13:24:16]

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

[sfiera, 2016-05-27 13:24:26]

https://codereview.chromium.org/1983063002/diff/260001/components/components_...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1983063002/diff/260001/components/components_...
components/components_tests.gyp:1264: '<@(ntp_tiles_unittest_sources)',
On 2016/05/27 13:12:54, blundell wrote:
> you need an addition to components.gyp too

ntp_tiles is already in components.gyp--we created the component earlier with
some flags and things, but didn't yet have tests.

https://codereview.chromium.org/1983063002/diff/260001/components/ntp_tiles.gypi
File components/ntp_tiles.gypi (right):

https://codereview.chromium.org/1983063002/diff/260001/components/ntp_tiles.g...
components/ntp_tiles.gypi:7: ['OS != "ios"', {
On 2016/05/27 13:12:54, blundell wrote:
> You might as well build this on iOS now that it's buildable on iOS :).

Sure, done.

https://codereview.chromium.org/1983063002/diff/260001/components/ntp_tiles.g...
components/ntp_tiles.gypi:18: '../content/content.gyp:content_browser',
On 2016/05/27 13:12:54, blundell wrote:
> kill this now

Done.

[commit-bot: I haz the power, 2016-05-27 13:24:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/280001

[commit-bot: I haz the power, 2016-05-27 13:46:04]

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

[commit-bot: I haz the power, 2016-05-27 13:46:06]

Dry run: Try jobs failed on following builders:
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[blundell, 2016-05-27 14:10:06]

On 2016/05/27 13:46:06, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   ios-simulator on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

Oof, this uses the safe_json component, which doesn't work on iOS. Sylvain, how
has this been solved in previous cases?

[Marc Treib, 2016-05-27 14:14:19]

On 2016/05/27 14:10:06, blundell (OOO until June 2) wrote:
> On 2016/05/27 13:46:06, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   ios-simulator on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
> 
> Oof, this uses the safe_json component, which doesn't work on iOS. Sylvain,
how
> has this been solved in previous cases?

For ntp_snippets::NTPSnippetsFetcher, we pass in a "ParseJSONCallback", which on
non-iOS forwards to safe_json, while on iOS is implemented in terms of
base::JSONReader IIRC.
I guess the same would work here.

[blundell, 2016-05-27 14:15:25]

On 2016/05/27 14:14:19, Marc Treib wrote:
> On 2016/05/27 14:10:06, blundell (OOO until June 2) wrote:
> > On 2016/05/27 13:46:06, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   ios-simulator on tryserver.chromium.mac (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
> > 
> > Oof, this uses the safe_json component, which doesn't work on iOS. Sylvain,
> how
> > has this been solved in previous cases?
> 
> For ntp_snippets::NTPSnippetsFetcher, we pass in a "ParseJSONCallback", which
on
> non-iOS forwards to safe_json, while on iOS is implemented in terms of
> base::JSONReader IIRC.
> I guess the same would work here.

Ah right, that's the previous case I'm recalling. SGTM.

[sfiera, 2016-05-27 14:25:22]

On 2016/05/27 14:15:25, blundell (OOO until June 2) wrote:
> On 2016/05/27 14:14:19, Marc Treib wrote:
> > On 2016/05/27 14:10:06, blundell (OOO until June 2) wrote:
> > > On 2016/05/27 13:46:06, commit-bot: I haz the power wrote:
> > > > Dry run: Try jobs failed on following builders:
> > > >   ios-simulator on tryserver.chromium.mac (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
> > > 
> > > Oof, this uses the safe_json component, which doesn't work on iOS.
Sylvain,
> > how
> > > has this been solved in previous cases?
> > 
> > For ntp_snippets::NTPSnippetsFetcher, we pass in a "ParseJSONCallback",
which
> on
> > non-iOS forwards to safe_json, while on iOS is implemented in terms of
> > base::JSONReader IIRC.
> > I guess the same would work here.
> 
> Ah right, that's the previous case I'm recalling. SGTM.

Wouldn't it make more sense for safe_json to implement that fallback itself? Of
course, we would be parsing JSON less safely on iOS, but right now as a
prospective user, I can choose:

1. Write my own fallback code to parse JSON less safely on iOS
2. Use the less-safe but cross-platform ParseJSONCallback
3. Don't parse JSON on iOS

#1 and #2 help no one and #3 seems pretty unlikely.

[Bernhard Bauer, 2016-05-27 15:38:18]

On 2016/05/27 14:25:22, sfiera wrote:
> On 2016/05/27 14:15:25, blundell (OOO until June 2) wrote:
> > On 2016/05/27 14:14:19, Marc Treib wrote:
> > > On 2016/05/27 14:10:06, blundell (OOO until June 2) wrote:
> > > > On 2016/05/27 13:46:06, commit-bot: I haz the power wrote:
> > > > > Dry run: Try jobs failed on following builders:
> > > > >   ios-simulator on tryserver.chromium.mac (JOB_FAILED,
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
> > > > 
> > > > Oof, this uses the safe_json component, which doesn't work on iOS.
> Sylvain,
> > > how
> > > > has this been solved in previous cases?
> > > 
> > > For ntp_snippets::NTPSnippetsFetcher, we pass in a "ParseJSONCallback",
> which
> > on
> > > non-iOS forwards to safe_json, while on iOS is implemented in terms of
> > > base::JSONReader IIRC.
> > > I guess the same would work here.
> > 
> > Ah right, that's the previous case I'm recalling. SGTM.
> 
> Wouldn't it make more sense for safe_json to implement that fallback itself?
Of
> course, we would be parsing JSON less safely on iOS, but right now as a
> prospective user, I can choose:
> 
> 1. Write my own fallback code to parse JSON less safely on iOS
> 2. Use the less-safe but cross-platform ParseJSONCallback
> 3. Don't parse JSON on iOS
> 
> #1 and #2 help no one and #3 seems pretty unlikely.

Well, we're always parsing JSON unsafely on iOS, but at least this way people
have to be aware of it.

[sfiera, 2016-05-27 15:59:50]

Well, regardless of whether or how the lack of safe_json on iOS guides developer
behavior, I don't think it makes sense to factor it out here. At this point, we
might switch to gRPC and protos before ntp_tiles is used on iOS.

[sfiera, 2016-05-30 07:50:28]

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

[commit-bot: I haz the power, 2016-05-30 07:50:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983063002/300001

[commit-bot: I haz the power, 2016-05-30 09:01:30]

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

[commit-bot: I haz the power, 2016-05-30 09:01:32]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-06-02 08:00:52]

On 2016/05/27 15:59:50, sfiera wrote:
> Well, regardless of whether or how the lack of safe_json on iOS guides
developer
> behavior, I don't think it makes sense to factor it out here. At this point,
we
> might switch to gRPC and protos before ntp_tiles is used on iOS.

I don't agree with this reasoning. The ntp_snippets component already has the
precedent of using ParseJSONCallback, which parses safely on all platforms but
iOS (I'm not sure what you meant by "Use the less-safe but cross-platform
ParseJSONCallback"; this approach is just as safe as using the safe_json
component directly on all platforms for which the safe_json component can be
used). Dealing with the difference in how to parse JSON is an unfortunate but
necessary consequence of writing components that are used on iOS, which the
ntp_tiles component is being regarded as as already discussed.

We had discussions in the past on integrating the difference into the safe_json
component; IIRC one of the owners of the component (bauerb@ perhaps?) made the
counter-argument that it's misleading to have unsafe JSON parsing within a
component called safe_json. That argument made sense to me then and makes sense
to me now. It looks like this will be the third instance of this pattern in the
codebase. If more instances creep in I could imagine creating a wrapper
component to avoid the code duplication. I don't think that it's worth it at
this point though, especially given that the tradeoff is that it would then be
confusing for *everyone* who wants JSON parsing whether they should use the
safe_json component or the cross_platform_json_parser (or whatever) component.

[sfiera, 2016-06-02 08:45:10]

On 2016/06/02 08:00:52, blundell wrote:
> On 2016/05/27 15:59:50, sfiera wrote:
> > Well, regardless of whether or how the lack of safe_json on iOS guides
> developer
> > behavior, I don't think it makes sense to factor it out here. At this point,
> we
> > might switch to gRPC and protos before ntp_tiles is used on iOS.
> 
> I don't agree with this reasoning. The ntp_snippets component already has the
> precedent of using ParseJSONCallback, which parses safely on all platforms but
> iOS (I'm not sure what you meant by "Use the less-safe but cross-platform
> ParseJSONCallback"; this approach is just as safe as using the safe_json
> component directly on all platforms for which the safe_json component can be
> used). Dealing with the difference in how to parse JSON is an unfortunate but
> necessary consequence of writing components that are used on iOS, which the
> ntp_tiles component is being regarded as as already discussed.

Oops, I meant to say "2. Use the less-safe but cross-platform
base::JSONReader::Read". ParseJsonCallback was option #1.

And, we talked to some iOS people and they had some interest in using this
sooner than later. I'll be changing the classes to take a ParseJsonCallback
right away (in other CLs).

> We had discussions in the past on integrating the difference into the
safe_json
> component; IIRC one of the owners of the component (bauerb@ perhaps?) made the
> counter-argument that it's misleading to have unsafe JSON parsing within a
> component called safe_json. That argument made sense to me then and makes
sense
> to me now. It looks like this will be the third instance of this pattern in
the
> codebase. If more instances creep in I could imagine creating a wrapper
> component to avoid the code duplication. I don't think that it's worth it at
> this point though, especially given that the tradeoff is that it would then be
> confusing for *everyone* who wants JSON parsing whether they should use the
> safe_json component or the cross_platform_json_parser (or whatever) component.

I would assume that the wrapper component (untrusted_json?) would replace
safe_json at that point. I could imagine untrusted_json having a SAFE_ONLY
option (or some other way to make iOS not work), but I doubt I would ever use
it.

[blundell, 2016-06-02 08:49:55]

On 2016/06/02 08:45:10, sfiera wrote:
> On 2016/06/02 08:00:52, blundell wrote:
> > On 2016/05/27 15:59:50, sfiera wrote:
> > > Well, regardless of whether or how the lack of safe_json on iOS guides
> > developer
> > > behavior, I don't think it makes sense to factor it out here. At this
point,
> > we
> > > might switch to gRPC and protos before ntp_tiles is used on iOS.
> > 
> > I don't agree with this reasoning. The ntp_snippets component already has
the
> > precedent of using ParseJSONCallback, which parses safely on all platforms
but
> > iOS (I'm not sure what you meant by "Use the less-safe but cross-platform
> > ParseJSONCallback"; this approach is just as safe as using the safe_json
> > component directly on all platforms for which the safe_json component can be
> > used). Dealing with the difference in how to parse JSON is an unfortunate
but
> > necessary consequence of writing components that are used on iOS, which the
> > ntp_tiles component is being regarded as as already discussed.
> 
> Oops, I meant to say "2. Use the less-safe but cross-platform
> base::JSONReader::Read". ParseJsonCallback was option #1.

Gotcha, makes sense.

> 
> And, we talked to some iOS people and they had some interest in using this
> sooner than later. I'll be changing the classes to take a ParseJsonCallback
> right away (in other CLs).
> 

SGTM.

> > We had discussions in the past on integrating the difference into the
> safe_json
> > component; IIRC one of the owners of the component (bauerb@ perhaps?) made
the
> > counter-argument that it's misleading to have unsafe JSON parsing within a
> > component called safe_json. That argument made sense to me then and makes
> sense
> > to me now. It looks like this will be the third instance of this pattern in
> the
> > codebase. If more instances creep in I could imagine creating a wrapper
> > component to avoid the code duplication. I don't think that it's worth it at
> > this point though, especially given that the tradeoff is that it would then
be
> > confusing for *everyone* who wants JSON parsing whether they should use the
> > safe_json component or the cross_platform_json_parser (or whatever)
component.
> 
> I would assume that the wrapper component (untrusted_json?) would replace
> safe_json at that point. I could imagine untrusted_json having a SAFE_ONLY
> option (or some other way to make iOS not work), but I doubt I would ever use
> it.

If we were to change the safe_json component to work transparently on iOS,
change its name to something like untrusted_json, and have a README file in
there that explains that it processes untrusted JSON safely to the extent that
the platform makes that possible, that seems reasonable to me. It would
ultimately be up to the owners of the component, however.

[sfiera, 2016-06-06 16:58:21]

I've worked around safe_json and http://crrev.com/2045563002 is out for review.

[sfiera, 2016-06-06 17:05:08]

However, that just exposed another issue, because MostVisitedSites still uses
SkImage and JPEGCodec, which isn't available on iOS. +markusheintz is actively
working on moving to gfx::Image.

Once that's done, this will work on iOS, or maybe it will just uncover another
issue--it's difficult to update the existing code, because until we finish the
move, it lives in the android tree, and we can't easily test iOS compilation.
I'd really like to go ahead with the move, because it's going to be easiest to
get everything working on iOS once it's in a component.

(somehow, my previous message was sent before I finished)

[Marc Treib, 2016-06-07 09:11:58]

On 2016/06/06 17:05:08, sfiera wrote:
> However, that just exposed another issue, because MostVisitedSites still uses
> SkImage and JPEGCodec, which isn't available on iOS. +markusheintz is actively
> working on moving to gfx::Image.
> 
> Once that's done, this will work on iOS, or maybe it will just uncover another
> issue--it's difficult to update the existing code, because until we finish the
> move, it lives in the android tree, and we can't easily test iOS compilation.
> I'd really like to go ahead with the move, because it's going to be easiest to
> get everything working on iOS once it's in a component.

+1 - we're currently adding dead ios code in the android tree
(http://crrev.com/2045563002), which (a) is just wrong, and (b) is causing pain
during development.
Let's just move this into the component now without ios support, and then figure
out any remaining ios troubles in-place.

[blundell, 2016-06-07 14:12:59]

On 2016/06/07 09:11:58, Marc Treib wrote:
> On 2016/06/06 17:05:08, sfiera wrote:
> > However, that just exposed another issue, because MostVisitedSites still
uses
> > SkImage and JPEGCodec, which isn't available on iOS. +markusheintz is
actively
> > working on moving to gfx::Image.
> > 
> > Once that's done, this will work on iOS, or maybe it will just uncover
another
> > issue--it's difficult to update the existing code, because until we finish
the
> > move, it lives in the android tree, and we can't easily test iOS
compilation.
> > I'd really like to go ahead with the move, because it's going to be easiest
to
> > get everything working on iOS once it's in a component.
> 
> +1 - we're currently adding dead ios code in the android tree
> (http://crrev.com/2045563002), which (a) is just wrong, and (b) is causing
pain
> during development.
> Let's just move this into the component now without ios support, and then
figure
> out any remaining ios troubles in-place.

SGTM. File a bug noting the desire to bring it up on iOS and the known
challenges to doing so, and add a TODO referencing the bug where it's built on
!iOS?

In any case the work you've done so far is that much less work that will have to
be done.

[sfiera, 2016-06-07 14:48:58]

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

[commit-bot: I haz the power, 2016-06-07 14:49:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/460001

[sfiera, 2016-06-07 15:29:16]

Done (crbug.com/617966).

[sfiera, 2016-06-08 07:53:05]

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

[commit-bot: I haz the power, 2016-06-08 07:53:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/500001

[commit-bot: I haz the power, 2016-06-08 08:55:46]

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

[commit-bot: I haz the power, 2016-06-08 08:55:48]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-06-08 11:46:33]

lgtm

https://codereview.chromium.org/1983063002/diff/500001/components/ntp_tiles.gypi
File components/ntp_tiles.gypi (right):

https://codereview.chromium.org/1983063002/diff/500001/components/ntp_tiles.g...
components/ntp_tiles.gypi:7: ['OS != "ios"', {
nit: I would just only build this in the !iOS section in components.gyp instead
of having the conditional here.

https://codereview.chromium.org/1983063002/diff/500001/components/ntp_tiles/B...
File components/ntp_tiles/BUILD.gn (right):

https://codereview.chromium.org/1983063002/diff/500001/components/ntp_tiles/B...
components/ntp_tiles/BUILD.gn:33: "//components/safe_json",
nit: you don't need this dependency (or DEPS allowance) anymore, do you?

[sfiera, 2016-06-08 13:06:33]

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

[commit-bot: I haz the power, 2016-06-08 13:06:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/540001

[sfiera, 2016-06-08 13:07:06]

+pauljensen for net DEP

https://codereview.chromium.org/1983063002/diff/500001/components/ntp_tiles.gypi
File components/ntp_tiles.gypi (right):

https://codereview.chromium.org/1983063002/diff/500001/components/ntp_tiles.g...
components/ntp_tiles.gypi:7: ['OS != "ios"', {
On 2016/06/08 11:46:32, blundell wrote:
> nit: I would just only build this in the !iOS section in components.gyp
instead
> of having the conditional here.

Done.

https://codereview.chromium.org/1983063002/diff/500001/components/ntp_tiles/B...
File components/ntp_tiles/BUILD.gn (right):

https://codereview.chromium.org/1983063002/diff/500001/components/ntp_tiles/B...
components/ntp_tiles/BUILD.gn:33: "//components/safe_json",
On 2016/06/08 11:46:32, blundell wrote:
> nit: you don't need this dependency (or DEPS allowance) anymore, do you?

Still needed on non-iOS (c.f. crrev.com/2041353002, which moves it to a
conditional)

[blundell, 2016-06-08 13:59:32]

Comment is not for this CL obviously.

https://codereview.chromium.org/1983063002/diff/560001/components/ntp_tiles/p...
File components/ntp_tiles/popular_sites.cc (right):

https://codereview.chromium.org/1983063002/diff/560001/components/ntp_tiles/p...
components/ntp_tiles/popular_sites.cc:178: using UntrustedJsonParser =
JsonUnsafeParser;
This seems unfortunately hacky :(. Why not just pass in a ParseJson() callback
that the embedder provides?

[sfiera, 2016-06-09 14:42:41]

https://codereview.chromium.org/1983063002/diff/560001/components/ntp_tiles/p...
File components/ntp_tiles/popular_sites.cc (right):

https://codereview.chromium.org/1983063002/diff/560001/components/ntp_tiles/p...
components/ntp_tiles/popular_sites.cc:178: using UntrustedJsonParser =
JsonUnsafeParser;
On 2016/06/08 13:59:32, blundell wrote:
> This seems unfortunately hacky :(. Why not just pass in a ParseJson() callback
> that the embedder provides?

What do you find hacky about it? Our problem is that we want to use a different
JSON parser on iOS, and this seems like a straightforward way of expressing
that.

[blundell, 2016-06-09 14:51:09]

On 2016/06/09 14:42:41, sfiera wrote:
>
https://codereview.chromium.org/1983063002/diff/560001/components/ntp_tiles/p...
> File components/ntp_tiles/popular_sites.cc (right):
> 
>
https://codereview.chromium.org/1983063002/diff/560001/components/ntp_tiles/p...
> components/ntp_tiles/popular_sites.cc:178: using UntrustedJsonParser =
> JsonUnsafeParser;
> On 2016/06/08 13:59:32, blundell wrote:
> > This seems unfortunately hacky :(. Why not just pass in a ParseJson()
callback
> > that the embedder provides?
> 
> What do you find hacky about it? Our problem is that we want to use a
different
> JSON parser on iOS, and this seems like a straightforward way of expressing
> that.

The platform-specific ifdef'ing. Components exist to be used by different
embedders, i.e., they are modular by definition. Given that, it would be more
natural to express this difference via delegation to the embedder (i.e., by
having the embedder pass in the callback).

[sdefresne, 2016-06-09 15:53:33]

On 2016/06/09 14:51:09, blundell wrote:
> On 2016/06/09 14:42:41, sfiera wrote:
> >
>
https://codereview.chromium.org/1983063002/diff/560001/components/ntp_tiles/p...
> > File components/ntp_tiles/popular_sites.cc (right):
> > 
> >
>
https://codereview.chromium.org/1983063002/diff/560001/components/ntp_tiles/p...
> > components/ntp_tiles/popular_sites.cc:178: using UntrustedJsonParser =
> > JsonUnsafeParser;
> > On 2016/06/08 13:59:32, blundell wrote:
> > > This seems unfortunately hacky :(. Why not just pass in a ParseJson()
> callback
> > > that the embedder provides?
> > 
> > What do you find hacky about it? Our problem is that we want to use a
> different
> > JSON parser on iOS, and this seems like a straightforward way of expressing
> > that.
> 
> The platform-specific ifdef'ing. Components exist to be used by different
> embedders, i.e., they are modular by definition. Given that, it would be more
> natural to express this difference via delegation to the embedder (i.e., by
> having the embedder pass in the callback).

I agree with Colin here. This is how it is done by the other components, and not
hard to do.

Injecting the callback in the component make incorrect dependencies harder to
inadvertently add on iOS as we can use DEPS to forbid the component to depend on
//components/safe_json. Usually it is better to avoid OS or embedder-specific
conditional dependencies in component code (as it make it easier to reason
about).

[sfiera, 2016-06-13 11:22:23]

sfiera@chromium.org changed reviewers:
+ pauljensen@chromium.org

[sfiera, 2016-06-13 11:22:25]

+pauljensen for net in DEPS. Code already uses net; it's just moving locations.

[pauljensen, 2016-06-13 11:44:54]

On 2016/06/13 11:22:25, sfiera wrote:
> +pauljensen for net in DEPS. Code already uses net; it's just moving
locations.

DEPS lgtm

[sfiera, 2016-06-13 11:50:00]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-06-13 11:50:01]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org, sky@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1983063002/#ps560001
(title: " ")

[sfiera, 2016-06-13 11:50:01]

The CQ bit was unchecked by sfiera@chromium.org

[chromium-reviews, 2016-06-13 11:50:06]

Thanks for the quick response!

On Mon, Jun 13, 2016 at 1:44 PM,  <pauljensen@chromium.org> wrote:
> On 2016/06/13 11:22:25, sfiera wrote:
>> +pauljensen for net in DEPS. Code already uses net; it's just moving
> locations.
>
> DEPS lgtm
>
> https://codereview.chromium.org/1983063002/



-- 
Chris Pickel <sfiera@google.com>
Software Engineer, Google Germany GmbH
Erika-Mann-Straße 33, 80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls sie diese fälschlicherweise
erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes
weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich
bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.

This e-mail is confidential. If you received this communication by
mistake, please don't forward it to anyone else, please erase all
copies and attachments, and please let me know that it has gone to the
wrong person.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sfiera, 2016-06-13 11:52:24]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-06-13 11:52:25]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org, sky@chromium.org, blundell@chromium.org,
pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/1983063002/#ps580001
(title: " ")

[commit-bot: I haz the power, 2016-06-13 11:52:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/580001

[commit-bot: I haz the power, 2016-06-13 11:55:06]

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

[commit-bot: I haz the power, 2016-06-13 11:55:09]

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

[sfiera, 2016-06-13 12:00:57]

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

[commit-bot: I haz the power, 2016-06-13 12:02:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/600001

[commit-bot: I haz the power, 2016-06-13 13:22:04]

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

[commit-bot: I haz the power, 2016-06-13 13:22:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sfiera, 2016-06-13 13:24:53]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-06-13 13:24:54]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sky@chromium.org, treib@chromium.org, pauljensen@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1983063002/#ps600001
(title: "sync")

[commit-bot: I haz the power, 2016-06-13 13:25:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983063002/600001

[mastiz, 2016-06-13 13:33:01]

Description was changed from

==========
Move classes to //components/ntp_tiles.

Move MostVisitedSites and PopularSites; leave behind ChromePopularSites,
which depends on Chrome paths.

In the short term, there are no changes introduced by this CL; the same
code builds as before against the same dependencies. In the future,
multiple platforms' versions of the new tab page will share this part of
their implementation. (Android's NTP currently is all native code; on
other platforms we use HTML)

BUG=603026
==========

to

==========
Move classes to //components/ntp_tiles.

Move MostVisitedSites and PopularSites; leave behind ChromePopularSites,
which depends on Chrome paths.

In the short term, there are no changes introduced by this CL; the same
code builds as before against the same dependencies. In the future,
multiple platforms' versions of the new tab page will share this part of
their implementation. (Android's NTP currently is all native code; on
other platforms we use HTML)

BUG=603026
==========

[commit-bot: I haz the power, 2016-06-13 15:33:53]

Description was changed from

==========
Move classes to //components/ntp_tiles.

Move MostVisitedSites and PopularSites; leave behind ChromePopularSites,
which depends on Chrome paths.

In the short term, there are no changes introduced by this CL; the same
code builds as before against the same dependencies. In the future,
multiple platforms' versions of the new tab page will share this part of
their implementation. (Android's NTP currently is all native code; on
other platforms we use HTML)

BUG=603026
==========

to

==========
Move classes to //components/ntp_tiles.

Move MostVisitedSites and PopularSites; leave behind ChromePopularSites,
which depends on Chrome paths.

In the short term, there are no changes introduced by this CL; the same
code builds as before against the same dependencies. In the future,
multiple platforms' versions of the new tab page will share this part of
their implementation. (Android's NTP currently is all native code; on
other platforms we use HTML)

BUG=603026
==========

[commit-bot: I haz the power, 2016-06-13 15:33:57]

Committed patchset #31 (id:600001)

[commit-bot: I haz the power, 2016-06-13 15:34:12]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-13 15:38:23]

Description was changed from

==========
Move classes to //components/ntp_tiles.

Move MostVisitedSites and PopularSites; leave behind ChromePopularSites,
which depends on Chrome paths.

In the short term, there are no changes introduced by this CL; the same
code builds as before against the same dependencies. In the future,
multiple platforms' versions of the new tab page will share this part of
their implementation. (Android's NTP currently is all native code; on
other platforms we use HTML)

BUG=603026
==========

to

==========
Move classes to //components/ntp_tiles.

Move MostVisitedSites and PopularSites; leave behind ChromePopularSites,
which depends on Chrome paths.

In the short term, there are no changes introduced by this CL; the same
code builds as before against the same dependencies. In the future,
multiple platforms' versions of the new tab page will share this part of
their implementation. (Android's NTP currently is all native code; on
other platforms we use HTML)

BUG=603026

Committed: https://crrev.com/3ff01c0dd7b7b6955dafda763cbc120718dc00ef
Cr-Commit-Position: refs/heads/master@{#399455}
==========

[commit-bot: I haz the power, 2016-06-13 15:38:25]

Patchset 31 (id:??) landed as
https://crrev.com/3ff01c0dd7b7b6955dafda763cbc120718dc00ef
Cr-Commit-Position: refs/heads/master@{#399455}