[SetupList] Integrate Two-Cell Layout Logic (2/2) [chromium/src : main]
Xi Han (Gerrit)
Xi Han added 4 comments![]( "Open in Gerrit")
.onModuleConfigChanged(ModuleType.SETUP_LIST_TWO_CELL_CONTAINER, true);Nit: Maybe change it to be false and update the expected results?
public void testCreateInputContextForRanking_WithTwoCell() {See comment of the test below.
public void testGetCombinedRankedModules_WithTwoCell() {This test looks very similar to testGetCombinedRankedModules but giving a different set of manual ordering modules. Is it necessary?
Related details
- Gazal Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gazal Agarwal (Gerrit)
Gazal Agarwal added 4 comments![]( "Open in Gerrit")
Mostly nits.
Thanks for reviewing! Addressed the comments.
.onModuleConfigChanged(ModuleType.SETUP_LIST_TWO_CELL_CONTAINER, true);Nit: Maybe change it to be false and update the expected results?
Updated.
See comment of the test below.
Addressed below
This test looks very similar to testGetCombinedRankedModules but giving a different set of manual ordering modules. Is it necessary?
IMO, both are necessary.
`testCreateInputContextForRanking_WithTwoCell` primarily tests the logic within the `createInputContextForRankingAndUpdateManuallyRankedModuleList()` and ensures the two-cell container isn't sent for ranking.
`testGetCombinedRankedModules_WithTwoCell` tests the logic within the `getCombinedRankedModules()` method and ensures the two-cell container is placed correctly at the top of the display list.
Related details
- Xi Han
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xi Han (Gerrit)
Xi Han added 2 comments![]( "Open in Gerrit")
public void testGetCombinedRankedModules_WithTwoCell() {Gazal AgarwalThis test looks very similar to testGetCombinedRankedModules but giving a different set of manual ordering modules. Is it necessary?
IMO, both are necessary.
`testCreateInputContextForRanking_WithTwoCell` primarily tests the logic within the `createInputContextForRankingAndUpdateManuallyRankedModuleList()` and ensures the two-cell container isn't sent for ranking.
`testGetCombinedRankedModules_WithTwoCell` tests the logic within the `getCombinedRankedModules()` method and ensures the two-cell container is placed correctly at the top of the display list.
My point is: createInputContextForRankingAndUpdateManuallyRankedModuleList () only cares about the value of the build#hasManualOrdering(). Thus, it doesn't matter whether the module is SETUP_LIST_TWO_CELL_CONTAINER or ADDRESS_BAR_PLACEMENT_PROMO. This looks like a duplication of tests. Same as getCombinedRankedModules(). Unless it handles SETUP_LIST_TWO_CELL_CONTAINER differently, otherwise, we can remove this test or adding SETUP_LIST_TWO_CELL_CONTAINER to the other tests. Does it make sense to you?
Related details
- Gazal Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gazal Agarwal (Gerrit)
Gazal Agarwal added 1 comment![]( "Open in Gerrit")
public void testGetCombinedRankedModules_WithTwoCell() {Gazal AgarwalThis test looks very similar to testGetCombinedRankedModules but giving a different set of manual ordering modules. Is it necessary?
Xi HanIMO, both are necessary.
`testCreateInputContextForRanking_WithTwoCell` primarily tests the logic within the `createInputContextForRankingAndUpdateManuallyRankedModuleList()` and ensures the two-cell container isn't sent for ranking.
`testGetCombinedRankedModules_WithTwoCell` tests the logic within the `getCombinedRankedModules()` method and ensures the two-cell container is placed correctly at the top of the display list.
My point is: createInputContextForRankingAndUpdateManuallyRankedModuleList () only cares about the value of the build#hasManualOrdering(). Thus, it doesn't matter whether the module is SETUP_LIST_TWO_CELL_CONTAINER or ADDRESS_BAR_PLACEMENT_PROMO. This looks like a duplication of tests. Same as getCombinedRankedModules(). Unless it handles SETUP_LIST_TWO_CELL_CONTAINER differently, otherwise, we can remove this test or adding SETUP_LIST_TWO_CELL_CONTAINER to the other tests. Does it make sense to you?
Oh I see what you mean. I mistakenly thought you meant `testCreateInputContextForRanking_WithTwoCell` and `testGetCombinedRankedModules_WithTwoCell` both aren't needed (hence my previous explanation).
This test looks very similar to testGetCombinedRankedModules but giving a different set of manual ordering modules. Is it necessary?
You're right, the module doesn't really matter. Removed `testGetCombinedRankedModules_WithTwoCell`.
Related details
- Xi Han
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xi Han (Gerrit)
Xi Han voted and added 2 comments![]( "Open in Gerrit")
Votes added by Xi Han
| Code-Review | +1 |
2 comments
public void testCreateInputContextForRanking_WithTwoCell() {Nit: this looks like a duplicate of testCreateInputContextForRanking:) It's up to you whether to keep it or not.
Related details
- Gazal Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gazal Agarwal (Gerrit)
Gazal Agarwal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gazal Agarwal
| Auto-Submit | +1 |
1 comment
public void testCreateInputContextForRanking_WithTwoCell() {Nit: this looks like a duplicate of testCreateInputContextForRanking:) It's up to you whether to keep it or not.
Yeah I see what you mean. I feel that we should have some test case to check the rank of SETUP_LIST_TWO_CELL_CONTAINER. I've made a note to see if I can have a single parameterized test for the use case.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gazal Agarwal (Gerrit)
Gazal Agarwal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[SetupList] Integrate Two-Cell Layout Logic (2/2)
This change integrates the conditional logic for displaying the Setup
List using the new two-cell container UI. The layout choice is based on
the time elapsed since the Setup List was first shown to the user. This
completes the implementation of the time-based two-cell layout for the
Setup List.
- M chrome/browser/educational_tip/java/res/layout/educational_tip_module_two_cell_layout.xml
- M chrome/browser/educational_tip/java/src/org/chromium/chrome/browser/educational_tip/HomeTipsModulesProvider.java
- M chrome/browser/educational_tip/java/src/org/chromium/chrome/browser/setup_list/SetupListManager.java
- M chrome/browser/educational_tip/java/src/org/chromium/chrome/browser/setup_list/SetupListModuleUtils.java
- M chrome/browser/educational_tip/junit/src/org/chromium/chrome/browser/educational_tip/HomeTipsModulesProviderUnitTest.java
- M chrome/browser/educational_tip/junit/src/org/chromium/chrome/browser/setup_list/SetupListManagerUnitTest.java
- M chrome/browser/magic_stack/android/junit/src/org/chromium/chrome/browser/magic_stack/HomeModulesCoordinatorUnitTest.java
- M chrome/browser/magic_stack/android/junit/src/org/chromium/chrome/browser/magic_stack/HomeModulesMediatorUnitTest.java
Code-Review: +1 by Xi Han
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |