Closed Bug 1414084 Opened 7 years ago Closed 6 years ago

Add-on menu items/browser actions aren't shown if Gecko is started through Custom Tabs/PWA

Categories

(Firefox for Android Graveyard :: Custom Tabs, defect, P3)

Product:
Firefox for Android Graveyard
Component:
Custom Tabs
Platform:
All
Android
Type:
defect

Tracking

(firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: 61.1p57, Assigned: JanH)

References

Details

Attachments

(18 files)

Bug 1414084 - Part 0 - Cleanups.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 9 - Move add-on menu item cache out of BrowserApp.
59 bytes, text/x-review-board-request
Details
Bug 1414084 - Part 13 - Cache PageActions.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 15 - Correctly remove already resolved PageActions, too.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 17 - Fix setting of private browsing theme state.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 1 - No longer track MenuItemInfo "added" data.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 2 - Use UUIDs for the NativeWindow menu API, too.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 3 - Store NativeWindow menu callbacks by UUID.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 4 - Move menu item ID generation into the UI.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 5 - Unify menu click event handling.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 6 - Use only one list to store menu items added from Gecko.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 7 - Use one set of functions for managing MenuItems in the UI.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 8 - Unify menu item EventDispatcher messages.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 10 - Init MenuItemInfo list right from the start.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 11 - Use a Map for the MenuItemInfo list.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 12 - Add some tests for the processing of Menu messages.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 14 - Rename isPwaAdded for more clarity.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1414084 - Part 16 - Add some tests for the handling of PageActions.
59 bytes, text/x-review-board-request
Grisha
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171102100041

Steps to reproduce:

1. Install an addon that uses browserAction
2. Force stop Nightly
3. Open a page from other apps using custom tab, then click "Open in Firefox Nightly"


Actual results:

The browserAction is not shown


Expected results:

The browserAction should be shown
Assignee: nobody → jh+bugzilla
Blocks: customtabs_geckoview, pwa_geckoview
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → All
Summary: browserAction is not shown when open from custom tabs → Add-on menu items/browser actions aren't shown if Gecko is started through Custom Tabs/PWA
See Also: → 832990
Basically this means that the menu item cache needs to be moved out of BrowserApp into something whose lifetime more closely matches that of Gecko itself (i.e. probably something hanging off GeckoApplication), so we don't miss any menu items that are added if Gecko and add-ons are loaded through GeckoView while BrowserApp isn't running.

As a follow-up to bug 832990, URL bar page actions should be handled as well.
Comment on attachment 8955828 [details]
Bug 1414084 - Part 9 - Move add-on menu item cache out of BrowserApp.

https://reviewboard.mozilla.org/r/224840/#review233630

This seems mostly like a straightforward refactoring patch. I've left some questions for you, and commented on some stuff that I think may be simplified/improved - since you're touching this code, might as well try to cautiously make it better :)

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:21
(Diff revision 1)
> +import org.mozilla.gecko.util.EventCallback;
> +import org.mozilla.gecko.util.GeckoBundle;
> +
> +import java.util.ArrayList;
> +
> +public class AddonUICache implements BundleEventListener {

I assume that other than a few changes such as "mark it as static", most of these methods are copy-pasted without changes, right?

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:21
(Diff revision 1)
> +import org.mozilla.gecko.util.EventCallback;
> +import org.mozilla.gecko.util.GeckoBundle;
> +
> +import java.util.ArrayList;
> +
> +public class AddonUICache implements BundleEventListener {

Can you please add a class-level comment describing the purpose of this cache, how it's used, its life-cycle, etc?

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:22
(Diff revision 1)
> +import org.mozilla.gecko.util.GeckoBundle;
> +
> +import java.util.ArrayList;
> +
> +public class AddonUICache implements BundleEventListener {
> +    private static final String LOGTAG = "GeckoAddonUICache";

super nit: don't we use LOG_TAG elsewhere?

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:45
(Diff revision 1)
> +        public String uuid;
> +    }
> +
> +    private static final AddonUICache instance = new AddonUICache();
> +
> +    private List<MenuItemInfo> mAddonMenuItemsCache;

At the cost of a tiny overhead, you should be able to `final` these lists and init them whenever we create an instance, right? That will help avoid all those pesky null checks and maybe even some silly bugs over time.

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:45
(Diff revision 1)
> +        public String uuid;
> +    }
> +
> +    private static final AddonUICache instance = new AddonUICache();
> +
> +    private List<MenuItemInfo> mAddonMenuItemsCache;

Can you make these into maps, keyed IDs?

I'm not familiar with the consumers of this cache very much, but if we expect them to have non-clashing IDs, a lot of the code below could be simplifed and made faster (constant time lookups instead of iterations, etc).

It will also help enforce the "IDs shouldn't clash" contract.

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:56
(Diff revision 1)
> +    }
> +
> +    private AddonUICache() { }
> +
> +    public void init() {
> +        EventDispatcher.getInstance().registerUiThreadListener(this,

You never unregister these listeners. Should you?

An appropriate place might be GeckoApplication's onDestroy, but I'm uncertain how that will interact with the cache and its consumers.

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:74
(Diff revision 1)
> +     */
> +    public void onCreateOptionsMenu(Menu menu) {
> +        mMenu = menu;
> +
> +        // Add browser action menu items, if any exist.
> +        if (mBrowserActionItemsCache != null && !mBrowserActionItemsCache.isEmpty()) {

Here and elsewhere where the same pattern is followed: 

I suspect that .isEmpty likely just slows this down a tad, and if you init the lists right away, the null check can go away as well.

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:91
(Diff revision 1)
> +    }
> +
> +    public void onDestroyOptionsMenu() {
> +        mMenu = null;
> +
> +        // Reset "added" state.

No need to explain what you're doing - rather, please explain why you're doing it. It's not clear to me what we're using this flag for.

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:94
(Diff revision 1)
> +        mMenu = null;
> +
> +        // Reset "added" state.
> +        if (mBrowserActionItemsCache != null && !mBrowserActionItemsCache.isEmpty()) {
> +            for (BrowserActionItemInfo item : mBrowserActionItemsCache) {
> +                item.added = false;

I can't figure out how 'added' property is actually used - it's flipped here and there, but never actually checked?

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:156
(Diff revision 1)
> +        item.setEnabled(info.enabled);
> +        item.setVisible(info.visible);
> +    }
> +
> +    private void addAddonMenuItem(final MenuItemInfo info) {
> +        if (mAddonMenuItemsCache == null) {

Take the opportunity to clean up this sort of thing!

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:184
(Diff revision 1)
> +                    break;
> +                }
> +            }
> +        }
> +
> +        if (mMenu == null)

Braces, please. I'm surprised our linter isn't catching this stuff!

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:196
(Diff revision 1)
> +
> +    private void updateAddonMenuItem(int id, final GeckoBundle options) {
> +        // Set attribute for the menu item in cache, if available
> +        if (mAddonMenuItemsCache != null && !mAddonMenuItemsCache.isEmpty()) {
> +            for (MenuItemInfo item : mAddonMenuItemsCache) {
> +                if (item.id == id) {

What are the chances we'll have duplicates in our cache, and not notice them since code like this just silently picks the first one?

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:269
(Diff revision 1)
> +
> +        addBrowserActionMenuItemToMenu(mMenu, info);
> +    }
> +
> +    /**
> +     * Removes a WebExtension browser action from the menu by its UUID.

Oh, they're using uuids? Definitely turn the list into the map then!

What's the relationship between item.id and item.uuid?

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:343
(Diff revision 1)
> +
> +        return null;
> +    }
> +
> +    @Override
> +    public void handleMessage(String event, GeckoBundle message, EventCallback callback) {

nit: move this right below the registration code at the top of this class?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1540
(Diff revision 1)
>          mDynamicToolbar.destroy();
>  
>          final GeckoApplication app = (GeckoApplication) getApplication();
>          app.getLightweightTheme().removeListener(this);
>  
> +        if (mMenu != null) {

This is kinda awkard; you're coupling internal state of the cache with an internal state of this class. Could you instead just call 'onDestroyOptionsMenu' regardless of what mMenu is, and let the cache do the right thing?
Attachment #8955828 - Flags: review?(gkruglov) → review+
Comment on attachment 8955828 [details]
Bug 1414084 - Part 9 - Move add-on menu item cache out of BrowserApp.

https://reviewboard.mozilla.org/r/224840/#review233630

> might as well try to cautiously make it better :)

I'll probably spin some of those changes off into an additional patch on top of this one, but yes, I guess I might :)

> I assume that other than a few changes such as "mark it as static", most of these methods are copy-pasted without changes, right?

Correct. The menu handling is somewhat entwined with the message listener, but other than that relatively self-contained, so I thought it best to just move everything together.

> super nit: don't we use LOG_TAG elsewhere?

Searching on DXR within path:mobile\*java returns 1597 results for LOGTAG vs. 1480 for LOG_TAG - but the services directory hierarchy has 1196 of the latter and none of the former, which I guess is why you're finding LOGTAG strange. For me, it's just the other way round :-)

> At the cost of a tiny overhead, you should be able to `final` these lists and init them whenever we create an instance, right? That will help avoid all those pesky null checks and maybe even some silly bugs over time.

Yes, that would be possible.

> Can you make these into maps, keyed IDs?
> 
> I'm not familiar with the consumers of this cache very much, but if we expect them to have non-clashing IDs, a lot of the code below could be simplifed and made faster (constant time lookups instead of iterations, etc).
> 
> It will also help enforce the "IDs shouldn't clash" contract.

I think I'd want to preserve the order in which items were added, but it looks like a LinkedHashMap should do the trick for that.
Re non-clashing IDs, browser.js keeps incrementing an ID counter, so those should be unique, too (https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/mobile/android/chrome/content/browser.js#2327).

> You never unregister these listeners. Should you?
> 
> An appropriate place might be GeckoApplication's onDestroy, but I'm uncertain how that will interact with the cache and its consumers.

GeckoApplication has an onDestroy method? Weird indeed...

I'd assumed the normal Android wisdom that the application only lives once (per process), but I suppose I should guard against double-initialisation just in case.

> I can't figure out how 'added' property is actually used - it's flipped here and there, but never actually checked?

Hmm yes, I might as well remove it then.

> Braces, please. I'm surprised our linter isn't catching this stuff!

Copy & paste, but you're right.

> What are the chances we'll have duplicates in our cache, and not notice them since code like this just silently picks the first one?

Current internal callers go via browser.js which does the right thing and old-style add-ons (as far as they're still relevant on Nightly for better or for worse) normally do so, too.

> Oh, they're using uuids? Definitely turn the list into the map then!
> 
> What's the relationship between item.id and item.uuid?

Search me... I'll try and see if I can find out more, though.

> This is kinda awkard; you're coupling internal state of the cache with an internal state of this class. Could you instead just call 'onDestroyOptionsMenu' regardless of what mMenu is, and let the cache do the right thing?

Now that you mention it, yes.
Comment on attachment 8955829 [details]
Bug 1414084 - Part 13 - Cache PageActions.

https://reviewboard.mozilla.org/r/224842/#review233712

First pass - sorry for an incomplete review, i'll update it later today.

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:60
(Diff revision 2)
>      private Menu mMenu;
>  
> +    // A collection of PageAction messages that are still pending transformation into
> +    // a full PageAction - most importantly transformation of the image data URI
> +    // into a Drawable.
> +    private Map<String, GeckoBundle> mPendingPageActionCache;

Same comment as in part1 around initialization.

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:79
(Diff revision 2)
>              "Menu:Update",
>              "Menu:Remove",
>              "Menu:AddBrowserAction",
>              "Menu:UpdateBrowserAction",
>              "Menu:RemoveBrowserAction",
> +            "PageActions:Add",

Same comment as in part1 about never unregistering these listeners.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java:79
(Diff revision 2)
>  
>      @Override
>      protected void onAttachedToWindow() {
>          super.onAttachedToWindow();
>  
> -        EventDispatcher.getInstance().registerUiThreadListener(this,
> +        AddonUICache.getInstance().setPageActionLayoutDelegate(new PageActionLayoutDelegate() {

What is the lifecycle of the context that's encapsulated by PageActionLayout?

This pattern will leak whatever is encapsulated in the PageActionLayout class (context, attributeset, etc); an instance of an anonymous class will have access to its surrounding environment.

As long as lifetimes of your classes line up, that might be ok... And this pattern is used a lot in the surrounding code. The main difference here is that AddonUICache is a global singleton, so the stakes here are higher. Is there any chance that, for example, onDetachedFromWindow won't always get called when it should be? It's a bit murky to think through the various scenarious quickly - but also important to not introduce a memory leak!

Anyway, try to think this through. If in doubt, just turn this into a private class and a private static instance of it with a WeakReference to PageActionLayout.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java
(Diff revision 2)
>      }
>  
> -    @Override // BundleEventListener
> +    private void onAddPageAction(final GeckoBundle message) {
> -    public void handleMessage(final String event, final GeckoBundle message,
> -                              final EventCallback callback) {
> -        ThreadUtils.assertOnUiThread();

Are you removing the assertion intentionally? We still need to be on the UI thread in these methods, right?

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java:138
(Diff revision 2)
>          hidePreviousConfirmPrompt();
>  
> -        if ("PageActions:Add".equals(event)) {
> -            final String id = message.getString("id");
> +        final String id = message.getString("id");
>  
> -            boolean alreadyAdded = isPwaAdded(id);
> +        boolean alreadyAdded = isPageActionAlreadyAdded(id);

you don't need to keep around this bool, right?
Comment on attachment 8955829 [details]
Bug 1414084 - Part 13 - Cache PageActions.

https://reviewboard.mozilla.org/r/224842/#review233712

> Same comment as in part1 about never unregistering these listeners.

Same reply as above - both us and the EventDispatcher live as long the process, so once registered, there's no need for any further action.

> What is the lifecycle of the context that's encapsulated by PageActionLayout?
> 
> This pattern will leak whatever is encapsulated in the PageActionLayout class (context, attributeset, etc); an instance of an anonymous class will have access to its surrounding environment.
> 
> As long as lifetimes of your classes line up, that might be ok... And this pattern is used a lot in the surrounding code. The main difference here is that AddonUICache is a global singleton, so the stakes here are higher. Is there any chance that, for example, onDetachedFromWindow won't always get called when it should be? It's a bit murky to think through the various scenarious quickly - but also important to not introduce a memory leak!
> 
> Anyway, try to think this through. If in doubt, just turn this into a private class and a private static instance of it with a WeakReference to PageActionLayout.

You're right about being careful about leaks, but I'm confident that using the pair of onAttached/onDetached to register/unregister is fine. Besides, this pattern of using onAttached/onDetached to manage listeners etc. is used by enough other classes in our codebase.
Comment on attachment 8955829 [details]
Bug 1414084 - Part 13 - Cache PageActions.

Cancelling until I've finished updating the patch series.
Attachment #8955829 - Flags: review?(gkruglov)
Attachment #8955830 - Flags: review?(gkruglov)
Attachment #8955831 - Flags: review?(gkruglov)
Comment on attachment 8955828 [details]
Bug 1414084 - Part 9 - Move add-on menu item cache out of BrowserApp.

https://reviewboard.mozilla.org/r/224840/#review233630

> Search me... I'll try and see if I can find out more, though.

The ID is what is used for actually managing the native Android menu. In the original design, the JS-side API was responsible for supplying an appropriate unique ID that would be transformed by a fixed offset on the Java side, but otherwise used as is for managing the native menu. The Webextension API added UUIDs presumably because they wanted a 1:1 mapping between Webextensions and BrowserActions and Webextensions already have an UUID. They kept supplying an ID as well from the JS-side, probably to minimise the changes necessary to the rest of the code.

My additional patches I've added change this completely - communications between Gecko and the Android UI only use UUIDs and the menu ID remains as an internal implementation detail of the Android UI.
Comment on attachment 8955829 [details]
Bug 1414084 - Part 13 - Cache PageActions.

https://reviewboard.mozilla.org/r/224842/#review233712

> Are you removing the assertion intentionally? We still need to be on the UI thread in these methods, right?

That class seems a little paranoid about assertOnUiThread() usage, but in any case I've added it back.

> you don't need to keep around this bool, right?

No, but I've also split this out into a separate commit, since it's not directly necessary for the main patch.
Comment on attachment 8955828 [details]
Bug 1414084 - Part 9 - Move add-on menu item cache out of BrowserApp.

(I see that you're supposed to be away until May, so take your time)
Attachment #8955828 - Flags: review+ → review?(gkruglov)
Attachment #8955828 - Flags: review?(gkruglov)
(In reply to Jan Henning [:JanH] from comment #36)
> (I see that you're supposed to be away until May, so take your time)

I'll try to go through these in the next few days. Thanks, Jan!
I don't know if this is the right place to comment, but this is happening to me in Firefox 61.0b7 accessing firefox from the Tab Queuing toast.

Tell me please if you need more info or if this is more related to some other bug.
If Grisha is apparently indisposed, would you mind taking a look at these patches (or suggest someone else who could)?
Flags: needinfo?(nalexander)
(In reply to Jan Henning [:JanH] from comment #60)
> If Grisha is apparently indisposed, would you mind taking a look at these
> patches (or suggest someone else who could)?

Hi Jan!  Grisha is back.  He and I will work out a review process for this; leaving NI so it doesn't linger too long.
No changes per se, just a rebase to the latest mozilla-central (although there haven't been too many changes on the Fennec side in any case).
Attachment #8955828 - Flags: review?(gkruglov)
Comment on attachment 8961937 [details]
Bug 1414084 - Part 1 - No longer track MenuItemInfo "added" data.

https://reviewboard.mozilla.org/r/230764/#review262594
Attachment #8961937 - Flags: review?(gkruglov) → review+
Comment on attachment 8961948 [details]
Bug 1414084 - Part 14 - Rename isPwaAdded for more clarity.

https://reviewboard.mozilla.org/r/230786/#review262614
Attachment #8961948 - Flags: review?(gkruglov) → review+
Comment on attachment 8955831 [details]
Bug 1414084 - Part 17 - Fix setting of private browsing theme state.

https://reviewboard.mozilla.org/r/224846/#review262616
Attachment #8955831 - Flags: review?(gkruglov) → review+
Comment on attachment 8955830 [details]
Bug 1414084 - Part 15 - Correctly remove already resolved PageActions, too.

https://reviewboard.mozilla.org/r/224844/#review262618
Attachment #8955830 - Flags: review?(gkruglov) → review+
Comment on attachment 8955829 [details]
Bug 1414084 - Part 13 - Cache PageActions.

https://reviewboard.mozilla.org/r/224842/#review262612

This makes sense to me, but I'd like to read an answer to my question about `onAttachedToWindow` before an r+.

::: mobile/android/base/java/org/mozilla/gecko/AddonUICache.java:74
(Diff revision 5)
> +    // a full PageAction - most importantly transformation of the image data URI
> +    // into a Drawable.
> +    private final Map<String, GeckoBundle> mPendingPageActionCache = new LinkedHashMap<>();
> +    // A collection of PageActions ready for immediate usage.
> +    private List<PageAction> mResolvedPageActionCache;
> +    private PageActionLayoutDelegate mPageActionDelegate;

Semantics, but I think only one of these is actually a cache - the `mResolvedPageActionCache`. The other list - `mPendingPageActionCache` - is a queue of PageActions that need to be transformed, and then eventually cached.

If you rename these into `mPageActionCache` and `mPendingPageActionQueue`, the code will make a bit more sense when reading casually (to me, anyway). Having two caches is a little confusing.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java:79
(Diff revision 5)
>  
>      @Override
>      protected void onAttachedToWindow() {
>          super.onAttachedToWindow();
>  
> -        EventDispatcher.getInstance().registerUiThreadListener(this,
> +        AddonUICache.getInstance().setPageActionLayoutDelegate(new PageActionLayoutDelegate() {

This change is interesting, and I don't understand the flow of things enough to say if it's problematic.

What used to be a very inexpensive call before in the UI thread codepath - I don't believe `registerUiThreadListener` will do very much - is now a potentially expensive operation. If `AddonUICache` has bunch of PageActions queued up for processing, a call to `setPageActionLayoutDelegate` will immediatly invoke bunch of expensive looking calls to `addPageAction`, which will perform some processing of drawables, etc.

Can this create a chokepoint where you wouldn't want to have one? I think `onAttachedToWindow` is called before the first call to `onDraw`, which seems like a bad time to do expensive things.

Do you think this could be a possible source of jank in some circumstances?

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java:96
(Diff revision 5)
> +            public void setCachedPageActions(final List<PageAction> cachedPageActions) {
> +                mPageActionList = cachedPageActions;
> +            }
> +        });
> +
> +        if (mPageActionList == null) {

Here you're assuming that your call above will synchronously call into setCachedPageActions. On first 'onAttachedToWindow', I think, that call will be made with a `null` instead of a list. You're then checking for `null`, and initializing the list.

While this works now, isn't it a little brittle? If the above flow changes you might end up with a null `value` being passed into `setCachedPageActions` _after_ you've done your initial null check, thus causing a runtime NPE down the line.

If something can't be null (and this class assumes that `mPageActionList` is never null) - don't create situations where we might end up setting it to `null`, and then depending on a `null` check in just the right place to save ourselves.

I think at the very least you should move the null check into `setCachedPageActions` function body, and  annotate the `PageActionLayoutDelegate` interface with `NotNull` to indicate intent here a bit better.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java:175
(Diff revision 5)
> -                }
> +            }
> -            }, important);
> +        }, important);
> +    }
>  
> -        } else if ("PageActions:Remove".equals(event)) {
> +    private void onRemovePageAction(final GeckoBundle message) {
> +        hidePreviousConfirmPrompt();

`ThreadUtils.assertOnUiThread()`
Comment on attachment 8961946 [details]
Bug 1414084 - Part 11 - Use a Map for the MenuItemInfo list.

https://reviewboard.mozilla.org/r/230782/#review262642

I would have folded this into your UUID conversion patches for clarity (to avoid reviewers having to look-ahead into the patch series), but this works too.
Attachment #8961946 - Flags: review?(gkruglov) → review+
Comment on attachment 8961945 [details]
Bug 1414084 - Part 10 - Init MenuItemInfo list right from the start.

https://reviewboard.mozilla.org/r/230780/#review262644
Attachment #8961945 - Flags: review?(gkruglov) → review+
Comment on attachment 8961938 [details]
Bug 1414084 - Part 2 - Use UUIDs for the NativeWindow menu API, too.

https://reviewboard.mozilla.org/r/230766/#review262602

This makes sense to me, but I would have folded a few subsequent commits together and called this a Pre.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:273
(Diff revision 3)
>      private ViewGroup mHomeScreenContainer;
>      private int mCachedRecentTabsCount;
>      private ActionModeCompat mActionMode;
>      private TabHistoryController tabHistoryController;
>  
>      private static final int GECKO_TOOLS_MENU = -1;

GECKO_TOOLS_MENU_ID

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:274
(Diff revision 3)
>      private int mCachedRecentTabsCount;
>      private ActionModeCompat mActionMode;
>      private TabHistoryController tabHistoryController;
>  
>      private static final int GECKO_TOOLS_MENU = -1;
> +    private static final String GECKO_TOOLS_MENU_UUID = "{115b9308-2023-44f1-a4e9-3e2197669f07}";

Can you list a source of this value (browser.js) in a comment? And in browser.js, where this is defined, add a comment pointing to this hard-coded value.

It'd be a shame if these got out of sync by accident in the future.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3417
(Diff revision 3)
>  
> -        if (mMenu == null)
> +        if (mMenu == null || id == -1)
>              return;
>  
>          final MenuItem menuItem = mMenu.findItem(id);
>          if (menuItem != null)

Feel free to sprinkle braces whenever you see one life if statemenets like this!

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3422
(Diff revision 3)
>          if (menuItem != null)
>              mMenu.removeItem(id);
>      }
>  
> -    private void updateAddonMenuItem(int id, final GeckoBundle options) {
> +    private void updateAddonMenuItem(String uuid, final GeckoBundle options) {
> +        int id = -1;

Looking ahead, you're switching cache to a map, and I suspect you'll be simplifying this code.

::: mobile/android/chrome/content/browser.js:2336
(Diff revision 3)
>    },
>  
>    menu: {
>      _callbacks: [],
>      _menuId: 1,
> -    toolsMenuID: -1,
> +    toolsMenuID: "{115b9308-2023-44f1-a4e9-3e2197669f07}",

nit: I'm not a fan of mixing names like this - something that is called an 'id' can now be a numeric value or a string...

::: mobile/android/chrome/content/browser.js:2361
(Diff revision 3)
>        this._callbacks[this._menuId] = options.callback;
>        this._menuId++;
> -      return this._menuId - 1;
> +      return options.uuid;
>      },
>  
> -    remove: function(aId) {
> +    remove: function(aUuid) {

Presumably callers of this function (and update) will use value returned by 'add' - a uuid - and so don't need to be changed?
Attachment #8961938 - Flags: review?(gkruglov) → review+
Comment on attachment 8961939 [details]
Bug 1414084 - Part 3 - Store NativeWindow menu callbacks by UUID.

https://reviewboard.mozilla.org/r/230768/#review262658
Attachment #8961939 - Flags: review?(gkruglov) → review+
Comment on attachment 8961940 [details]
Bug 1414084 - Part 4 - Move menu item ID generation into the UI.

https://reviewboard.mozilla.org/r/230770/#review262660
Attachment #8961940 - Flags: review?(gkruglov) → review+
Comment on attachment 8961941 [details]
Bug 1414084 - Part 5 - Unify menu click event handling.

https://reviewboard.mozilla.org/r/230772/#review262662

::: mobile/android/modules/BrowserActions.jsm:56
(Diff revision 3)
> -      throw new Error(`Expected "Menu:BrowserActionClicked" event - received "${event}" instead`);
> +      throw new Error(`Expected "Menu:Clicked" event - received "${event}" instead`);
>      }
>  
>      let browserAction = this._browserActions[data.item];
>      if (!browserAction) {
> -      throw new Error(`No browser action found with UUID ${data.item}`);
> +      // This was probably meant for the NativeWindow menu handler.

What's wrong with throwing here?
Comment on attachment 8961941 [details]
Bug 1414084 - Part 5 - Unify menu click event handling.

https://reviewboard.mozilla.org/r/230772/#review262664
Attachment #8961941 - Flags: review?(gkruglov) → review+
Comment on attachment 8961942 [details]
Bug 1414084 - Part 6 - Use only one list to store menu items added from Gecko.

https://reviewboard.mozilla.org/r/230774/#review262666
Attachment #8961942 - Flags: review?(gkruglov) → review+
Comment on attachment 8961943 [details]
Bug 1414084 - Part 7 - Use one set of functions for managing MenuItems in the UI.

https://reviewboard.mozilla.org/r/230776/#review262668

To me this change (and a similar one just ahead in the series) is somewhat surprising. I would expect the set of remaining functions to support only the webextensions case, but we're actually going the other way and hoping that unused (by webextensions) code won't get in a way or cause problems somehow. Can you explain why?
Hi Jan - apologies for the (rather long) delay. I've left some comments and asked some questions. I'll follow-up with a final look-over once you get back on these. Overall, this is looking good - thank you!
Flags: needinfo?(nalexander)
Comment on attachment 8961941 [details]
Bug 1414084 - Part 5 - Unify menu click event handling.

https://reviewboard.mozilla.org/r/230772/#review262662

> What's wrong with throwing here?

What it says on the tin - both BrowserActions.jsm and browser.js/NativeWindow are now listening for "Menu:Clicked" messages, because a few patches later down the road, the UI won't make any difference between the two kinds of menu items, so it won't be able to send different message types either.
If both BrowserActions.jsm and browser.js/NativeWindow are potential candidates for a particular "Menu:Clicked" message, then BrowserActions.jsm can no longer assert an error if it sees a message with an unknown (UU)ID, because the most likely explanation is that that message was intended for the NativeWindow handler instead.
Comment on attachment 8961943 [details]
Bug 1414084 - Part 7 - Use one set of functions for managing MenuItems in the UI.

https://reviewboard.mozilla.org/r/230776/#review262668

My thinking was that this might still be required again for internal usage, also potential Webextension experiments and suchlike, plus e.g. on Desktop extensions are (perhaps somewhat infamously) permitted only one top level item, with everything else shunted off into submenus. If the API on mobile evolved into a similar direction to allow multiple menu items per add-on, then sub menu support might be required as well.

Now on the other hand given that we can't even manage to finish off webextensions context menu support and also the fact that there's a big UI rewrite on the cards for better or for worse, it might be indeed unlikely that this will actually get used again, but I don't feel that the additional code really gets in the way and is absolutely worth deleting. The sub menu code seems fairly stable and can stay as is and other than that it's just checking and setting a few additional menu item properties.
Comment on attachment 8961946 [details]
Bug 1414084 - Part 11 - Use a Map for the MenuItemInfo list.

https://reviewboard.mozilla.org/r/230782/#review262642

Yes, it would have fitted thematically there as well, but at that point I then would have had to change both sets of functions. My strategy was to unify the addon menu and browser action sets of functions first and then do any further changes later.
Comment on attachment 8961938 [details]
Bug 1414084 - Part 2 - Use UUIDs for the NativeWindow menu API, too.

https://reviewboard.mozilla.org/r/230766/#review262602

I wanted to break things down into small steps, but yes, I might have gone a bit overboard on micro-commits here. But each individual commit should leave things in a working state.

> Can you list a source of this value (browser.js) in a comment? And in browser.js, where this is defined, add a comment pointing to this hard-coded value.
> 
> It'd be a shame if these got out of sync by accident in the future.

I would have seen things the other way round, i.e. BrowserApp.java as the source of truth, but either way you're right.

> Feel free to sprinkle braces whenever you see one life if statemenets like this!

I feel, I probably just hadn't been looking very hard for them.

> Looking ahead, you're switching cache to a map, and I suspect you'll be simplifying this code.

Hmm... I'll be removing the for loops and such like, since I'll be able to retrieve the menu item directly. As for the `id == - 1` business, yes, good point. Here I'm just copying the logic used by the browser action functions to the addon menu functions, but I'll update the patch for switching to a map to get rid of that again as well.

> nit: I'm not a fan of mixing names like this - something that is called an 'id' can now be a numeric value or a string...

Yes, but it would be a breaking change.

> Presumably callers of this function (and update) will use value returned by 'add' - a uuid - and so don't need to be changed?

Yes, see also my comments in the commit message.
Comment on attachment 8955829 [details]
Bug 1414084 - Part 13 - Cache PageActions.

https://reviewboard.mozilla.org/r/224842/#review262612

> This change is interesting, and I don't understand the flow of things enough to say if it's problematic.
> 
> What used to be a very inexpensive call before in the UI thread codepath - I don't believe `registerUiThreadListener` will do very much - is now a potentially expensive operation. If `AddonUICache` has bunch of PageActions queued up for processing, a call to `setPageActionLayoutDelegate` will immediatly invoke bunch of expensive looking calls to `addPageAction`, which will perform some processing of drawables, etc.
> 
> Can this create a chokepoint where you wouldn't want to have one? I think `onAttachedToWindow` is called before the first call to `onDraw`, which seems like a bad time to do expensive things.
> 
> Do you think this could be a possible source of jank in some circumstances?

The drawable is normally (always?) a data URL, so at least shouldn't involve file system/network access, but it does take a few ms and getting the drawable seems to be indeed the bulk of that time.

For typical situations I still don't think this should be too much of a problem, though and it seems that unlike menu items, merely starting up Gecko by loading a custom tab/web app isn't enough to queue up a bunch of page actions - that only seems to happen when actually loading pages in the Fennec UI. So if my observations generally hold true, the `PendingPageActionQueue` would only be used in anger if you started Fennec, moved it into the background while the intial page load was still proceeding and then the BrowserApp UI (including the PageActionLayout) was destroyed (easier to replicate if you use the "Don't keep activities" debug option). In that case page actions would be generated without an active PageActionLayout and might then be transferred en masse during `onAttachedToWindow`, potentially causing jank.

If you think that it is still worth to really avoid this, I think it might be possible to move the `ResourceDrawableUtils.getDrawable` call onto the background thread without too much pain, although I need to look at it a little more in detail.
Hi Jan - thanks for addressing my feedback! I'll do another pass on this later today.
Comment on attachment 8961943 [details]
Bug 1414084 - Part 7 - Use one set of functions for managing MenuItems in the UI.

https://reviewboard.mozilla.org/r/230776/#review266856

OK, I trust your judgement on this and generally agree that keeping this code around shouldn't cause harm.
Attachment #8961943 - Flags: review?(gkruglov) → review+
Comment on attachment 8961944 [details]
Bug 1414084 - Part 8 - Unify menu item EventDispatcher messages.

https://reviewboard.mozilla.org/r/230778/#review266866

Same comment as on the other similar commit - let's unify around the larger set of functionality for now, and re-assess if that proves problematic.
Attachment #8961944 - Flags: review?(gkruglov) → review+
Comment on attachment 8955829 [details]
Bug 1414084 - Part 13 - Cache PageActions.

https://reviewboard.mozilla.org/r/224842/#review262612

> The drawable is normally (always?) a data URL, so at least shouldn't involve file system/network access, but it does take a few ms and getting the drawable seems to be indeed the bulk of that time.
> 
> For typical situations I still don't think this should be too much of a problem, though and it seems that unlike menu items, merely starting up Gecko by loading a custom tab/web app isn't enough to queue up a bunch of page actions - that only seems to happen when actually loading pages in the Fennec UI. So if my observations generally hold true, the `PendingPageActionQueue` would only be used in anger if you started Fennec, moved it into the background while the intial page load was still proceeding and then the BrowserApp UI (including the PageActionLayout) was destroyed (easier to replicate if you use the "Don't keep activities" debug option). In that case page actions would be generated without an active PageActionLayout and might then be transferred en masse during `onAttachedToWindow`, potentially causing jank.
> 
> If you think that it is still worth to really avoid this, I think it might be possible to move the `ResourceDrawableUtils.getDrawable` call onto the background thread without too much pain, although I need to look at it a little more in detail.

It sounds like this might be a potential source of jank in a pretty narrow set of circumstances. On top of what you described, we'd also need to actually have a lot of page actions to process, as well.

I'm OK with having this as-is, provided you add a little comment summarizing our conversation.
Comment on attachment 8955829 [details]
Bug 1414084 - Part 13 - Cache PageActions.

https://reviewboard.mozilla.org/r/224842/#review266872

This LGTM, with the two open clean-up comment addressed.
Attachment #8955829 - Flags: review?(gkruglov) → review+
Comment on attachment 8961947 [details]
Bug 1414084 - Part 12 - Add some tests for the processing of Menu messages.

https://reviewboard.mozilla.org/r/230784/#review266874
Attachment #8961947 - Flags: review?(gkruglov) → review+
Comment on attachment 8961949 [details]
Bug 1414084 - Part 16 - Add some tests for the handling of PageActions.

https://reviewboard.mozilla.org/r/230788/#review266876
Attachment #8961949 - Flags: review?(gkruglov) → review+
Thanks a lot, JanH! This is good work.

Something's off with my Try permissions and reviewboard isn't letting me push to see if it's all green; I'll get that resolved tomorrow. In the meantime, push it up yourself if you have try access.
Comment on attachment 8955829 [details]
Bug 1414084 - Part 13 - Cache PageActions.

https://reviewboard.mozilla.org/r/224842/#review262612

> Here you're assuming that your call above will synchronously call into setCachedPageActions. On first 'onAttachedToWindow', I think, that call will be made with a `null` instead of a list. You're then checking for `null`, and initializing the list.
> 
> While this works now, isn't it a little brittle? If the above flow changes you might end up with a null `value` being passed into `setCachedPageActions` _after_ you've done your initial null check, thus causing a runtime NPE down the line.
> 
> If something can't be null (and this class assumes that `mPageActionList` is never null) - don't create situations where we might end up setting it to `null`, and then depending on a `null` check in just the right place to save ourselves.
> 
> I think at the very least you should move the null check into `setCachedPageActions` function body, and  annotate the `PageActionLayoutDelegate` interface with `NotNull` to indicate intent here a bit better.

> I think at the very least you should move the null check into setCachedPageActions function body

I did that, if it was that what you meant...

> I think at the very least you should move the null check into setCachedPageActions function body

but I'm not quite sure where I'd be able to add such an annotation, unless I made the AddonUICache always pass in a non-null PageAction list when calling setCachedPageActions, in which case however the null check within the PageActionLayoutDelegate would become superfluous, wouldn't it?
@Grisha: I'm still not quite sure regarding https://reviewboard.mozilla.org/r/224842/#comment337646 - is the current version acceptable now, or does it need some more work?
Flags: needinfo?(gkruglov)
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/74feab038c33
Part 0 - Cleanups. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/65a6e2ffdbae
Part 1 - No longer track MenuItemInfo "added" data. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/9fc1c87f952f
Part 2 - Use UUIDs for the NativeWindow menu API, too. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/d2aa38b39fa0
Part 3 - Store NativeWindow menu callbacks by UUID. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/c82c7117206f
Part 4 - Move menu item ID generation into the UI. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/b6e63d47b9bb
Part 5 - Unify menu click event handling. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/261e4bb6efd8
Part 6 - Use only one list to store menu items added from Gecko. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/98ed7d6f8a7a
Part 7 - Use one set of functions for managing MenuItems in the UI. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/84bac01f3a35
Part 8 - Unify menu item EventDispatcher messages. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/eb1ac381e2fb
Part 9 - Move add-on menu item cache out of BrowserApp. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/47963c089051
Part 10 - Init MenuItemInfo list right from the start. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/39819b958e01
Part 11 - Use a Map for the MenuItemInfo list. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/3dcf1f67d7e4
Part 12 - Add some tests for the processing of Menu messages. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/a40e70bbfe1a
Part 13 - Cache PageActions. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/e8d7b4fd9838
Part 14 - Rename isPwaAdded for more clarity. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/c2580679282d
Part 15 - Correctly remove already resolved PageActions, too. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/f565a28721bf
Part 16 - Add some tests for the handling of PageActions. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/8bc5f3cdc25c
Part 17 - Fix setting of private browsing theme state. r=Grisha
It's good - sorry, missed your question. But I see that you pushed it already!
Flags: needinfo?(gkruglov)
Verified as fixed on latest Nightly build (63.0a1 - 08/21);
Devices: Nexus 5(Android 6.0.1), OnePlus 5T (8.1.0).
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: