Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Refactor DownloadItemView state transitions, part 1.
The ultimate goal here is to eliminate duplication of code and work and make the actual effects of state transitions obvious. This CL combines the various TransitionTo...(), Show...(), Clear...(), and SetMode() functions into a single UpdateMode() function and does some initial basic combining of shared code. It makes the following two assumptions: * The item does not transition from one warning state to the other, or from one mixed content state to the other; it transitions into or out of one of these states from an unrelated state (most commonly kNormal). This seems true from my reading of the various model-side state transitions, and the existing code would not have properly handled such state changes anyway (e.g. |mode_| would never have been updated). Making this assumption lets us do conversions akin to this: OLD: if (is_mixed_content(mode) && !is_mixed_content(mode_)) { ... } NEW: if (is_mixed_content(mode)) { ... } * It's undesirable to trigger LoadIcon()'s functionality again when the file path hasn't actually changed. While only one call chain actually checked for this, it seems correct in principle in all cases (such repeated calls would just be extra work). Making this assumption lets us put that check in LoadIcon(), and then call it unconditionally at the end of UpdateMode() instead of in many other places. The UpdateMode() function here is far too sprawling, but even making fairly few actual logic changes, this CL already pushes the bounds of what a reviewer can verify is still correct. Further refactoring to turn this into something shorter and more comprehensible will come in a subsequent patch. Bug: none Change-Id: I6b0e6dfcfef5c1057c4e59e593adcee27e537950 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2296271 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Min Qin <qinmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#788857}
- Loading branch information