900234 - Force reader to always be the left most icon in page actions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

See description.

Reader mode is a good differentiator for us. We want to show it off.

Comment 1

[Shilpan Bhagat]

Attachment: Part 1: Replacing LinkedHashMap with ArrayList to allow insertions

Insertion is required for forcing priority pageactions to always remain visible. This patch replaces LinkedHashMap with an ArrayList for the same.

Comment 2

[Shilpan Bhagat]

Attachment: Part 1: Replacing LinkedHashMap with ArrayList to allow insertions

Forgot to clean up an integer. This should be it.

Comment 3

[Shilpan Bhagat]

Attachment: Part 2: Force reader mode to be the left most pageaction

With this patch, you can now set priority pageactions which will strive be visible over other pageactions. Currently reader mode is the only one and hence appears on the leftmost side of the page action layout.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 786418 [details] [diff] [review]
Part 1: Replacing LinkedHashMap with ArrayList to allow insertions

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

::: mobile/android/base/PageActionLayout.java
@@ +256,5 @@
>              mPageActionsMenu.inflate(0);
>              mPageActionsMenu.setOnMenuItemClickListener(new GeckoPopupMenu.OnMenuItemClickListener() {
>                  @Override
>                  public boolean onMenuItemClick(MenuItem item) {
> +                    mPageActionList.get(item.getItemId()).onClick();

Hmm.. does this cause issues if items are added/removed while the menu is showing?

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 786512 [details] [diff] [review]
Part 2: Force reader mode to be the left most pageaction

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

Looks pretty good to me. I just want to see the cleaned up insertAt algorithm.

::: mobile/android/base/PageActionLayout.java
@@ +121,5 @@
> +        // Add priority pageactions at the very end of the list.
> +        if (mPageActionList.size() == 0) {
> +            mPageActionList.add(pageAction);
> +        } else {
> +            for(int i =  mPageActionList.size() - 1; i >= 0; i--) {

This is all pretty confusing. Can we do something simpler?

int insertAt = mPageActionList.size() - 1;
while(insertAt > 0 && mPageActionList.get(insertAt).isImportant()) {
  insertAt--;
}
mPageActionList.add(insertAt, pageAction);

@@ +300,5 @@
>          private OnPageActionClickListeners mOnPageActionClickListeners;
>          private Drawable mDrawable;
>          private String mTitle;
>          private String mId;
> +        private boolean hasPriority;

Lets just call this priority... or important?

::: mobile/android/chrome/content/browser.js
@@ +1652,5 @@
>          type: "PageActions:Add",
>          id: id,
>          title: aOptions.title,
> +        icon: resolveGeckoURI(aOptions.icon),
> +        hasPriority: (aOptions.hasPriority != undefined) ? aOptions.hasPriority : false

I think we typically do "hasPriority" in aOptions ? aOptions.hasPriority : false

Comment 6

[Shilpan Bhagat]

Attachment: Part 1: Replacing LinkedHashMap with ArrayList to allow insertions

Carry forward from last r+ for part 1.

Comment 7

[Shilpan Bhagat]

Attachment: Part 2: Force reader mode to be the left most pageaction

Made changes as mentioned

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 789111 [details] [diff] [review]
Part 2: Force reader mode to be the left most pageaction

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

r- because this throws and fails

::: mobile/android/base/PageActionLayout.java
@@ +118,5 @@
> +    public void addPageAction(final String id, final String title, final String imageData, final OnPageActionClickListeners mOnPageActionClickListeners, boolean mImportant) {
> +        final PageAction pageAction = new PageAction(id, title, null, mOnPageActionClickListeners, mImportant);
> +
> +        int insertAt = mPageActionList.size();
> +        if (mPageActionList.size() > 0) {

Do you need this if? Won't insertAt > 0 cover this already?

@@ +119,5 @@
> +        final PageAction pageAction = new PageAction(id, title, null, mOnPageActionClickListeners, mImportant);
> +
> +        int insertAt = mPageActionList.size();
> +        if (mPageActionList.size() > 0) {
> +            while (insertAt > 0 && mPageActionList.get(insertAt).isImportant()) {

mPageActionList.get(insertAt) will fail when insertAt = size.

Comment 9

[Shilpan Bhagat]

Attachment: Part 2: Force reader mode to be the left most pageaction

With revised insertAt algorithm

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 790329 [details] [diff] [review]
Part 2: Force reader mode to be the left most pageaction

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

Nice and clean. Land and nom this for uplift too!

::: mobile/android/chrome/content/browser.js
@@ +1639,5 @@
>          type: "PageActions:Add",
>          id: id,
>          title: aOptions.title,
> +        icon: resolveGeckoURI(aOptions.icon),
> +        isImportant: "isImportant" in aOptions ? aOptions.isImportant : false

I'd probably rename this property "important" and drop the "is" part.

Comment 11

[Shilpan Bhagat]

Attachment: Part 2: Force reader mode to be the left most pageaction

Carry forward r+ with mentioned changes

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7b7b35e4350c
https://hg.mozilla.org/integration/fx-team/rev/a550304775f1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b7b35e4350c
https://hg.mozilla.org/mozilla-central/rev/a550304775f1

Comment 14

[Wesley Johnston (:wesj)]

Nom this for uplift.

Comment 15

[Wesley Johnston (:wesj)]

Hmm... Too late to uplift. This has shipped :)