893529 - [fig] Allow Tap of reader list icon to open about:home READING LIST page

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Mark Capella [:capella]]

Functionality not yet re-incorporated into fig.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v0)

This is functional, but could use your feedback. There's a couple areas for improvement I see, but wonder about...

In Tab.java, I wasn't sure if it was alright to re-purpose the data Object in the LOCATION_CHANGE event message to also include the |page| information, so I finessed a new / seperate HOMEPAGER_CHANGE event.

In aboutReader.js, instead of issuing message "Reader:GoToReadingList" to BrowserApp.java to perform |loadUrl(ABOUT_HOME, Tabs.LOADURL_READINGLISTPAGE)|, I wonder if we can simply load the Tab/URL right there, and remove the message / listeners altogether from BrowserApp.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#326

I think these are both ok, but I need to play with it some more, and I wanted to check in before I got much further.

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 775312 [details] [diff] [review]
Patch (v0)

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

I like the direction. Just needs to cover the suggesteed changes.

::: mobile/android/base/GeckoApp.java
@@ -551,5 @@
>              } else if (event.equals("Reader:FaviconRequest")) {
>                  final String url = message.getString("url");
>                  handleFaviconRequest(url);
> -            } else if (event.equals("Reader:GoToReadingList")) {
> -                showReadingList();

These changes to remove unused code should probably go in a separate patch.

::: mobile/android/base/Tab.java
@@ +595,5 @@
>          setZoomConstraints(new ZoomConstraints(true));
>          setHasTouchListeners(false);
>          setBackgroundColor(DEFAULT_BACKGROUND_COLOR);
>  
> +        if (message.getString("loadHomePage").equals("readingListPage")) {

I'd prefer this to be a little type-safer by relying on the enum values a bit more. Something like:

final String homePage = message.getString("loadHomePage");
if (!TextUtils.isEmpty(homePage)) {
    HomePager.Page page = HomePager.Page.valueOf(homePage);
    Tabs.getInstance().notifyListeners(this, Tabs.TabEvents.HOMEPAGER_CHANGE, page);
}

This is also more future-proof as it's not specific to reading list. You can specify the string representation of the each enum value with something like this: http://stackoverflow.com/questions/6667243/using-enum-values-as-string-literals

@@ +596,5 @@
>          setHasTouchListeners(false);
>          setBackgroundColor(DEFAULT_BACKGROUND_COLOR);
>  
> +        if (message.getString("loadHomePage").equals("readingListPage")) {
> +            Tabs.getInstance().notifyListeners(this, Tabs.TabEvents.HOMEPAGER_CHANGE, HomePager.Page.READING_LIST);

I'm not too keen on having a specific message just for the homepager page. Maybe you should just store the page in the tab and use it when handling the LOCATION_CHANGE in BrowserApp for the about:home case. You'd to showHomePager(tab.getAboutHomePage()) or something.

::: mobile/android/base/Tabs.java
@@ +58,5 @@
>      public static final int LOADURL_PINNED = 8;
>      public static final int LOADURL_DELAY_LOAD = 16;
>      public static final int LOADURL_DESKTOP = 32;
>      public static final int LOADURL_BACKGROUND = 64;
> +    public static final int LOADURL_READINGLISTPAGE = 128;

LOADURL_READING_LIST would be good enough I think.

@@ +669,5 @@
>              args.put("pinned", (flags & LOADURL_PINNED) != 0);
>              args.put("delayLoad", delayLoad);
>              args.put("desktopMode", desktopMode);
>              args.put("selected", !background);
> +            args.put("loadHomePage", (flags & LOADURL_READINGLISTPAGE) != 0 ? "readingListPage" : "");

Define string representation of enum values and here use something like HomePager.Page.READING_LIST.toString().

::: mobile/android/base/home/HomePager.java
@@ +70,5 @@
>          adapter.addTab(Page.READING_LIST, ReadingListPage.class, null, getContext().getString(R.string.reading_list));
>  
>          setAdapter(adapter);
>  
> +        setCurrentItem(adapter.getItemPosition((page != null) ? page : Page.BOOKMARKS), false);

This null check should not be there as it would probably hide an actual bug. In other words, if page is null here, it's probably a bug that should be fixed.

@@ +136,5 @@
>              mTabs.add(info);
>              notifyDataSetChanged();
>          }
>  
> +        public Integer getItemPosition(Page page) {

I'd prefer this method to simply return an int instead. Return -1 if item is not found. This is more consistent with Android's API.

@@ +142,5 @@
> +                TabInfo info = mTabs.get(i);
> +                if (info.page == page) {
> +                    return i;
> +                }
> +            }

nit: add empty line here.

::: mobile/android/chrome/content/browser.js
@@ +761,5 @@
>      let referrerURI = "referrerURI" in aParams ? aParams.referrerURI : null;
>      let charset = "charset" in aParams ? aParams.charset : null;
>  
> +    let tab = this.getTabForBrowser(aBrowser);
> +    if (tab) {

Tip for future patches: avoid mixing these small refactorings with the actual changes you want to make. Do the refactoring in a separate patch.

@@ +762,5 @@
>      let charset = "charset" in aParams ? aParams.charset : null;
>  
> +    let tab = this.getTabForBrowser(aBrowser);
> +    if (tab) {
> +      if (!tab.loadHomePage) tab.loadHomePage = ("loadHomePage" in aParams) ? aParams.loadHomePage : "";

I'd prefer something like "aboutHomePage" instead, to be consistent. "loadHomePage" sounds too generic. It should be obviously connected to about:home at first sight.

@@ +3566,5 @@
>        tabID: this.id,
>        uri: fixedURI.spec,
>        userSearch: this.userSearch || "",
>        documentURI: documentURI,
> +      loadHomePage: (this.loadHomePage ? this.loadHomePage : ""),

You can simply do:

loadHomePage: this.loadHomePage || ""

Comment 3

[Mark Capella [:capella]]

Attachment: bug893529ro - Remove Old Code First

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 777797 [details] [diff] [review]
bug893529ro - Remove Old Code First

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

Nice.

Comment 5

[Mark Capella [:capella]]

Comment on attachment 777797 [details] [diff] [review]
bug893529ro - Remove Old Code First

https://hg.mozilla.org/projects/fig/rev/a4c2fc0828c7

Comment 6

[Mark Capella [:capella]]

Attachment: bug893529 - The Main Patch

Awesome suggestions ... I didn't know we could serialize / stringify an ENUM so this is just fantastic ...

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 777924 [details] [diff] [review]
bug893529 - The Main Patch

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

Excellent.

::: mobile/android/base/Tab.java
@@ +141,5 @@
> +    public HomePager.Page getAboutHomePage() {
> +        return mAboutHomePage;
> +    }
> +
> +    public void setAboutHomePage(HomePager.Page page) {

Hmm, this shouldn't really be a public API. The Tab instance should be the only one able to set its target about:home page.

::: mobile/android/base/Tabs.java
@@ +651,5 @@
>              args.put("pinned", (flags & LOADURL_PINNED) != 0);
>              args.put("delayLoad", delayLoad);
>              args.put("desktopMode", desktopMode);
>              args.put("selected", !background);
> +            args.put("aboutHomePage", (flags & LOADURL_READING_LIST) != 0 ? HomePager.Page.READING_LIST : "");

I assume this code does right thing in terms of converting Page.READING_LIST to String?

Comment 8

[Mark Capella [:capella]]

Comment on attachment 777924 [details] [diff] [review]
bug893529 - The Main Patch

https://hg.mozilla.org/projects/fig/rev/3ce8681e5602

The ENUM default toString() returns name() (or some other keyword if we choose to override the method) so I just coded it the laziest way :-)

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a4c2fc0828c7
https://hg.mozilla.org/mozilla-central/rev/3ce8681e5602