734877 - Add support for 'PageActions' so add-ons can add indicators to the URLBar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Firefox and Chrome both allow add-ons to easily add actions/indicators to the URL bar itself, mainly as images the appear inside the right side of the URL textbox.

We can add a simple API to NativeWindow which would allow that in Native Fennec as well. The actions are tab-centric, so actions could show/hide as you change tabs, in addition to loading new pages in a tab. Actions have an icon, text and a click callback.

What we need is a few UX rules for how to support them in the UI:
* We are space constrained already, how do we handle "too many actions"? Limit the visible number and use a overflow dropdown list for the rest? For tablets and landscape mode, we might want to show more than the minimum limit of indicators.
* The site security "lock" indicator is already on the right edge of the URLbar. How do we not confuse add-on indicators with site security indicators?

Comment 1

[Paul [pwd]]

Is there a target milestone for this? Getting ABP for example on Fennec could be a huge marketing win.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

I decided to start playing with this to see what we could do. This is an early WIP. Its pretty similar to the NativeWindow.menu api (in fact, I wonder if we should just combine the two and allow you to say which menu to show this item in...). It basically allows us to show 1 item at a time. If there's more than one thing to show, the native ui falls back to showing a "menu" button in the urlbar. API is something like:

var id = NativeWindow.pageactions.add(title, icon, visibilityCallback, callback);
NativeWindow.pageactions.remove(id);

visibilityCallback is called on DOMContentLoaded to determine whether or not the item is enabled. I was hoping that would avoid a lot of jitter in the urlbar as things declared themselves shown/hidden. Needs some work to ensure it works well with things that take a long time to determine if they should show or not (like reader mode, which I kinda hacked in here). It also needs to fire on LocationChange (i.e. tab switching).

Comment 3

[Wesley Johnston (:wesj)]

Assigning to Shilpan. Lets find some slightly simpler bugs to start out on, but wanted to make sure you had this one ready for a little later.

This patch is so bitrotted, I wouldn't try to apply it. Last I talked to Mark, I think we want a much simpler API that what I was trying to do here. I think we'd like something more like our current menu api:

https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/menu/

i.e. no visibilityCallback. Addons would add an item, and then would be required to do all the updating enabled/diabled/etc states on their own through some sort of NativeWindow.pageactions.update command.

Comment 4

[Shilpan Bhagat]

Attachment: First patch for the page actions feature

- Added the pageaction feature.
- Has two simple apis for adding and removing
   var id = NativeWindow.pageactions.add(title, image, callback)
   NativeWindow.pageactions.remove(id)

Points to discuss: 
- Which popup menu to use? (Currently using geckopopupmenu)

Comment 5

[Shilpan Bhagat]

Attachment: First patch for the page actions feature

Comment 6

[:Margaret Leibovic]

I haven't looked closely at the GeckoPopupMenu code, but I would think it would be nice to try to use the new ArrowPopup I'm working on in bug 751205. However, that needs a bit more work before it's going to land, so maybe we can just transition to use that in a follow-up bug (it would also be nice to have GeckoPopupMenu use it).

I also think you should put the page actions logic in it's own file, instead of adding it to BrowserToolbar, since that file is already pretty big/complicated.

Comment 7

[Shilpan Bhagat]

Attachment: Page actions screenshot (multiple pageactions represented as a menu pop up)

Comment 8

[Wesley Johnston (:wesj)]

Lets ping ian to start looking at UX here.

Comment 9

[Paul [pwd]]

Comment on attachment 763632 [details]
Page actions screenshot (multiple pageactions represented as a menu pop up)

Three dots would be terribly confusing for devices without a menu button. I'd suggest something like a double or even the single arrow we currently use within the menus.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 763395 [details] [diff] [review]
First patch for the page actions feature

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

Looking good. Some thoughts.

::: mobile/android/base/BrowserToolbar.java
@@ +143,5 @@
>      private boolean mShowUrl;
>  
>      private Integer mPrefObserverId;
>  
> +    private PageActionList mPageActionList;

I wonder if we can factor all of this out into a custom View. Just to keep BrowserToolbar from becoming more bloated...

@@ +349,3 @@
>                  Tab tab = Tabs.getInstance().getSelectedTab();
> +                if (tab != null)
> +                    inReaderMode = ReaderModeUtils.isAboutReader(tab.getURL());

Hmmm... I really hoped this didn't have to know anything about reader. Can we factor reader out of BrowserToolbar as well? It can register itself for Tab events and automatically add/remove/change icon etc. Just like we expect js actions to do?

TBH, I'd like to just move reader to JS if we can....

@@ +1258,5 @@
> +    public void handleMessage(String event, JSONObject message) {
> +        try {
> +            if (event.equals("PageActions:Add")) {
> +                String title = message.getString("title");
> +                String bitmapString = message.getString("bitmap");

You should probably use optString("bitmap") if this is optional.

@@ +1262,5 @@
> +                String bitmapString = message.getString("bitmap");
> +                Drawable image = null;
> +
> +                if (!TextUtils.isEmpty(bitmapString)) {
> +                    image = new BitmapDrawable(GeckoAppShell.getContext().getResources(), BitmapUtils.getBitmapFromDataURI(bitmapString));

I want to abstract this out a bit so that we just have BitmapUtils.getDrawable(string, callback); That guy can take the string and open 1.) Android drawable resources, 2.) chrome uri's, 3.) http uris, or 4.) data uris.

@@ +1277,5 @@
> +    }
> +
> +    @Override
> +    public String getResponse(JSONObject response) {
> +        return Integer.toString(mPageActionList.valueAt(mPageActionList.size() - 1).getID());

I'd rather just remove this and let the javascript callers get their ID from gecko.

::: mobile/android/chrome/content/browser.js
@@ +309,5 @@
>      // the "Press ESC to exit" warning message.
>      window.addEventListener("MozShowFullScreenWarning", showFullScreenWarning, true);
>  
>      NativeWindow.init();
> +    NativeWindow.pageactions.add("Custom Item 1", "", function() {

These should probably return a uuid of some sort that you can hold on to to keep track of your id. Otherwise, multiple people trying to use this will hit lots of races.

@@ +316,5 @@
> +    NativeWindow.pageactions.add("Custom Item 2", "", function() {
> +      alert("Test 2");
> +    });
> +    NativeWindow.pageactions.remove(1);
> +    NativeWindow.pageactions.remove(2);

Good for testing. Remember to remove in the end.

@@ +1577,5 @@
> +        callback: callback,
> +      };
> +      return id;
> +    },
> +    remove: function(id) {

Delete the callback from our item list here.

Comment 11

[Lucas Rocha (:lucasr)]

Any test builds around? Just curious.

Comment 12

[Ian Barlow (:ibarlow)]

Attachment: Mockup of page actions in URL bar

Sorry these designs are coming in so late here. I think generally this work is on the right track but I wanted to make a few suggestions here that might make this a little more useful.

* Allow a maximum of 2 icons on the right (not just 1)
* If more than 2, show the first icon + an overflow arrow that pops open a menu.

Comment 13

[Shilpan Bhagat]

Attachment: [WIP] Page action as per the new design with custom page action button view

The page action can be added with:
- id = NativeWindow.pageactions.add(title, icon, clickcallback [, longclickcallback])

This returns an id.

- NativeWindow.pageactions.remove(id) removes the pageaction.

Sorry for the huge patch. This patch takes care of calling the reader mode from the js along with the design implementation as shown in the above picture.

Waiting on ibarlow to send in the down arrow to finish up on the look completely.

Comment 14

[Shilpan Bhagat]

Attachment: [WIP] Page action as per the new design with custom page action button view (Revision)

Oops, forgot to add the custom view to the patch. This is all it now.

Comment 15

[Ian Barlow (:ibarlow)]

Attachment: graphics for page action icon (mdpi/hdpi/xhdpi)

Comment 16

[Shilpan Bhagat]

Attachment: [WIP] Page action as per the new design with custom page action button view (w/ updated graphics)

Patch with updated menu image.

Comment 17

[Shilpan Bhagat]

Attachment: Screenshot of working page actions

Adding a screenshot to show how page actions look currently.

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 766990 [details] [diff] [review]
[WIP] Page action as per the new design with custom page action button view (Revision)

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

This is coming along well. I have some questions/ideas, and I want to loop lucas in to talk about the reader mode changes. I don't love it the way it is. The question in my mind is, do we create a Java API so that we can leave the reader mode bits where they are, or do we just move it to js, or do we do this hybrid approach here and move it in a separate bug?

I don't really want to hardcode these two buttons here. Seems like it will make it hard to change that number in the future. I'd like to create something that's more like a PageActionsBar and dynamically add/remove PageActionsButtons (maybe those can just be Buttons then?) as we need them.

::: mobile/android/base/BrowserToolbar.java
@@ +100,5 @@
>      public ImageButton mFavicon;
>      public ImageButton mStop;
>      public ImageButton mSiteSecurity;
> +    public PageActionButton mPageActionButtonOne;
> +    public PageActionButton mPageActionButtonTwo;

Hmm... Do we need these hardcoded?

@@ +115,5 @@
>      private Handler mHandler;
>      private boolean mHasSoftMenuButton;
>  
>      private boolean mShowSiteSecurity;
>      private boolean mShowReader;

Do we still need this?

@@ +193,5 @@
>  
> +        mPageActionHandler = new Handler();
> +
> +        registerEventListener("PageActions:Add");
> +        registerEventListener("PageActions:Remove");

Can the PageActionsButton widget listen for these instead?

@@ +447,5 @@
>                  }
>              }
>          }
>  
> +        mFocusOrder = Arrays.asList(mBack, mForward, mLayout, mPageActionButtonOne, mPageActionButtonOne, mSiteSecurity, mStop, mTabs);

This part might be harder to do with dynamically added buttons...

@@ +1244,5 @@
> +            } else if (event.equals("PageActions:Remove")) {
> +                String id = message.getString("id");
> +
> +                PageActionButton.removePageAction(id);
> +                mPageActionHandler.post(new Runnable() {

I don't think we need this handler (I don't think we need mHandler). Can you use ThreadUtils.postToUiThread.

::: mobile/android/base/PageActionButton.java
@@ +28,5 @@
> +	private int mDisplayNumber;
> +	private String LOGTAG = "GeckoPageActionButton";
> +
> +	private static final int FIRST = 0;
> +	private static final int SECOND = 1;

Yeah. Not in love with this.

@@ +112,5 @@
> +	        break;
> +    	case SECOND:
> +   			if(mPageActionList.size() > 1)
> +   				return mPageActionList.valueAt(mPageActionList.size() - 1).getClickMethods().onLongClick();
> +   			break;

there's an extra level of indentation here

@@ +219,5 @@
> +        private OnClickMethods mClickMethods;
> +        private Drawable mDrawable;
> +        private String mTitle;
> +        private String mID;
> +        public PageAction(String title, Drawable image, OnClickMethods mClickMethods, String id) {

id should probably be the first argument here

@@ +227,5 @@
> +            this.mClickMethods = mClickMethods;
> +        }
> +
> +        public OnClickMethods getClickMethods() {
> +            return mClickMethods;

mClickListner and OnClickListener?

::: mobile/android/chrome/content/browser.js
@@ +318,5 @@
> +      alert("Test 1");
> +    });
> +    NativeWindow.pageactions.add("Custom Item 2", "drawable://home_star", function() {
> +      alert("Test 2");
> +    });*/

Yeah, remove in final patch, but fine for now.

@@ +1592,5 @@
>    },
>  
> +  pageactions: {
> +    _items: { },
> +    add: function(title, bitmap, clickCallback, longClickCallback) {

For new api's, we're better to just pass a single options object. i.e.:

add: function(aOptions) {

where aOptions = {
  title: "title",
  bitmap: "b",
  clickCallback: function() { },
  longClickCallback: function() { }
}

@@ +1760,5 @@
> +        this.readerModePageAction.id = this.pageactions.add("Reader", "drawable://reader", this.readerModePageAction.readerModeCallback, this.readerModePageAction.readerModeActiveCallback);
> +    } else if(aTopic == "ReaderPageAction:Active") {
> +        this.readerModePageAction.id = this.pageactions.add("Reader Active", "drawable://reader_active", this.readerModePageAction.readerModeCallback);
> +    } else if(aTopic == "ReaderPageAction:Remove") {
> +        this.pageactions.remove(this.readerModePageAction.id);

I'm pretty sure we can do this much more efficiently. Lets ping lucasr to make sure he sees this. Maybe we're best to leave it in place for now and make it better in a follow up? (i.e. this patch is already kinda big...)

Comment 19

[Paul [pwd]]

(In reply to Wesley Johnston (:wesj) from comment #18)
> This is coming along well. I have some questions/ideas, and I want to loop
> lucas in to talk about the reader mode changes. I don't love it the way it
> is. The question in my mind is, do we create a Java API so that we can leave
> the reader mode bits where they are, or do we just move it to js, or do we
> do this hybrid approach here and move it in a separate bug?

What is the usage percentage of reader mode at present in regards to total users? If it's low enough, an alternative to the above could be that we do for reader mode what we're doing for Panorama on desktop, make it into an add-on that installs automatically for active users.

Comment 20

[Ian Barlow (:ibarlow)]

(In reply to Shilpan Bhagat from comment #17)
> Created attachment 767296 [details]
> Screenshot of working page actions
> 
> Adding a screenshot to show how page actions look currently.

Looking good -- just a few visual tweaks and we are good to go! http://cl.ly/image/0E190u0y2V2r

Comment 21

[Ian Barlow (:ibarlow)]

(In reply to Paul [sabret00the] from comment #19)

> What is the usage percentage of reader mode at present in regards to total
> users? If it's low enough, an alternative to the above could be that we do
> for reader mode what we're doing for Panorama on desktop, make it into an
> add-on that installs automatically for active users.

Reader Mode is a key differentiator for us on Firefox for Android, and a heavily used feature. There are zero plans to change it to an add-on.

Comment 22

[Paul [pwd]]

(In reply to Ian Barlow (:ibarlow) from comment #21)
> Reader Mode is a key differentiator for us on Firefox for Android, and a
> heavily used feature. There are zero plans to change it to an add-on.

Ah, that data isn't available on the telemetry that I can see and so I'll have to take your word for it.

Comment 23

[Wesley Johnston (:wesj)]

Lucas, see above ^. Can you provide any hints as to how to integrate reader into this?

Comment 24

[Shilpan Bhagat]

Attachment: [WIP] Page action as per the new design with custom page action button view (w/ custom PageActionLayout view)

After talking to Wes, introduced a layout to remove the hard coded buttons. Moving pageaction event handlers to the layout and made the mentioned changes to the code.

Comment 25

[Shilpan Bhagat]

Attachment: [WIP] patch for pageactions (w/ updated design and few code improvements)

- Event listeners for reader pageactions are now gone from browser.js
- Few tweaks to the positioning of the pageaction buttons
- Arrow points just right

Comment 26

[Shilpan Bhagat]

Attachment: Screen shot of page actions

The stars that you see besides the page actions are simply place-holder icons. In the event that you do not want an icon, do not give a source/url for the image while calling NativeWindow.pageactions.add and the icon won't be visible

Comment 27

[Wesley Johnston (:wesj)]

Comment on attachment 768065 [details] [diff] [review]
[WIP] patch for pageactions (w/ updated design and few code improvements)

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

Nice work! This is starting to look really good! But I think it can be simplified a bit more, and we need to be a little more careful about the js reader mode bits.

Some questions/comments:

::: mobile/android/base/BrowserToolbar.java
@@ +242,5 @@
>          });
>  
>          mShowSiteSecurity = false;
>          mShowReader = false;
> +        mInReaderMode = false;

Can we kill this? You added it in this patch, and update it here, but I don't see it used anywhere else?

@@ +462,5 @@
>                      Boolean showProgress = (Boolean)data;
>                      if (showProgress && tab.getState() == Tab.STATE_LOADING)
>                          setProgressVisibility(true);
>                      setSecurityMode(tab.getSecurityMode());
> +                    setPageActionVisibility(mStop.getVisibility() == View.VISIBLE);

Is Stop a page action? I'm a bit confused by this.

@@ +823,2 @@
>  
> +        mPageActionLayout.setVisibility(!isLoading ? View.VISIBLE : View.GONE);

Hmm... interesting. I wonder if there are states where we're loading and we do want page actions to show....

@@ +1186,5 @@
> +    public void handleMessage(String event, JSONObject message) {
> +        if (event.equals("ReaderPageAction:Click")) {
> +            Tab tab = Tabs.getInstance().getSelectedTab();
> +            if (tab != null)
> +                tab.toggleReaderMode();

braces

::: mobile/android/base/PageActionLayout.java
@@ +52,5 @@
> +        registerEventListener("PageActions:Add");
> +        registerEventListener("PageActions:Remove");
> +    }
> +
> +    public void totalPageActionsButtons(int count) {

I think we can name this better. Something like setNumberShown(int)?

@@ +75,5 @@
> +        mPageActionList.getInstance().remove(id);
> +        refreshPageActionIcons();
> +    }
> +
> +    public ImageButton createImageButton(Drawable d) {

This function might be better inside the PageAction itself. i.e. we'd just call:

mLayout.addView(PageAction.getButton());

PageAction itself can even be a OnClickListener and OnLongClickListener itself...

Also, you need to add a setContentDescription line here so that this works well for screen readers.

@@ +80,5 @@
> +        ImageButton imageButton = new ImageButton(mContext, null, R.style.AddressBar_ImageButton_Icon);
> +        imageButton.setLayoutParams(new LayoutParams(mContext.getResources().getDimensionPixelSize(R.dimen.page_action_button_width), LayoutParams.MATCH_PARENT));
> +        imageButton.setScaleType(ImageView.ScaleType.CENTER_INSIDE);
> +        imageButton.setImageDrawable(d);
> +        imageButton.setTag(this.getChildCount());

Do you need this tag? Can we instead just pass the id in, and then call performClick(id);

@@ +122,5 @@
> +
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run () {
> +                        addPageAction(id, title, bitmapString);

We may not have a bitmap yet, right? I'd avoid running anything on the UI thread until we do.

@@ +150,5 @@
> +                mPageActionList.valueAt(0).getClickMethods().onClick();
> +            else if(mPageActionList.size() >  mMaxVisiblePageActions)
> +                mPageActionList.showMenu(v, mPageActionList.size() - mMaxVisiblePageActions + 1);
> +        }
> +        else {

put this on one line:

} else {

@@ +182,5 @@
> +                v.setImageDrawable((mPageActionList.size() > buttonNumber) ? mPageActionList.valueAt(mPageActionList.size() - mMaxVisiblePageActions + buttonNumber).getDrawable() : null);
> +        }
> +    }
> +
> +	@Override

Indentation is off here.

@@ +195,5 @@
> +            v.setVisibility((visibility == View.VISIBLE) ? (((Integer)v.getTag() < mPageActionList.size()) ? View.VISIBLE : View.GONE) : View.GONE);
> +        }
> +    }
> +
> +    private interface OnClickMethods {

OnClickMethods -> ClickListener but I'm not sure we need this. When the Button is created, the page action can just register itself as a listener?

@@ +200,5 @@
> +        public void onClick();
> +        public boolean onLongClick();
> +    }
> +
> +    private static class OnPageActionClick implements OnClickMethods {

OnPageActionClick -> PageActionClickListener

@@ +218,5 @@
> +            return true;
> +        }
> +    }
> +
> +    private static class PageActionList {

I'd like to kill this and instead store a LinkedHashMap<int, PageAction> if we can. Can we?

@@ +236,5 @@
> +	    private static class PageActionListHolder {
> +	    	private static final PageActionList INSTANCE = new PageActionList();
> +	    }
> +
> +	    public static PageActionList getInstance() {

Indentation is off here too.

@@ +283,5 @@
> +                });
> +            }
> +            Menu menu = mPageActionsMenu.getMenu();
> +            menu.clear();
> +            for (int i = 0; i < toShow; i++) {

Hmm... this gets a bit harder if we just use a LinkedHashMap, but at least it should maintain order for the keys so we should be able to smartly skip the first one...

@@ +285,5 @@
> +            Menu menu = mPageActionsMenu.getMenu();
> +            menu.clear();
> +            for (int i = 0; i < toShow; i++) {
> +                PageAction pageAction = valueAt(i);
> +                MenuItem item = menu.add(Menu.NONE, mPageActions.keyAt(i), Menu.NONE, pageAction.getTitle());

Similarly, it'd be nice to go pageAction.addToMenu(menu);

::: mobile/android/base/gfx/BitmapUtils.java
@@ +76,5 @@
> +                @Override
> +                public void onPostExecute(Drawable drawable) {
> +                    loader.onBitmapFound(drawable);
> +                }
> +            }).execute();

Do you need to return early here?

::: mobile/android/base/widget/GeckoPopupMenu.java
@@ +90,5 @@
>       * Inflates a menu resource to the menu using the menu inflater.
>       *
>       * @param menuRes The menu resource to be inflated.
>       */
> +    public void inflate(int ... menuRes) {

I think I'd rather just make this check if menuRes < 0. Does Android allow negative resource ids?

@@ +92,5 @@
>       * @param menuRes The menu resource to be inflated.
>       */
> +    public void inflate(int ... menuRes) {
> +        if(menuRes.length == 1)
> +            mMenuInflater.inflate(menuRes[0], mMenu);

We use braces on single line ifs in java (but not in js, yes its confusing to everyone).

::: mobile/android/chrome/content/browser.js
@@ +1627,5 @@
> +        gecko: {
> +          type: "ReaderPageAction:LongClick",
> +        }
> +      });
> +    },

I would move this into the Reader object (also in browser.js)

@@ +3521,5 @@
>              // use the article from the previous page
>              if (!tabURL.startsWith("about:reader"))
>                this.savedArticle = null;
> +            else
> +              NativeWindow.readerModePageAction.id = NativeWindow.pageactions.add({title: "Reader Active", icon: "drawable://reader_active", clickCallback: NativeWindow.readerModePageAction.readerModeCallback});

Split these objects up. i.e.:
{
  title: "Title",
  icon: ""
}

Also, you need to localize these strings. We can just set them from a StringBundle:
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIStringBundle

The one we use most often is browser.properties:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties

which in browser.js is mapped to Strings.browser. i.e. you can call:
Strings.browser.getStringFromName("my.name");

@@ +3532,5 @@
>              type: "Content:ReaderEnabled",
>              tabID: this.id
>            });
> +
> +          NativeWindow.readerModePageAction.id = NativeWindow.pageactions.add({title: "Reader", icon: "drawable://reader", clickCallback: NativeWindow.readerModePageAction.readerModeCallback, longClickCallback: NativeWindow.readerModePageAction.readerModeActiveCallback});

Yeah, it bugs me we have these two messages right next to each other that both tell java reader is enabled, but like we've talked about, lets fix that in a second patch.

We can store this id in the Reader object though:
Reader.pageAction = NativeWindow.pageactions.add();
or maybe even better

and
if (Reader.pageAction)
NativeWindow.pageactions.remove(Reader.pageAction);

Also, does this work if you flip back and forth between some open tabs (or swipe closed a reader mode tab and wind up on a non-reader mode tab?). Maybe we can do this in the Tab.setActive and Tab.getActive methods. So here we'd set:

tab.readerEnabled = true/false;

and in other places

Tab.prototype = {
  set readerEnabled(val) {
    this._readerEnabled = val;
    if (this.getActive())
      Reader.updatePageAction(val);
  },
  get readerEnabled() {
    return this._readerEnabled;
  },
  setActive: function(active) {
    // other things
    Reader.updatePageAction(this.readerEanbled);
  },
  onLocationChange: function() {
    // other things?
    this.readerEnabled = false;
  },
}

and:

Reader.updatePageAction = function(tab) {
  if (tab.readerEnabled) {
    this.pageAction = NativeWindow.pageactions.add(..);
  } else if (this.pageAction) {
    NativeWindow.pageactions.remove(this.pageAction);
    delete this.pageAction;
  }
}

Comment 28

[Ian Barlow (:ibarlow)]

(In reply to Shilpan Bhagat from comment #26)
> Created attachment 768072 [details]
> Screen shot of page actions
> 
> The stars that you see besides the page actions are simply place-holder
> icons. In the event that you do not want an icon, do not give a source/url
> for the image while calling NativeWindow.pageactions.add and the icon won't
> be visible

Nailed it! Looks great :)

Comment 29

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #23)
> Lucas, see above ^. Can you provide any hints as to how to integrate reader
> into this?

Replace the Reader:Enabled message with the new actions API?

Comment 30

[Shilpan Bhagat]

(In reply to Wesley Johnston (:wesj) from comment #27)
> Comment on attachment 768065 [details] [diff] [review]
> [WIP] patch for pageactions (w/ updated design and few code improvements)
> 
> Review of attachment 768065 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice work! This is starting to look really good! But I think it can be
> simplified a bit more, and we need to be a little more careful about the js
> reader mode bits.
> 
> Some questions/comments:
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +242,5 @@
> >          });
> >  
> >          mShowSiteSecurity = false;
> >          mShowReader = false;
> > +        mInReaderMode = false;
> 
> Can we kill this? You added it in this patch, and update it here, but I
> don't see it used anywhere else?
> 
Yup, should've seen that. Killed.
> @@ +462,5 @@
> >                      Boolean showProgress = (Boolean)data;
> >                      if (showProgress && tab.getState() == Tab.STATE_LOADING)
> >                          setProgressVisibility(true);
> >                      setSecurityMode(tab.getSecurityMode());
> > +                    setPageActionVisibility(mStop.getVisibility() == View.VISIBLE);
> 
> Is Stop a page action? I'm a bit confused by this.
>
It's not. It's the "cross" sign that you see when the page is loading. I make the page actions invisible when the page is loading. If not, the "x" will come besides the page actions and take up more space. I don't know if this is desired. Reader mode previously did the same thing. 
> @@ +823,2 @@
> >  
> > +        mPageActionLayout.setVisibility(!isLoading ? View.VISIBLE : View.GONE);
> 
> Hmm... interesting. I wonder if there are states where we're loading and we
> do want page actions to show....
>
Same explanation as above 
> @@ +1186,5 @@
> > +    public void handleMessage(String event, JSONObject message) {
> > +        if (event.equals("ReaderPageAction:Click")) {
> > +            Tab tab = Tabs.getInstance().getSelectedTab();
> > +            if (tab != null)
> > +                tab.toggleReaderMode();
> 
> braces
>
No more indented 'if' blocks. got it. 
> ::: mobile/android/base/PageActionLayout.java
> @@ +52,5 @@
> > +        registerEventListener("PageActions:Add");
> > +        registerEventListener("PageActions:Remove");
> > +    }
> > +
> > +    public void totalPageActionsButtons(int count) {
> 
> I think we can name this better. Something like setNumberShown(int)?
>
Done 
> @@ +75,5 @@
> > +        mPageActionList.getInstance().remove(id);
> > +        refreshPageActionIcons();
> > +    }
> > +
> > +    public ImageButton createImageButton(Drawable d) {
> 
> This function might be better inside the PageAction itself. i.e. we'd just
> call:
> 
> mLayout.addView(PageAction.getButton());
> 
> PageAction itself can even be a OnClickListener and OnLongClickListener
> itself...
>
Initially I thought of this. But pageaction itself is an element of a data structure (PageActionList) based on which the buttons are added. Also, adding a pageaction does not guarantee an addition of an ImageButton. 

PageActionList itself is a data structure based on which the layout decides if views need to be added or not. Hence I did not want to add any methods here which are related to the visual aspect of pageactions. I figured the layout can handle all the addition of ImageButtons.

Secondly, the onClick listener and how it responds is largely dependent on the number of page actions currently added. Would it make sense to move it into pageaction? Primarily since we want the button to also convert into a menu button incase the number of pageactions increase more than the number of allowed buttons

Isolating the views from the data kind of optimizes things, in the sense that we would not have to add/remove views based on the addition/removal of page actions. We can simply have the image button dynamically respond to the active pageaction and adapt to their onclick, onlongclick and icon 
> Also, you need to add a setContentDescription line here so that this works
> well for screen readers.
>
I did not quite get what you mean here 
> @@ +80,5 @@
> > +        ImageButton imageButton = new ImageButton(mContext, null, R.style.AddressBar_ImageButton_Icon);
> > +        imageButton.setLayoutParams(new LayoutParams(mContext.getResources().getDimensionPixelSize(R.dimen.page_action_button_width), LayoutParams.MATCH_PARENT));
> > +        imageButton.setScaleType(ImageView.ScaleType.CENTER_INSIDE);
> > +        imageButton.setImageDrawable(d);
> > +        imageButton.setTag(this.getChildCount());
> 
> Do you need this tag? Can we instead just pass the id in, and then call
> performClick(id);
> 
Since buttons are ordered, it is required for the onclicklistener to know which button was pressed and its current order. It is for this reason, that this tag is required. ids or their hashcode cannot be used since they aren't ordered.
> @@ +122,5 @@
> > +
> > +                ThreadUtils.postToUiThread(new Runnable() {
> > +                    @Override
> > +                    public void run () {
> > +                        addPageAction(id, title, bitmapString);
> 
> We may not have a bitmap yet, right? I'd avoid running anything on the UI
> thread until we do.
> 
Done.
> @@ +150,5 @@
> > +                mPageActionList.valueAt(0).getClickMethods().onClick();
> > +            else if(mPageActionList.size() >  mMaxVisiblePageActions)
> > +                mPageActionList.showMenu(v, mPageActionList.size() - mMaxVisiblePageActions + 1);
> > +        }
> > +        else {
> 
> put this on one line:
> 
> } else {
> 
Done. Dunno how i overlooked that
> @@ +182,5 @@
> > +                v.setImageDrawable((mPageActionList.size() > buttonNumber) ? mPageActionList.valueAt(mPageActionList.size() - mMaxVisiblePageActions + buttonNumber).getDrawable() : null);
> > +        }
> > +    }
> > +
> > +	@Override
> 
> Indentation is off here.
> 
Done
> @@ +195,5 @@
> > +            v.setVisibility((visibility == View.VISIBLE) ? (((Integer)v.getTag() < mPageActionList.size()) ? View.VISIBLE : View.GONE) : View.GONE);
> > +        }
> > +    }
> > +
> > +    private interface OnClickMethods {
> 
> OnClickMethods -> ClickListener but I'm not sure we need this. When the
> Button is created, the page action can just register itself as a listener?
> 
Took it out. I had implemented this earlier to accommodate reader mode. Not required anymore
> @@ +200,5 @@
> > +        public void onClick();
> > +        public boolean onLongClick();
> > +    }
> > +
> > +    private static class OnPageActionClick implements OnClickMethods {
> 
> OnPageActionClick -> PageActionClickListener
> 
> @@ +218,5 @@
> > +            return true;
> > +        }
> > +    }
> > +
> > +    private static class PageActionList {
> 
> I'd like to kill this and instead store a LinkedHashMap<int, PageAction> if
> we can. Can we?
> 
Based on our talk, PageActionList simply acting as a data structure with certain methods that are important to removing pageactions. I can remove it if you want. Let me know if the current implementation still looks bothersome.
> @@ +236,5 @@
> > +	    private static class PageActionListHolder {
> > +	    	private static final PageActionList INSTANCE = new PageActionList();
> > +	    }
> > +
> > +	    public static PageActionList getInstance() {
> 
> Indentation is off here too.
>
Done 
> @@ +283,5 @@
> > +                });
> > +            }
> > +            Menu menu = mPageActionsMenu.getMenu();
> > +            menu.clear();
> > +            for (int i = 0; i < toShow; i++) {
> 
> Hmm... this gets a bit harder if we just use a LinkedHashMap, but at least
> it should maintain order for the keys so we should be able to smartly skip
> the first one...
> 
> @@ +285,5 @@
> > +            Menu menu = mPageActionsMenu.getMenu();
> > +            menu.clear();
> > +            for (int i = 0; i < toShow; i++) {
> > +                PageAction pageAction = valueAt(i);
> > +                MenuItem item = menu.add(Menu.NONE, mPageActions.keyAt(i), Menu.NONE, pageAction.getTitle());
> 
> Similarly, it'd be nice to go pageAction.addToMenu(menu);
> 
> ::: mobile/android/base/gfx/BitmapUtils.java
> @@ +76,5 @@
> > +                @Override
> > +                public void onPostExecute(Drawable drawable) {
> > +                    loader.onBitmapFound(drawable);
> > +                }
> > +            }).execute();
> 
> Do you need to return early here?
> 
Yeah, done.
> ::: mobile/android/base/widget/GeckoPopupMenu.java
> @@ +90,5 @@
> >       * Inflates a menu resource to the menu using the menu inflater.
> >       *
> >       * @param menuRes The menu resource to be inflated.
> >       */
> > +    public void inflate(int ... menuRes) {
> 
> I think I'd rather just make this check if menuRes < 0. Does Android allow
> negative resource ids?
> 
Done
> @@ +92,5 @@
> >       * @param menuRes The menu resource to be inflated.
> >       */
> > +    public void inflate(int ... menuRes) {
> > +        if(menuRes.length == 1)
> > +            mMenuInflater.inflate(menuRes[0], mMenu);
> 
> We use braces on single line ifs in java (but not in js, yes its confusing
> to everyone).
> 
got it
> ::: mobile/android/chrome/content/browser.js
> @@ +1627,5 @@
> > +        gecko: {
> > +          type: "ReaderPageAction:LongClick",
> > +        }
> > +      });
> > +    },
> 
> I would move this into the Reader object (also in browser.js)
> 
Done
> @@ +3521,5 @@
> >              // use the article from the previous page
> >              if (!tabURL.startsWith("about:reader"))
> >                this.savedArticle = null;
> > +            else
> > +              NativeWindow.readerModePageAction.id = NativeWindow.pageactions.add({title: "Reader Active", icon: "drawable://reader_active", clickCallback: NativeWindow.readerModePageAction.readerModeCallback});
> 
> Split these objects up. i.e.:
> {
>   title: "Title",
>   icon: ""
> }
> 
> Also, you need to localize these strings. We can just set them from a
> StringBundle:
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIStringBundle
> 
> The one we use most often is browser.properties:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/
> chrome/browser.properties
> 
> which in browser.js is mapped to Strings.browser. i.e. you can call:
> Strings.browser.getStringFromName("my.name");
> 
> @@ +3532,5 @@
> >              type: "Content:ReaderEnabled",
> >              tabID: this.id
> >            });
> > +
> > +          NativeWindow.readerModePageAction.id = NativeWindow.pageactions.add({title: "Reader", icon: "drawable://reader", clickCallback: NativeWindow.readerModePageAction.readerModeCallback, longClickCallback: NativeWindow.readerModePageAction.readerModeActiveCallback});
> 
Done
> Yeah, it bugs me we have these two messages right next to each other that
> both tell java reader is enabled, but like we've talked about, lets fix that
> in a second patch.
> 
> We can store this id in the Reader object though:
> Reader.pageAction = NativeWindow.pageactions.add();
> or maybe even better
> 
> and
> if (Reader.pageAction)
> NativeWindow.pageactions.remove(Reader.pageAction);
> 
> Also, does this work if you flip back and forth between some open tabs (or
> swipe closed a reader mode tab and wind up on a non-reader mode tab?). Maybe
> we can do this in the Tab.setActive and Tab.getActive methods. So here we'd
> set:
> 
> tab.readerEnabled = true/false;
> 
> and in other places
> 
> Tab.prototype = {
>   set readerEnabled(val) {
>     this._readerEnabled = val;
>     if (this.getActive())
>       Reader.updatePageAction(val);
>   },
>   get readerEnabled() {
>     return this._readerEnabled;
>   },
>   setActive: function(active) {
>     // other things
>     Reader.updatePageAction(this.readerEanbled);
>   },
>   onLocationChange: function() {
>     // other things?
>     this.readerEnabled = false;
>   },
> }
> 
> and:
> 
> Reader.updatePageAction = function(tab) {
>   if (tab.readerEnabled) {
>     this.pageAction = NativeWindow.pageactions.add(..);
>   } else if (this.pageAction) {
>     NativeWindow.pageactions.remove(this.pageAction);
>     delete this.pageAction;
>   }
> }
Had to make a few changes to accomodate the active mode but done.

Comment 31

[Shilpan Bhagat]

Attachment: [WIP] patch for pageactions (w/ improvements)

Adding patch. Made changes to the code as mentioned above.

Comment 32

[Wesley Johnston (:wesj)]

Comment on attachment 769765 [details] [diff] [review]
[WIP] patch for pageactions (w/ improvements)

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

This is looking really really nice. I like! Most of my comments have to do with just trying to make sure things are readable for future people who have to touch this code. Picking variable and method names carefully. Adding comments any place where you had to spend more than a little bit thinking about the code. Simplifying code where we can.

::: mobile/android/base/BrowserToolbar.java
@@ +27,4 @@
>  import org.mozilla.gecko.PrefsHelper;
>  
> +import org.json.JSONArray;
> +import org.json.JSONException;

I think a bunch of these imports in BrowserToolbar aren't needed anymore. The hard way to fix that is just remove and build. If you want, you can try dumping this file into Eclipse or something to make that easier. liuche has a system for that.

@@ +460,5 @@
>                      Boolean showProgress = (Boolean)data;
>                      if (showProgress && tab.getState() == Tab.STATE_LOADING)
>                          setProgressVisibility(true);
>                      setSecurityMode(tab.getSecurityMode());
> +                    setPageActionVisibility(mStop.getVisibility() == View.VISIBLE);

I'm still confused by this. The code you're removing doesn't make it look like reader did the same thing to me.. maybe I'm missing something? In fact, since we're showing and hiding icons as they're added/removed, I'm not sure we need this at all....

This patch is big enough, if we decide to change this, I'd be willing to take a follow up to fix this.

::: mobile/android/base/GeckoViewsFactory.java
@@ +91,5 @@
>              mFactoryMap.put("RelativeLayout", GeckoRelativeLayout.class.getConstructor(arg1Class, arg2Class));
>              mFactoryMap.put("TextSwitcher", GeckoTextSwitcher.class.getConstructor(arg1Class, arg2Class));
>              mFactoryMap.put("TextView", GeckoTextView.class.getConstructor(arg1Class, arg2Class));
>              mFactoryMap.put("FaviconView", FaviconView.class.getConstructor(arg1Class, arg2Class));
> +            mFactoryMap.put("PageActionLayout", PageActionLayout.class.getConstructor(arg1Class, arg2Class));

I think this file just died.

::: mobile/android/base/PageActionLayout.java
@@ +33,5 @@
> +import java.util.ArrayList;
> +import java.util.UUID;
> +
> +public class PageActionLayout extends LinearLayout implements GeckoEventListener {
> +	private String LOGTAG = "GeckoPageActionLayout";

Careful with indentation. This is indented too far, as are a few lines (mContext) below. Maybe you've got tabs and spaces intermixed?

@@ +85,5 @@
> +            } else if (event.equals("PageActions:Remove")) {
> +                final String id = message.getString("id");
> +
> +                removePageAction(id);
> +                setPageActionButtonsVisibility(View.VISIBLE);

This feels confusing. We're removing a page action, and then saying "make 'em visible". removePageAction actually calls this for you too. I think if add/removePageAction can both call refreshPageActionIcons() and setPageactionButtonsVisibility() that would be nice.

Also, refreshPageActionIcons() and setPageactionButtonsVisibility() both seem like fairly similar functions. At a glance its a bit hard to tell the difference. Would it be useful to combine them.

@@ +96,5 @@
> +    public void addPageAction(final String id, final String title, final String imageData) {
> +        BitmapUtils.getDrawable(mContext, imageData, new BitmapUtils.BitmapLoader(){
> +            @Override
> +            public void onBitmapFound(final Drawable d) {
> +                mPageActionList.add(new PageAction(id, title, d));

You may need to be careful here. You could hit a race where:

page calls pageaction.add(...);
// we start loading the icon
page calls pageaction.remove(...);
// we finish loading the icon
// the page action is added

@@ +102,5 @@
> +                if(mLayout.getChildCount() < mMaxVisiblePageActions) {
> +                    ThreadUtils.postToUiThread(new Runnable() {
> +                        @Override
> +                        public void run () {
> +                            mLayout.addView(createImageButton(d), 0);

This also just feels a bit strange. I wonder if we're better just adding the buttons in the PageActionsLayout contructor (with drawables set to null), and then just calling refreshPageActionIcons here. I know that feels in contradiction with the old patch where I had you remove the explicitly created icons, but in this case at least we're creating them dynamically. If we want to show 3 at some point, we only have to change a single number.

@@ +128,5 @@
> +
> +        imageButton.setOnClickListener(new OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                performClick(v);

I think based on your comment this is trying to maintain some separation between the buttons and the layout. I think for now I'd rather keep this simple. PageActionLayout can implement OnClickListener and OnLongClickListener and we can rename performClick and performLongClick to onClick and onLongClick. This can become:

imageButton.setOnClickListener(this);

@@ +142,5 @@
> +        return imageButton;
> +    }
> +
> +    private void performClick(View v) {
> +        int buttonClicked = (Integer)v.getTag();

Ahh. We do need the tags here..... kinda. I think we'd be better to store the pageaction's key in the tag field here, and create a special key for the overflow button

@@ +146,5 @@
> +        int buttonClicked = (Integer)v.getTag();
> +        if(buttonClicked == 0) {
> +            if(mPageActionList.size() <=  mMaxVisiblePageActions) {
> +                mPageActionList.get(0).onClick();
> +            } else if(mPageActionList.size() >  mMaxVisiblePageActions) {

Don't really need the else if here. Just an else will do.

@@ +178,5 @@
> +            if(buttonNumber == 0) {
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run () {
> +                        v.setImageDrawable((mPageActionList.size() > mMaxVisiblePageActions) ? resources.getDrawable(R.drawable.icon_pageaction) : ((mPageActionList.size() > 0) ? mPageActionList.get(0).getDrawable() : null));

This line is confusing enough, you should add some comments explaining what's going on.

@@ +185,5 @@
> +            } else {
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run () {
> +                        v.setImageDrawable((mPageActionList.size() > buttonNumber) ? mPageActionList.get(mPageActionList.size() - mMaxVisiblePageActions + buttonNumber).getDrawable() : null);

I think the same here. Its confusing to me why we're pulling from the end of the list here. I also don't really understand why we're using buttonNumber and not just index... A utility method like

isOverflowButton(int buttonNumber)
or
isOverflowButton(View v)

might be useful to just make this code readable.

@@ +198,5 @@
> +            final ImageButton v = (ImageButton)this.getChildAt(index);
> +            ThreadUtils.postToUiThread(new Runnable() {
> +                @Override
> +                public void run () {
> +                    v.setVisibility((visibility == View.VISIBLE) ? (((Integer)v.getTag() < mPageActionList.size()) ? View.VISIBLE : View.GONE) : View.GONE);

Yeah, these nested ternarys are hard to read. I'm not exactly sure what's happening here. Let use if statements with some comments explaining them if necessary.

I don't hate ternarys, but they're better in situations where they're very simple. i.e.:
color = tab.isPrivateMode() ? red : blue;

@@ +226,5 @@
> +        }
> +        mPageActionsMenu.show();
> +    }
> +
> +    private class PageActionList {

I still think you'd be better with a Hash table of some sort here.

@@ +265,5 @@
> +            }
> +        }
> +    }
> +
> +    private static class PageAction {

I like this class a whole lot! At some point in the future, we can even make it abstract, split the onClick handlers off in to a separate GeckoPageAction and then Java can add items as well with something like:

PageActionsLayout.add(new PageAction(uid, "Title", drawable) {
  @Override public void onClick() { }
  @Override public void onLongClick() { }
});

but that's a future patch....

::: mobile/android/chrome/content/browser.js
@@ +2824,1 @@
>      }

If we get rid of the mode we can just put one Reader.updatePageAction(this) call at the bottom of this.

@@ +3642,5 @@
>        this.sendViewportUpdate();
>      }
> +
> +    if(this.readerEnabled)
> +      Reader.updatePageAction(Reader.pageAction.mode.DISABLED);

This doesn't look right. We're disabling the page action if reader is enabled? If it is right, can you put in a comment explaining why.

@@ +3982,5 @@
> +  },
> +
> +  set readerActive(isReaderActive) {
> +    this._readerActive = isReaderActive;
> +    if (this.getActive()){

Space before the {

If we get rid of the modes we can just call Reader.updatePageAction(this); here and above this in several places.

@@ +6967,5 @@
> +      });
> +    },
> +  },
> +
> +  updatePageAction: function(pageActionMode) {

Hmm... Can we skip this mode argument and just do something like:

if (tab.readerActive) {
  // show exit page action
} else if (tab.readerEnabled) {
  // show enter page action
} else {
  // remove any page actions
}

@@ +6985,5 @@
> +        }
> +        break;
> +      case this.pageAction.mode.ACTIVE:
> +        this.pageAction.id = NativeWindow.pageactions.add({
> +          title: Strings.browser.GetStringFromName("readerMode.active"),

Should this be something like readerMode.exit?

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +269,5 @@
>  getUserMedia.sharingMicrophone.message2 = Microphone is on
>  getUserMedia.sharingCameraAndMicrophone.message2 = Camera and microphone are on
> +
> +#Reader mode
> +readerMode.active = Reader Active

Yeah, something like "Exit Reader Mode" and "Enter Reader Mode" (I think title case fits with our other menus...) I see our current reader mode icon just has the contentDescription "Reader" which seems kinda useless. This seems like a good time to improve it.

Since R.string.reader isn't used anymore, you could remove it too.

Comment 33

[Shilpan Bhagat]

Attachment: [WIP] patch for pageactions (w/ improvements based on comments)

Made changes according to the comments above.

Comment 34

[Shilpan Bhagat]

(In reply to Wesley Johnston (:wesj) from comment #32)
> Comment on attachment 769765 [details] [diff] [review]
> [WIP] patch for pageactions (w/ improvements)
> 
> Review of attachment 769765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking really really nice. I like! Most of my comments have to do
> with just trying to make sure things are readable for future people who have
> to touch this code. Picking variable and method names carefully. Adding
> comments any place where you had to spend more than a little bit thinking
> about the code. Simplifying code where we can.
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +27,4 @@
> >  import org.mozilla.gecko.PrefsHelper;
> >  
> > +import org.json.JSONArray;
> > +import org.json.JSONException;
> 
> I think a bunch of these imports in BrowserToolbar aren't needed anymore.
> The hard way to fix that is just remove and build. If you want, you can try
> dumping this file into Eclipse or something to make that easier. liuche has
> a system for that.
> 
> @@ +460,5 @@
> >                      Boolean showProgress = (Boolean)data;
> >                      if (showProgress && tab.getState() == Tab.STATE_LOADING)
> >                          setProgressVisibility(true);
> >                      setSecurityMode(tab.getSecurityMode());
> > +                    setPageActionVisibility(mStop.getVisibility() == View.VISIBLE);
> 
> I'm still confused by this. The code you're removing doesn't make it look
> like reader did the same thing to me.. maybe I'm missing something? In fact,
> since we're showing and hiding icons as they're added/removed, I'm not sure
> we need this at all....
> 
> This patch is big enough, if we decide to change this, I'd be willing to
> take a follow up to fix this.
> 
If you look at the code without this patch, setReaderMode() called setPageActionVisibility inside it. We do not need the readerMode anymore but we still need the setPageActionVisibility for the pageActions to act as they did before.
> ::: mobile/android/base/GeckoViewsFactory.java
> @@ +91,5 @@
> >              mFactoryMap.put("RelativeLayout", GeckoRelativeLayout.class.getConstructor(arg1Class, arg2Class));
> >              mFactoryMap.put("TextSwitcher", GeckoTextSwitcher.class.getConstructor(arg1Class, arg2Class));
> >              mFactoryMap.put("TextView", GeckoTextView.class.getConstructor(arg1Class, arg2Class));
> >              mFactoryMap.put("FaviconView", FaviconView.class.getConstructor(arg1Class, arg2Class));
> > +            mFactoryMap.put("PageActionLayout", PageActionLayout.class.getConstructor(arg1Class, arg2Class));
> 
> I think this file just died.
> 
> ::: mobile/android/base/PageActionLayout.java
> @@ +33,5 @@
> > +import java.util.ArrayList;
> > +import java.util.UUID;
> > +
> > +public class PageActionLayout extends LinearLayout implements GeckoEventListener {
> > +	private String LOGTAG = "GeckoPageActionLayout";
> 
> Careful with indentation. This is indented too far, as are a few lines
> (mContext) below. Maybe you've got tabs and spaces intermixed?
> 
> @@ +85,5 @@
> > +            } else if (event.equals("PageActions:Remove")) {
> > +                final String id = message.getString("id");
> > +
> > +                removePageAction(id);
> > +                setPageActionButtonsVisibility(View.VISIBLE);
> 
> This feels confusing. We're removing a page action, and then saying "make
> 'em visible". removePageAction actually calls this for you too. I think if
> add/removePageAction can both call refreshPageActionIcons() and
> setPageactionButtonsVisibility() that would be nice.
> 
> Also, refreshPageActionIcons() and setPageactionButtonsVisibility() both
> seem like fairly similar functions. At a glance its a bit hard to tell the
> difference. Would it be useful to combine them.
> 
> @@ +96,5 @@
> > +    public void addPageAction(final String id, final String title, final String imageData) {
> > +        BitmapUtils.getDrawable(mContext, imageData, new BitmapUtils.BitmapLoader(){
> > +            @Override
> > +            public void onBitmapFound(final Drawable d) {
> > +                mPageActionList.add(new PageAction(id, title, d));
> 
> You may need to be careful here. You could hit a race where:
> 
> page calls pageaction.add(...);
> // we start loading the icon
> page calls pageaction.remove(...);
> // we finish loading the icon
> // the page action is added
> 
> @@ +102,5 @@
> > +                if(mLayout.getChildCount() < mMaxVisiblePageActions) {
> > +                    ThreadUtils.postToUiThread(new Runnable() {
> > +                        @Override
> > +                        public void run () {
> > +                            mLayout.addView(createImageButton(d), 0);
> 
> This also just feels a bit strange. I wonder if we're better just adding the
> buttons in the PageActionsLayout contructor (with drawables set to null),
> and then just calling refreshPageActionIcons here. I know that feels in
> contradiction with the old patch where I had you remove the explicitly
> created icons, but in this case at least we're creating them dynamically. If
> we want to show 3 at some point, we only have to change a single number.
> 
> @@ +128,5 @@
> > +
> > +        imageButton.setOnClickListener(new OnClickListener() {
> > +            @Override
> > +            public void onClick(View v) {
> > +                performClick(v);
> 
> I think based on your comment this is trying to maintain some separation
> between the buttons and the layout. I think for now I'd rather keep this
> simple. PageActionLayout can implement OnClickListener and
> OnLongClickListener and we can rename performClick and performLongClick to
> onClick and onLongClick. This can become:
> 
> imageButton.setOnClickListener(this);
> 
> @@ +142,5 @@
> > +        return imageButton;
> > +    }
> > +
> > +    private void performClick(View v) {
> > +        int buttonClicked = (Integer)v.getTag();
> 
> Ahh. We do need the tags here..... kinda. I think we'd be better to store
> the pageaction's key in the tag field here, and create a special key for the
> overflow button
> 
> @@ +146,5 @@
> > +        int buttonClicked = (Integer)v.getTag();
> > +        if(buttonClicked == 0) {
> > +            if(mPageActionList.size() <=  mMaxVisiblePageActions) {
> > +                mPageActionList.get(0).onClick();
> > +            } else if(mPageActionList.size() >  mMaxVisiblePageActions) {
> 
> Don't really need the else if here. Just an else will do.
> 
> @@ +178,5 @@
> > +            if(buttonNumber == 0) {
> > +                ThreadUtils.postToUiThread(new Runnable() {
> > +                    @Override
> > +                    public void run () {
> > +                        v.setImageDrawable((mPageActionList.size() > mMaxVisiblePageActions) ? resources.getDrawable(R.drawable.icon_pageaction) : ((mPageActionList.size() > 0) ? mPageActionList.get(0).getDrawable() : null));
> 
> This line is confusing enough, you should add some comments explaining
> what's going on.
> 
> @@ +185,5 @@
> > +            } else {
> > +                ThreadUtils.postToUiThread(new Runnable() {
> > +                    @Override
> > +                    public void run () {
> > +                        v.setImageDrawable((mPageActionList.size() > buttonNumber) ? mPageActionList.get(mPageActionList.size() - mMaxVisiblePageActions + buttonNumber).getDrawable() : null);
> 
> I think the same here. Its confusing to me why we're pulling from the end of
> the list here. I also don't really understand why we're using buttonNumber
> and not just index... A utility method like
> 
> isOverflowButton(int buttonNumber)
> or
> isOverflowButton(View v)
> 
> might be useful to just make this code readable.
> 
> @@ +198,5 @@
> > +            final ImageButton v = (ImageButton)this.getChildAt(index);
> > +            ThreadUtils.postToUiThread(new Runnable() {
> > +                @Override
> > +                public void run () {
> > +                    v.setVisibility((visibility == View.VISIBLE) ? (((Integer)v.getTag() < mPageActionList.size()) ? View.VISIBLE : View.GONE) : View.GONE);
> 
> Yeah, these nested ternarys are hard to read. I'm not exactly sure what's
> happening here. Let use if statements with some comments explaining them if
> necessary.
> 
> I don't hate ternarys, but they're better in situations where they're very
> simple. i.e.:
> color = tab.isPrivateMode() ? red : blue;
> 
> @@ +226,5 @@
> > +        }
> > +        mPageActionsMenu.show();
> > +    }
> > +
> > +    private class PageActionList {
> 
> I still think you'd be better with a Hash table of some sort here.
> 
> @@ +265,5 @@
> > +            }
> > +        }
> > +    }
> > +
> > +    private static class PageAction {
> 
> I like this class a whole lot! At some point in the future, we can even make
> it abstract, split the onClick handlers off in to a separate GeckoPageAction
> and then Java can add items as well with something like:
> 
> PageActionsLayout.add(new PageAction(uid, "Title", drawable) {
>   @Override public void onClick() { }
>   @Override public void onLongClick() { }
> });
> 
> but that's a future patch....
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +2824,1 @@
> >      }
> 
> If we get rid of the mode we can just put one Reader.updatePageAction(this)
> call at the bottom of this.
> 
> @@ +3642,5 @@
> >        this.sendViewportUpdate();
> >      }
> > +
> > +    if(this.readerEnabled)
> > +      Reader.updatePageAction(Reader.pageAction.mode.DISABLED);
> 
> This doesn't look right. We're disabling the page action if reader is
> enabled? If it is right, can you put in a comment explaining why.
> 
> @@ +3982,5 @@
> > +  },
> > +
> > +  set readerActive(isReaderActive) {
> > +    this._readerActive = isReaderActive;
> > +    if (this.getActive()){
> 
> Space before the {
> 
> If we get rid of the modes we can just call Reader.updatePageAction(this);
> here and above this in several places.
> 
> @@ +6967,5 @@
> > +      });
> > +    },
> > +  },
> > +
> > +  updatePageAction: function(pageActionMode) {
> 
> Hmm... Can we skip this mode argument and just do something like:
> 
> if (tab.readerActive) {
>   // show exit page action
> } else if (tab.readerEnabled) {
>   // show enter page action
> } else {
>   // remove any page actions
> }
> 
> @@ +6985,5 @@
> > +        }
> > +        break;
> > +      case this.pageAction.mode.ACTIVE:
> > +        this.pageAction.id = NativeWindow.pageactions.add({
> > +          title: Strings.browser.GetStringFromName("readerMode.active"),
> 
> Should this be something like readerMode.exit?
> 
> ::: mobile/android/locales/en-US/chrome/browser.properties
> @@ +269,5 @@
> >  getUserMedia.sharingMicrophone.message2 = Microphone is on
> >  getUserMedia.sharingCameraAndMicrophone.message2 = Camera and microphone are on
> > +
> > +#Reader mode
> > +readerMode.active = Reader Active
> 
> Yeah, something like "Exit Reader Mode" and "Enter Reader Mode" (I think
> title case fits with our other menus...) I see our current reader mode icon
> just has the contentDescription "Reader" which seems kinda useless. This
> seems like a good time to improve it.
> 
> Since R.string.reader isn't used anymore, you could remove it too.

Comment 35

[Wesley Johnston (:wesj)]

Comment on attachment 772136 [details] [diff] [review]
[WIP] patch for pageactions (w/ improvements based on comments)

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

Looking good. One last little fix. Lets kill mMenuButtonKey and replace it with a constant for the menu button. Something like MENU_BUTTON_KEY = "MENU_BUTTON_KEY" and then just set that tag in refreshPageActions if we decide to show the overflow button.

::: mobile/android/base/PageActionLayout.java
@@ +47,5 @@
> +        mContext = context;
> +        mLayout = this;
> +
> +        mPageActionList = new LinkedHashMap<String, PageAction>();
> +        setNumberShown(2);

Move this magic number to a static final field in PageActionLayout. MAX_PAGE_ACTIONS_SHOWN or something.

@@ +173,5 @@
> +            final ImageButton v = (ImageButton)this.getChildAt(index);
> +            final PageAction pageAction = getPageActionForViewAt(index);
> +
> +            if(index == (this.getChildCount() - 1)) {
> +                mMenuButtonKey = ((pageAction!= null) ? pageAction.getID() : null);

Spaces between pageAction and !=

@@ +178,5 @@
> +                v.setTag(mMenuButtonKey);
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run () {
> +                        //If there are more pageactions then buttons, set the menu icon else set the page action's icon if there is a page action.

Space after the //. Also, lets make this "// If there are more pageactions then buttons, set the menu icon. Otherwise <other stuff>"

@@ +197,5 @@
> +        }
> +    }
> +
> +    private PageAction getPageActionForViewAt(int index) {
> +        //buttonIndex is the insertion order of the page action button which is reverse of the index

Is there some reason we want this reversed? You should add that to the comment if there is (why is sometimes more important than what..)

@@ +275,5 @@
> +            return mDrawable;
> +        }
> +
> +        public void setDrawable(Drawable d) {
> +            this.mDrawable = d;

Don't need 'this' here.

@@ +287,5 @@
> +            return mId;
> +        }
> +
> +        public int key() {
> +            return key;

I'd be fine just making key public in this case. Same with all of these tbh. If you are going to make them private, they should start with 'm'.

::: mobile/android/base/widget/GeckoPopupMenu.java
@@ +91,5 @@
>       *
>       * @param menuRes The menu resource to be inflated.
>       */
>      public void inflate(int menuRes) {
> +        if(menuRes > 0) {

It would be good to add a comment about why this might be <= 0 (i.e. for menus that are entirely generated content I guess).

::: mobile/android/chrome/content/browser.js
@@ +3973,5 @@
> +  set readerActive(isReaderActive) {
> +    this._readerActive = isReaderActive;
> +    if (this.getActive()) {
> +      Reader.updatePageAction(this);
> +    }

In JS, no braces on single line ifs.

Comment 36

[Wesley Johnston (:wesj)]

Also, we should file a bug to make long press work in menus. Since we're doing them all custom, that should be doable.

Comment 37

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #36)
> Also, we should file a bug to make long press work in menus. Since we're
> doing them all custom, that should be doable.

Still we want long press in places that's not obvious even after android killing it, and partly supporting it with a menu-button-plus-popup-menu?? Wait.. we have the same popup menu and a long press on it -- when it is supposed to be a replacement for the long press? :O :O :O :O

Comment 38

[Wesley Johnston (:wesj)]

I'm not sure what you're saying. We support long press on the reader button, and implemented support for it here in order to not regress the feature. We can file a separate bug to kill it if you want.

For now, page actions won't have any idea if they're showing in the urlbar or the menu. While I'd hope no one ever hides important functionality in long press (they will), it would be nice if it was at least available consistently. i.e. I don't want authors to accidentally put users in a state where some functionality is no longer available because they never tested with more than two page actions showing.

Comment 39

[Mark Finkle (:mfinkle) (use needinfo?)]

We need long press support for Reader Mode button, right? I also thought that the Reader Mode button was "always in the URL bar" and never shown in the drop down. Is that right?

If so, then we only need to add long-press support for add-on provided buttons, and that might be a lower priority. Make sense?

Comment 40

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 772136 [details] [diff] [review]
[WIP] patch for pageactions (w/ improvements based on comments)

drive-by:

* Look for "if(" in the patch. It happens in a few files. Change to "if ("
* Look for "\ No newline at end of file" in the patch and add some newlines at the end of files.
* My OCD wants "ReaderPageAction:Click" and "ReaderPageAction:LongClick" to just be "Reader:Click" and "Reader:LongClick" since we already have a bunch of "Reader:Xxx" messages. Any reason we should use "PageAction" in the name?

Comment 41

[Sriram Ramasubramanian [:sriram]]

Until now I didn't know that we can long press on "reader" icon in the URL bar. I don't know how many users are unaware of it as it lacks enough affordance for a long press!

Comment 42

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #39)
> We need long press support for Reader Mode button, right? I also thought
> that the Reader Mode button was "always in the URL bar" and never shown in
> the drop down. Is that right?

We're not "forcing" reader to always show (yet?). Its up to page actions to show/hide themselves. So its really just a race to see who adds themselves first.

Comment 43

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #42)
> (In reply to Mark Finkle (:mfinkle) from comment #39)
> > We need long press support for Reader Mode button, right? I also thought
> > that the Reader Mode button was "always in the URL bar" and never shown in
> > the drop down. Is that right?
> 
> We're not "forcing" reader to always show (yet?). Its up to page actions to
> show/hide themselves. So its really just a race to see who adds themselves
> first.

Just to clarify. I meant, if the reader icon is active, the reader icon always gets the prime spot in the URL bar and would never be shown in the "overflow" spot. I'm not sure if that is the design goal anymore or not.

Comment 44

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #41)
> Until now I didn't know that we can long press on "reader" icon in the URL
> bar. I don't know how many users are unaware of it as it lacks enough
> affordance for a long press!

The long-press behaviour on reader is meant to be just a power-user shortcut btw.

Comment 45

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #43)
> (In reply to Wesley Johnston (:wesj) from comment #42)
> > (In reply to Mark Finkle (:mfinkle) from comment #39)
> > > We need long press support for Reader Mode button, right? I also thought
> > > that the Reader Mode button was "always in the URL bar" and never shown in
> > > the drop down. Is that right?
> > 
> > We're not "forcing" reader to always show (yet?). Its up to page actions to
> > show/hide themselves. So its really just a race to see who adds themselves
> > first.
> 
> Just to clarify. I meant, if the reader icon is active, the reader icon
> always gets the prime spot in the URL bar and would never be shown in the
> "overflow" spot. I'm not sure if that is the design goal anymore or not.

I would really like us to ensure reader is always the first action (i.e. icon in URL bar). It's a built-in feature that users are used to have there. It would be bad UX if the reader behaviour suddenly becomes random when certain addons are installed i.e. sometimes reader is in the URL bar, sometimes in the overflow menu.

Comment 46

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #45)
> I would really like us to ensure reader is always the first action (i.e.
> icon in URL bar). It's a built-in feature that users are used to have there.
> It would be bad UX if the reader behaviour suddenly becomes random when
> certain addons are installed i.e. sometimes reader is in the URL bar,
> sometimes in the overflow menu.

No strong opinion, except I don't really want to block this on that. Ideally I'd like to make reader optional for users who don't use it.

Comment 47

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #46)
> (In reply to Lucas Rocha (:lucasr) from comment #45)
> > I would really like us to ensure reader is always the first action (i.e.
> > icon in URL bar). It's a built-in feature that users are used to have there.
> > It would be bad UX if the reader behaviour suddenly becomes random when
> > certain addons are installed i.e. sometimes reader is in the URL bar,
> > sometimes in the overflow menu.
> 
> No strong opinion, except I don't really want to block this on that. Ideally
> I'd like to make reader optional for users who don't use it.

Perhaps we don't need to block-landing-on-nightly on it, but I do think we need to block-ship on it.

Comment 48

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #46)
> (In reply to Lucas Rocha (:lucasr) from comment #45)
> > I would really like us to ensure reader is always the first action (i.e.
> > icon in URL bar). It's a built-in feature that users are used to have there.
> > It would be bad UX if the reader behaviour suddenly becomes random when
> > certain addons are installed i.e. sometimes reader is in the URL bar,
> > sometimes in the overflow menu.
> 
> No strong opinion, except I don't really want to block this on that. Ideally
> I'd like to make reader optional for users who don't use it.

Sorry, I should have been more clear. I didn't mean to say we should block this bug on this. I agree with mfinkle that this should be a release blocker though.

Comment 49

[Shilpan Bhagat]

Attachment: Patch for pageactions (w/ more fixes)

Made changes according to Mark's and Wes' comments.

Comment 50

[Shilpan Bhagat]

Attachment: Patch for pageactions (w/ more fixes)

Sorry about the mix-up. This is the right patch.

Comment 51

[Wesley Johnston (:wesj)]

Comment on attachment 775857 [details] [diff] [review]
Patch for pageactions (w/ more fixes)

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

Yay! I like. Lets land it.

::: mobile/android/base/PageActionLayout.java
@@ +144,5 @@
> +
> +    @Override
> +    public void onClick(View v) {
> +        String buttonClickedId = (String)v.getTag();
> +        if (buttonClickedId != null) {

We usually prefer early returns if we can. i.e.

if (buttonClickedId == null)
  return;

@@ +176,5 @@
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run () {
> +                        // If there are more pageactions then buttons, set the menu icon. Otherwise set the page action's icon if there is a page action.
> +                        v.setImageDrawable((mPageActionList.size() > mMaxVisiblePageActions) ? resources.getDrawable(R.drawable.icon_pageaction) : ((pageAction != null) ? pageAction.getDrawable() : null));

Lets not nest these ternary operators two deep.

::: mobile/android/base/resources/layout-large-v11/browser_toolbar.xml
@@ +105,5 @@
>  
> +        <org.mozilla.gecko.PageActionLayout android:id="@+id/page_action_layout"
> +                                            android:layout_width="wrap_content"
> +                                            android:layout_height="match_parent"
> +                                            android:layout_marginRight="12dp"

Use @dimen/browser_toolbar_button_padding

::: mobile/android/base/widget/GeckoPopupMenu.java
@@ +94,5 @@
>      public void inflate(int menuRes) {
> +        /**
> +         * menuRes is auto generated and is always positive.
> +         * We use this to initialize the menu
> +         */

I know I asked for this comment. I think maybe I was wrong. Lets just remove it.

Comment 52

[Shilpan Bhagat]

Attachment: Patch for pageactions

Patch with updated changes

Comment 53

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72b0a69fde77

Comment 54

[Ed Morley [:emorley]]

Backed out since this conflicted with the backout of bug 884075:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3f12656a5eef

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/696633740d85

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/696633740d85

Comment 57

[Alex Keybl [:akeybl]]

Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.

Comment 58

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 768072 [details]
Screen shot of page actions

This seems like review? is no longer needed, feel free to change if necessary.