950919 - Get rid of "aboutHomePage" flag

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

In bug 949178, I described the dance we do to open the reading list page in about:home. Instead of passing around these custom flags, we should try passing the desired page as a parameter on the URL.

A bonus of this approach is that we could then open any about:home page from JS, including new pages that add-ons create.

Comment 1

[:Margaret Leibovic]

Attachment: (Part 1) Get rid of loadHomePage method

Spreading the review love around a little, since a lot of people are on PTO and I've just been dumping reviews on wesj :)

This patch just simplifies things a bit (including getting rid of an unused version of this method).

Comment 2

[:Margaret Leibovic]

Attachment: (Part 2) Get rid of "aboutHomePage" flag

(These patches are built on top of the patches for bug 949181.)

This still doesn't address the problem described in bug 949178 where the user may have chosen to remove the reading list page from about:home, but it does simplify our about:home loading logic by removing some custom flags (and it improves modularity, since Tab doesn't store data about the current about:home page anymore).

I also like this approach because it will make it easier for us to potentially open different about:home pages in the future.

I stole a lot of the query parsing logic from ReaderModeUtils.getUrlFromAboutReader, so perhaps it would be better to factor that out to somewhere else...

Comment 3

[:Margaret Leibovic]

Attachment: (Part 3) Create utility StringUtils.getQueryParameter

I decided to move the copy/pasted code into a shared utility routine in StringUtils.

Comment 4

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

Comment on attachment 8348352 [details] [diff] [review]
(Part 2) Get rid of "aboutHomePage" flag

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

lgtm, though you may want to address the nits.

::: mobile/android/base/AboutPages.java
@@ +34,5 @@
>      public static final boolean isAboutHome(final String url) {
> +        if (url == null || !url.startsWith("about:home")) {
> +            return false;
> +        }
> +        return HOME.equals(url.split("\\?")[0]);

I think the format of the url needs to be documented somewhere (i.e. GET request syntax to open a specific page) and this seems like an okay place to do it because it's a little ambiguous as to why you're splitting the URL like this to a first time reader.

@@ +37,5 @@
> +        }
> +        return HOME.equals(url.split("\\?")[0]);
> +    }
> +
> +    public static final String getPageIdFromAboutHome(final String aboutHomeUrl) {

nit: getPageIdFromAboutHomeUrl

@@ +48,5 @@
> +            return null;
> +        }
> +
> +        final String query = urlParts[1];
> +        for (String param : query.split("&")) {

nit: final String param

::: mobile/android/base/home/HomeAdapter.java
@@ +151,5 @@
>              mPageEntry = pageEntry;
>          }
>  
>          public String getId() {
>              return mId;

Instead of storing this value, maybe you should:

`return mPageEntry.getId();`

::: mobile/android/chrome/content/aboutReader.js
@@ +364,5 @@
>    _onList: function Reader_onList() {
>      if (!this._article || this._readingListCount < 1)
>        return;
>  
> +    gChromeWin.BrowserApp.loadURI("about:home?page=reading_list");

gChromeWin refers to browser.js, yes?

If not, then I revoke my r+! :)

Comment 5

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

Comment on attachment 8348355 [details] [diff] [review]
(Part 3) Create utility StringUtils.getQueryParameter

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

Nice cleanup.

::: mobile/android/base/util/StringUtils.java
@@ +100,5 @@
>          return host.substring(start);
>      }
> +
> +    /**
> +     * Searches the url query string for the first value with the given key. 

nit: whitespace.

@@ +102,5 @@
> +
> +    /**
> +     * Searches the url query string for the first value with the given key. 
> +     */
> +    public static String getQueryParameter(String url, String key) {

nit: key -> desiredKey. Then you can use "key" instead of "k" below.

nit: It seems you lost some of the newline whitespace on transition - I liked the spacing before.

@@ +103,5 @@
> +    /**
> +     * Searches the url query string for the first value with the given key. 
> +     */
> +    public static String getQueryParameter(String url, String key) {
> +        if (url == null) {

nit: You can short circuit this by calling `TextUtils.isEmpty` instead (since the empty String will eventually return null).

nit: You can also validate the key parameter, perhaps throwing on error so the debugging is obvious (which reminds me, maybe we should throw whenever url is null?).

@@ +111,5 @@
> +        if (urlParts.length < 2) {
> +            return null;
> +        }
> +        final String query = urlParts[1];
> +        for (String param : query.split("&")) {

(from the previous patch) nit: `final String param`

Comment 6

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #4)

> ::: mobile/android/chrome/content/aboutReader.js
> @@ +364,5 @@
> >    _onList: function Reader_onList() {
> >      if (!this._article || this._readingListCount < 1)
> >        return;
> >  
> > +    gChromeWin.BrowserApp.loadURI("about:home?page=reading_list");
> 
> gChromeWin refers to browser.js, yes?

Yeah, gChromeWin is our variable name convention for the global chrome window, which is browser.xul, which loads browser.js :)

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/9c9c3e3e7bc2
https://hg.mozilla.org/integration/fx-team/rev/ba6f87308d48
https://hg.mozilla.org/integration/fx-team/rev/eb61ae4b4bd8

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c9c3e3e7bc2
https://hg.mozilla.org/mozilla-central/rev/ba6f87308d48
https://hg.mozilla.org/mozilla-central/rev/eb61ae4b4bd8