Cannot edit/pin tile top sites

RESOLVED FIXED in Firefox 47

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kbrosnan, Assigned: ahunt)

Tracking

Trunk
Firefox 47
All
Android
Points:
---

Firefox Tracking Flags

(firefox47 verified, fennec47+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

2 years ago
tracking-fennec: --- → ?
I see related problems: I lost my pinned sites on upgrade.  I have not yet tried to pin/unpin manually.
(Assignee)

Comment 2

2 years ago
Created attachment 8725356 [details]
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/
Attachment #8725356 - Flags: review?(rnewman)
(Assignee)

Comment 3

2 years ago
Created attachment 8725357 [details]
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/
Attachment #8725357 - Flags: review?(rnewman)
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Updated

2 years ago
Assignee: nobody → ahunt
(Assignee)

Comment 5

2 years ago
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 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?
Attachment #8725356 - Flags: review?(rnewman)
(Assignee)

Comment 7

2 years ago
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/
Attachment #8725356 - Attachment description: MozReview Request: Bug 1252501 - Don't place lower frecency items in front of top and pinned items r=rnewman → MozReview Request: Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman
Attachment #8725356 - Flags: review?(rnewman)
(Assignee)

Updated

2 years ago
Attachment #8725357 - Attachment is obsolete: true
Attachment #8725357 - Flags: review?(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!
Attachment #8725356 - Flags: review?(rnewman)
(Assignee)

Comment 9

2 years ago
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/
Attachment #8725356 - Flags: review?(rnewman)
(Assignee)

Comment 10

2 years ago
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 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.
Attachment #8725356 - Flags: review?(rnewman)
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1252714
(Assignee)

Comment 13

2 years ago
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.)
(Assignee)

Updated

2 years ago
Attachment #8725356 - Flags: review?(rnewman)
(Assignee)

Comment 14

2 years ago
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/
(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.
I'll get to this tomorrow.
Status: NEW → ASSIGNED
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).
Attachment #8725356 - Flags: review?(rnewman) → review+
(Assignee)

Comment 18

2 years ago
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/
(Assignee)

Comment 19

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/82e8f730b3e1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
tracking-fennec: ? → 47+
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)
status-firefox47: fixed → verified
You need to log in before you can comment on or make changes to this bug.