Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2352763006:
On macOS 10.10+, use NSURL methods in quarantine_mac. (Closed)
DescriptionOn macOS 10.10+, use NSURL methods in quarantine_mac.
FSRef has been deprecated since macOS 10.8, but the replacement for
kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old
logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL
adds a test for the new logic.
BUG=650790
Committed: https://crrev.com/1ad622362c0e3aafc09e34eec49079230c3f5d77
Cr-Commit-Position: refs/heads/master@{#421660}
Patch Set 1 #Patch Set 2 : More changes. #Patch Set 3 : clang format. #Patch Set 4 : More comments. #Patch Set 5 : minor fixes. #Patch Set 6 : minor fix. #Patch Set 7 : Rebase. #Patch Set 8 : More rebase errors. #Patch Set 9 : More cleanup. #Patch Set 10 : Rename test. #Patch Set 11 : use static cast. #Patch Set 12 : Add debug logging. #Patch Set 13 : More logging. #Patch Set 14 : more logging. #Patch Set 15 : more debugging. #Patch Set 16 : test #Patch Set 17 : more debugging. #Patch Set 18 : get stacktrace on crash. #Patch Set 19 : Fix test. #Patch Set 20 : add ifdef. #
Total comments: 10
Patch Set 21 : Comments from avi. #Patch Set 22 : Fix logic. #
Messages
Total messages: 61 (54 generated)
Description was changed from ========== Remove deprecated code from file_metadata_mac. BUG= ========== to ========== On macOS 10.10+, use NSURL methods in file_metadata_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=649044 ==========
Description was changed from ========== On macOS 10.10+, use NSURL methods in file_metadata_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=649044 ========== to ========== On macOS 10.10+, use NSURL methods in file_metadata_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=649044 ==========
The CQ bit was checked by erikchen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@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 ========== On macOS 10.10+, use NSURL methods in file_metadata_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=649044 ========== to ========== On macOS 10.10+, use NSURL methods in quarantine_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=649044 ==========
The CQ bit was checked by erikchen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@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...
The CQ bit was checked by erikchen@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...
The CQ bit was checked by erikchen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by erikchen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by erikchen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by erikchen@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...
The CQ bit was checked by erikchen@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 ========== On macOS 10.10+, use NSURL methods in quarantine_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=649044 ========== to ========== On macOS 10.10+, use NSURL methods in quarantine_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=650790 ==========
erikchen@chromium.org changed reviewers: + avi@chromium.org
avi: Please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with helper nits. https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... File content/browser/download/quarantine_mac.mm (right): https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:37: CFTypeRef quarantine_properties_base = NULL; Can you do a ScopedCFTypeRef here and use InitializeInto() below in the call to LSCopyItemAttribute? https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:41: if (CFGetTypeID(quarantine_properties_base) == CFDictionaryGetTypeID()) { CFCast<> https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:45: [(NSDictionary*)quarantine_properties_base mutableCopy]); CFToNSCast https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:51: CFRelease(quarantine_properties_base); (then you wouldn't need this) https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:100: if (![quarantine_properties isKindOfClass:[NSDictionary class]]) { Rather than using an NSDictionary* up there, use an "id", which is what the type is, and ObjCCast to a dictionary here.
asanka@chromium.org changed reviewers: + asanka@chromium.org
Thanks! LGTM stamp from owners.
https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... File content/browser/download/quarantine_mac.mm (right): https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:37: CFTypeRef quarantine_properties_base = NULL; On 2016/09/28 02:32:13, Avi wrote: > Can you do a ScopedCFTypeRef here and use InitializeInto() below in the call to > LSCopyItemAttribute? Done. Note that this was copy-pasted from the previous logic. I wasn't going to update it because it will go away when we drop 10.9 support. *shrug* https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:41: if (CFGetTypeID(quarantine_properties_base) == CFDictionaryGetTypeID()) { On 2016/09/28 02:32:14, Avi wrote: > CFCast<> Done. https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:45: [(NSDictionary*)quarantine_properties_base mutableCopy]); On 2016/09/28 02:32:14, Avi wrote: > CFToNSCast Done. https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:51: CFRelease(quarantine_properties_base); On 2016/09/28 02:32:13, Avi wrote: > (then you wouldn't need this) Done. https://codereview.chromium.org/2352763006/diff/380001/content/browser/downlo... content/browser/download/quarantine_mac.mm:100: if (![quarantine_properties isKindOfClass:[NSDictionary class]]) { On 2016/09/28 02:32:14, Avi wrote: > Rather than using an NSDictionary* up there, use an "id", which is what the type > is, and ObjCCast to a dictionary here. Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2352763006/#ps420001 (title: "Fix logic.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== On macOS 10.10+, use NSURL methods in quarantine_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=650790 ========== to ========== On macOS 10.10+, use NSURL methods in quarantine_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=650790 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== On macOS 10.10+, use NSURL methods in quarantine_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=650790 ========== to ========== On macOS 10.10+, use NSURL methods in quarantine_mac. FSRef has been deprecated since macOS 10.8, but the replacement for kLSItemQuarantineProperties was not available until macOS 10.10+. Keep the old logic for macOS 10.9, but use non-deprecated methods in macOS 10.10+. This CL adds a test for the new logic. BUG=650790 Committed: https://crrev.com/1ad622362c0e3aafc09e34eec49079230c3f5d77 Cr-Commit-Position: refs/heads/master@{#421660} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/1ad622362c0e3aafc09e34eec49079230c3f5d77 Cr-Commit-Position: refs/heads/master@{#421660} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
