Closed Bug 867125 Opened 7 years ago Closed 6 years ago

Unpinning a site from about:home removes it from about:home regardless of the number of visits

Categories

(Firefox for Android :: General, defect)

23 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
firefox23 --- verified

People

(Reporter: u421692, Assigned: Margaret)

References

Details

Attachments

(1 file)

Environment:
Build: Firefox for Android 23.0a1 (2013-04-29)
Device: Samsung Galaxy Tab 10.1(GT-P7510)
OS: Android 4.0.4

Steps to reproduce:
1. Go to about:home.
2. Pin a site.
3. Unpin the site.

Expected result:
The site is unpinned, but is not removed from about:home top sites

Actual result:
The site is removed even if there are empty thumbnails in about:home.

!Note: The unpinned thumbnail will not populate about:home page, no matter how many times you visit that site.
Severity: normal → major
This is current expected behaviour; see bug 837132.
Severity: major → normal
Summary: Unpinning a site from about:home would make the thumbnail disappear → Unpinning a site from about:home removes it from about:home regardless of the number of visits
bug 837132 refers to the "Remove" context menu option, which was removed with the patch for the bug. This issue refers to the unpin option.
This sounds like a bug. Unpinning a site is only supposed to remove the pinned bookmark:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#1189

So that shouldn't stop it from showing up again in the same place in the top sites query that made it appear in the first place.

Is this a regression, or something that's always happened?
I think this has always been the case, unpinning on Firefox 20 (Release), does drop it from the screen.
tracking-fennec: --- → ?
Wes - What is the expected behavior for an Unpin? I know we blank the thumbnail, but we should not clear the site from history.
Flags: needinfo?(wjohnston)
Margaret - Can you do a code inspection to see if Unpinning results in clearing the site from history?
(In reply to Mark Finkle (:mfinkle) from comment #6)
> Margaret - Can you do a code inspection to see if Unpinning results in
> clearing the site from history?

I started looking into this yesterday, and I don't see anywhere in the code that we're removing the site from history. In fact, if you open the awesomescreen, you will still see it in the correct place in top sites.

I am suspicious that something is causing that previously pinned site to be filtered out here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#239

I can poke into the DB to see what's going on when we unpin a site.
Assignee: nobody → margaret.leibovic
When I wrote the pinning stuff, I intentionally forced the slot to be "blank" after unpinning until about:home is refreshed again. I did that

1.) So that things would feel responsive and instantaneous. The new query can take a few seconds
2.) Since we aren't animating things around (yet) the transition felt really clunky.

But the sites should return if you leave and visit something and then come back to about:home. Is that not happening?
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #8)
> When I wrote the pinning stuff, I intentionally forced the slot to be
> "blank" after unpinning until about:home is refreshed again. I did that
> 
> 1.) So that things would feel responsive and instantaneous. The new query
> can take a few seconds
> 2.) Since we aren't animating things around (yet) the transition felt really
> clunky.
> 
> But the sites should return if you leave and visit something and then come
> back to about:home. Is that not happening?

Yeah, that's the problem. The site shows up in the awesomescreen top sites query, but not in the about:home top sites query. I took a look at the DB, and there wasn't anything obviously wrong that would prevent the query from returning that site (the pinned bookmark was just deleted as expected). I can continue to investigate this.
Oh, I see what's happening now. The pinned bookmark is getting "deleted", but that just means that it's getting marked as deleted:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java#3253

So we need to update our selection in getTopSites to account for that.

The reason we're not seeing that pinned site appear is that getPinnedSites calls BrowserProvider.query to get the pinned sites, which filters out deleted pinned sites:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java#2629

Patch on its way.
Attached patch patchSplinter Review
Attachment #746008 - Flags: review?(wjohnston)
Comment on attachment 746008 [details] [diff] [review]
patch

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +238,5 @@
>          // Filter out sites that are pinned
>          String selection = DBUtils.concatenateWhere("", Combined.URL + " NOT IN (SELECT " +
>                                               Bookmarks.URL + " FROM bookmarks WHERE bookmarks." +
> +                                             Bookmarks.PARENT + " == ? AND bookmarks." +
> +                                             Bookmarks.IS_DELETED + " == 0)");

Move the "bookmarks." stuff onto the same line as IS_DELETED, but also, you should use DBUtils.qualifyColumn:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/DBUtils.java#17
Attachment #746008 - Flags: review?(wjohnston) → review+
Flags: in-moztrap?(fennec)
https://hg.mozilla.org/mozilla-central/rev/def13deac493
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Status: RESOLVED → VERIFIED
Test Case added in moztrap:
https://moztrap.mozilla.org/manage/case/8080/
Flags: in-moztrap?(fennec) → in-moztrap+
Blocks: 875242
Duplicate of this bug: 875242
Bug 875242 was closed as duplicate, but there was no uplift request for beta made. Should the patch be uplifted, since it's still reproducing on Firefox 22 Beta 5 on Samsung Galaxy R(Android 2.3.4)?
Flags: needinfo?(aaron.train)
Flags: needinfo?(aaron.train)
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.