Closed Bug 949181 Opened 7 years ago Closed 7 years ago

Get rid of the hardcoded Page enum in HomePager

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: lucasr, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

(Whiteboard: shovel-ready)

Attachments

(2 files)

The only use case where we need the Page enum right now is for accessing Reading List directly from the Reader Mode UI (see 949178). We can remove the Page enum if we find another to implement the Reader Mode use case.
This is also used in testing for AboutHomeComponent.assertCurrentPage(Page) [1], though we can just add a "PageType" enum to UITest instead.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/components/AboutHomeComponent.java?rev=399ecd6a6187#52
(In reply to Michael Comella (:mcomella) from comment #1)
> This is also used in testing for AboutHomeComponent.assertCurrentPage(Page)
> [1], though we can just add a "PageType" enum to UITest instead.
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/
> components/AboutHomeComponent.java?rev=399ecd6a6187#52

Sounds like you're interested in working on this bug once bug 942231 lands! :D

We might also be able to close bug 940565 as INVALID if we're going to go ahead with this approach instead.
I'm working on this.
Assignee: nobody → margaret.leibovic
Blocks: 944160
This patch kills HomePager.Page. Instead of keeping track of Pages in HomePager/HomeAdapter, I switched the logic to keep track of page ids.

This patch exposes the unfortunate situation that is that reading list button, and the amount of logic we have in place just to support that. I'd like to see if we can clean this up in bug 949178, and I'm hoping this hacky string passing might be an acceptable intermediate solution.
Attachment #8348272 - Flags: review?(wjohnston)
I decided to just make a new hardcoded PageType enum, in order to keep most of the existing test logic the same. I considered dynamically hooking this into HomeConfig.PageType, but I don't think we need that level of dynamic support (at least not now).

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=f9acda0354c7
Attachment #8348276 - Flags: review?(michael.l.comella)
Comment on attachment 8348276 [details] [diff] [review]
(Part 2) Update robocop tests to get rid of dependency on HomePager.Page

Review of attachment 8348276 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.
Attachment #8348276 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8348272 [details] [diff] [review]
(Part 1) Get rid of the Page enum in HomePager

Review of attachment 8348272 [details] [diff] [review]:
-----------------------------------------------------------------

Wow. Yeah, the reader mode stuff should die.
Attachment #8348272 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #7)
> Comment on attachment 8348272 [details] [diff] [review]
> (Part 1) Get rid of the Page enum in HomePager
> 
> Review of attachment 8348272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wow. Yeah, the reader mode stuff should die.

Take a look at bug 950919! :)
You need to log in before you can comment on or make changes to this bug.