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)
Firefox for Android Graveyard
Add-on Manager
Tracking
(firefox57 verified, firefox58 verified)
VERIFIED
FIXED
Firefox 57
People
(Reporter: aswan, Assigned: aswan)
References
Details
Attachments
(7 files, 4 obsolete files)
16.67 KB,
image/png
|
Details | |
10.65 KB,
application/zip
|
Details | |
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
jwu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
jwu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
jwu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
jwu
:
review+
|
Details |
1.28 MB,
image/gif
|
Details |
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
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
Sure. For being sure, I also attached the .png versions.
Flags: needinfo?(emanuela)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
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
Assignee | ||
Comment 7•7 years ago
|
||
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?
Comment 8•7 years ago
|
||
patch 1: add attribute itemType to GeckoMenuItem
Comment 9•7 years ago
|
||
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.
Comment 10•7 years ago
|
||
patch 3: to use the ItemType for add-on-menu-item
Comment 11•7 years ago
|
||
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
Assignee | ||
Comment 12•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → aswan
Priority: -- → P2
Comment 13•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(walkingice0204)
Assignee | ||
Comment 14•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
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!)
Assignee | ||
Updated•7 years ago
|
Attachment #8900314 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8900318 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8900319 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8906174 -
Flags: review?(s.kaspari)
Attachment #8906718 -
Flags: review?(s.kaspari)
Attachment #8906719 -
Flags: review?(s.kaspari)
Attachment #8906175 -
Flags: review?(s.kaspari)
Updated•7 years ago
|
Attachment #8906718 -
Flags: review?(topwu.tw)
Attachment #8906719 -
Flags: review?(topwu.tw)
Attachment #8906175 -
Flags: review?(topwu.tw)
Updated•7 years ago
|
Attachment #8906174 -
Flags: review?(topwu.tw)
Comment 22•7 years ago
|
||
mozreview-review |
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 23•7 years ago
|
||
mozreview-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 24•7 years ago
|
||
mozreview-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 25•7 years ago
|
||
mozreview-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+
Comment 26•7 years ago
|
||
The changes overall look good to me. I flagged Jing-wei as a second reviewer because I only had time for a quick review.
Assignee | ||
Comment 27•7 years ago
|
||
mozreview-review-reply |
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 28•7 years ago
|
||
mozreview-review |
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 29•7 years ago
|
||
mozreview-review |
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 30•7 years ago
|
||
mozreview-review-reply |
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 31•7 years ago
|
||
mozreview-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/#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 32•7 years ago
|
||
mozreview-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+
Comment 33•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 38•7 years ago
|
||
(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 39•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 40•7 years ago
|
||
mozreview-review-reply |
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
Comment 41•7 years ago
|
||
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
Comment 42•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bb3a37d321e2 https://hg.mozilla.org/mozilla-central/rev/7048637ef80c https://hg.mozilla.org/mozilla-central/rev/f25a9201e1ea https://hg.mozilla.org/mozilla-central/rev/314cd9bfe986
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 43•7 years ago
|
||
mozreview-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/#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=
Flags: needinfo?(aswan)
Blocks: 1401394
Comment 44•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 45•7 years ago
|
||
clearing needinfo, we'll pick this up over on bug 1401394
Flags: needinfo?(aswan)
Comment 46•7 years ago
|
||
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.
Updated•7 years ago
|
Flags: needinfo?(walkingice0204)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•