Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
125052:
First draft of web resource service; fetches data from a JSON feed ... (Closed)
|
Created:
11 years, 6 months ago by Miranda Callahan Modified:
9 years, 7 months ago CC:
Glen Murphy, chromium-reviews_googlegroups.com, Ben Goodger (Google) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionFirst draft of web resource service; fetches data from a JSON feed
and stores it in user prefs, where it can be used by the new tab page.
BUG = http://crbug.com/13363
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=18766
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=18842
Patch Set 1 #
Total comments: 56
Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 26
Patch Set 8 : '' #
Total comments: 14
Patch Set 9 : '' #
Total comments: 6
Patch Set 10 : Fixed typos in a few web resource files.... #
Messages
Total messages: 15 (0 generated)
Hi, Matt -- I was hoping you would have a little bit of time to look over this first draft of a backend service that caches JSON data in the user's preferences file. I am asking you because I leaned heavily on the extension service, as I discussed with you earlier in the week, and I want to get your thoughts on how I've used and modified your code -- and get a basic sanity check. This code will change significantly -- for one thing, it's now hard-coded to draw from an experimental server, and the final data format(s) haven't been set yet; thus, it won't be checked in in this format, and nits probably aren't necessary -- just big picture items, to make sure I'm on the right path before I get in too deep (am I doing things on the right threads? Leaking memory? Other stupid, big errors?). Thanks for any time you can give me! - Miranda
Couple high level comments. I don't really understand the NTP resource stuff, so forgive any stupid comments there. http://codereview.chromium.org/125052/diff/1/8 File chrome/browser/dom_ui/new_tab_ui.cc (right): http://codereview.chromium.org/125052/diff/1/8#newcode1317 Line 1317: web_resource_cache_ = dom_ui_->GetProfile()->GetPrefs()-> So this will only get a snapshot of the cache at the time the NTP is created, right? Seems like - the NTP on startup will always show nothing, since you start the cache updater after 5 sec - this won't change dynamically if we get new tips after opening the NTP http://codereview.chromium.org/125052/diff/1/7 File chrome/browser/utility_process_host.cc (right): http://codereview.chromium.org/125052/diff/1/7#newcode42 Line 42: if (!StartProcess(resource_dir.DirName())) This gives the child process read/write access to this directory. ExtensionUnpacker needs this to unzip files to that dir. Right now, the WebResourceUnpacker doesn't need file access. Are you planning on needing that in the future? If not, we shouldn't expose any dirs to the child process. http://codereview.chromium.org/125052/diff/1/10 File chrome/browser/web_resource/web_resource_unpacker.h (right): http://codereview.chromium.org/125052/diff/1/10#newcode13 Line 13: class WebResourceUnpacker { kind of a nit at this stage, but "unpacker" doesn't seem like the right terminology here. The ExtensionUnpacker unpacks in the sense of unzipping a package into the filesystem. This is more of a parser. http://codereview.chromium.org/125052/diff/1/13 File chrome/common/pref_names.cc (right): http://codereview.chromium.org/125052/diff/1/13#newcode533 Line 533: const wchar_t kNTPWebResourceCache[] = L"ntp.web_resource_cache"; It seems kind of silly to me to use the preference system as a cache. This stuff is not a preference. It feels like it belongs in some other storage area. I don't really have a better suggestion though. And maybe we abuse the prefs system like this for other stuff already.
http://codereview.chromium.org/125052/diff/1/6 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/125052/diff/1/6#newcode732 Line 732: // on a five-second delay so as not to interfere with startup time. Can we delay it more than 5s? http://codereview.chromium.org/125052/diff/1/8 File chrome/browser/dom_ui/new_tab_ui.cc (right): http://codereview.chromium.org/125052/diff/1/8#newcode295 Line 295: l10n_util::GetString(IDS_NEW_TAB_WEB_RESOURCE_CACHE)); indent 4 spaces http://codereview.chromium.org/125052/diff/1/8#newcode1292 Line 1292: class WebResourcesHandler : public DOMMessageHandler { Would you mind putting this in a separate file? This file is already too big http://codereview.chromium.org/125052/diff/1/8#newcode1325 Line 1325: // "0": { Can this be a List instead of a Dictionary? http://codereview.chromium.org/125052/diff/1/8#newcode1332 Line 1332: // this is where we get our next tip from the cache! This ... http://codereview.chromium.org/125052/diff/1/8#newcode1333 Line 1333: // gather the data from the cache into the list_value, then Gather ... http://codereview.chromium.org/125052/diff/1/8#newcode1339 Line 1339: // Real tips will load in a few seconds (when to display them?) I think this will only be an issue on first run and we probably want a different experience for the first run anyway. http://codereview.chromium.org/125052/diff/1/8#newcode1344 Line 1344: tipone = L"Tips and recommendations to make your browsing smarter!"; Add a TODO(mrc): l10n http://codereview.chromium.org/125052/diff/1/8#newcode1350 Line 1350: DictionaryValue* tipdict = new DictionaryValue(); tip_dict (separate words) http://codereview.chromium.org/125052/diff/1/8#newcode1351 Line 1351: tipdict->SetString(L"snippet",tipone); missing space after comma http://codereview.chromium.org/125052/diff/1/8#newcode1361 Line 1361: // TODO (mrc): register source of resource as one of the prefs! TODO(mrc): no space between http://codereview.chromium.org/125052/diff/1/8#newcode1361 Line 1361: // TODO (mrc): register source of resource as one of the prefs! This is pretty easy to do. Do you mind doing it in this CL? http://codereview.chromium.org/125052/diff/1/3 File chrome/browser/profile.h (right): http://codereview.chromium.org/125052/diff/1/3#newcode24 Line 24: undo this line change http://codereview.chromium.org/125052/diff/1/5 File chrome/browser/utility_process_host.h (right): http://codereview.chromium.org/125052/diff/1/5#newcode46 Line 46: virtual void OnUnpackWebResourceSucceeded( Add descriptions to these 2 http://codereview.chromium.org/125052/diff/1/11 File chrome/browser/web_resource/web_resource_service.cc (right): http://codereview.chromium.org/125052/diff/1/11#newcode26 Line 26: // with startup time. TODO (mrc): save last cache update time in prefs Keep TODO(mrc): on its own line http://codereview.chromium.org/125052/diff/1/11#newcode28 Line 28: void StartAfterDelay(int delay) { Please add unit to name or description http://codereview.chromium.org/125052/diff/1/11#newcode104 Line 104: NewRunnableMethod(this, &UnpackerClient::StartProcessOnIOThread, indent 4 http://codereview.chromium.org/125052/diff/1/11#newcode114 Line 114: // test! TODO(mrc): Test? http://codereview.chromium.org/125052/diff/1/11#newcode140 Line 140: "http://hokiepokie.nyc.corp.google.com:8125/labs/popgadget/world?view=json"; should we find a public URL? http://codereview.chromium.org/125052/diff/1/11#newcode140 Line 140: "http://hokiepokie.nyc.corp.google.com:8125/labs/popgadget/world?view=json"; fix indentation http://codereview.chromium.org/125052/diff/1/11#newcode160 Line 160: prefs_->RegisterStringPref(prefs::kNTPWebResourceCacheUpdate,L"0"); whitespace after comma http://codereview.chromium.org/125052/diff/1/11#newcode190 Line 190: if ((!web_resource_cache_->Get(wr_counter_str, ¤t_wr)) || too many parentheses http://codereview.chromium.org/125052/diff/1/11#newcode220 Line 220: ((ms_since_update > (kCacheUpdateDelay + kStartResourceFetchDelay)) ? here as well http://codereview.chromium.org/125052/diff/1/12 File chrome/browser/web_resource/web_resource_service.h (right): http://codereview.chromium.org/125052/diff/1/12#newcode72 Line 72: static const int kCacheUpdateDelay = 30000; //14400000; 4 * 60 * 60 * 1000 Easier to read and the compile will fold it.
Thanks, Matt and Arv, for your help! http://codereview.chromium.org/125052/diff/1/6 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/125052/diff/1/6#newcode732 Line 732: // on a five-second delay so as not to interfere with startup time. On 2009/06/12 19:22:40, arv wrote: > Can we delay it more than 5s? Sure -- I took the five seconds from google_url_tracker, which uses the same strategy. What sort of time do you think would make sense? http://codereview.chromium.org/125052/diff/1/8 File chrome/browser/dom_ui/new_tab_ui.cc (right): http://codereview.chromium.org/125052/diff/1/8#newcode295 Line 295: l10n_util::GetString(IDS_NEW_TAB_WEB_RESOURCE_CACHE)); On 2009/06/12 19:22:40, arv wrote: > indent 4 spaces Done. http://codereview.chromium.org/125052/diff/1/8#newcode1292 Line 1292: class WebResourcesHandler : public DOMMessageHandler { On 2009/06/12 19:22:40, arv wrote: > Would you mind putting this in a separate file? This file is already too big Sure. My only concern is that most of the file is taken up by a set of DOMMessageHandlers -- seems like taking only WRH out would break the symmetry. Then again, maybe each of these handlers should be in its own file. http://codereview.chromium.org/125052/diff/1/8#newcode1317 Line 1317: web_resource_cache_ = dom_ui_->GetProfile()->GetPrefs()-> Well, what happens is that the NTP will only show nothing (or a "default tip") the very first time Chrome is run -- then the preferences will be loaded, and the NTP will pull from the (possibly stale) prefs cache for the beginning of the next loads. So after the initial Chrome run (where the tip can be something like "Look here for suggestions!", we'll always have a suggestion in the cached prefs to pull from. The other issue, of non-dynamic updating, is something that I think is a "feature, not a bug" -- that is, I don't think that the correct behavior of the NTP would be to change content while the user is looking at it; it should be more like web page content update, that will change next time the user opens the page, or refreshes... http://codereview.chromium.org/125052/diff/1/8#newcode1325 Line 1325: // "0": { On 2009/06/12 19:22:40, arv wrote: > Can this be a List instead of a Dictionary? Yes, but I think that in the end we will have data that's more complex than this -- including not just a snippet, but a title, url, thumbnail link, etc.; so each entry will have a set of data associated with it... http://codereview.chromium.org/125052/diff/1/8#newcode1332 Line 1332: // this is where we get our next tip from the cache! On 2009/06/12 19:22:40, arv wrote: > This ... Done. http://codereview.chromium.org/125052/diff/1/8#newcode1333 Line 1333: // gather the data from the cache into the list_value, then On 2009/06/12 19:22:40, arv wrote: > Gather ... Done. http://codereview.chromium.org/125052/diff/1/8#newcode1339 Line 1339: // Real tips will load in a few seconds (when to display them?) On 2009/06/12 19:22:40, arv wrote: > I think this will only be an issue on first run and we probably want a different > experience for the first run anyway. Yes, definitely -- this is just a placeholder. http://codereview.chromium.org/125052/diff/1/8#newcode1344 Line 1344: tipone = L"Tips and recommendations to make your browsing smarter!"; On 2009/06/12 19:22:40, arv wrote: > Add a TODO(mrc): l10n Thanks! Done. http://codereview.chromium.org/125052/diff/1/8#newcode1350 Line 1350: DictionaryValue* tipdict = new DictionaryValue(); On 2009/06/12 19:22:40, arv wrote: > tip_dict (separate words) Done. http://codereview.chromium.org/125052/diff/1/8#newcode1351 Line 1351: tipdict->SetString(L"snippet",tipone); On 2009/06/12 19:22:40, arv wrote: > missing space after comma Done. http://codereview.chromium.org/125052/diff/1/8#newcode1361 Line 1361: // TODO (mrc): register source of resource as one of the prefs! On 2009/06/12 19:22:40, arv wrote: > TODO(mrc): > > no space between Done. http://codereview.chromium.org/125052/diff/1/8#newcode1361 Line 1361: // TODO (mrc): register source of resource as one of the prefs! On 2009/06/12 19:22:40, arv wrote: > This is pretty easy to do. Do you mind doing it in this CL? Definitely, it will be there. http://codereview.chromium.org/125052/diff/1/3 File chrome/browser/profile.h (right): http://codereview.chromium.org/125052/diff/1/3#newcode24 Line 24: On 2009/06/12 19:22:40, arv wrote: > undo this line change Done. http://codereview.chromium.org/125052/diff/1/7 File chrome/browser/utility_process_host.cc (right): http://codereview.chromium.org/125052/diff/1/7#newcode42 Line 42: if (!StartProcess(resource_dir.DirName())) On 2009/06/12 18:36:02, Matt Perry wrote: > This gives the child process read/write access to this directory. > ExtensionUnpacker needs this to unzip files to that dir. Right now, the > WebResourceUnpacker doesn't need file access. Are you planning on needing that > in the future? If not, we shouldn't expose any dirs to the child process. You're right, this is for the future -- in the next CL, we're going to add in image downloading and saving... http://codereview.chromium.org/125052/diff/1/5 File chrome/browser/utility_process_host.h (right): http://codereview.chromium.org/125052/diff/1/5#newcode46 Line 46: virtual void OnUnpackWebResourceSucceeded( On 2009/06/12 19:22:40, arv wrote: > Add descriptions to these 2 Done. http://codereview.chromium.org/125052/diff/1/11 File chrome/browser/web_resource/web_resource_service.cc (right): http://codereview.chromium.org/125052/diff/1/11#newcode26 Line 26: // with startup time. TODO (mrc): save last cache update time in prefs On 2009/06/12 19:22:40, arv wrote: > Keep TODO(mrc): on its own line Done. http://codereview.chromium.org/125052/diff/1/11#newcode28 Line 28: void StartAfterDelay(int delay) { On 2009/06/12 19:22:40, arv wrote: > Please add unit to name or description Done. http://codereview.chromium.org/125052/diff/1/11#newcode104 Line 104: NewRunnableMethod(this, &UnpackerClient::StartProcessOnIOThread, On 2009/06/12 19:22:40, arv wrote: > indent 4 Done. http://codereview.chromium.org/125052/diff/1/11#newcode114 Line 114: // test! On 2009/06/12 19:22:40, arv wrote: > TODO(mrc): Test? Removed. http://codereview.chromium.org/125052/diff/1/11#newcode140 Line 140: "http://hokiepokie.nyc.corp.google.com:8125/labs/popgadget/world?view=json"; On 2009/06/12 19:22:40, arv wrote: > should we find a public URL? Yes -- we are waiting for the poptart people to check in this code to the public server; they will let me know when it's ready, and I will make the change. Should definitely do this before uploading CL, or this poor guy will be ambushed. http://codereview.chromium.org/125052/diff/1/11#newcode140 Line 140: "http://hokiepokie.nyc.corp.google.com:8125/labs/popgadget/world?view=json"; On 2009/06/12 19:22:40, arv wrote: > fix indentation Done. http://codereview.chromium.org/125052/diff/1/11#newcode160 Line 160: prefs_->RegisterStringPref(prefs::kNTPWebResourceCacheUpdate,L"0"); On 2009/06/12 19:22:40, arv wrote: > whitespace after comma Done. http://codereview.chromium.org/125052/diff/1/11#newcode190 Line 190: if ((!web_resource_cache_->Get(wr_counter_str, ¤t_wr)) || On 2009/06/12 19:22:40, arv wrote: > too many parentheses Done. http://codereview.chromium.org/125052/diff/1/11#newcode220 Line 220: ((ms_since_update > (kCacheUpdateDelay + kStartResourceFetchDelay)) ? On 2009/06/12 19:22:40, arv wrote: > here as well Done. http://codereview.chromium.org/125052/diff/1/12 File chrome/browser/web_resource/web_resource_service.h (right): http://codereview.chromium.org/125052/diff/1/12#newcode72 Line 72: static const int kCacheUpdateDelay = 30000; //14400000; On 2009/06/12 19:22:40, arv wrote: > 4 * 60 * 60 * 1000 > > Easier to read and the compile will fold it. Done. http://codereview.chromium.org/125052/diff/1/10 File chrome/browser/web_resource/web_resource_unpacker.h (right): http://codereview.chromium.org/125052/diff/1/10#newcode13 Line 13: class WebResourceUnpacker { On 2009/06/12 18:36:02, Matt Perry wrote: > kind of a nit at this stage, but "unpacker" doesn't seem like the right > terminology here. The ExtensionUnpacker unpacks in the sense of unzipping a > package into the filesystem. This is more of a parser. You're right; I kept the unpacker name because I think that in the future this will have unpacking duties as well as parsing ones -- we will need to verify images as well as parsing JSON data. Though maybe it should be called something that means both unpacking and parsing, eventually... http://codereview.chromium.org/125052/diff/1/13 File chrome/common/pref_names.cc (right): http://codereview.chromium.org/125052/diff/1/13#newcode533 Line 533: const wchar_t kNTPWebResourceCache[] = L"ntp.web_resource_cache"; On 2009/06/12 18:36:02, Matt Perry wrote: > It seems kind of silly to me to use the preference system as a cache. This > stuff is not a preference. It feels like it belongs in some other storage area. > I don't really have a better suggestion though. And maybe we abuse the prefs > system like this for other stuff already. I agree -- it feels weird. I decided to do it this way because the point was made to me in the beginning, when I wanted to set up a whole separate system for the cache, that it would be quicker to just bolt the cache onto the side of prefs; that way, we could spare extra time for file or DB I/O that would be used for a whole separate system that, in the end, didn't contain all that much data.
Latest version has more tweaks and is, I hope, less of a first draft. Changed some hard-coded settings to make it committable (refers to external as opposed to internal servers), and put the whole change behind a switch (--enable-web-resources).
This looks good to me but please wait on Matt since my C++ skills are still very limited. http://codereview.chromium.org/125052/diff/135/1182 File chrome/browser/dom_ui/web_resource_handler.cc (right): http://codereview.chromium.org/125052/diff/135/1182#newcode19 Line 19: NewCallback(this, &WebResourceHandler::HandleGetCachedWebResource)); indentation for statements that don't fit on one line is 4 spaces http://codereview.chromium.org/125052/diff/135/1191 File chrome/browser/web_resource/web_resource_service.cc (right): http://codereview.chromium.org/125052/diff/135/1191#newcode177 Line 177: // Iterate through the web resource data instances. (Ea ...? http://codereview.chromium.org/125052/diff/135/1191#newcode190 Line 190: wr_dict->GetString(L"snippet", &result_snippet); You should probably check the return value of these. http://codereview.chromium.org/125052/diff/135/1191#newcode207 Line 207: reinterpret_cast<DictionaryValue*>(current_wr); Can't you do a static_cast here? http://codereview.chromium.org/125052/diff/135/1197 File chrome/utility/utility_thread.cc (right): http://codereview.chromium.org/125052/diff/135/1197#newcode48 Line 48: void UtilityThread::OnUnpackWebResource(const std::string& resource_data, Add a new line before this function
http://codereview.chromium.org/125052/diff/135/1188 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/125052/diff/135/1188#newcode781 Line 781: // on a five-second delay so as not to interfere with startup time. Don't mention the delay length in the comment, it will just become stale. http://codereview.chromium.org/125052/diff/135/1182 File chrome/browser/dom_ui/web_resource_handler.cc (right): http://codereview.chromium.org/125052/diff/135/1182#newcode17 Line 17: dom_ui_(dom_ui) { indent with DOM http://codereview.chromium.org/125052/diff/135/1182#newcode48 Line 48: web_resource_cache_->GetDictionary(L"0", &web_resource_value); should we check the return values here? http://codereview.chromium.org/125052/diff/135/1189 File chrome/browser/utility_process_host.cc (right): http://codereview.chromium.org/125052/diff/135/1189#newcode45 Line 45: Send(new UtilityMsg_UnpackWebResource(data, "", resource_dir)); I'm not clear on how resource_dir and json_template will be used here. I would remove them until you actually need them. Also, don't expose a dir to the child process (use sandbox::StartProcess instead of StartProcessWithAccess) until it actually is needed. http://codereview.chromium.org/125052/diff/135/1191 File chrome/browser/web_resource/web_resource_service.cc (right): http://codereview.chromium.org/125052/diff/135/1191#newcode248 Line 248: new UnpackerClient(this, json_data); unnecessary wrap http://codereview.chromium.org/125052/diff/135/1176 File chrome/common/web_resource/web_resource_unpacker.h (right): http://codereview.chromium.org/125052/diff/135/1176#newcode15 Line 15: class WebResourceUnpacker { this class needs some comments http://codereview.chromium.org/125052/diff/135/1197 File chrome/utility/utility_thread.cc (right): http://codereview.chromium.org/125052/diff/135/1197#newcode51 Line 51: // parse json data according to template, and download and verify any capitalize Parse http://codereview.chromium.org/125052/diff/135/1197#newcode56 Line 56: *unpacker.parsed_json())); indent should be 4 here, since it's not a continuation of the Send call, but the call inside it.
Thanks for your review; please take another look. I put many strings into constants, as well as fixing these errors, and I jiggered process calling in utility_process_host.cc. I think that in the near future the web_resource_unpacker is going to need access to a directory, so I just had it pass an empty filepath for now -- the startprocess function checks for this, and calls a process without access to any directories if it sees this dummy filepath. This will change. http://codereview.chromium.org/125052/diff/135/1188 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/125052/diff/135/1188#newcode781 Line 781: // on a five-second delay so as not to interfere with startup time. On 2009/06/17 01:41:15, Matt Perry wrote: > Don't mention the delay length in the comment, it will just become stale. Done. http://codereview.chromium.org/125052/diff/135/1182 File chrome/browser/dom_ui/web_resource_handler.cc (right): http://codereview.chromium.org/125052/diff/135/1182#newcode17 Line 17: dom_ui_(dom_ui) { On 2009/06/17 01:41:15, Matt Perry wrote: > indent with DOM Done. http://codereview.chromium.org/125052/diff/135/1182#newcode19 Line 19: NewCallback(this, &WebResourceHandler::HandleGetCachedWebResource)); On 2009/06/17 01:25:19, arv wrote: > indentation for statements that don't fit on one line is 4 spaces Done. http://codereview.chromium.org/125052/diff/135/1182#newcode48 Line 48: web_resource_cache_->GetDictionary(L"0", &web_resource_value); On 2009/06/17 01:41:15, Matt Perry wrote: > should we check the return values here? Done. I also moved the strings into constants. We will also need to decide what default values make sense. This will change when we finally decide what web resources are going to be used. http://codereview.chromium.org/125052/diff/135/1189 File chrome/browser/utility_process_host.cc (right): http://codereview.chromium.org/125052/diff/135/1189#newcode45 Line 45: Send(new UtilityMsg_UnpackWebResource(data, "", resource_dir)); On 2009/06/17 01:41:15, Matt Perry wrote: > I'm not clear on how resource_dir and json_template will be used here. I would > remove them until you actually need them. Also, don't expose a dir to the child > process (use sandbox::StartProcess instead of StartProcessWithAccess) until it > actually is needed. You're right, this is a relic from the distant future -- removed. http://codereview.chromium.org/125052/diff/135/1191 File chrome/browser/web_resource/web_resource_service.cc (right): http://codereview.chromium.org/125052/diff/135/1191#newcode177 Line 177: // Iterate through the web resource data instances. (Ea On 2009/06/17 01:25:19, arv wrote: > ...? Removed tail. http://codereview.chromium.org/125052/diff/135/1191#newcode190 Line 190: wr_dict->GetString(L"snippet", &result_snippet); On 2009/06/17 01:25:19, arv wrote: > You should probably check the return value of these. Done. http://codereview.chromium.org/125052/diff/135/1191#newcode207 Line 207: reinterpret_cast<DictionaryValue*>(current_wr); On 2009/06/17 01:25:19, arv wrote: > Can't you do a static_cast here? Done. Also changed the reinterpret_cast above to be static. http://codereview.chromium.org/125052/diff/135/1191#newcode248 Line 248: new UnpackerClient(this, json_data); On 2009/06/17 01:41:15, Matt Perry wrote: > unnecessary wrap Done. http://codereview.chromium.org/125052/diff/135/1176 File chrome/common/web_resource/web_resource_unpacker.h (right): http://codereview.chromium.org/125052/diff/135/1176#newcode15 Line 15: class WebResourceUnpacker { On 2009/06/17 01:41:15, Matt Perry wrote: > this class needs some comments Done. http://codereview.chromium.org/125052/diff/135/1197 File chrome/utility/utility_thread.cc (right): http://codereview.chromium.org/125052/diff/135/1197#newcode48 Line 48: void UtilityThread::OnUnpackWebResource(const std::string& resource_data, On 2009/06/17 01:25:19, arv wrote: > Add a new line before this function Done. http://codereview.chromium.org/125052/diff/135/1197#newcode51 Line 51: // parse json data according to template, and download and verify any On 2009/06/17 01:41:15, Matt Perry wrote: > capitalize Parse Done. http://codereview.chromium.org/125052/diff/135/1197#newcode56 Line 56: *unpacker.parsed_json())); On 2009/06/17 01:41:15, Matt Perry wrote: > indent should be 4 here, since it's not a continuation of the Send call, but the > call inside it. Done.
LGTM except for the indentations. Please review the indentation rules in the style guide. http://codereview.chromium.org/125052/diff/1215/1223 File chrome/browser/dom_ui/web_resource_handler.cc (right): http://codereview.chromium.org/125052/diff/1215/1223#newcode20 Line 20: L"New: Suggestion Box!"; fix indentation http://codereview.chromium.org/125052/diff/1215/1223#newcode68 Line 68: if (web_resource_dictionary->GetString( If you line break the conditional and/or linebreak the single statement you should use curly braces. http://codereview.chromium.org/125052/diff/1215/1232 File chrome/browser/web_resource/web_resource_service.cc (right): http://codereview.chromium.org/125052/diff/1215/1232#newcode19 Line 19: L"title"; Doesn't this fit on the previous line?
Almost there, couple small changes. http://codereview.chromium.org/125052/diff/1215/1223 File chrome/browser/dom_ui/web_resource_handler.cc (right): http://codereview.chromium.org/125052/diff/1215/1223#newcode68 Line 68: if (web_resource_dictionary->GetString( Does it make sense for any of these items to be absent? If not, you should verify that they all exist, and do nothing / report an error otherwise. http://codereview.chromium.org/125052/diff/1215/1230 File chrome/browser/utility_process_host.cc (right): http://codereview.chromium.org/125052/diff/1215/1230#newcode42 Line 42: if (!StartProcess(emptypath)) nit: you can just do StartProcess(FilePath()) http://codereview.chromium.org/125052/diff/1215/1228 File chrome/browser/utility_process_host.h (right): http://codereview.chromium.org/125052/diff/1215/1228#newcode84 Line 84: CommandLine InitializeProcessCommandLine(); this doesn't seem to be used http://codereview.chromium.org/125052/diff/1215/1233 File chrome/browser/web_resource/web_resource_service.h (right): http://codereview.chromium.org/125052/diff/1215/1233#newcode34 Line 34: static const std::wstring kWebResourceTitle; google style forbids static/global class types. you should use wchar_t* for these.
Thanks for all your help -- I reviewed the style guidelines, and also fixed VAssistX to stop "helping" me by reformatting my lines (with the wrong indentation) whenever I hit enter. http://codereview.chromium.org/125052/diff/1215/1223 File chrome/browser/dom_ui/web_resource_handler.cc (right): http://codereview.chromium.org/125052/diff/1215/1223#newcode20 Line 20: L"New: Suggestion Box!"; On 2009/06/17 22:03:02, arv wrote: > fix indentation Done. http://codereview.chromium.org/125052/diff/1215/1223#newcode68 Line 68: if (web_resource_dictionary->GetString( On 2009/06/17 22:03:02, arv wrote: > If you line break the conditional and/or linebreak the single statement you > should use curly braces. Done. http://codereview.chromium.org/125052/diff/1215/1223#newcode68 Line 68: if (web_resource_dictionary->GetString( On 2009/06/17 22:19:58, Matt Perry wrote: > Does it make sense for any of these items to be absent? If not, you should > verify that they all exist, and do nothing / report an error otherwise. Fixed. http://codereview.chromium.org/125052/diff/1215/1230 File chrome/browser/utility_process_host.cc (right): http://codereview.chromium.org/125052/diff/1215/1230#newcode42 Line 42: if (!StartProcess(emptypath)) On 2009/06/17 22:19:58, Matt Perry wrote: > nit: you can just do StartProcess(FilePath()) Much better, thanks -- done. http://codereview.chromium.org/125052/diff/1215/1228 File chrome/browser/utility_process_host.h (right): http://codereview.chromium.org/125052/diff/1215/1228#newcode84 Line 84: CommandLine InitializeProcessCommandLine(); On 2009/06/17 22:19:58, Matt Perry wrote: > this doesn't seem to be used Removed. http://codereview.chromium.org/125052/diff/1215/1232 File chrome/browser/web_resource/web_resource_service.cc (right): http://codereview.chromium.org/125052/diff/1215/1232#newcode19 Line 19: L"title"; On 2009/06/17 22:03:02, arv wrote: > Doesn't this fit on the previous line? They all fit on the previous line; sorry, refactored a set of longer names and neglected to pull them back together. Fixed. http://codereview.chromium.org/125052/diff/1215/1233 File chrome/browser/web_resource/web_resource_service.h (right): http://codereview.chromium.org/125052/diff/1215/1233#newcode34 Line 34: static const std::wstring kWebResourceTitle; On 2009/06/17 22:19:58, Matt Perry wrote: > google style forbids static/global class types. you should use wchar_t* for > these. Done.
driveby nits http://codereview.chromium.org/125052/diff/1252/1260 File chrome/browser/dom_ui/web_resource_handler.cc (right): http://codereview.chromium.org/125052/diff/1252/1260#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. 2009? http://codereview.chromium.org/125052/diff/1252/1261 File chrome/browser/dom_ui/web_resource_handler.h (right): http://codereview.chromium.org/125052/diff/1252/1261#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. nit: 2009? http://codereview.chromium.org/125052/diff/1252/1269 File chrome/browser/web_resource/web_resource_service.cc (right): http://codereview.chromium.org/125052/diff/1252/1269#newcode143 Line 143: // L"http://hokiepokie.nyc.corp.google.com:8125/" should this be here?
On 2009/06/19 03:22:43, eroman wrote: > driveby nits > > http://codereview.chromium.org/125052/diff/1252/1260 > File chrome/browser/dom_ui/web_resource_handler.cc (right): > > http://codereview.chromium.org/125052/diff/1252/1260#newcode1 > Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. > 2009? > > http://codereview.chromium.org/125052/diff/1252/1261 > File chrome/browser/dom_ui/web_resource_handler.h (right): > > http://codereview.chromium.org/125052/diff/1252/1261#newcode1 > Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. > nit: 2009? > > http://codereview.chromium.org/125052/diff/1252/1269 > File chrome/browser/web_resource/web_resource_service.cc (right): > > http://codereview.chromium.org/125052/diff/1252/1269#newcode143 > Line 143: // L"http://hokiepokie.nyc.corp.google.com:8125/" > should this be here? Thanks, Eric -- already committed this, so I will fix on another patch.
Fixed these nits -- thanks, Eric. http://codereview.chromium.org/125052/diff/1252/1260 File chrome/browser/dom_ui/web_resource_handler.cc (right): http://codereview.chromium.org/125052/diff/1252/1260#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. On 2009/06/19 03:22:43, eroman wrote: > 2009? Done. http://codereview.chromium.org/125052/diff/1252/1261 File chrome/browser/dom_ui/web_resource_handler.h (right): http://codereview.chromium.org/125052/diff/1252/1261#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. On 2009/06/19 03:22:43, eroman wrote: > nit: 2009? Done. http://codereview.chromium.org/125052/diff/1252/1269 File chrome/browser/web_resource/web_resource_service.cc (right): http://codereview.chromium.org/125052/diff/1252/1269#newcode143 Line 143: // L"http://hokiepokie.nyc.corp.google.com:8125/" On 2009/06/19 03:22:43, eroman wrote: > should this be here? No! Removed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
