Closed
Bug 1116693
Opened 9 years ago
Closed 9 years ago
Support batching GeckoMenuItem property setters
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 37
People
(Reporter: mfinkle, Assigned: mfinkle)
References
Details
(Keywords: perf)
Attachments
(3 files)
5.05 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
1.30 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Use the dispatch blocking during GeckoMenuItem inflation
Attachment #8542855 -
Flags: review?(rnewman)
Assignee | ||
Comment 4•9 years ago
|
||
Use a simpler setter for mTitle in the GeckoMenuItem constructors so they don't dispatch item changes.
Attachment #8542858 -
Flags: review?(rnewman)
Assignee | ||
Comment 5•9 years ago
|
||
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.
Updated•9 years ago
|
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
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•9 years ago
|
||
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Assignee | ||
Comment 12•8 years ago
|
||
(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)
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
•