1377286 - Support pinning from other panels

Status: RESOLVED FIXED

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

Comment 1

[:Grisha Kruglov]

Since we have multiple styles of context menus (Bug 1377292), two types of pins (Bug 1377295), and two types of Top Sites, this gets a bit tricky. Code handling the menus needs to segment what its doing based on Top Sites panel style first, and pinned/not pinned state of a history item second. Essentially, we're looking at some code duplication between classic context menu and friends vs what A-S is doing.

All of this becomes progressively less hacky if we may assume that positioned pins are dead, Activity Stream is the one and only Top Sites UI, and that context menus are unified.

Comment 2

[:Grisha Kruglov]

Attachment: Bug 1377286 - Pre: firm up access levels in HomeContextMenuInfo

No functional change, simply tightening things up.

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

Comment 3

[:Grisha Kruglov]

Attachment: Bug 1377286 - Allow pinning items to Activity Stream from other home panels

Menu item to pin/unpin items from Bookmarks and History panels is only displayed when
Activity Stream is enabled.

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

Comment 4

[:Grisha Kruglov]

Comment on attachment 8883704 [details]
Bug 1377286 - Allow pinning items to Activity Stream from other home panels

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

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8883703 [details]
Bug 1377286 - Pre: firm up access levels in HomeContextMenuInfo

https://reviewboard.mozilla.org/r/154604/#review160794

::: mobile/android/base/java/org/mozilla/gecko/home/HomeContextMenuInfo.java:25
(Diff revision 1)
>   */
>  public class HomeContextMenuInfo extends AdapterContextMenuInfo {
>  
>      public String url;
>      public String title;
> -    public boolean isFolder;
> +    boolean isFolder;

nit: In some other classes we prefix them with a comment:

> /* package */

or

> /* package-private */

Just to make it explicit that this is a deliberate decission and not a mistake.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8883704 [details]
Bug 1377286 - Allow pinning items to Activity Stream from other home panels

https://reviewboard.mozilla.org/r/154606/#review160796

::: mobile/android/base/java/org/mozilla/gecko/home/HomeContextMenuInfo.java:31
(Diff revision 2)
> +    // Ensure memory visibility, as this is accessed non-concurrently from different threads.
> +    // See HomeFragment's onCreateContextMenu.
> +    @Nullable volatile Boolean isAsPinned;

Isn't this only accessed on the UI thread?

Comment 7

[:Grisha Kruglov]

Comment on attachment 8883703 [details]
Bug 1377286 - Pre: firm up access levels in HomeContextMenuInfo

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

Comment 8

[:Grisha Kruglov]

Comment on attachment 8883704 [details]
Bug 1377286 - Allow pinning items to Activity Stream from other home panels

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

Comment 9

[:Grisha Kruglov]

Comment on attachment 8883704 [details]
Bug 1377286 - Allow pinning items to Activity Stream from other home panels

https://reviewboard.mozilla.org/r/154606/#review160796

> Isn't this only accessed on the UI thread?

You are correct!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6b2a8854efc
Pre: firm up access levels in HomeContextMenuInfo r=sebastian
https://hg.mozilla.org/integration/autoland/rev/107cb0545ba3
Allow pinning items to Activity Stream from other home panels r=sebastian

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6b2a8854efc
https://hg.mozilla.org/mozilla-central/rev/107cb0545ba3