Closed Bug 1398834 Opened 7 years ago Closed 7 years ago

Add ability to add custom top sites

Categories

(Firefox for Android Graveyard :: Awesomescreen, enhancement, P1)

All
Android
enhancement

Tracking

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

VERIFIED FIXED
Firefox 57
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

(Whiteboard: [mobileAS])

Attachments

(4 files)

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
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+
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. :)
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
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.