Support batching GeckoMenuItem property setters

RESOLVED FIXED in Firefox 37

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

(Blocks: 1 bug, {perf})

unspecified
Firefox 37
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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
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.
Created attachment 8542854 [details] [diff] [review]
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
Assignee: nobody → mark.finkle
Attachment #8542854 - Flags: review?(rnewman)
Created attachment 8542855 [details] [diff] [review]
menuinflation-batching v0.1

Use the dispatch blocking during GeckoMenuItem inflation
Attachment #8542855 - Flags: review?(rnewman)
Created attachment 8542858 [details] [diff] [review]
simple-title-setter v0.1

Use a simpler setter for mTitle in the GeckoMenuItem constructors so they don't dispatch item changes.
Attachment #8542858 - Flags: review?(rnewman)
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.
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
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.
Attachment #8542854 - Flags: review?(rnewman) → review+
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."
Attachment #8542855 - Flags: review?(rnewman) → review+
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.
Attachment #8542858 - Flags: review?(rnewman) → review+
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
These are super simple, so we might consider chasing these up for Aurora if they make a noticeable perf difference. ni? to consider.
Flags: needinfo?(mark.finkle)
Keywords: perf
https://hg.mozilla.org/mozilla-central/rev/e517f97929b4
https://hg.mozilla.org/mozilla-central/rev/f0568ac27834
https://hg.mozilla.org/mozilla-central/rev/677ee53bc06e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
(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
Flags: needinfo?(mark.finkle)
You need to log in before you can comment on or make changes to this bug.