Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567383002:
Add FLAG_CAN_DELETE_ON_CLOSE and DeleteOnClose for use on Windows. (Closed)
|
Created:
4 years ago by grt (UTC plus 2) Modified:
3 years, 11 months ago Reviewers:
Nico CC:
chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd FLAG_CAN_DELETE_ON_CLOSE and DeleteOnClose for use on Windows.
Files opened with FLAG_CAN_DELETE_ON_CLOSE can be marked for
delete-on-close. Due to Windows' limitations, files opened with
FLAG_DELETE_ON_CLOSE cannot have this state cleared.
Review-Url: https://codereview.chromium.org/2567383002
Cr-Commit-Position: refs/heads/master@{#443686}
Committed: https://chromium.googlesource.com/chromium/src/+/37dd92324bb996ba0c96e27f1e54f8d0f511294c
Patch Set 1 #
Total comments: 6
Patch Set 2 : thakis comments #
Total comments: 4
Patch Set 3 : better documentation, more tests #Patch Set 4 : also test that mapping a file blocks DeleteOnClose #Patch Set 5 : clang compile fix #
Messages
Total messages: 29 (20 generated)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
grt@chromium.org changed reviewers: + thakis@chromium.org
PTAL. I will use this in the other related CLs. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I guess I'm reading these in the wrong order. https://codereview.chromium.org/2567383002/diff/1/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/2567383002/diff/1/base/files/file.h#newcode88 base/files/file.h:88: FLAG_DELETE = 1 << 20, // Used on Windows only. Maybe this could describe what this does, and when to use FLAG_DELETE and when FLAG_DELETE_ON_CLOSE and what the difference is? https://codereview.chromium.org/2567383002/diff/1/base/files/file_unittest.cc File base/files/file_unittest.cc (right): https://codereview.chromium.org/2567383002/diff/1/base/files/file_unittest.cc... base/files/file_unittest.cc:554: // Creating a file with DELETE, mraking it for delete, then clearing delete on (typo mraking) https://codereview.chromium.org/2567383002/diff/1/base/files/file_unittest.cc... base/files/file_unittest.cc:563: } what happens if you file.DeleteOnClose(true) on a file that wasn't opened with FLAG_DELETE?
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. https://codereview.chromium.org/2567383002/diff/1/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/2567383002/diff/1/base/files/file.h#newcode88 base/files/file.h:88: FLAG_DELETE = 1 << 20, // Used on Windows only. On 2017/01/10 18:04:15, Nico wrote: > Maybe this could describe what this does, and when to use FLAG_DELETE and when > FLAG_DELETE_ON_CLOSE and what the difference is? Done. https://codereview.chromium.org/2567383002/diff/1/base/files/file_unittest.cc File base/files/file_unittest.cc (right): https://codereview.chromium.org/2567383002/diff/1/base/files/file_unittest.cc... base/files/file_unittest.cc:554: // Creating a file with DELETE, mraking it for delete, then clearing delete on On 2017/01/10 18:04:16, Nico wrote: > (typo mraking) Done. https://codereview.chromium.org/2567383002/diff/1/base/files/file_unittest.cc... base/files/file_unittest.cc:563: } On 2017/01/10 18:04:15, Nico wrote: > what happens if you file.DeleteOnClose(true) on a file that wasn't opened with > FLAG_DELETE? The call to set attributes fails and DOC returns false. I've added a test for this.
Thanks! https://codereview.chromium.org/2567383002/diff/20001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/2567383002/diff/20001/base/files/file.h#newco... base/files/file.h:89: // via DeleteOnClose() (Windows only). It now describe what this does, but not when to use FLAG_DELETE and when FLAG_DELETE_ON_CLOSE https://codereview.chromium.org/2567383002/diff/20001/base/files/file_unittes... File base/files/file_unittest.cc (right): https://codereview.chromium.org/2567383002/diff/20001/base/files/file_unittes... base/files/file_unittest.cc:526: // Creating and closing a file with DELETE perms should do nothing special. The Windows API flag is called DELETE, but maybe a different name might be clearer? Maybe CAN_DELETE_ON_CLOSE, MAYBE_DELETE_ON_CLOSE, DYNAMIC_DELETE_ON_CLOSE or something like that?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add FLAG_DELETE and DeleteOnClose for use on Windows. Files opened with FLAG_DELETE can be marked for delete-on-close. Due to Windows' limitations, files opened with FLAG_DELETE_ON_CLOSE cannot have this state cleared. BUG=571906 ========== to ========== Add FLAG_CAN_DELETE_ON_CLOSE and DeleteOnClose for use on Windows. Files opened with FLAG_CAN_DELETE_ON_CLOSE can be marked for delete-on-close. Due to Windows' limitations, files opened with FLAG_DELETE_ON_CLOSE cannot have this state cleared. ==========
Now with better documentation and more tests! PTAL. https://codereview.chromium.org/2567383002/diff/20001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/2567383002/diff/20001/base/files/file.h#newco... base/files/file.h:89: // via DeleteOnClose() (Windows only). On 2017/01/11 15:47:18, Nico wrote: > It now describe what this does, but not when to use FLAG_DELETE and when > FLAG_DELETE_ON_CLOSE Better comments added to DeleteOnClose. https://codereview.chromium.org/2567383002/diff/20001/base/files/file_unittes... File base/files/file_unittest.cc (right): https://codereview.chromium.org/2567383002/diff/20001/base/files/file_unittes... base/files/file_unittest.cc:526: // Creating and closing a file with DELETE perms should do nothing special. On 2017/01/11 15:47:18, Nico wrote: > The Windows API flag is called DELETE, but maybe a different name might be > clearer? Maybe CAN_DELETE_ON_CLOSE, MAYBE_DELETE_ON_CLOSE, > DYNAMIC_DELETE_ON_CLOSE or something like that? CAN sounds good to me. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm++
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2567383002/#ps60001 (title: "also test that mapping a file blocks DeleteOnClose")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2567383002/#ps80001 (title: "clang compile fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484328272313750,
"parent_rev": "0bfeb6b121c5cfd7658c485dcf2077826bad2f2b", "commit_rev":
"37dd92324bb996ba0c96e27f1e54f8d0f511294c"}
Message was sent while issue was closed.
Description was changed from ========== Add FLAG_CAN_DELETE_ON_CLOSE and DeleteOnClose for use on Windows. Files opened with FLAG_CAN_DELETE_ON_CLOSE can be marked for delete-on-close. Due to Windows' limitations, files opened with FLAG_DELETE_ON_CLOSE cannot have this state cleared. ========== to ========== Add FLAG_CAN_DELETE_ON_CLOSE and DeleteOnClose for use on Windows. Files opened with FLAG_CAN_DELETE_ON_CLOSE can be marked for delete-on-close. Due to Windows' limitations, files opened with FLAG_DELETE_ON_CLOSE cannot have this state cleared. Review-Url: https://codereview.chromium.org/2567383002 Cr-Commit-Position: refs/heads/master@{#443686} Committed: https://chromium.googlesource.com/chromium/src/+/37dd92324bb996ba0c96e27f1e54... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/37dd92324bb996ba0c96e27f1e54... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
