Closed Bug 1116693 Opened 10 years ago Closed 10 years ago

Support batching GeckoMenuItem property setters


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)

Firefox 37


(Reporter: mfinkle, Assigned: mfinkle)



(Keywords: perf)


(3 files)

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:

We don't even need the View to be updated during inflation. We can squelch the onItemChanged call completely.
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.
Assignee: nobody → mark.finkle
Attachment #8542854 - Flags: review?(rnewman)
Use the dispatch blocking during GeckoMenuItem inflation
Attachment #8542855 - Flags: review?(rnewman)
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.
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/
@@ +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) {

  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/
@@ +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+
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
(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)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.