Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1881043003:
Delete the font cache file. (Closed)
|
Created:
4 years, 8 months ago by Ilya Kulshin Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelete the font cache file.
Now that we switched everyone to the font proxy and removed
the font cache code, we should also remove the font cache
itself.
BUG=602387
Committed: https://crrev.com/8f3ae7bface068710759e90184731e1f1f26a3cb
Cr-Commit-Position: refs/heads/master@{#387678}
Patch Set 1 : #Patch Set 2 : Fix typo in comment #
Total comments: 6
Patch Set 3 : Eliminate a useless "helper" and use PostAfterStartupTask #
Total comments: 1
Patch Set 4 : Fix comment #Messages
Total messages: 19 (10 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Delete the font cache file. BUG=602387 ========== to ========== Delete the font cache file. Now that we switched everyone to the font proxy and removed the font cache code, we should also remove the font cache itself. BUG=602387 ==========
kulshin@chromium.org changed reviewers: + gab@chromium.org
ptal
lgtm w/ comments https://codereview.chromium.org/1881043003/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/1881043003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:210: base::DeleteFile(path, false /*recursive*/); Use kFontCacheSharedSectionName directly here instead of binding the path (otherwise you might as well just Bind DeleteFile(kFontCacheSharedSectionName, false) -- I do prefer having the custom method but without |path| param since it's always constant) https://codereview.chromium.org/1881043003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:210: base::DeleteFile(path, false /*recursive*/); Define static const char kFontCacheSharedSectionName[] = "ChromeDWriteFontCache"; locally in this method instead. https://codereview.chromium.org/1881043003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:327: content::BrowserThread::PostDelayedTask( Use BrowserThread::PostAfterStartupTask() instead of an arbitrary delay.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/1881043003/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/1881043003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:210: base::DeleteFile(path, false /*recursive*/); On 2016/04/14 15:04:29, gab wrote: > Define > > static const char kFontCacheSharedSectionName[] = "ChromeDWriteFontCache"; > > locally in this method instead. Inlined the string constant altogether, since it was used in only one place. https://codereview.chromium.org/1881043003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:210: base::DeleteFile(path, false /*recursive*/); On 2016/04/14 15:04:29, gab wrote: > Use kFontCacheSharedSectionName directly here instead of binding the path > (otherwise you might as well just Bind DeleteFile(kFontCacheSharedSectionName, > false) -- I do prefer having the custom method but without |path| param since > it's always constant) Unfortunately I still need the path, since I need the profile directory. I can't really make this a member since this code runs on a different thread, so I switched to binding DeleteFile directly. I have no objections to switching back to a helper function with a |path| parameter if you prefer that instead. https://codereview.chromium.org/1881043003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:327: content::BrowserThread::PostDelayedTask( On 2016/04/14 15:04:29, gab wrote: > Use BrowserThread::PostAfterStartupTask() instead of an arbitrary delay. Done.
kulshin@chromium.org changed reviewers: + jochen@chromium.org
+jochen@ - could I get an OWNERS approval? Thanks!
lgtm
re-lgtm w/ comment nit https://codereview.chromium.org/1881043003/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/1881043003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_win.cc:316: // TODO(kulshin): delete this function and associated code in 2017. s/delete this function and associated code in..../remove this cleanup code in.../ (i.e. no longer a function and "associated code" is now all local)
The CQ bit was checked by kulshin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1881043003/#ps120001 (title: "Fix comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881043003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881043003/120001
Message was sent while issue was closed.
Description was changed from ========== Delete the font cache file. Now that we switched everyone to the font proxy and removed the font cache code, we should also remove the font cache itself. BUG=602387 ========== to ========== Delete the font cache file. Now that we switched everyone to the font proxy and removed the font cache code, we should also remove the font cache itself. BUG=602387 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Delete the font cache file. Now that we switched everyone to the font proxy and removed the font cache code, we should also remove the font cache itself. BUG=602387 ========== to ========== Delete the font cache file. Now that we switched everyone to the font proxy and removed the font cache code, we should also remove the font cache itself. BUG=602387 Committed: https://crrev.com/8f3ae7bface068710759e90184731e1f1f26a3cb Cr-Commit-Position: refs/heads/master@{#387678} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8f3ae7bface068710759e90184731e1f1f26a3cb Cr-Commit-Position: refs/heads/master@{#387678} |
