-
Notifications
You must be signed in to change notification settings - Fork 973
Move ledger data:* favicons to external files #11754
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11754 +/- ##
==========================================
- Coverage 55.09% 46.34% -8.75%
==========================================
Files 275 270 -5
Lines 26619 25849 -770
Branches 4288 4116 -172
==========================================
- Hits 14667 11981 -2686
- Misses 11952 13868 +1916
|
@diracdeltas this is really cool. Maybe one suggestion. Now we have synopsis and about synopsis, but synopsis is parent of about synopsis. So what is in about synopsis is for sure in synopsis. If I understand this code correctly we are create all favicons twice. My suggestion would be to generate favicons for synopsis and then just assign this values to about synopsis as well. wdyt? |
In near future we would like to remove this duplicate entries, so that you would only have one synopsis. |
app/sessionStore.js
Outdated
) | ||
fs.writeFile(faviconPath, parsed.data, 'base64', (err) => { | ||
if (err) { | ||
console.error(`Error writing file: ${faviconPath} ${err}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we ok with potentially losing these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc the ledger tries to get the favicon whenever the user visits the page, so the icon would be restored on the next visit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diracdeltas that's not the case. We only get favicon if there is not one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i could add an img onerror in the synopsis table that sets the faviconURL to '' / null
if the file does not exist on disk - would that cause the favicon to be re-fetched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes if you set it to null it will work. More info here: https://github.com/brave/browser-laptop/blob/master/app/browser/api/ledger.js#L1146
app/sessionStore.js
Outdated
? `favicon-${index}-${Date.now()}.${parsed.ext}` | ||
: `favicon-${index}.${parsed.ext}` | ||
) | ||
fs.writeFile(faviconPath, parsed.data, 'base64', (err) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be better to use muon.file.writeImportant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fs.writeFile was easier because it supported the base64 encoding parameter
app/sessionStore.js
Outdated
try { | ||
const basePath = getStoragePath('ledger-favicons') | ||
const fs = require('fs') | ||
if (!fs.existsSync(basePath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use the async versions of these to avoid blocking the UI thread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it needs to be synchnrous because this directory must exist before cleanFavicons
is called, and cleanFavicons
returns immutableData synchronously. I could add a migration flag that indicates whether this directory was created, which would avoid having to do the synchronous mkdir in future runs.
For review status, @bridiver @NejcZdovc do you approve / not approve? @diracdeltas was any action needed on your part after the above comments? |
@bsclifton there's some follow-up enhancements which i'll try to do early this week |
Moving to 0.20.x (as the next 0.19.x release is reserved for Chromium 63) |
f2e02fd
to
fb28e2e
Compare
@NejcZdovc i addressed all review comments. however, i thought of something which is that the |
we have issue up for this option #8537, so I think that we should finally implement it 😃 will take a look and probably add it |
ok, if there's currently no way to clear ledger data from the session store in the UI, i don't think this PR needs to address that. ready for re-review. |
This moves data:* URLs in the ledger appState to external image files, which reduces the size of my session-store-1 from ~25 MB to 5 MB. Fix #11582 Test plan: 1. Copy session-store-1 from a Brave profile that has had payments enabled for a while to your brave-development appData directory. 2. Start the browser and go to about:preferences#payments. The synopsis table should appear fine. 3. Close the browser. 4. In your brave-development appData directory, you should now see a ledger-favicons subdirectory which contains a lot of image files. You should also notice that session-store-1 is now much smaller. 5. Repeat step 2.
fb28e2e
to
e17af72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ this will save a lot of space
Move ledger data:* favicons to external files
Move ledger data:* favicons to external files
Do we have any sample large profiles with this data in, so we can assess impact and clear this change from any perf regressions we're seeing? @diracdeltas @NejcZdovc |
This moves data:* favicon URLs in the ledger appState to external image files during session store cleanup. Reduces the size of my session-store-1 from ~23 MB to 5 MB.
Fix #11582
Test plan:
Submitter Checklist:
git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests