Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2343763002:
Make OnPopularURLsAvailable optional. (Closed)
|
Created:
4 years, 3 months ago by sfiera Modified:
4 years, 3 months ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake OnPopularURLsAvailable optional.
This is used by Android to pre-cache icons for the tiles. But that
should really be done by MostVisitedSites itself. In the meantime, no
point in having iOS implement the method.
BUG=631990
Committed: https://crrev.com/1dfeab55f101d05538c80cb1b3228dfa14f9db9e
Cr-Commit-Position: refs/heads/master@{#420019}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Put in header. #Messages
Total messages: 19 (12 generated)
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sfiera@chromium.org changed reviewers: + treib@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Clearly we'll need the proper implementation of icon loading before we can launch anything on iOS anyway, but LGTM if this makes your live easier in the meantime. https://codereview.chromium.org/2343763002/diff/1/components/ntp_tiles/most_v... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2343763002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites.cc:144: const PopularSitesVector& sites) {} Might as well put the empty body in the header. The dtor already does that.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/15 09:13:33, Marc Treib wrote: > Clearly we'll need the proper implementation of icon loading before we can > launch anything on iOS anyway, but LGTM if this makes your live easier in the > meantime. We can get iOS on MostVisitedSites without launching Popular Sites there, and I think that would be worth doing. https://codereview.chromium.org/2343763002/diff/1/components/ntp_tiles/most_v... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2343763002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites.cc:144: const PopularSitesVector& sites) {} On 2016/09/15 09:13:33, Marc Treib wrote: > Might as well put the empty body in the header. The dtor already does that. Done.
On 2016/09/21 08:35:27, sfiera wrote: > On 2016/09/15 09:13:33, Marc Treib wrote: > > Clearly we'll need the proper implementation of icon loading before we can > > launch anything on iOS anyway, but LGTM if this makes your live easier in the > > meantime. > > We can get iOS on MostVisitedSites without launching Popular Sites there, and I > think that would be worth doing. Good point, I didn't think of that. Carry on :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sfiera@chromium.org
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/2343763002/#ps20001 (title: "Put in header.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make OnPopularURLsAvailable optional. This is used by Android to pre-cache icons for the tiles. But that should really be done by MostVisitedSites itself. In the meantime, no point in having iOS implement the method. BUG=631990 ========== to ========== Make OnPopularURLsAvailable optional. This is used by Android to pre-cache icons for the tiles. But that should really be done by MostVisitedSites itself. In the meantime, no point in having iOS implement the method. BUG=631990 Committed: https://crrev.com/1dfeab55f101d05538c80cb1b3228dfa14f9db9e Cr-Commit-Position: refs/heads/master@{#420019} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1dfeab55f101d05538c80cb1b3228dfa14f9db9e Cr-Commit-Position: refs/heads/master@{#420019} |
