Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439683003:
[Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. (Closed)
|
Created:
4 years, 2 months ago by Bernhard Bauer Modified:
4 years, 1 month ago Reviewers:
dgn CC:
chromium-reviews, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android NTP] Move suggestions in SuggestionSection into a separate TreeNode.
This makes SnippetArticle a pure model class that doesn't know about the adapter.
Goals of the overall refactoring:
1) Decoupling the various pieces of logic, by moving them from the adapter into
the tree structure. For example, dismissal is something that could be handled by
a TreeNode instead of doing the big switch statement we have right now in the
Adapter.
2) Stabilizing the structure of the tree. Right now we tend to throw away
everything and rebuild, which means we have to manually figure out what changes
exactly to notify about (cf. removeSuggestion). If we keep the tree structure
stable and just adjust the number of children below each node as necessary (like
SigninPromo already does), we get correct notifications for free, and it will
allow for some optimizations as well (we could cache the number of children for
each node and update it incrementally when a subtree changes. Right now we'd
have to contort ourselves a bit to make that work with things like
resetChildren()).
BUG=616090
Committed: https://crrev.com/311b9cac063765ab69fb69f76c84bdeba8f722e3
Cr-Commit-Position: refs/heads/master@{#427060}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : review #
Messages
Total messages: 27 (19 generated)
The CQ bit was checked by bauerb@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 ========== Move suggestions in SuggestionSection into a separate TreeNode. BUG=616090 ========== to ========== [Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. BUG=616090 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bauerb@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: This issue passed the CQ dry run.
Description was changed from ========== [Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. BUG=616090 ========== to ========== [Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. This makes SnippetArticle a pure model class that doesn't know about the adapter. BUG=616090 ==========
bauerb@chromium.org changed reviewers: + dgn@chromium.org
Please review.
https://codereview.chromium.org/2439683003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2439683003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:41: private final List<SnippetArticle> mSuggestions; I find it a bit weird that we have another reference to the suggestion list stored here. It's fine in the current case because mSuggestions is final in both objects, but if the class was accessed from outside, we would have no guarantee that the list if used/modified properly, right? Maybe make the inner class non static and use directly the mSuggestions list? Actually, what is the target end state/goal of the refactor? Do we have a design doc or quick description for that somewhere?
The CQ bit was checked by bauerb@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...
https://codereview.chromium.org/2439683003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2439683003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:41: private final List<SnippetArticle> mSuggestions; On 2016/10/21 17:13:30, dgn wrote: > I find it a bit weird that we have another reference to the suggestion list > stored here. It's fine in the current case because mSuggestions is final in both > objects, but if the class was accessed from outside, we would have no guarantee > that the list if used/modified properly, right? > > Maybe make the inner class non static and use directly the mSuggestions list? Yeah, I considered that as well. I'm not sure what would be preferable here, as inner classes are somewhat discouraged, but then again in this case there is not much risk of a memory leak. So, uh, TL;DR: Done. > Actually, what is the target end state/goal of the refactor? Do we have a design > doc or quick description for that somewhere? Hm, I suppose I should update that design Doc Nicole started at some point… I don't have an exact end state in mind, but there are a couple of things I would like to achieve: 1) Decoupling the various pieces of logic, by moving them from the adapter into the tree structure. For example, dismissal is something that could be handled by a TreeNode instead of doing the big switch statement we have right now in the Adapter. 2) Stabilizing the structure of the tree. Right now we tend to throw away everything and rebuild, which means we have to manually figure out what changes exactly to notify about (cf. removeSuggestion). If we keep the tree structure stable and just adjust the number of children below each node as necessary (like SigninPromo already does), we get correct notifications for free, and it will allow for some optimizations as well (we could cache the number of children for each node and update it incrementally when a subtree changes. Right now we'd have to contort ourselves a bit to make that work with things like resetChildren()).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/22 16:43:48, Bernhard Bauer wrote: > https://codereview.chromium.org/2439683003/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java > (right): > > https://codereview.chromium.org/2439683003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:41: > private final List<SnippetArticle> mSuggestions; > On 2016/10/21 17:13:30, dgn wrote: > > I find it a bit weird that we have another reference to the suggestion list > > stored here. It's fine in the current case because mSuggestions is final in > both > > objects, but if the class was accessed from outside, we would have no > guarantee > > that the list if used/modified properly, right? > > > > Maybe make the inner class non static and use directly the mSuggestions list? > > Yeah, I considered that as well. I'm not sure what would be preferable here, as > inner classes are somewhat discouraged, but then again in this case there is not > much risk of a memory leak. > > So, uh, TL;DR: Done. > > > Actually, what is the target end state/goal of the refactor? Do we have a > design > > doc or quick description for that somewhere? > > Hm, I suppose I should update that design Doc Nicole started at some point… > > I don't have an exact end state in mind, but there are a couple of things I > would like to achieve: > 1) Decoupling the various pieces of logic, by moving them from the adapter into > the tree structure. For example, dismissal is something that could be handled by > a TreeNode instead of doing the big switch statement we have right now in the > Adapter. > 2) Stabilizing the structure of the tree. Right now we tend to throw away > everything and rebuild, which means we have to manually figure out what changes > exactly to notify about (cf. removeSuggestion). If we keep the tree structure > stable and just adjust the number of children below each node as necessary (like > SigninPromo already does), we get correct notifications for free, and it will > allow for some optimizations as well (we could cache the number of children for > each node and update it incrementally when a subtree changes. Right now we'd > have to contort ourselves a bit to make that work with things like > resetChildren()). lgtm, thanks! That bit would be nice as an update to the issue description :)
Description was changed from ========== [Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. This makes SnippetArticle a pure model class that doesn't know about the adapter. BUG=616090 ========== to ========== [Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. This makes SnippetArticle a pure model class that doesn't know about the adapter. Goals of the overall refactoring: 1) Decoupling the various pieces of logic, by moving them from the adapter into the tree structure. For example, dismissal is something that could be handled by a TreeNode instead of doing the big switch statement we have right now in the Adapter. 2) Stabilizing the structure of the tree. Right now we tend to throw away everything and rebuild, which means we have to manually figure out what changes exactly to notify about (cf. removeSuggestion). If we keep the tree structure stable and just adjust the number of children below each node as necessary (like SigninPromo already does), we get correct notifications for free, and it will allow for some optimizations as well (we could cache the number of children for each node and update it incrementally when a subtree changes. Right now we'd have to contort ourselves a bit to make that work with things like resetChildren()). BUG=616090 ==========
On 2016/10/24 12:14:42, dgn wrote: > On 2016/10/22 16:43:48, Bernhard Bauer wrote: > > > https://codereview.chromium.org/2439683003/diff/20001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java > > (right): > > > > > https://codereview.chromium.org/2439683003/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:41: > > private final List<SnippetArticle> mSuggestions; > > On 2016/10/21 17:13:30, dgn wrote: > > > I find it a bit weird that we have another reference to the suggestion list > > > stored here. It's fine in the current case because mSuggestions is final in > > both > > > objects, but if the class was accessed from outside, we would have no > > guarantee > > > that the list if used/modified properly, right? > > > > > > Maybe make the inner class non static and use directly the mSuggestions > list? > > > > Yeah, I considered that as well. I'm not sure what would be preferable here, > as > > inner classes are somewhat discouraged, but then again in this case there is > not > > much risk of a memory leak. > > > > So, uh, TL;DR: Done. > > > > > Actually, what is the target end state/goal of the refactor? Do we have a > > design > > > doc or quick description for that somewhere? > > > > Hm, I suppose I should update that design Doc Nicole started at some point… > > > > I don't have an exact end state in mind, but there are a couple of things I > > would like to achieve: > > 1) Decoupling the various pieces of logic, by moving them from the adapter > into > > the tree structure. For example, dismissal is something that could be handled > by > > a TreeNode instead of doing the big switch statement we have right now in the > > Adapter. > > 2) Stabilizing the structure of the tree. Right now we tend to throw away > > everything and rebuild, which means we have to manually figure out what > changes > > exactly to notify about (cf. removeSuggestion). If we keep the tree structure > > stable and just adjust the number of children below each node as necessary > (like > > SigninPromo already does), we get correct notifications for free, and it will > > allow for some optimizations as well (we could cache the number of children > for > > each node and update it incrementally when a subtree changes. Right now we'd > > have to contort ourselves a bit to make that work with things like > > resetChildren()). > > lgtm, thanks! That bit would be nice as an update to the issue description :) Done.
The CQ bit was checked by bauerb@chromium.org
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 ========== [Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. This makes SnippetArticle a pure model class that doesn't know about the adapter. Goals of the overall refactoring: 1) Decoupling the various pieces of logic, by moving them from the adapter into the tree structure. For example, dismissal is something that could be handled by a TreeNode instead of doing the big switch statement we have right now in the Adapter. 2) Stabilizing the structure of the tree. Right now we tend to throw away everything and rebuild, which means we have to manually figure out what changes exactly to notify about (cf. removeSuggestion). If we keep the tree structure stable and just adjust the number of children below each node as necessary (like SigninPromo already does), we get correct notifications for free, and it will allow for some optimizations as well (we could cache the number of children for each node and update it incrementally when a subtree changes. Right now we'd have to contort ourselves a bit to make that work with things like resetChildren()). BUG=616090 ========== to ========== [Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. This makes SnippetArticle a pure model class that doesn't know about the adapter. Goals of the overall refactoring: 1) Decoupling the various pieces of logic, by moving them from the adapter into the tree structure. For example, dismissal is something that could be handled by a TreeNode instead of doing the big switch statement we have right now in the Adapter. 2) Stabilizing the structure of the tree. Right now we tend to throw away everything and rebuild, which means we have to manually figure out what changes exactly to notify about (cf. removeSuggestion). If we keep the tree structure stable and just adjust the number of children below each node as necessary (like SigninPromo already does), we get correct notifications for free, and it will allow for some optimizations as well (we could cache the number of children for each node and update it incrementally when a subtree changes. Right now we'd have to contort ourselves a bit to make that work with things like resetChildren()). BUG=616090 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. This makes SnippetArticle a pure model class that doesn't know about the adapter. Goals of the overall refactoring: 1) Decoupling the various pieces of logic, by moving them from the adapter into the tree structure. For example, dismissal is something that could be handled by a TreeNode instead of doing the big switch statement we have right now in the Adapter. 2) Stabilizing the structure of the tree. Right now we tend to throw away everything and rebuild, which means we have to manually figure out what changes exactly to notify about (cf. removeSuggestion). If we keep the tree structure stable and just adjust the number of children below each node as necessary (like SigninPromo already does), we get correct notifications for free, and it will allow for some optimizations as well (we could cache the number of children for each node and update it incrementally when a subtree changes. Right now we'd have to contort ourselves a bit to make that work with things like resetChildren()). BUG=616090 ========== to ========== [Android NTP] Move suggestions in SuggestionSection into a separate TreeNode. This makes SnippetArticle a pure model class that doesn't know about the adapter. Goals of the overall refactoring: 1) Decoupling the various pieces of logic, by moving them from the adapter into the tree structure. For example, dismissal is something that could be handled by a TreeNode instead of doing the big switch statement we have right now in the Adapter. 2) Stabilizing the structure of the tree. Right now we tend to throw away everything and rebuild, which means we have to manually figure out what changes exactly to notify about (cf. removeSuggestion). If we keep the tree structure stable and just adjust the number of children below each node as necessary (like SigninPromo already does), we get correct notifications for free, and it will allow for some optimizations as well (we could cache the number of children for each node and update it incrementally when a subtree changes. Right now we'd have to contort ourselves a bit to make that work with things like resetChildren()). BUG=616090 Committed: https://crrev.com/311b9cac063765ab69fb69f76c84bdeba8f722e3 Cr-Commit-Position: refs/heads/master@{#427060} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/311b9cac063765ab69fb69f76c84bdeba8f722e3 Cr-Commit-Position: refs/heads/master@{#427060} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
