949181 - Get rid of the hardcoded Page enum in HomePager

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

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.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 2

[:Margaret Leibovic]

(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.

Comment 3

[:Margaret Leibovic]

I'm working on this.

Comment 4

[:Margaret Leibovic]

Attachment: (Part 1) Get rid of the Page enum in HomePager

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.

Comment 5

[:Margaret Leibovic]

Attachment: (Part 2) Update robocop tests to get rid of dependency on HomePager.Page

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

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 7

[Wesley Johnston (:wesj)]

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.

Comment 8

[:Margaret Leibovic]

(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! :)

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/2dd48acdb5c0
https://hg.mozilla.org/integration/fx-team/rev/014d5ead8420

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2dd48acdb5c0
https://hg.mozilla.org/mozilla-central/rev/014d5ead8420