Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1957313003:
Remove PopularSites' dependencies on //chrome/.... (Closed)
DescriptionRemove PopularSites' dependencies on //chrome/....
1. Fetch popular sites directly from PopularSites.
2. Move chrome paths dependency out of PopularSites.
Also, add better error-handling for if PathService::Get() fails. In that
case, immediately bail in PopularSites with failure.
3. Sanitize JSON before writing it to file.
Rather than downloading directly to a temporary file, download to a
string and sanitize it before writing to the file. When reading from a
cached file, don't bother sanitizing it.
BUG=603026
Committed: https://crrev.com/685a987e98a2f1d4ae54fcb526a923602bf9b998
Cr-Commit-Position: refs/heads/master@{#393278}
Patch Set 1 #
Total comments: 21
Patch Set 2 : #Patch Set 3 : Rebase #
Total comments: 12
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 7
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 10
Patch Set 10 : #Patch Set 11 : ChromePopularSites #
Total comments: 2
Patch Set 12 : #
Messages
Total messages: 34 (5 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
The three changes here are separate git commits--easy to split up if you want to look at them separately. #2 will eventually need approval from bauerb@ again for the minor webui change.
Oh yeah, and I was thinking that actually what I should be doing after I finish the components move is writing tests, because we don't have unit test coverage for this and I have no idea if our regression test coverage for it is even good enough.
1 and 2 are fine together. 3 would have deserved its own CL IMO, since it's basically unrelated to the rest. But it's fine to keep it in the same CL now, up to you. General comment: Instead of saving the json to a file, we could also store it in a pref. It's kind of an abuse of prefs, but lots of other places do that kind of thing. And it'd save us a lot of thread hopping here. (Before starting that, I'd get one more opinion though :) ) https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:128: std::unique_ptr<std::vector<PopularSites::Site>> ParseJsonFile( Just ParseJson now? Not a file anymore at this point. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:166: // TODO(sfiera): is there some helper for this somewhere? Not that I'm aware. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:291: ->PostTask(FROM_HERE, base::Bind(callback_, false)); Why are you posting the callback to another thread? That's bound to mess up the client. In fact, why not just Run() it directly? https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:295: is_fallback_ = false; Initialize this in the initializer list above? I dislike uninitialized variables, even if it's only for a very short time here. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:307: void PopularSites::FetchPopularSites(const GURL& url, bool force_download) { Split this into two methods, since the force vs non-force cases are completely different? Or maybe the non-force case should be inlined since it only has one call site (as far as I can see)? https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:320: .get(), IMO these four lines are now repeated often enough to warrant a helper function - WDYT? https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:345: if (base::ReadFileToString(local_path_, &json)) { This runs on the UI thread, right? That thread isn't allowed to do file I/O. Maybe make a helper that does both the FileExists check and (if it exists) the read, and post that to another thread? (Actually, you should hit a DCHECK when you try to do this. Have you run this? Unit tests probably wouldn't catch it, even if we had any...) https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:397: DLOG(WARNING) << "could not write file to " nitty nit: Start with a capital letter https://codereview.chromium.org/1957313003/diff/1/chrome/browser/ui/webui/pop... File chrome/browser/ui/webui/popular_sites_internals_message_handler.cc (right): https://codereview.chromium.org/1957313003/diff/1/chrome/browser/ui/webui/pop... chrome/browser/ui/webui/popular_sites_internals_message_handler.cc:36: base::FilePath GetPopularSitesDirectory() { Can we move this to a common place somewhere? Maybe a new popular_sites_util.h/cc in c/b/android/ntp/ if there isn't a place for it already.
https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:345: if (base::ReadFileToString(local_path_, &json)) { On 2016/05/10 08:32:43, Marc Treib wrote: > This runs on the UI thread, right? That thread isn't allowed to do file I/O. > Maybe make a helper that does both the FileExists check and (if it exists) the > read, and post that to another thread? > (Actually, you should hit a DCHECK when you try to do this. Have you run this? > Unit tests probably wouldn't catch it, even if we had any...) I should have said up front: I have no real idea what is correct w.r.t. threading. I know not to do file IO on the UI thread, but I have no idea how to tell when we're on the UI thread. Does base::PostTaskAndReplyWithResult() run the second callback in a different thread pool than the first? The docs for it and TaskRunner are not helpful on this point. There's a failure in the trybots, but I haven't diagnosed it yet. Certainly could be this; it happens in dbg and not rel.
https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:345: if (base::ReadFileToString(local_path_, &json)) { On 2016/05/10 08:44:51, sfiera wrote: > On 2016/05/10 08:32:43, Marc Treib wrote: > > This runs on the UI thread, right? That thread isn't allowed to do file I/O. > > Maybe make a helper that does both the FileExists check and (if it exists) the > > read, and post that to another thread? > > (Actually, you should hit a DCHECK when you try to do this. Have you run this? > > Unit tests probably wouldn't catch it, even if we had any...) > > I should have said up front: I have no real idea what is correct w.r.t. > threading. I know not to do file IO on the UI thread, but I have no idea how to > tell when we're on the UI thread. Rule of thumb: We're on the UI thread. :D DCHECK_CURRENTLY_ON might help. > Does base::PostTaskAndReplyWithResult() run the second callback in a different > thread pool than the first? The docs for it and TaskRunner are not helpful on > this point. Yes, the second callback is posted back to the original calling thread. I agree that the documentation could be better. It's there somewhere, but we have so many Runners and Loops and helpers and wrappers, and the comments aren't replicated everywhere... For this case: https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner.h... > There's a failure in the trybots, but I haven't diagnosed it yet. Certainly > could be this; it happens in dbg and not rel. All the trybots should have DCHECKs enabled, even the release bots. (In fact, I recommend you do the same, if you usually use release builds - add "dcheck_always_on = true" to your gn args.)
Still trying to reproduce test failures, but figured I might as well flush this buffer. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:128: std::unique_ptr<std::vector<PopularSites::Site>> ParseJsonFile( On 2016/05/10 08:32:43, Marc Treib wrote: > Just ParseJson now? Not a file anymore at this point. Done. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:166: // TODO(sfiera): is there some helper for this somewhere? On 2016/05/10 08:32:43, Marc Treib wrote: > Not that I'm aware. Well. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:291: ->PostTask(FROM_HERE, base::Bind(callback_, false)); On 2016/05/10 08:32:43, Marc Treib wrote: > Why are you posting the callback to another thread? That's bound to mess up the > client. > In fact, why not just Run() it directly? I had thought that it was supposed to run on the blocking pool, but it's not. I've kept the behavior of posting the callback, except now to the UI thread. I want to keep the behavior the same in all cases (run callback after call returns) because changing it only in an exceptional case seems like it could harbor bugs. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:295: is_fallback_ = false; On 2016/05/10 08:32:42, Marc Treib wrote: > Initialize this in the initializer list above? I dislike uninitialized > variables, even if it's only for a very short time here. Done. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:307: void PopularSites::FetchPopularSites(const GURL& url, bool force_download) { On 2016/05/10 08:32:42, Marc Treib wrote: > Split this into two methods, since the force vs non-force cases are completely > different? Or maybe the non-force case should be inlined since it only has one > call site (as far as I can see)? I've inlined it above and kept only the actual fetch. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:320: .get(), On 2016/05/10 08:32:43, Marc Treib wrote: > IMO these four lines are now repeated often enough to warrant a helper function > - WDYT? Did one further. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:345: if (base::ReadFileToString(local_path_, &json)) { On 2016/05/10 09:04:07, Marc Treib wrote: > On 2016/05/10 08:44:51, sfiera wrote: > > On 2016/05/10 08:32:43, Marc Treib wrote: > > > This runs on the UI thread, right? That thread isn't allowed to do file I/O. > > > Maybe make a helper that does both the FileExists check and (if it exists) > the > > > read, and post that to another thread? > > > (Actually, you should hit a DCHECK when you try to do this. Have you run > this? > > > Unit tests probably wouldn't catch it, even if we had any...) > > > > I should have said up front: I have no real idea what is correct w.r.t. > > threading. I know not to do file IO on the UI thread, but I have no idea how > to > > tell when we're on the UI thread. > > Rule of thumb: We're on the UI thread. :D > DCHECK_CURRENTLY_ON might help. Added it to the constructor. Should I add a comment to the header, or is it just assumed? > > Does base::PostTaskAndReplyWithResult() run the second callback in a different > > thread pool than the first? The docs for it and TaskRunner are not helpful on > > this point. > > Yes, the second callback is posted back to the original calling thread. I agree > that the documentation could be better. It's there somewhere, but we have so > many Runners and Loops and helpers and wrappers, and the comments aren't > replicated everywhere... > For this case: > https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner.h... > > > There's a failure in the trybots, but I haven't diagnosed it yet. Certainly > > could be this; it happens in dbg and not rel. > > All the trybots should have DCHECKs enabled, even the release bots. (In fact, I > recommend you do the same, if you usually use release builds - add > "dcheck_always_on = true" to your gn args.) I've redone this file to have proper threading, I think. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:397: DLOG(WARNING) << "could not write file to " On 2016/05/10 08:32:43, Marc Treib wrote: > nitty nit: Start with a capital letter Done.
https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:291: ->PostTask(FROM_HERE, base::Bind(callback_, false)); On 2016/05/11 09:00:24, sfiera wrote: > On 2016/05/10 08:32:43, Marc Treib wrote: > > Why are you posting the callback to another thread? That's bound to mess up > the > > client. > > In fact, why not just Run() it directly? > > I had thought that it was supposed to run on the blocking pool, but it's not. > > I've kept the behavior of posting the callback, except now to the UI thread. I > want to keep the behavior the same in all cases (run callback after call > returns) because changing it only in an exceptional case seems like it could > harbor bugs. Acknowledged. https://codereview.chromium.org/1957313003/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/popular_sites.cc:345: if (base::ReadFileToString(local_path_, &json)) { On 2016/05/11 09:00:24, sfiera wrote: > On 2016/05/10 09:04:07, Marc Treib wrote: > > On 2016/05/10 08:44:51, sfiera wrote: > > > On 2016/05/10 08:32:43, Marc Treib wrote: > > > > This runs on the UI thread, right? That thread isn't allowed to do file > I/O. > > > > Maybe make a helper that does both the FileExists check and (if it exists) > > the > > > > read, and post that to another thread? > > > > (Actually, you should hit a DCHECK when you try to do this. Have you run > > this? > > > > Unit tests probably wouldn't catch it, even if we had any...) > > > > > > I should have said up front: I have no real idea what is correct w.r.t. > > > threading. I know not to do file IO on the UI thread, but I have no idea how > > to > > > tell when we're on the UI thread. > > > > Rule of thumb: We're on the UI thread. :D > > DCHECK_CURRENTLY_ON might help. > > Added it to the constructor. Should I add a comment to the header, or is it just > assumed? Adding a comment to the header is definitely appreciated, especially when this moves to a component. > > > Does base::PostTaskAndReplyWithResult() run the second callback in a > different > > > thread pool than the first? The docs for it and TaskRunner are not helpful > on > > > this point. > > > > Yes, the second callback is posted back to the original calling thread. I > agree > > that the documentation could be better. It's there somewhere, but we have so > > many Runners and Loops and helpers and wrappers, and the comments aren't > > replicated everywhere... > > For this case: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner.h... > > > > > There's a failure in the trybots, but I haven't diagnosed it yet. Certainly > > > could be this; it happens in dbg and not rel. > > > > All the trybots should have DCHECKs enabled, even the release bots. (In fact, > I > > recommend you do the same, if you usually use release builds - add > > "dcheck_always_on = true" to your gn args.) > > I've redone this file to have proper threading, I think. https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:322: url, base::Passed(std::move(file_data)))); There's also base::Owned which takes ownership of a raw pointer, and is arguably a bit more convenient. Not sure which way I'd prefer, it's kinda icky either way. https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:347: // a failed download. This comment is kinda obsolete now. https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:361: FetchPopularSites(url); Add a comment? I guess this usually means the file didn't exist? https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:367: fetcher_.reset(); I think this may be why stuff fails: This deletes source, which you use later. One pattern to solve this is to have a local unique_ptr<URLFetcher> and move fetcher_ in there. https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.h:122: void OnFileWriteDone(const std::string& json, bool success); nit: "OnReadFileDone" vs "OnFileWriteDone" Also, could you reorder all of these callbacks into (roughly) chronological order?
https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:367: fetcher_.reset(); On 2016/05/11 10:20:29, Marc Treib wrote: > I think this may be why stuff fails: This deletes source, which you use later. > One pattern to solve this is to have a local unique_ptr<URLFetcher> and move > fetcher_ in there. Oh yeah, obviously. Now I wonder why it doesn't fail locally... I know there are ways to build like is_asan to check things like this. Are there others and do they generally work on Android?
https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:367: fetcher_.reset(); On 2016/05/11 11:06:02, sfiera wrote: > On 2016/05/11 10:20:29, Marc Treib wrote: > > I think this may be why stuff fails: This deletes source, which you use later. > > One pattern to solve this is to have a local unique_ptr<URLFetcher> and move > > fetcher_ in there. > > Oh yeah, obviously. Now I wonder why it doesn't fail locally... > > I know there are ways to build like is_asan to check things like this. Are there > others and do they generally work on Android? I think there's a few more, maybe "is_msan" or something. It's been a while since I used any of those locally. Not sure if they work on Android. I'd recommend just running the corresponding trybots when you're worried about something. Doing this locally was rather painful.
Ah, right, I see there's a linux_chromium_asan_rel_ng but no android asan target, probably because it wouldn't work. And for now, this code only runs on Android.
Ah, right, I see there's a linux_chromium_asan_rel_ng but no android asan target, probably because it wouldn't work. And for now, this code only runs on Android.
https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:322: url, base::Passed(std::move(file_data)))); On 2016/05/11 10:20:29, Marc Treib wrote: > There's also base::Owned which takes ownership of a raw pointer, and is arguably > a bit more convenient. Not sure which way I'd prefer, it's kinda icky either > way. I like the current way because a reader of OnReadFileDone() doesn't have to wonder about the lifetime of the pointer. https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:347: // a failed download. On 2016/05/11 10:20:29, Marc Treib wrote: > This comment is kinda obsolete now. Done. https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:361: FetchPopularSites(url); On 2016/05/11 10:20:29, Marc Treib wrote: > Add a comment? I guess this usually means the file didn't exist? Done. https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:367: fetcher_.reset(); On 2016/05/11 11:35:28, Marc Treib wrote: > On 2016/05/11 11:06:02, sfiera wrote: > > On 2016/05/11 10:20:29, Marc Treib wrote: > > > I think this may be why stuff fails: This deletes source, which you use > later. > > > One pattern to solve this is to have a local unique_ptr<URLFetcher> and move > > > fetcher_ in there. > > > > Oh yeah, obviously. Now I wonder why it doesn't fail locally... > > > > I know there are ways to build like is_asan to check things like this. Are > there > > others and do they generally work on Android? > > I think there's a few more, maybe "is_msan" or something. It's been a while > since I used any of those locally. Not sure if they work on Android. > I'd recommend just running the corresponding trybots when you're worried about > something. Doing this locally was rather painful. Acknowledged.
https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:322: url, base::Passed(std::move(file_data)))); On 2016/05/11 14:23:53, sfiera wrote: > On 2016/05/11 10:20:29, Marc Treib wrote: > > There's also base::Owned which takes ownership of a raw pointer, and is > arguably > > a bit more convenient. Not sure which way I'd prefer, it's kinda icky either > > way. > > I like the current way because a reader of OnReadFileDone() doesn't have to > wonder about the lifetime of the pointer. Acknowledged. https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:35: base::FilePath GetPopularSitesDirectory(); Could you put this into a namespace? e.g. popular_sites::GetDirectory() (I guess it doesn't really matter for a temporary thing, but it'll have to happen eventually anyway.)
lgtm https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.cc:177: // Run |call| on the thread pool for blocking file operations, then call |done| nit: "blocking operations" really, nothing specific to *file*. https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.cc:361: std::unique_ptr<net::URLFetcher> free_fetcher(fetcher_.release()); nit: I'd do std::move(fetcher_), to make it clearer that ownership is being transferred.
https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.cc:177: // Run |call| on the thread pool for blocking file operations, then call |done| On 2016/05/11 14:56:34, Marc Treib wrote: > nit: "blocking operations" really, nothing specific to *file*. Done. https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.cc:361: std::unique_ptr<net::URLFetcher> free_fetcher(fetcher_.release()); On 2016/05/11 14:56:34, Marc Treib wrote: > nit: I'd do std::move(fetcher_), to make it clearer that ownership is being > transferred. Done. https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:35: base::FilePath GetPopularSitesDirectory(); On 2016/05/11 14:47:25, Marc Treib wrote: > Could you put this into a namespace? e.g. popular_sites::GetDirectory() > > (I guess it doesn't really matter for a temporary thing, but it'll have to > happen eventually anyway.) Stuff will go into ntp_tiles for the component. Is it appropriate to reuse that here outside the component? Seems silly to have a namespace 'popular_sites' when PopularSites won't end up in it.
https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/100001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:35: base::FilePath GetPopularSitesDirectory(); On 2016/05/12 09:01:16, sfiera wrote: > On 2016/05/11 14:47:25, Marc Treib wrote: > > Could you put this into a namespace? e.g. popular_sites::GetDirectory() > > > > (I guess it doesn't really matter for a temporary thing, but it'll have to > > happen eventually anyway.) > > Stuff will go into ntp_tiles for the component. Is it appropriate to reuse that > here outside the component? Seems silly to have a namespace 'popular_sites' when > PopularSites won't end up in it. Good point. ntp_tiles:: SGTM.
sfiera@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb for webui
https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.cc:168: // TODO(sfiera): is there some helper for this somewhere? Yes :) base::ImportantFileWriter. https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.cc:178: // its result on the original thread. The common way to do this is to get a single SequencedTaskRunner from the blocking pool, store it in a member, and then call PostTaskAndReplyWithResult with that. https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:37: base::FilePath GetPopularSitesDirectory(); Wait, where will this eventually end up? In ntp_tiles the implementation can't depend on //chrome either. (And if both this and PopularSites will live in the same layer, there is no reason to separate them.)
https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.cc:168: // TODO(sfiera): is there some helper for this somewhere? On 2016/05/12 12:24:24, Bernhard Bauer wrote: > Yes :) base::ImportantFileWriter. Oh, nice, this is an important file now. Done. (Generally, it seems to me that this is the way all files should be written, unless there's a compelling reason not to) https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.cc:178: // its result on the original thread. On 2016/05/12 12:24:24, Bernhard Bauer wrote: > The common way to do this is to get a single SequencedTaskRunner from the > blocking pool, store it in a member, and then call PostTaskAndReplyWithResult > with that. I grabbed a TaskRunner instead of a SequencedTaskRunner because we should be getting all the synchronization guarantees we need by returning to the UI thread after each call. If Sequenced... is in fact better, say the word, it's a trivial change. https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:37: base::FilePath GetPopularSitesDirectory(); On 2016/05/12 12:24:24, Bernhard Bauer wrote: > Wait, where will this eventually end up? In ntp_tiles the implementation can't > depend on //chrome either. (And if both this and PopularSites will live in the > same layer, there is no reason to separate them.) This function will stay here but the PopularSites class will move.
https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:37: base::FilePath GetPopularSitesDirectory(); On 2016/05/12 14:03:05, sfiera wrote: > On 2016/05/12 12:24:24, Bernhard Bauer wrote: > > Wait, where will this eventually end up? In ntp_tiles the implementation can't > > depend on //chrome either. (And if both this and PopularSites will live in the > > same layer, there is no reason to separate them.) > > This function will stay here but the PopularSites class will move. But ntp_tiles is the name of the component, no? So why is this in a namespace but PopularSites isn't?
https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:37: base::FilePath GetPopularSitesDirectory(); On 2016/05/12 14:28:27, Bernhard Bauer wrote: > On 2016/05/12 14:03:05, sfiera wrote: > > On 2016/05/12 12:24:24, Bernhard Bauer wrote: > > > Wait, where will this eventually end up? In ntp_tiles the implementation > can't > > > depend on //chrome either. (And if both this and PopularSites will live in > the > > > same layer, there is no reason to separate them.) > > > > This function will stay here but the PopularSites class will move. > > But ntp_tiles is the name of the component, no? So why is this in a namespace > but PopularSites isn't? Because PopularSites isn't actually in the component yet, but I insisted on a namespace for the global function ;)
https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:37: base::FilePath GetPopularSitesDirectory(); On 2016/05/12 14:28:27, Bernhard Bauer wrote: > On 2016/05/12 14:03:05, sfiera wrote: > > On 2016/05/12 12:24:24, Bernhard Bauer wrote: > > > Wait, where will this eventually end up? In ntp_tiles the implementation > can't > > > depend on //chrome either. (And if both this and PopularSites will live in > the > > > same layer, there is no reason to separate them.) > > > > This function will stay here but the PopularSites class will move. > > But ntp_tiles is the name of the component, no? So why is this in a namespace > but PopularSites isn't? Short answer: Marc asked for it ;) I had figured that I would move the moving code into the namespace at the time that I move it into the component, but this code will be untouched by that change.
https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:37: base::FilePath GetPopularSitesDirectory(); On 2016/05/12 14:32:23, sfiera wrote: > On 2016/05/12 14:28:27, Bernhard Bauer wrote: > > On 2016/05/12 14:03:05, sfiera wrote: > > > On 2016/05/12 12:24:24, Bernhard Bauer wrote: > > > > Wait, where will this eventually end up? In ntp_tiles the implementation > > can't > > > > depend on //chrome either. (And if both this and PopularSites will live in > > the > > > > same layer, there is no reason to separate them.) > > > > > > This function will stay here but the PopularSites class will move. > > > > But ntp_tiles is the name of the component, no? So why is this in a namespace > > but PopularSites isn't? > > Short answer: Marc asked for it ;) > > I had figured that I would move the moving code into the namespace at the time > that I move it into the component, but this code will be untouched by that > change. OK, but if this will stay outside of the component, ntp_tiles is not the right namespace for it. The common thing would be to use a namespace named after the filename (i.e. popular_sites), or to move it into a purely static class. (The most common thing for that class in turn would be to be named "Chrome" + the name of the thing it embeds, i.e. ChromePopularSites.
On 2016/05/12 14:37:09, Bernhard Bauer wrote: > https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... > File chrome/browser/android/ntp/popular_sites.h (right): > > https://codereview.chromium.org/1957313003/diff/160001/chrome/browser/android... > chrome/browser/android/ntp/popular_sites.h:37: base::FilePath > GetPopularSitesDirectory(); > On 2016/05/12 14:32:23, sfiera wrote: > > On 2016/05/12 14:28:27, Bernhard Bauer wrote: > > > On 2016/05/12 14:03:05, sfiera wrote: > > > > On 2016/05/12 12:24:24, Bernhard Bauer wrote: > > > > > Wait, where will this eventually end up? In ntp_tiles the implementation > > > can't > > > > > depend on //chrome either. (And if both this and PopularSites will live > in > > > the > > > > > same layer, there is no reason to separate them.) > > > > > > > > This function will stay here but the PopularSites class will move. > > > > > > But ntp_tiles is the name of the component, no? So why is this in a > namespace > > > but PopularSites isn't? > > > > Short answer: Marc asked for it ;) > > > > I had figured that I would move the moving code into the namespace at the time > > that I move it into the component, but this code will be untouched by that > > change. > > OK, but if this will stay outside of the component, ntp_tiles is not the right > namespace for it. The common thing would be to use a namespace named after the > filename (i.e. popular_sites), or to move it into a purely static class. (The > most common thing for that class in turn would be to be named "Chrome" + the > name of the thing it embeds, i.e. ChromePopularSites. ChromePopularSites::GetDirectory() it is.
lgtm https://codereview.chromium.org/1957313003/diff/200001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/200001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:35: class ChromePopularSites { Add a TODO to move this to a file chrome_popular_sites? Also, DISALLOW_IMPLICIT_CONSTRUCTORS.
Submitting. https://codereview.chromium.org/1957313003/diff/200001/chrome/browser/android... File chrome/browser/android/ntp/popular_sites.h (right): https://codereview.chromium.org/1957313003/diff/200001/chrome/browser/android... chrome/browser/android/ntp/popular_sites.h:35: class ChromePopularSites { On 2016/05/12 15:56:21, Bernhard Bauer wrote: > Add a TODO to move this to a file chrome_popular_sites? > > Also, DISALLOW_IMPLICIT_CONSTRUCTORS. Done.
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, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1957313003/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1957313003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1957313003/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from
==========
Remove PopularSites' dependencies on //chrome/....
1. Fetch popular sites directly from PopularSites.
2. Move chrome paths dependency out of PopularSites.
Also, add better error-handling for if PathService::Get() fails. In that
case, immediately bail in PopularSites with failure.
3. Sanitize JSON before writing it to file.
Rather than downloading directly to a temporary file, download to a
string and sanitize it before writing to the file. When reading from a
cached file, don't bother sanitizing it.
BUG=603026
==========
to
==========
Remove PopularSites' dependencies on //chrome/....
1. Fetch popular sites directly from PopularSites.
2. Move chrome paths dependency out of PopularSites.
Also, add better error-handling for if PathService::Get() fails. In that
case, immediately bail in PopularSites with failure.
3. Sanitize JSON before writing it to file.
Rather than downloading directly to a temporary file, download to a
string and sanitize it before writing to the file. When reading from a
cached file, don't bother sanitizing it.
BUG=603026
Committed: https://crrev.com/685a987e98a2f1d4ae54fcb526a923602bf9b998
Cr-Commit-Position: refs/heads/master@{#393278}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/685a987e98a2f1d4ae54fcb526a923602bf9b998 Cr-Commit-Position: refs/heads/master@{#393278} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
