Add more features to ntp-tiles-internal

Add more features to ntp-tiles-internal

Let user fetch MostLikely tiles. If that's a no-op, e.g. because the
user is not signed in, print a note to that effect.

Show the Popular Sites JSON file when requested (pretty-printed).

BUG=655622
Review-Url: https://codereview.chromium.org/2572133002
Cr-Commit-Position: refs/heads/master@{#448593}
Committed: https://chromium.googlesource.com/chromium/src/+/65cefe9d0f5cd6a7892476b55e6431a91c05d11b

[sfiera, 2016-12-14 14:52:13]

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

[sfiera, 2016-12-14 14:52:14]

After this, we can probably remove chrome://popular-sites-internals. We're not
far from removing chrome://suggestions either. The one thing that
popular-sites-internals has that we don't is that it exposes the status of a
manually-initiated fetch.

[Marc Treib, 2016-12-14 15:06:27]

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

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:115: MostVisitedSitesSupervisor*
supervisor() const { return supervisor_.get(); }
If these return non-const pointers, then they should probably be non-const
themselves.

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right):

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:27:
std::unique_ptr<std::string> ReadFileToString(const base::FilePath& path) {
IIUC, this is a unique_ptr mostly so that it can be null in case of failure,
right? If so, should it be base::Optional instead?

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:34:
std::string FormatJson(std::string* content) {
const std::string* ?

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:95:
popular_sites_json_ = "";
nit: .clear()

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:158:
suggestions_status_ = "not synced";
What's this message saying?

[sfiera, 2016-12-15 09:57:32]

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

[commit-bot: I haz the power, 2016-12-15 09:57:49]

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

[sfiera, 2016-12-15 09:58:42]

Patchset #4 (id:60001) has been deleted

[sfiera, 2016-12-15 09:59:04]

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

[sfiera, 2016-12-15 09:59:13]

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

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:115: MostVisitedSitesSupervisor*
supervisor() const { return supervisor_.get(); }
On 2016/12/14 15:06:26, Marc Treib wrote:
> If these return non-const pointers, then they should probably be non-const
> themselves.

Made non-const (const would be nice, but SuggestionsService::Fetch…() is
non-const and should be)

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right):

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:27:
std::unique_ptr<std::string> ReadFileToString(const base::FilePath& path) {
On 2016/12/14 15:06:26, Marc Treib wrote:
> IIUC, this is a unique_ptr mostly so that it can be null in case of failure,
> right? If so, should it be base::Optional instead?

Yes, done.

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:34:
std::string FormatJson(std::string* content) {
On 2016/12/14 15:06:26, Marc Treib wrote:
> const std::string* ?

Changed to base::Optional<> as well.

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:95:
popular_sites_json_ = "";
On 2016/12/14 15:06:26, Marc Treib wrote:
> nit: .clear()

Done.

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:158:
suggestions_status_ = "not synced";
On 2016/12/14 15:06:26, Marc Treib wrote:
> What's this message saying?

FetchSuggestionsData() can return false for a number of reasons:
 * Sync is completely disabled
 * Sync hasn't yet initialized
 * History sync is excluded from sync

If it does, then no fetch occurs.

[commit-bot: I haz the power, 2016-12-15 09:59:21]

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

[Marc Treib, 2016-12-15 10:29:17]

lgtm

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right):

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:158:
suggestions_status_ = "not synced";
On 2016/12/15 09:59:13, sfiera wrote:
> On 2016/12/14 15:06:26, Marc Treib wrote:
> > What's this message saying?
> 
> FetchSuggestionsData() can return false for a number of reasons:
>  * Sync is completely disabled
>  * Sync hasn't yet initialized
>  * History sync is excluded from sync
> 
> If it does, then no fetch occurs.

That's not what I would understand from "not synced" :)
"history sync is disabled, or not initalized yet" ?

https://codereview.chromium.org/2572133002/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right):

https://codereview.chromium.org/2572133002/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:30: return
base::Optional<std::string>();
Could this just be "base::nullopt"?

[commit-bot: I haz the power, 2016-12-15 10:42:24]

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

[commit-bot: I haz the power, 2016-12-15 10:42:25]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-01-25 15:54:56]

On 2016/12/15 10:29:17, Marc Treib wrote:
> lgtm
> 
>
https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
> File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
(right):
> 
>
https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
> components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:158:
> suggestions_status_ = "not synced";
> On 2016/12/15 09:59:13, sfiera wrote:
> > On 2016/12/14 15:06:26, Marc Treib wrote:
> > > What's this message saying?
> > 
> > FetchSuggestionsData() can return false for a number of reasons:
> >  * Sync is completely disabled
> >  * Sync hasn't yet initialized
> >  * History sync is excluded from sync
> > 
> > If it does, then no fetch occurs.
> 
> That's not what I would understand from "not synced" :)
> "history sync is disabled, or not initalized yet" ?
> 
>
https://codereview.chromium.org/2572133002/diff/40001/components/ntp_tiles/we...
> File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
(right):
> 
>
https://codereview.chromium.org/2572133002/diff/40001/components/ntp_tiles/we...
> components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:30: return
> base::Optional<std::string>();
> Could this just be "base::nullopt"?

Pinging up this old CL: Is this still relevant and just fell through the cracks,
or is it obsolete?

[sfiera, 2017-01-25 16:07:07]

Still relevant. Put it aside while fixing bugs reported against
ntp-tiles-internals; I think there's still one outstanding.

On Wed, Jan 25, 2017 at 4:54 PM,  <treib@chromium.org> wrote:
> On 2016/12/15 10:29:17, Marc Treib wrote:
>> lgtm
>>
>>
>
https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
>> File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
> (right):
>>
>>
>
https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
>> components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:158:
>> suggestions_status_ = "not synced";
>> On 2016/12/15 09:59:13, sfiera wrote:
>> > On 2016/12/14 15:06:26, Marc Treib wrote:
>> > > What's this message saying?
>> >
>> > FetchSuggestionsData() can return false for a number of reasons:
>> > * Sync is completely disabled
>> > * Sync hasn't yet initialized
>> > * History sync is excluded from sync
>> >
>> > If it does, then no fetch occurs.
>>
>> That's not what I would understand from "not synced" :)
>> "history sync is disabled, or not initalized yet" ?
>>
>>
>
https://codereview.chromium.org/2572133002/diff/40001/components/ntp_tiles/we...
>> File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
> (right):
>>
>>
>
https://codereview.chromium.org/2572133002/diff/40001/components/ntp_tiles/we...
>> components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:30:
>> return
>> base::Optional<std::string>();
>> Could this just be "base::nullopt"?
>
> Pinging up this old CL: Is this still relevant and just fell through the
> cracks,
> or is it obsolete?
>
> https://codereview.chromium.org/2572133002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "New Tab Page development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ntp-dev+unsubscribe@chromium.org.
> To post to this group, send email to ntp-dev@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a11418388124cc005....

-- 
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, 2017-02-02 17:16:44]

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right):

https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:158:
suggestions_status_ = "not synced";
On 2016/12/15 10:29:17, Marc Treib wrote:
> On 2016/12/15 09:59:13, sfiera wrote:
> > On 2016/12/14 15:06:26, Marc Treib wrote:
> > > What's this message saying?
> > 
> > FetchSuggestionsData() can return false for a number of reasons:
> >  * Sync is completely disabled
> >  * Sync hasn't yet initialized
> >  * History sync is excluded from sync
> > 
> > If it does, then no fetch occurs.
> 
> That's not what I would understand from "not synced" :)
> "history sync is disabled, or not initalized yet" ?

Done.

https://codereview.chromium.org/2572133002/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right):

https://codereview.chromium.org/2572133002/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:30: return
base::Optional<std::string>();
On 2016/12/15 10:29:17, Marc Treib wrote:
> Could this just be "base::nullopt"?

Could've, but this went away with today's simpler PopularSites interface.

[sfiera, 2017-02-02 17:16:56]

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

[commit-bot: I haz the power, 2017-02-02 17:17:20]

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

[sfiera, 2017-02-02 17:17:36]

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

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

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

[commit-bot: I haz the power, 2017-02-02 17:24:36]

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

[commit-bot: I haz the power, 2017-02-02 17:24:37]

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

[sfiera, 2017-02-03 10:44:11]

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

[commit-bot: I haz the power, 2017-02-03 10:44:20]

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

[sfiera, 2017-02-03 11:24:13]

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

[commit-bot: I haz the power, 2017-02-03 11:24:21]

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

[sfiera, 2017-02-03 11:24:35]

Also removed the blocking pool, since it's no longer needed to get the JSON with
the newer PopularSites interface.

[commit-bot: I haz the power, 2017-02-03 11:31:19]

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

[commit-bot: I haz the power, 2017-02-03 11:31:20]

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

[sfiera, 2017-02-03 12:17:39]

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

[commit-bot: I haz the power, 2017-02-03 12:19:26]

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

[sfiera, 2017-02-03 12:51:02]

Description was changed from

==========
Add more features to ntp-tiles-internal

Let user fetch MostLikely tiles. If that's a no-op, e.g. because the
user is not signed in, print a note to that effect.

Show the Popular Sites JSON file when requested. Since it's stored on
disk in minified form, pretty-print it before displaying (if possible).

BUG=655622
==========

to

==========
Add more features to ntp-tiles-internal

Let user fetch MostLikely tiles. If that's a no-op, e.g. because the
user is not signed in, print a note to that effect.

Show the Popular Sites JSON file when requested (pretty-printed).

BUG=655622
==========

[commit-bot: I haz the power, 2017-02-03 13:05:21]

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

[commit-bot: I haz the power, 2017-02-03 13:05:22]

Dry run: This issue passed the CQ dry run.

[sfiera, 2017-02-03 13:05:36]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-02-03 13:05:37]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2572133002/#ps180001
(title: "Fix iOS again")

[commit-bot: I haz the power, 2017-02-03 13:14:24]

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-03 13:21:28]

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

[commit-bot: I haz the power, 2017-02-03 13:21:29]

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...)

[sfiera, 2017-02-03 13:55:04]

sfiera@chromium.org changed reviewers:
+ bauerb@chromium.org, eugenebut@chromium.org

[sfiera, 2017-02-03 13:55:05]

+bauerb,eugenebut for client owners.

I also added the ntp-tiles owners to the ntp-tiles and popular-sites internals
pages.

[Marc Treib, 2017-02-03 13:59:40]

Still lgtm

[Eugene But (OOO till 7-30), 2017-02-03 18:59:55]

ios lgtm

[Bernhard Bauer, 2017-02-07 10:18:11]

lgtm

[sfiera, 2017-02-07 10:20:31]

The CQ bit was checked by sfiera@chromium.org

[commit-bot: I haz the power, 2017-02-07 10:20:44]

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-07 11:02:56]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1486462831561530,
"parent_rev": "ea826e787b9356bf03d88dd43045af019270ee7d", "commit_rev":
"65cefe9d0f5cd6a7892476b55e6431a91c05d11b"}

[commit-bot: I haz the power, 2017-02-07 11:03:29]

Description was changed from

==========
Add more features to ntp-tiles-internal

Let user fetch MostLikely tiles. If that's a no-op, e.g. because the
user is not signed in, print a note to that effect.

Show the Popular Sites JSON file when requested (pretty-printed).

BUG=655622
==========

to

==========
Add more features to ntp-tiles-internal

Let user fetch MostLikely tiles. If that's a no-op, e.g. because the
user is not signed in, print a note to that effect.

Show the Popular Sites JSON file when requested (pretty-printed).

BUG=655622

Review-Url: https://codereview.chromium.org/2572133002
Cr-Commit-Position: refs/heads/master@{#448593}
Committed:
https://chromium.googlesource.com/chromium/src/+/65cefe9d0f5cd6a7892476b55e64...
==========

[commit-bot: I haz the power, 2017-02-07 11:03:31]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/65cefe9d0f5cd6a7892476b55e64...