...
Created attachment 453789 [details] Patch
Created attachment 453873 [details] Patch
Comment on attachment 453873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453873&action=review > Source/WebKit/ChangeLog:8 > + Let's do that on storage thread in network process at when the directory is used. Seems we should drop the "at" ?
Comment on attachment 453789 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453789&action=review > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:355 > + FileSystem::setExcludedFromBackup(localStorageDirectory, true); Why? I know this policy is not new, but it’s not obvious why the need for this is different on iOS than on, say, macOS. If that’s not a secret, I think we should mention it here. If it is a secret for some reason, that’s OK then to not mention it here. > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:357 > + FileSystem::setExcludedFromBackup(localStorageDirectory, false); Why? This seems to reverse what the code did before, and it’s not obvious why this needs to be done. After all, most things are not excluded from backups. So why do we need to explicitly do this? Is this because these directories were unintentionally excluded from backups in the past, and we are trying to heal that mistake? If so, that seems like a transitional issue. Not sure we’d want this code forever. But also, why don’t we want this like we have on iOS? If that’s not a secret, I think we should mention it here. If it is a secret for some reason, that’s OK then to not mention it here.
(In reply to Darin Adler from comment #4) > Comment on attachment 453789 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453789&action=review > > > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:355 > > + FileSystem::setExcludedFromBackup(localStorageDirectory, true); > > Why? I know this policy is not new, but it’s not obvious why the need for > this is different on iOS than on, say, macOS. If that’s not a secret, I > think we should mention it here. If it is a secret for some reason, that’s > OK then to not mention it here. I don't know if it's a secret since it's not mentioned in the original commit; the policy is added in rdar://29045531. > > > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:357 > > + FileSystem::setExcludedFromBackup(localStorageDirectory, false); > > Why? This seems to reverse what the code did before, and it’s not obvious > why this needs to be done. After all, most things are not excluded from > backups. So why do we need to explicitly do this? Is this because these > directories were unintentionally excluded from backups in the past, and we > are trying to heal that mistake? If so, that seems like a transitional > issue. Not sure we’d want this code forever. But also, why don’t we want > this like we have on iOS? If that’s not a secret, I think we should mention > it here. If it is a secret for some reason, that’s OK then to not mention it > here. I made a mistake on this patch, by thinking that I excluded the directory on all platforms (we should only do it on iOS). And then I realized that we have not turned off custom directory yet (so m_shouldUseCustomPaths is always true and this code path never hit), so we don't need to unset the attribute. Please check the updated patch for correct solution.
Oh thanks; I didn’t realize I was looking at the wrong patch.
Comment on attachment 453873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453873&action=review > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:366 > +#if PLATFORM(IOS_FAMILY) > + if (!m_resolvedLocalStoragePath.isEmpty()) > + FileSystem::excludeFromBackup(FileSystem::parentPath(m_customLocalStoragePath)); > +#endif This sure is crying out for a why comment. Some day someone should add one.
Comment on attachment 453873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453873&action=review >> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:366 >> +#endif > > This sure is crying out for a why comment. Some day someone should add one. Both why we exclude the directory from backups on iOS and why we do not on macOS.
(In reply to Darin Adler from comment #8) > Comment on attachment 453873 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453873&action=review > > >> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:366 > >> +#endif > > > > This sure is crying out for a why comment. Some day someone should add one. > > Both why we exclude the directory from backups on iOS and why we do not on > macOS. I can add comment like "We only exclude this directory on iOS due to rdar://29045531". Do you think it's sufficient?
(In reply to Sihui Liu from comment #9) > I can add comment like "We only exclude this directory on iOS due to > rdar://29045531". > Do you think it's sufficient? It might be sufficient for people at Apple who can follow that link. But is there a reason we have to use a link to explain and can’t give the explanation instead? And nothing about that comment explains why we do this only for iOS family.
Created attachment 453902 [details] Patch for landing
(In reply to Darin Adler from comment #10) > (In reply to Sihui Liu from comment #9) > > I can add comment like "We only exclude this directory on iOS due to > > rdar://29045531". > > Do you think it's sufficient? > > It might be sufficient for people at Apple who can follow that link. But is > there a reason we have to use a link to explain and can’t give the > explanation instead? And nothing about that comment explains why we do this > only for iOS family. Talked to Geoff and Brady about this: we did this to reduce backup traffic to server; macOS does backup on device so it does not need this. I added comment in code.
Committed r290863 (248094@main): <https://commits.webkit.org/248094@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453902 [details].
<rdar://problem/89850452>