1116693 - Support batching GeckoMenuItem property setters

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

There are times when many properties are set for a given MenuItem. Each setter will potentially cause the View to be updated. We can provide a beginBatch / endBatch system to handle these situations a little better.

Spun out from bug 1116615

Comment 1

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

The only place I'd like to try this for now is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenuInflater.java#159

We don't even need the View to be updated during inflation. We can squelch the onItemChanged call completely.

Comment 2

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

Attachment: menu-batching v0.1

Simple dispatch-blocking system like the one Richard suggested. Turns out Android's own impl has something similar. I borrowed the "Dispatch" naming from them.

http://androidxref.com/5.0.0_r2/xref/frameworks/support/v7/appcompat/src/android/support/v7/internal/view/menu/MenuBuilder.java#1032

Comment 3

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

Attachment: menuinflation-batching v0.1

Use the dispatch blocking during GeckoMenuItem inflation

Comment 4

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

Attachment: simple-title-setter v0.1

Use a simpler setter for mTitle in the GeckoMenuItem constructors so they don't dispatch item changes.

Comment 5

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

Without disabling the dispatch, Fennec fires 81 onItemChange calls during startup. With these patches, Fennec fires 24 onItemChanged calls during startup. Those 24 calls happen when about:home is loaded & selected.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8542854 [details] [diff] [review]
menu-batching v0.1

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

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +77,5 @@
> +    public void startDispatchingChanges() {
> +        startDispatchingChanges(true);
> +    }
> +
> +    public void startDispatchingChanges(boolean allowDispatch) {

If we have two methods, I'd rather get rid of the argument:

  public void startDispatchingChanges() {
      mShouldDispatchChanges = true;
      if (mDidChange) {
          mMenu.onItemChanged(this);
      }
  }

  public void resumeDispatchingChanges() {
      mShouldDispatchChanges = true;
  }

(or whatever naming).

Otherwise, have just one method with an argument, and use that required argument to force callers to make a good decision.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8542855 [details] [diff] [review]
menuinflation-batching v0.1

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

::: mobile/android/base/menu/GeckoMenuInflater.java
@@ +157,5 @@
>  
>      public void setValues(ParsedItem item, MenuItem menuItem) {
> +        // We are blocking any presenter updates during inflation.
> +        GeckoMenuItem geckoItem = (GeckoMenuItem) menuItem;
> +        if (geckoItem != null) {

This will throw a ClassCastException if menuItem isn't a GeckoMenuItem. It won't just be null. Try:

  GeckoMenuItem geckoItem = null;
  if (menuItem instanceof GeckoMenuItem) {
      geckoItem = (GeckoMenuItem) menuItem;
  }

@@ +172,5 @@
>              menuItem.setShowAsAction(item.showAsAction);
>          }
> +
> +        if (geckoItem != null) {
> +            // We don't need to allow presenter updates during inflation.

", so we use the weak form of re-enabling changes."

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8542858 [details] [diff] [review]
simple-title-setter v0.1

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

Assuming this doesn't break late-added menu items (e.g., from add-ons), LGTM.

Comment 9

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

Landed with changes from comments
remote:   https://hg.mozilla.org/integration/fx-team/rev/e517f97929b4
remote:   https://hg.mozilla.org/integration/fx-team/rev/f0568ac27834
remote:   https://hg.mozilla.org/integration/fx-team/rev/677ee53bc06e

Comment 10

[Richard Newman [:rnewman]]

These are super simple, so we might consider chasing these up for Aurora if they make a noticeable perf difference. ni? to consider.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e517f97929b4
https://hg.mozilla.org/mozilla-central/rev/f0568ac27834
https://hg.mozilla.org/mozilla-central/rev/677ee53bc06e

Comment 12

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

(In reply to Richard Newman [:rnewman] from comment #10)
> These are super simple, so we might consider chasing these up for Aurora if
> they make a noticeable perf difference. ni? to consider.

Nothing very noticeable, so letting the patches ride the train