fix: ElectronBrowserContext::PartitionKey comparisons (#41055)
* fix: ElectronBrowserContext::PartitionKey comparisons
Use c++20 default comparisons to simplify + fix PartitionKey sorting:
- The equality operator is broken. `PartitionKey{"foo", false}` is both
  equal, to and less than, `PartitionKey{"foo", true}`
- For some keys, the same session can be retrieved via both `fromPath()`
  and `fromPartition()`. This use case was discussed and removed from
  the original PR after code review said "always returning different
  sessions feels lower maintenance." The current behavior is a bug that
  comes from the comparison operators not checking the keys' types.
Xref: https://github.com/electron/electron/pull/36728/commits/3f1aea9af91e17b2605eb8a5837bbb81888d76da#r1099745359
Xref: https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++-features.md#Default-comparisons-allowed
* fixup! fix: ElectronBrowserContext::PartitionKey comparisons
	
	
This commit is contained in:
		
					parent
					
						
							
								1300e83884
							
						
					
				
			
			
				commit
				
					
						1af9612edf
					
				
			
		
					 1 changed files with 13 additions and 31 deletions
				
			
		|  | @ -9,6 +9,7 @@ | ||||||
| #include <memory> | #include <memory> | ||||||
| #include <optional> | #include <optional> | ||||||
| #include <string> | #include <string> | ||||||
|  | #include <string_view> | ||||||
| #include <variant> | #include <variant> | ||||||
| #include <vector> | #include <vector> | ||||||
| 
 | 
 | ||||||
|  | @ -80,41 +81,22 @@ class ElectronBrowserContext : public content::BrowserContext { | ||||||
| 
 | 
 | ||||||
|   // partition_id => browser_context
 |   // partition_id => browser_context
 | ||||||
|   struct PartitionKey { |   struct PartitionKey { | ||||||
|     enum class KeyType { Partition, FilePath }; |     PartitionKey(const std::string_view partition, bool in_memory) | ||||||
|     std::string location; |         : type_{Type::Partition}, location_{partition}, in_memory_{in_memory} {} | ||||||
|     bool in_memory; |  | ||||||
|     KeyType partition_type; |  | ||||||
| 
 | 
 | ||||||
|     PartitionKey(const std::string& partition, bool in_memory) |  | ||||||
|         : location(partition), |  | ||||||
|           in_memory(in_memory), |  | ||||||
|           partition_type(KeyType::Partition) {} |  | ||||||
|     explicit PartitionKey(const base::FilePath& file_path) |     explicit PartitionKey(const base::FilePath& file_path) | ||||||
|         : location(file_path.AsUTF8Unsafe()), |         : type_{Type::Path}, | ||||||
|           in_memory(false), |           location_{file_path.AsUTF8Unsafe()}, | ||||||
|           partition_type(KeyType::FilePath) {} |           in_memory_{false} {} | ||||||
| 
 | 
 | ||||||
|     bool operator<(const PartitionKey& other) const { |     friend auto operator<=>(const PartitionKey&, const PartitionKey&) = default; | ||||||
|       if (partition_type == KeyType::Partition) { |  | ||||||
|         if (location == other.location) |  | ||||||
|           return in_memory < other.in_memory; |  | ||||||
|         return location < other.location; |  | ||||||
|       } else { |  | ||||||
|         if (location == other.location) |  | ||||||
|           return false; |  | ||||||
|         return location < other.location; |  | ||||||
|       } |  | ||||||
|     } |  | ||||||
| 
 | 
 | ||||||
|     bool operator==(const PartitionKey& other) const { |    private: | ||||||
|       if (partition_type == KeyType::Partition) { |     enum class Type { Partition, Path }; | ||||||
|         return (location == other.location) && (in_memory < other.in_memory); | 
 | ||||||
|       } else { |     Type type_; | ||||||
|         if (location == other.location) |     std::string location_; | ||||||
|           return true; |     bool in_memory_; | ||||||
|         return false; |  | ||||||
|       } |  | ||||||
|     } |  | ||||||
|   }; |   }; | ||||||
| 
 | 
 | ||||||
|   using BrowserContextMap = |   using BrowserContextMap = | ||||||
|  |  | ||||||
		Loading…
	
	Add table
		Add a link
		
	
		Reference in a new issue
	
	 Charles Kerr
				Charles Kerr