Closed Bug 1328937 Opened 7 years ago Closed 7 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: