Remove metadata cache invalidation from directory_tree.js |
||||||
Issue descriptiondirectory_tree.js causes metadata cache invalidation when collapsing/expanding directories and when in the SubDirectoryItem constructor. Directory tree should only invalidate the metadata cache if the entry or its metadata has been updated, which doesn't happen directly from Directory. We should remove these invalidation parts and keep the UI updated based on the metadata "update" event, using "update" event is tracked here: crbug.com/880187
,
Sep 6
,
Sep 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f57f5f8ee29f223cefee213a86a1542e50f04b8b commit f57f5f8ee29f223cefee213a86a1542e50f04b8b Author: Luciano Pacheco <lucmult@chromium.org> Date: Mon Sep 10 07:39:01 2018 Change directory tree to use Metadata update event Metadata model provides an "update" event which is triggered anytime a metadata item has been updated from the backend. Any UI part that to display latest metadata should listen to it and update the UI accordingly. Change the directory tree to listen to Metadata "update" event to update the "shared" icon. A listener is added for every expanded folder to listen to updates for its sub-folders. When a folder is collapsed its listener is removed. Change |onExpand_| function to fetch all metadata values from |LIST_CONTAINER_METADATA_PREFETCH_PROPERTY_NAMES|, instead of |DIRECTORY_TREE_METADATA_PREFETCH_PROPERTY_NAMES|. Directory tree needs the metadata "can*" to setup permission accordingly in the context menu. This is only fetched for Drive roots, since only Drive implements these metadata. Change |updateSharedStatusIcon| function to use the cached metadata value. This fixes the issue where it would trigger metadata requests to backend too early, because this |updateSharedStatusIcon| is called from SubDirectoryItem constructor. Remove Metadata cache invalidation calls (notifyEntriesChanged and notifyEntriesRemoved) because directory tree doesn't update metadata directly. Fix MockVolumeManager to work with other volumes other than Drive, by extracting the VolumeManager implementation to detect RootType to a common function |getRootTypeFromVolumeType|. Remove constant |DIRECTORY_TREE_METADATA_PREFETCH_PROPERTY_NAMES| since its use has been removed in directory_tree.js. Add MetadataModel |removeEventListener| to be able to remove listener of MetadataModel "update" event. manual tests for Drive "shared" icon. New integration tests for "shared" icon will be added in follow up CL. Bug: 880187 , 880189 Test: Unittest for |insideMyDrive| and |insideDrive| functions and Change-Id: Ibe8320e7b0981d8e9556ebb8a922d7db2f16860c Reviewed-on: https://chromium-review.googlesource.com/1203596 Commit-Queue: Luciano Pacheco <lucmult@chromium.org> Reviewed-by: Noel Gordon <noel@chromium.org> Cr-Commit-Position: refs/heads/master@{#589846} [modify] https://crrev.com/f57f5f8ee29f223cefee213a86a1542e50f04b8b/ui/file_manager/file_manager/background/js/mock_volume_manager.js [modify] https://crrev.com/f57f5f8ee29f223cefee213a86a1542e50f04b8b/ui/file_manager/file_manager/background/js/volume_manager_impl.js [modify] https://crrev.com/f57f5f8ee29f223cefee213a86a1542e50f04b8b/ui/file_manager/file_manager/common/js/volume_manager_common.js [modify] https://crrev.com/f57f5f8ee29f223cefee213a86a1542e50f04b8b/ui/file_manager/file_manager/foreground/js/constants.js [modify] https://crrev.com/f57f5f8ee29f223cefee213a86a1542e50f04b8b/ui/file_manager/file_manager/foreground/js/metadata/metadata_model.js [modify] https://crrev.com/f57f5f8ee29f223cefee213a86a1542e50f04b8b/ui/file_manager/file_manager/foreground/js/ui/directory_tree.js [modify] https://crrev.com/f57f5f8ee29f223cefee213a86a1542e50f04b8b/ui/file_manager/file_manager/foreground/js/ui/directory_tree_unittest.js
,
Sep 11
,
Sep 27
Marking this fixed, I'll track merging this fix in M-70 on crbug.com/880187
,
Sep 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75263a99f0b6cd3b038d1a90f31321588fda779f commit 75263a99f0b6cd3b038d1a90f31321588fda779f Author: Luciano Pacheco <lucmult@chromium.org> Date: Thu Sep 27 20:45:24 2018 Add entriesMap and property names to Metadata cache event Add property names set to update event so event listeners can check if the metadata that they want has been updated in constant time. Add entriesMap to update event so event listeners can check if the relevant entry has been updated in constant time. These will be used in the directory_tree.js to update the 'shared' icon for sub-directories. Bug: 880187 , 880189 Change-Id: I5bb21e8b82b208c8d971e3193840f8b6afb8546e Reviewed-on: https://chromium-review.googlesource.com/1203352 Reviewed-by: Luciano Pacheco <lucmult@chromium.org> Reviewed-by: Noel Gordon <noel@chromium.org> Commit-Queue: Luciano Pacheco <lucmult@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#588758}(cherry picked from commit 25cb4e0cb2bcc9f223dd50cda2a416918222cb7e) Reviewed-on: https://chromium-review.googlesource.com/1249463 Cr-Commit-Position: refs/branch-heads/3538@{#713} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/75263a99f0b6cd3b038d1a90f31321588fda779f/ui/file_manager/file_manager/foreground/js/metadata/metadata_cache_set.js [modify] https://crrev.com/75263a99f0b6cd3b038d1a90f31321588fda779f/ui/file_manager/file_manager/foreground/js/metadata/metadata_cache_set_unittest.js [modify] https://crrev.com/75263a99f0b6cd3b038d1a90f31321588fda779f/ui/file_manager/file_manager/foreground/js/metadata/metadata_model.js
,
Sep 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/08b84980b8291568db24dea726ba7f1d5bd6d0d0 commit 08b84980b8291568db24dea726ba7f1d5bd6d0d0 Author: Luciano Pacheco <lucmult@chromium.org> Date: Thu Sep 27 20:46:20 2018 Change directory tree to use Metadata update event Metadata model provides an "update" event which is triggered anytime a metadata item has been updated from the backend. Any UI part that to display latest metadata should listen to it and update the UI accordingly. Change the directory tree to listen to Metadata "update" event to update the "shared" icon. A listener is added for every expanded folder to listen to updates for its sub-folders. When a folder is collapsed its listener is removed. Change |onExpand_| function to fetch all metadata values from |LIST_CONTAINER_METADATA_PREFETCH_PROPERTY_NAMES|, instead of |DIRECTORY_TREE_METADATA_PREFETCH_PROPERTY_NAMES|. Directory tree needs the metadata "can*" to setup permission accordingly in the context menu. This is only fetched for Drive roots, since only Drive implements these metadata. Change |updateSharedStatusIcon| function to use the cached metadata value. This fixes the issue where it would trigger metadata requests to backend too early, because this |updateSharedStatusIcon| is called from SubDirectoryItem constructor. Remove Metadata cache invalidation calls (notifyEntriesChanged and notifyEntriesRemoved) because directory tree doesn't update metadata directly. Fix MockVolumeManager to work with other volumes other than Drive, by extracting the VolumeManager implementation to detect RootType to a common function |getRootTypeFromVolumeType|. Remove constant |DIRECTORY_TREE_METADATA_PREFETCH_PROPERTY_NAMES| since its use has been removed in directory_tree.js. Add MetadataModel |removeEventListener| to be able to remove listener of MetadataModel "update" event. manual tests for Drive "shared" icon. New integration tests for "shared" icon will be added in follow up CL. Bug: 880187 , 880189 Test: Unittest for |insideMyDrive| and |insideDrive| functions and Change-Id: Ibe8320e7b0981d8e9556ebb8a922d7db2f16860c Reviewed-on: https://chromium-review.googlesource.com/1203596 Commit-Queue: Luciano Pacheco <lucmult@chromium.org> Reviewed-by: Noel Gordon <noel@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#589846}(cherry picked from commit f57f5f8ee29f223cefee213a86a1542e50f04b8b) Reviewed-on: https://chromium-review.googlesource.com/1249464 Reviewed-by: Luciano Pacheco <lucmult@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#714} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/08b84980b8291568db24dea726ba7f1d5bd6d0d0/ui/file_manager/file_manager/background/js/mock_volume_manager.js [modify] https://crrev.com/08b84980b8291568db24dea726ba7f1d5bd6d0d0/ui/file_manager/file_manager/background/js/volume_manager_impl.js [modify] https://crrev.com/08b84980b8291568db24dea726ba7f1d5bd6d0d0/ui/file_manager/file_manager/common/js/volume_manager_common.js [modify] https://crrev.com/08b84980b8291568db24dea726ba7f1d5bd6d0d0/ui/file_manager/file_manager/foreground/js/constants.js [modify] https://crrev.com/08b84980b8291568db24dea726ba7f1d5bd6d0d0/ui/file_manager/file_manager/foreground/js/metadata/metadata_model.js [modify] https://crrev.com/08b84980b8291568db24dea726ba7f1d5bd6d0d0/ui/file_manager/file_manager/foreground/js/ui/directory_tree.js [modify] https://crrev.com/08b84980b8291568db24dea726ba7f1d5bd6d0d0/ui/file_manager/file_manager/foreground/js/ui/directory_tree_unittest.js
,
Sep 27
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 08b84980b8291568db24dea726ba7f1d5bd6d0d0 was merged to refs/branch-heads/3538 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Sep 27
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 75263a99f0b6cd3b038d1a90f31321588fda779f was merged to refs/branch-heads/3538 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required -- |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bugdroid1@chromium.org
, Sep 5