Add ability to add custom top sites

VERIFIED FIXED in Firefox 57

Status

()

Firefox for Android
Awesomescreen
P1
normal
VERIFIED FIXED
5 months ago
4 months ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 57
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+, firefox55 unaffected, firefox56 unaffected, firefox57 verified)

Details

(Whiteboard: [mobileAS])

MozReview Requests

()

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

Attachments

(4 attachments)

Break out from bug 1334175 comment 6, see part 2: 

> Spoke with Bryan on Vidyo: we can break this into two parts:
> 
> 1) Long press -> Edit will allow a user to override the title of an existing
> top site (I'm thinking we have a map of url -> overridden title in
> sharedPrefs and don't bother with the complexities of browser DB titles).
> 
> 2) We add text and button to ^ dialog, "Did you want to add a completely new
> pinned site instead? [Yes]". This will let a user add a url and add a custom
> top site into the pinned section – afaik, we can add a pinned item (it's a
> bookmark with parent == pinned, if I'm not mistaken) without having to worry
> about "How many times was this visited?" and how that will affect how it is
> sorted since pinned sites are deterministically sorted everytime anyway.
> IDEALLY, we resurrect the old top site edit dialog – the awesomebar search –
> for this. But if not, it could be a manual url entry.
> 
> We came to this solution based on hashing out ideas and how easy I felt they
> would be to implement, given my understanding of the code. To me, this
> doesn't seem too bad and doable for 57 provided the other work doesn't take
> a ridiculous amount of time.
Rank 3: too much work for now.
Whiteboard: [mobileAS]
Chenxia figured out a simple way to do this and bbell thinks this is more important than bug 1334175 so I'm working on it now.
Assignee: nobody → michael.l.comella
Iteration: --- → 1.30
Rank: 3 → 0
Priority: P2 → P1
Created attachment 8907323 [details]
Screenshot post-patch
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
No longer depends on: 1334175
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8907324 [details]
Bug 1398834: Add pin/unpin context menu strings.

https://reviewboard.mozilla.org/r/178998/#review184132
Attachment #8907324 - Flags: review?(liuche) → review+
Comment on attachment 8907325 [details]
Bug 1398834: Add Pin to Top Sites page menu item.

https://reviewboard.mozilla.org/r/179000/#review184136

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3772
(Diff revision 2)
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run() {
> +                        item.setTitle(isPinned ?
> +                                R.string.contextmenu_unpin_from_top_sites : R.string.contextmenu_pin_to_top_sites);
> +                        item.setEnabled(true);

I wonder if there are situations that should not allow pinning? Going off of bookmarks I'm thinking maybe guest mode, but other than that I don't think there are other sites that shouldn't be pinned. about:home?
Attachment #8907325 - Flags: review?(liuche) → review+
Comment on attachment 8907325 [details]
Bug 1398834: Add Pin to Top Sites page menu item.

https://reviewboard.mozilla.org/r/179000/#review184138

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3772
(Diff revision 2)
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run() {
> +                        item.setTitle(isPinned ?
> +                                R.string.contextmenu_unpin_from_top_sites : R.string.contextmenu_pin_to_top_sites);
> +                        item.setEnabled(true);

This can also just be done in a follow-up, because I don't think Guest Mode is a commonly used feature.
Comment on attachment 8907359 [details]
Bug 1398834: Add telemetry for new browser menu pin site.

https://reviewboard.mozilla.org/r/179026/#review184140
Attachment #8907359 - Flags: review?(liuche) → review+
(Assignee)

Comment 13

5 months ago
mozreview-review-reply
Comment on attachment 8907325 [details]
Bug 1398834: Add Pin to Top Sites page menu item.

https://reviewboard.mozilla.org/r/179000/#review184136

> I wonder if there are situations that should not allow pinning? Going off of bookmarks I'm thinking maybe guest mode, but other than that I don't think there are other sites that shouldn't be pinned. about:home?

Good call.

It turns out the Page submenu is disabled on about:home so no need to address that case. As for guest mode, you can long press the top sites items and still pin them from the context menu (in addition to bookmarking them!) so I don't think it's worth addressing. But I'm biased because I think we should kill guest mode. :)

Comment 14

5 months ago
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d72893703bf7
Add pin/unpin context menu strings. r=liuche
https://hg.mozilla.org/integration/autoland/rev/885dbb7d0b17
Add Pin to Top Sites page menu item. r=liuche
https://hg.mozilla.org/integration/autoland/rev/cabd92c1a177
Add telemetry for new browser menu pin site. r=liuche
https://hg.mozilla.org/mozilla-central/rev/d72893703bf7
https://hg.mozilla.org/mozilla-central/rev/885dbb7d0b17
https://hg.mozilla.org/mozilla-central/rev/cabd92c1a177
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
Duplicate of this bug: 1359922

Comment 17

5 months ago
Verified as fixed in Beta 57.0b3.
Devices:
Asus ZenPad 8.0 Z380KL (Android 6.0.1)
LG G4 (Android 6.0)
Lenovo A536 (Android 4.4.2)

Question: Wasn't the "Pin site" string in the context menu (on Bookmarks/History panels) supposed to be "Pin to top sites" also?
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.