Closed Bug 1328937 Opened 9 years ago Closed 9 years ago

Simplify AS StreamItem class

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

(Whiteboard: [MobileAS])

Attachments

(6 files)

Bug 1328937 - StreamItem: Move inner classes to separate files.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1328937 - HighlightItem: Make all properties 'private'.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1328937 - HighlightItem: Move data into separate Highlight / Metadata model classes.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1328937 - Extract TopSite model from stream adapter.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1328937 - Introduce shared interface for activity stream models.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
disable-test.patch
1.24 KB, patch
Details | Diff | Splinter Review
The StreamItem class has grown a lot recently and it's getting more complex and unreadable. We should be able to make it simpler by just moving things around a bit.
It looks like there could be done more - but I'll stop now. :)
Iteration: 1.12 → 1.13
Comment on attachment 8825020 [details] Bug 1328937 - StreamItem: Move inner classes to separate files. https://reviewboard.mozilla.org/r/103298/#review104378
Attachment #8825020 - Flags: review?(gkruglov) → review+
Comment on attachment 8825021 [details] Bug 1328937 - HighlightItem: Make all properties 'private'. https://reviewboard.mozilla.org/r/103300/#review104380
Attachment #8825021 - Flags: review?(gkruglov) → review+
Comment on attachment 8825022 [details] Bug 1328937 - HighlightItem: Move data into separate Highlight / Metadata model classes. https://reviewboard.mozilla.org/r/103302/#review104384
Attachment #8825022 - Flags: review?(gkruglov) → review+
Comment on attachment 8825023 [details] Bug 1328937 - Extract TopSite model from stream adapter. https://reviewboard.mozilla.org/r/103304/#review104386 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:600 (Diff revision 1) > public static final int TYPE_TOP = 1; > public static final int TYPE_PINNED = 2; > public static final int TYPE_SUGGESTED = 3; > > + @IntDef({TYPE_BLANK, TYPE_TOP, TYPE_PINNED, TYPE_SUGGESTED}) > + public @interface TopSiteType {} neat.
Attachment #8825023 - Flags: review?(gkruglov) → review+
Comment on attachment 8825024 [details] Bug 1328937 - Introduce shared interface for activity stream models. https://reviewboard.mozilla.org/r/103306/#review104398 Looks great, thanks for refactoring! ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/menu/ActivityStreamContextMenu.java:175 (Diff revision 1) > }).execute(); > } > > > @Override > public boolean onNavigationItemSelected(MenuItem item) { Perhaps rename this to menuItem, to keep things consistent? `this.item` and `item` might be too subtle of a distinction. ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/model/TopSite.java:18 (Diff revision 1) > -public class TopSite { > +public class TopSite implements Item { > private final long id; > private final String url; > private final String title; > - private @Nullable final Boolean isBookmarked; > + private @Nullable Boolean isBookmarked; > + private @Nullable Boolean isPinned; Could mark this as final if you throw in updatePinned. ::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/model/TopSite.java:80 (Diff revision 1) > + this.isBookmarked = bookmarked; > + } > + > + @Override > + public void updatePinned(boolean pinned) { > + this.isPinned = pinned; We know pinned state for a top site, we won't be looking it up, so trying to set it anywhere outside of the constructor would indicate a mistake. I'd consider throwing an unsupported operation exception here.
Attachment #8825024 - Flags: review?(gkruglov) → review+
Pushed by s.kaspari@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5d1623b6bcca StreamItem: Move inner classes to separate files. r=Grisha https://hg.mozilla.org/integration/autoland/rev/e1dbcf0b7019 HighlightItem: Make all properties 'private'. r=Grisha https://hg.mozilla.org/integration/autoland/rev/927275a3ea2c HighlightItem: Move data into separate Highlight / Metadata model classes. r=Grisha https://hg.mozilla.org/integration/autoland/rev/1d0bc7e990fe Extract TopSite model from stream adapter. r=Grisha https://hg.mozilla.org/integration/autoland/rev/8e5f0fda8a89 Introduce shared interface for activity stream models. r=Grisha
Depends on: 1330280
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/21f392a1ac51 Disable testActivityStreamContextMenu. a=bustagefix
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b05382b04834 Backed out changeset 21f392a1ac51 https://hg.mozilla.org/integration/autoland/rev/9a54e7a26af9 Backed out changeset 8e5f0fda8a89 https://hg.mozilla.org/integration/autoland/rev/b5dea4d18371 Backed out changeset 1d0bc7e990fe https://hg.mozilla.org/integration/autoland/rev/dc639e004927 Backed out changeset 927275a3ea2c https://hg.mozilla.org/integration/autoland/rev/189fb14e8725 Backed out changeset e1dbcf0b7019 https://hg.mozilla.org/integration/autoland/rev/fa6ea059ddb6 Backed out changeset 5d1623b6bcca for continued bustage on android
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=67976595&repo=autoland
Flags: needinfo?(s.kaspari)
Flags: needinfo?(s.kaspari)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: