1252501 - Cannot edit/pin tile top sites

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

Can't pin sites. 
Try edit/pin a site on the topsites icons and it seems to disappear

Comment 1

[Nick Alexander :nalexander [he/him]]

I see related problems: I lost my pinned sites on upgrade.  I have not yet tried to pin/unpin manually.

Comment 2

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

Review commit: https://reviewboard.mozilla.org/r/37437/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37437/

Comment 3

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1252501 - Approach 2: use the entire numbers table instead of limiting it to the grid r=rnewman

Review commit: https://reviewboard.mozilla.org/r/37439/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37439/

Comment 4

[Andrzej Hunt :ahunt]

(In reply to Nick Alexander :nalexander from comment #1)
> I see related problems: I lost my pinned sites on upgrade.  I have not yet
> tried to pin/unpin manually.

It's the same bug - the ordering was messed up. I should have been testing with a more realistic dataset which would have made the problem a lot more obvious.

Comment 5

[Andrzej Hunt :ahunt]

I've pushed two different approaches to reviewboard:

1) Coalesce the position with a large number (9999 - we could just use totalLimit instead), which ensures we'll always order items without a number after the numbered items. This is safer if we ever increase the maximum limit for topsites beyond the size of the numbers table (both limits are 50)

or:

2) Join with the entire numbers table (instead of limiting to the grid, which is only 6 or 9 items), which ensures that every item in the list recevies a position - this is potentially more brittle?


If we add some better tests (we/I should...), then approach (2) shouldn't be an issue and is probably simpler to understand.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8725356 [details]
MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

https://reviewboard.mozilla.org/r/37437/#review33983

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:851
(Diff revision 1)
> -                        Bookmarks.POSITION + ", " +
> +                        "COALESCE( " + Bookmarks.POSITION + ", 9999) AS " + Bookmarks.POSITION + ", " +

I don't understand what this is trying to achieve. POSITION is a bookmark's position in its parent folder; they're not really comparable. Or are you using the fact that these apply to pinned sites? Can you explain how this resolves the bug?

Comment 7

[Andrzej Hunt :ahunt]

Comment on attachment 8725356 [details]
MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37437/diff/1-2/

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8725356 [details]
MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

https://reviewboard.mozilla.org/r/37437/#review34009

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:846
(Diff revision 2)
> +                                  DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") + " + (SELECT COUNT(*) " + pinnedSitesFromClause + ")" +

The only reason for the addition is to make sure that these positions are outside the indices of the pinned sites. So use `suggestedGridLimit`, not the full subquery!

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8725356 [details]
MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37437/diff/2-3/

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8725356 [details]
MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37437/diff/3-4/

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8725356 [details]
MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

https://reviewboard.mozilla.org/r/37437/#review34031

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:698
(Diff revision 4)
>          final String suggestedGridLimit = uri.getQueryParameter(BrowserContract.PARAM_SUGGESTEDSITES_LIMIT);

Parse this as an integer. If it's non-integer we should abort. If it's null, we should default it to something sane.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:701
(Diff revision 4)
>                  suggestedGridLimit

Because `suggestedGridLimit` is now numeric, you don't need to put it in an arg list anywhere in this method.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:710
(Diff revision 4)
>                  String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID)

This is numeric. You can concatenate it straight into the SQL clause and avoid passing args.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:762
(Diff revision 4)
>          final Cursor suggestedSitesCursor = suggestedSites.get(Integer.parseInt(suggestedGridLimit));

… and no need to parse here!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:854
(Diff revision 4)
> +            final String generatePositionFromPositionAndRowid =

If you inline this expression into the `SELECT` below, the compiler will have an easier time of smushing constant strings together.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:856
(Diff revision 4)
> +                                  DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") + " + ?" +

… and no need for the bind here; just concat the int directly.

Comment 13

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/37437/#review34031

> Parse this as an integer. If it's non-integer we should abort. If it's null, we should default it to something sane.

Just to double-check: do we want to do this for both limits? (The normal query method just uses the string, and passes that in as a bound parameter, I'm guessing we don't really want that though.)

Comment 14

[Andrzej Hunt :ahunt]

Comment on attachment 8725356 [details]
MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37437/diff/4-5/

Comment 15

[Richard Newman [:rnewman]]

(In reply to Andrzej Hunt :ahunt from comment #13)

> Just to double-check: do we want to do this for both limits? (The normal
> query method just uses the string, and passes that in as a bound parameter,
> I'm guessing we don't really want that though.)

Correct. Parse and validate early.

Comment 16

[Richard Newman [:rnewman]]

I'll get to this tomorrow.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8725356 [details]
MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

https://reviewboard.mozilla.org/r/37437/#review34151

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:698
(Diff revision 5)
> -        final String totalLimit = uri.getQueryParameter(BrowserContract.PARAM_LIMIT);
> +        Integer totalLimit = new Integer(uri.getQueryParameter(BrowserContract.PARAM_LIMIT));

Parameter might be missing, and this should drop down to a primitive type, so something like this avoids ending up with a possibly-null `Integer` object on the heap:

```
    final String limitParam = uri.getQueryParameter(BrowserContract.PARAM_LIMIT);
    final String gridLimit = uri.getQueryParameter(BrowserContract.PARAM_SUGGESTEDSITES_LIMIT);
    final int suggestedGridLimit;
    final int totalLimit;

    if (limitParam == null) {
        totalLimit = 50;
    } else {
        totalLimit = Integer.parseInt(limitParam, 10);
    }
    
    if (gridLimit == null) {
        suggestedGridLimit = getContext().getResources().getInteger(R.integer.number_of_top_sites);
    } else {
        suggestedGridLimit = Integer.parseInt(gridLimit, 10);
    }
```

Note that fetching the resource and parsing the integer can both throw runtime exceptions (`Resources.NotFoundException` and `NumberFormatException` respectively).

Comment 18

[Andrzej Hunt :ahunt]

Comment on attachment 8725356 [details]
MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37437/diff/5-6/

Comment 19

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/82e8f730b3e18f3be117c5a202f9fead8e7a9e78
Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82e8f730b3e1

Comment 21

[Teodora Vermesan (:TeoVermesan)]

Top Sites can be edited, pinned and unpinned in grid view, so:
Verified as fixed using:
Device: One A2001 (Android 5.1.1) 
Build: Firefox for Android 47.0a2 (2016-03-09)