Closed Bug 1391579 Opened 7 years ago Closed 7 years ago

Implement in-app add-on update notifications

Categories

(Firefox for Android Graveyard :: Add-on Manager, enhancement, P2)

enhancement

Tracking

(firefox57 verified, firefox58 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: aswan, Assigned: aswan)

References

Details

Attachments

(7 files, 4 obsolete files)

This is part of the flow documented in the flowchart on slide 3 of:
https://docs.google.com/presentation/d/1NSqxUiZDUdFrha0TvuNVEV1v2OqmBPc17XaPbzIwWmQ/edit

That chart is probably indecipherable to anybody who wasn't in the room when it was created but the in-app flow is as follows (for an addon update that requires new permissions):

1. update icon appears next to the "Add-ons" entry in the app menu
2. in the main addons view, update icon appears next to add-on(s) that have an update with new permissions
3. the detail view for an affected extension includes an "Update" button
4. pressing the update button generates a regular permission prompt
5. after the permission prompt is dismissed, browser returns to whatever page was previously being viewed
Emanuela, can you please provide an android vector xml file for the update badge (http://searchfox.org/mozilla-central/source/browser/themes/shared/update-badge.svg)?
Flags: needinfo?(emanuela)
Attached file update_badge.zip (obsolete) —
Sure. 

For being sure, I also attached the .png versions.
Flags: needinfo?(emanuela)
Looking into this, it looks like we don't actually always have a top-level "Add-ons" menu:
http://searchfox.org/mozilla-central/rev/7d43c93d35e4c0fbaba7d639278e892b4565db63/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#3689-3695

I don't know what conditions control whether we get the top-level menu or not but in my own local builds, I consistently see the top-level menu.
Given the limited time remaining for 57, I'm going to implement this just for the case where we have the top-level menu, please let me know ASAP if that's a problem.
Flags: needinfo?(jalin)
About the design, we have decided to have top-level "Add-ons" menu for 57 and after 57. 

I discussed with Julian, he mentioned that the code has "else" case just for code backup. So we don't need to care about that so far.

Thank you
Flags: needinfo?(jalin)
I've been banging my head against the wall trying to figure out how to get the update icon into the top level "Add-ons" menu item and I can't figure it out.  Julian, any suggestions?
Flags: needinfo?(walkingice0204)
Those Views you saw in menu are instances of *MenuItemDefault*. It always draws a state-drawable(">" or checkbox) in right side, if any.[1]

I think let MenuItemefault more flexible to draw another icon rather than only mState, than we can draw the badge icon in that position.

How do you think this way?

[1] https://dxr.mozilla.org/mozilla-central/rev/1867d7931c0a70ab90edf4aa84876525773a7139/mobile/android/base/java/org/mozilla/gecko/menu/MenuItemDefault.java#61
This sounds promising but I'm way over my head here.  I don't really understand how MenuItemDefault works, and if it got a new method or property to show both text and icon, its not clear to me how I would access that from other existing code.
Is there any chance you can help out with the MenuItemDefault part of this?
patch 1: add attribute itemType to GeckoMenuItem
patch 2: add one more item type for MenuItemIcon

The naming "MenuItemIcon" is not good enough. I am not sure whether to create a class from scratch or just extends MenuItemDefault, which one is better? But I let it extends MenuItemDefault so it would be easier to see what I changed.
patch 3: to use the ItemType for add-on-menu-item
Attached image menui item with an icon
I use previous patches for your reference to get this result. Wish it match your expectation. The naming 'MenuItemIcon' looks weird, but I guess you know what does it mean :P
Whoops, the request from comment 1 wasn't exactly right, the svg file just has the outline of an arrow filled in white.  Can you provide an Android icon suitable for using in the app menu?  The add-ons manager is regular html so I think I can just use the same svg and css we used on desktop for that...
Flags: needinfo?(emanuela)
Assignee: nobody → aswan
Priority: -- → P2
Sure!

I hope this is going to work. I don't have much experience with the Fennec design, so feel free to involve Jack if something is missing.
Attachment #8898721 - Attachment is obsolete: true
Flags: needinfo?(emanuela)
Flags: needinfo?(walkingice0204)
Julian, the patches you provided to extend MenuItemDefault are working great for me and I'd like to land changes built on top of them.  But at least your part 2 patch describes itself as prototype-quality but perhaps not suitable for landing?  Do we need to do something different?  Should we do it here or in a different bug?
Flags: needinfo?(walkingice0204)
Andrew, I tried to make the patches works as good as I can. It sounds good if it works for you. One of my concern is naming might good enough. If you think anything better than "MenuItemIcon", just do it and I appreciate it. Or if you think it is acceptable, then we can keep it.

I think you will update moz.build as well to fix build issue(sorry!) when you are preparing patches to land?

We need another reviewer to see if I missed any detail.(If so, I will update my solution as quick as possible!)
Attachment #8900314 - Attachment is obsolete: true
Attachment #8900318 - Attachment is obsolete: true
Attachment #8900319 - Attachment is obsolete: true
Attachment #8906174 - Flags: review?(s.kaspari)
Attachment #8906718 - Flags: review?(s.kaspari)
Attachment #8906719 - Flags: review?(s.kaspari)
Attachment #8906175 - Flags: review?(s.kaspari)
Attachment #8906718 - Flags: review?(topwu.tw)
Attachment #8906719 - Flags: review?(topwu.tw)
Attachment #8906175 - Flags: review?(topwu.tw)
Attachment #8906174 - Flags: review?(topwu.tw)
Comment on attachment 8906174 [details]
Bug 1391579 Part 1: add attribute itemType to GeckoMenuItem

https://reviewboard.mozilla.org/r/177926/#review183816
Attachment #8906174 - Flags: review?(s.kaspari) → review+
Comment on attachment 8906718 [details]
Bug 1391579 Part 2: Add MenuItemIcon

https://reviewboard.mozilla.org/r/178454/#review183820
Attachment #8906718 - Flags: review?(s.kaspari) → review+
Comment on attachment 8906719 [details]
Bug 1391579 Part 3: Add update indicator to top-level Add-ons menu entry

https://reviewboard.mozilla.org/r/178456/#review183822

::: mobile/android/app/src/main/res/drawable/ic_addon_update.xml:17
(Diff revision 1)
> +        android:fillColor="#000000"
> +        android:strokeColor="#008EA4"
> +        android:strokeWidth="2"
> +        android:strokeLineCap="round"
> +        android:pathData="M8,2 L8,14 M8,2 L4,6 M8,2 L12,6" />
> +</vector>

We had some problems with rendering vector images in the past - please make sure this works on devices starting with API version 16.
Attachment #8906719 - Flags: review?(s.kaspari) → review+
Comment on attachment 8906175 [details]
Bug 1391579 Part 4: Handle the in-app extension update flow for Fennec

https://reviewboard.mozilla.org/r/177928/#review183824
Attachment #8906175 - Flags: review?(s.kaspari) → review+
The changes overall look good to me. I flagged Jing-wei as a second reviewer because I only had time for a quick review.
Comment on attachment 8906719 [details]
Bug 1391579 Part 3: Add update indicator to top-level Add-ons menu entry

https://reviewboard.mozilla.org/r/178456/#review183822

> We had some problems with rendering vector images in the past - please make sure this works on devices starting with API version 16.

I've been doing my manual tests with Android version 4.3.1 which I understand is API version 18.  Are you saying you wanted me to check with something at least as new as 16?  Or to to back and check on something with exactly API version 16?
Comment on attachment 8906718 [details]
Bug 1391579 Part 2: Add MenuItemIcon

https://reviewboard.mozilla.org/r/178454/#review184154

::: mobile/android/base/java/org/mozilla/gecko/menu/GeckoMenu.java:828
(Diff revision 1)
> +                final int type = getItemViewType(position);
> +                if (type == VIEW_TYPE_ICON) {
> +                    view = new MenuItemIcon(parent.getContext(), null);
> +                } else {
> -                view = new MenuItemDefault(parent.getContext(), null);
> +                    view = new MenuItemDefault(parent.getContext(), null);
> +                }

IIUC, the change here is to create a view instance for the new item type `VIEW_TYPE_ICON`, we didn't expect to modify the original behavior of `VIEW_TYPE_DEFAULT` and `VIEW_TYPE_ACTION_MODE`.

However, what I observered is:

Before applying the patch:
1. for `VIEW_TYPE_DEFAULT`, view is created as an instance of MenuItemDefault.
2. for `VIEW_TYPE_ACTION_MODE`, view is null

After applying the patch:
1. for `VIEW_TYPE_ICON`, view is an instance of MenuItemIcon.
2. for `VIEW_TYPE_DEFAULT` and `VIEW_TYPE_ACTION_MODE`, view is created as an instance of MenuItemDefault.

The behavior of `VIEW_TYPE_ACTION_MODE` is changed, is that what we want?
Comment on attachment 8906174 [details]
Bug 1391579 Part 1: add attribute itemType to GeckoMenuItem

https://reviewboard.mozilla.org/r/177926/#review184156
Attachment #8906174 - Flags: review?(topwu.tw) → review+
Comment on attachment 8906718 [details]
Bug 1391579 Part 2: Add MenuItemIcon

https://reviewboard.mozilla.org/r/178454/#review184154

> IIUC, the change here is to create a view instance for the new item type `VIEW_TYPE_ICON`, we didn't expect to modify the original behavior of `VIEW_TYPE_DEFAULT` and `VIEW_TYPE_ACTION_MODE`.
> 
> However, what I observered is:
> 
> Before applying the patch:
> 1. for `VIEW_TYPE_DEFAULT`, view is created as an instance of MenuItemDefault.
> 2. for `VIEW_TYPE_ACTION_MODE`, view is null
> 
> After applying the patch:
> 1. for `VIEW_TYPE_ICON`, view is an instance of MenuItemIcon.
> 2. for `VIEW_TYPE_DEFAULT` and `VIEW_TYPE_ACTION_MODE`, view is created as an instance of MenuItemDefault.
> 
> The behavior of `VIEW_TYPE_ACTION_MODE` is changed, is that what we want?

No that is not what we want. Thanks for point it out. We should modify the patch to
```
             if (convertView == null) {
                 final int type = getItemViewType(position);
                 if (type == VIEW_TYPE_ICON) {
                     view = new MenuItemIcon(parent.getContext(), null);
-                } else {
+                } else if (type == VIEW_TYPE_DEFAULT) {
                     view = new MenuItemDefault(parent.getContext(), null);
                 }
             } else {
                 view = (GeckoMenuItem.Layout) convertView;
             }
```
Comment on attachment 8906719 [details]
Bug 1391579 Part 3: Add update indicator to top-level Add-ons menu entry

https://reviewboard.mozilla.org/r/178456/#review184204

::: mobile/android/base/java/org/mozilla/gecko/extensions/ExtensionPermissionsHelper.java:27
(Diff revision 1)
>  
>  public class ExtensionPermissionsHelper implements BundleEventListener {
> -    private final Context mContext;
> +    private final BrowserApp mApp;
> +    private boolean mShowUpdateIcon;
>  
> -    public ExtensionPermissionsHelper(Context context) {
> +    public ExtensionPermissionsHelper(BrowserApp app) {

Why do you prefer using `BrowserApp` instead of `Context`? By using base class `Context`, `ExtensionPermissionsHelper` can be reused if other activity(perhaps custom tab) also need to support web extension in the future.
Attachment #8906719 - Flags: review?(topwu.tw) → review+
Comment on attachment 8906175 [details]
Bug 1391579 Part 4: Handle the in-app extension update flow for Fennec

https://reviewboard.mozilla.org/r/177928/#review184216
Attachment #8906175 - Flags: review?(topwu.tw) → review+
(In reply to Andrew Swan [:aswan] from comment #27)
> I've been doing my manual tests with Android version 4.3.1 which I
> understand is API version 18.  Are you saying you wanted me to check with
> something at least as new as 16?  Or to to back and check on something with
> exactly API version 16?

I'd test with exactly 16 and a couple of other API levels. Support for vector drawables was introduces with API 21. We use a support library for using them on earlier versions of Android. Starting with 21 not all features of SVG where supported and therefore rendering issues can occur with some API levels. :(
Comment on attachment 8906719 [details]
Bug 1391579 Part 3: Add update indicator to top-level Add-ons menu entry

https://reviewboard.mozilla.org/r/178456/#review184204

> Why do you prefer using `BrowserApp` instead of `Context`? By using base class `Context`, `ExtensionPermissionsHelper` can be reused if other activity(perhaps custom tab) also need to support web extension in the future.

Whoops, that was from an earlier version of this patch when I had mShowUpdateIcon in the app instead of in this class, I've reverted that change.
(In reply to Sebastian Kaspari (:sebastian) from comment #33)
> I'd test with exactly 16 and a couple of other API levels. Support for
> vector drawables was introduces with API 21. We use a support library for
> using them on earlier versions of Android. Starting with 21 not all features
> of SVG where supported and therefore rendering issues can occur with some
> API levels. :(

I'm not really equipped to do this sort of testing in any reasonable amount of time, if the consequences here are just mis-rendered images (as opposed to say a crash) can we defer this to QA?
I'm also confused about exactly what the concern is, your most recent comment suggests the problems would occur with newer API versions but you asked me to test an older version (16)?  For what its worth, this image is a pretty simple <path>, it doesn't use anything very exotic.
Comment on attachment 8906718 [details]
Bug 1391579 Part 2: Add MenuItemIcon

https://reviewboard.mozilla.org/r/178454/#review184750
Attachment #8906718 - Flags: review?(topwu.tw) → review+
Comment on attachment 8906719 [details]
Bug 1391579 Part 3: Add update indicator to top-level Add-ons menu entry

https://reviewboard.mozilla.org/r/178456/#review183822

> I've been doing my manual tests with Android version 4.3.1 which I understand is API version 18.  Are you saying you wanted me to check with something at least as new as 16?  Or to to back and check on something with exactly API version 16?

Discussed with sebastian on IRC, we agreed to defer this to QA
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb3a37d321e2
Part 1: add attribute itemType to GeckoMenuItem r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/7048637ef80c
Part 2: Add MenuItemIcon r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/f25a9201e1ea
Part 3: Add update indicator to top-level Add-ons menu entry r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/314cd9bfe986
Part 4: Handle the in-app extension update flow for Fennec r=jwu,sebastian
Depends on: 1400129
Depends on: 1400818
Depends on: 1400837
No longer depends on: 1400837
Comment on attachment 8906719 [details]
Bug 1391579 Part 3: Add update indicator to top-level Add-ons menu entry

https://reviewboard.mozilla.org/r/178456/#review186768

::: mobile/android/app/src/main/res/menu/browser_app_menu.xml:59
(Diff revision 2)
>            android:title="@string/desktop_mode"
>            android:checkable="true" />
>  
>      <item android:id="@+id/addons_top_level"
>            android:title="@string/addons"
> +          gecko:itemType="icon_menu_item"

This was only added to the phone configuration - did you mean to add it to tablets too (and Android O)? If so, please add this tag to these the other browser_app_menu files as well:

http://searchfox.org/mozilla-central/search?q=path%3Amobile%2Fandroid+path%3Abrowser_app_menu&path=
Comment on attachment 8906719 [details]
Bug 1391579 Part 3: Add update indicator to top-level Add-ons menu entry

https://reviewboard.mozilla.org/r/178456/#review186768

> This was only added to the phone configuration - did you mean to add it to tablets too (and Android O)? If so, please add this tag to these the other browser_app_menu files as well:
> 
> http://searchfox.org/mozilla-central/search?q=path%3Amobile%2Fandroid+path%3Abrowser_app_menu&path=

We filed bug 1401394 for this work.
clearing needinfo, we'll pick this up over on bug 1401394
Flags: needinfo?(aswan)
Attached image UpdateNotification.gif
This issue is verified as fixed on Fennec 58.0a1 (2017-09-25) and Fennec 57.0b3  under Android 7.1.2

The update notification is working as presented in description.

Please see the attached video.
Status: RESOLVED → VERIFIED
Depends on: 1407219
Flags: needinfo?(walkingice0204)
Depends on: 1432167
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.