Closed
Bug 906088
Opened 11 years ago
Closed 11 years ago
move preferences get/observe from java into native-ish code
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(7 files, 4 obsolete files)
4.02 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
2.86 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
3.20 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
8.00 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
2.89 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
17.07 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
24.25 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
We want JSONArray to go away, since we're going to be transferring strings via JNI.
Attachment #791325 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 2•11 years ago
|
||
Related to the last patch, this is the lone place where we transfer an ArrayList. Just move the code into PrefsHelper in case anybody ever wants to do something similar.
Attachment #791327 -
Flags: review?
Assignee | ||
Comment 3•11 years ago
|
||
We duplicate code in several places, and we're going to need this same code for reading a String[].
Attachment #791329 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 4•11 years ago
|
||
This is just the C++ side of things; it's simpler to have this code split out as a separate patch to review.
Attachment #791330 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 5•11 years ago
|
||
This is a small patch, but I think it makes part 5 a little easier to understand. In part five, we're going to be introducing XPCOM methods that browser.js needs to implement. That patch is going to require something very much like handlePreferencesRequest, so just make it visible here and then part 5 should be much easier to understand.
Attachment #791331 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 6•11 years ago
|
||
This is the big patch that hooks up handling of the preferences AndroidGeckoEvent type in nsAppShell and routes requests to JS through our normal mechanism, rather than through the observer service. Overhead for JSON serialization drops away in traces due to this patch and I daresay it might even be slightly more efficient. Haven't figured out a good way of measuring that, yet, since it requires monitoring several languages at once...
Attachment #791332 -
Flags: review?(mark.finkle)
Attachment #791332 -
Flags: review?(blassey.bugs)
Comment 7•11 years ago
|
||
Comment on attachment 791325 [details] [diff] [review] part 1 - pass String[] to PrefsHelper.getPrefs wherever possible >+ final String[] prefs = { PREF_SCROLLING_FRICTION_FAST, >+ PREF_SCROLLING_FRICTION_SLOW, >+ PREF_SCROLLING_MAX_EVENT_ACCELERATION, >+ PREF_SCROLLING_OVERSCROLL_DECEL_RATE, >+ PREF_SCROLLING_OVERSCROLL_SNAP_LIMIT, >+ PREF_SCROLLING_MIN_SCROLLABLE_DISTANCE }; I'm not sure if there is a convention or not, but since we *love* our 80 chars in Java code, could we use this format: final String[] prefs = { PREF_SCROLLING_FRICTION_FAST, PREF_SCROLLING_FRICTION_SLOW, PREF_SCROLLING_MAX_EVENT_ACCELERATION, PREF_SCROLLING_OVERSCROLL_DECEL_RATE, PREF_SCROLLING_OVERSCROLL_SNAP_LIMIT, PREF_SCROLLING_MIN_SCROLLABLE_DISTANCE };
Comment 8•11 years ago
|
||
Comment on attachment 791331 [details] [diff] [review] part 4 - refactor browser.js's getPreferences function into a JSON frontend and a "raw" backend > getPreferences: function getPreferences(aPrefsRequest, aListen) { >+ this.handlePreferencesRequest(aPrefsRequest.requestId, >+ aPrefsRequest.preferences, >+ aListen); No need to wrap these. We don't limit to 80 in this file. >+ }, >+ >+ handlePreferencesRequest: function handlePreferencesRequest(aRequestId, >+ aPrefNames, >+ aListen) { Same here
Attachment #791331 -
Flags: review?(mark.finkle) → review+
Comment 9•11 years ago
|
||
Don't forget to also update the tests in mobile/android/base/tests. Some of that code also reads prefs via Preferences:Get.
Comment 10•11 years ago
|
||
Comment on attachment 791332 [details] [diff] [review] part 5 - send preference requests to JS through JNI and XPConnect rather than JSON >diff --git a/widget/android/nsIAndroidBridge.idl b/widget/android/nsIAndroidBridge.idl > interface nsIAndroidBrowserApp : nsISupports { > nsIBrowserTab getBrowserTab(in int32_t tabId); >+ void getPreferences(in int32_t requestId, >+ [array, size_is(count)] in wstring prefNames, >+ in unsigned long count); >+ void observePreferences(in int32_t requestId, >+ [array, size_is(count)] in wstring prefNames, >+ in unsigned long count); The only thing about this patch that I'd want to change would be this part. Could we create a new nsIBrowserPreferences { get(...), observe(...) } interface? We'd add a getBrowserPreferences to nsIBrowserApp. You could keep the impl of these methods in BrowserApp (in browser.js) for now. Meaning: getBrowserPreferences: function() { return this; } When we decide to move the preference code out of BrowserApp, we can update this imp.
Attachment #791332 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 791327 [details] [diff] [review] part 1b - add ArrayList getPrefs method to PrefsHelper Apparently I didn't set reviewers properly on this one.
Attachment #791327 -
Flags: review? → review?(bnicholson)
Comment 12•11 years ago
|
||
Comment on attachment 791332 [details] [diff] [review] part 5 - send preference requests to JS through JNI and XPConnect rather than JSON Review of attachment 791332 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/PrefsHelper.java @@ +58,2 @@ > GeckoAppShell.sendEventToGecko(event); > } catch (Exception e) { I wonder what exceptions this is here to to catch
Attachment #791332 -
Flags: review?(blassey.bugs) → review+
Updated•11 years ago
|
Attachment #791330 -
Flags: review?(blassey.bugs) → review+
Updated•11 years ago
|
Attachment #791329 -
Flags: review?(blassey.bugs) → review+
Updated•11 years ago
|
Attachment #791325 -
Flags: review?(blassey.bugs) → review+
Comment 13•11 years ago
|
||
Comment on attachment 791327 [details] [diff] [review] part 1b - add ArrayList getPrefs method to PrefsHelper Review of attachment 791327 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure I understand this patch. Before, we had 'new JSONArray(prefs)' to do the ArrayList->JSONArray conversion. Now you manually iterate over the ArrayList to populate the JSONArray -- why the change? 'new JSONArray(prefs)' seems like a more concise way to do the same thing. I also don't see the need for a second getPrefs() here since we do this in only one place as you said, but I don't have a strong opinion either way.
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #13) > I'm not sure I understand this patch. Before, we had 'new JSONArray(prefs)' > to do the ArrayList->JSONArray conversion. Now you manually iterate over the > ArrayList to populate the JSONArray -- why the change? 'new > JSONArray(prefs)' seems like a more concise way to do the same thing. Ah, I'm not quite sure why I did it that way either. I can change it. In either event, the JSONArray bits go away in patch 5. > I also don't see the need for a second getPrefs() here since we do this in > only one place as you said, but I don't have a strong opinion either way. I think it is marginally nicer to have clients be able to pass in the data in their preferred format and have PreferencesHelper deal with converting it to a String[].
Comment 15•11 years ago
|
||
Comment on attachment 791327 [details] [diff] [review] part 1b - add ArrayList getPrefs method to PrefsHelper Review of attachment 791327 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Nathan Froyd (:froydnj) from comment #14) > Ah, I'm not quite sure why I did it that way either. I can change it. In > either event, the JSONArray bits go away in patch 5. I didn't realize this code was dropped in patch 5, so it's not really an issue. LGTM.
Attachment #791327 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 16•11 years ago
|
||
This patch changes all the tests to use the new, specialized preferences events. It's pretty straightforward, but it's causing testMasterPassword (which is already intermittent) to perma-orange on try: https://tbpl.mozilla.org/?tree=Try&rev=36db0d36daa2 (with some known rc1 failures) https://tbpl.mozilla.org/?tree=Try&rev=1319f8947c1c (with changes to fix rc1 failures) Do these changes look sane enough? The console messages in the log suggest that I should go looking in the XPCOM bits to make sure we're passing preferences to JS correctly, but it's also possible the testMasterPassword bits are just poorly written.
Attachment #794182 -
Flags: feedback?(bugmail.mozilla)
Comment 17•11 years ago
|
||
Comment on attachment 794182 [details] [diff] [review] part 6 - update tests to use the new API Review of attachment 794182 [details] [diff] [review]: ----------------------------------------------------------------- These changes look sane enough, although I would prefer using a single "random number" for the id rather than a handful. I looked at the second try link you have there and the testMasterPassword failure appears to be related to this in the console output: 12:29:54 INFO - 08-22 12:29:25.421 E/GeckoConsole( 3133): MasterPassword.setPassword: TypeError: aPrefNames is undefined It should be not too hard to track that down, I think. The exception is logged at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/MasterPassword.js#48 so it's coming from somewhere in that block. Not sure if that try run had the latest version of your patches or not.
Attachment #794182 -
Flags: feedback?(bugmail.mozilla) → feedback+
Assignee | ||
Comment 18•11 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17) > Comment on attachment 794182 [details] [diff] [review] > part 6 - update tests to use the new API > > Review of attachment 794182 [details] [diff] [review]: > ----------------------------------------------------------------- > > These changes look sane enough, although I would prefer using a single > "random number" for the id rather than a handful. > > I looked at the second try link you have there and the testMasterPassword > failure appears to be related to this in the console output: > > 12:29:54 INFO - 08-22 12:29:25.421 E/GeckoConsole( 3133): > MasterPassword.setPassword: TypeError: aPrefNames is undefined > > It should be not too hard to track that down, I think. The exception is > logged at > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ > MasterPassword.js#48 so it's coming from somewhere in that block. Not sure > if that try run had the latest version of your patches or not. It didn't, but the second try run (which did have the latest version of the patches) is showing the same error. I didn't modify the setPreferences (Preferences:Set) path, though, so I'm slightly skeptical that my changes had anything to do with this. And there's no reference to aPrefNames in MasterPassword.js (though there is in my browser.js changes)...
Assignee | ||
Comment 19•11 years ago
|
||
Small fix to notifyPrefObservers to call getPreferences with the new argument style. Carrying over r+.
Attachment #791332 -
Attachment is obsolete: true
Attachment #794262 -
Flags: review+
Assignee | ||
Comment 20•11 years ago
|
||
Fixed testPrefsObserver to use the correct requestId for unregistering observers. Changed all tests to use th same hexadecimal constant, per Kats's feedback. Asking for actual review now. Local robocop testing shows the failing tests on try now pass locally, so I'm assuming I have fixed the issues...
Attachment #794182 -
Attachment is obsolete: true
Attachment #794263 -
Flags: review?(bugmail.mozilla)
Comment 21•11 years ago
|
||
Comment on attachment 794263 [details] [diff] [review] part 6 - update tests to use the new API Review of attachment 794263 [details] [diff] [review]: ----------------------------------------------------------------- r=me. I'd like to see the comment below addressed but won't block this just for that. Also I would still suggest doing a try push to make sure the tests pass; our robocop tests have a tendency to work just fine locally and then blow up on tbpl. ::: mobile/android/base/tests/testAddonManager.java.in @@ +59,5 @@ > mActions.sendGeckoEvent("Preferences:Set", jsonPref.toString()); > > // Wait for confirmation of the pref change before proceeding with the test. > + final String[] prefNames = { "extensions.getAddons.browseAddons" }; > + final int ourRequestId = 0x7357; I'm still a little unhappy with this constant. Can we move it to be a package static final int in BaseTest.java, with a comment that explains it's just some random number that shouldn't overlap with any actual request IDs from the app? We can use that const value in all the tests rather than having this number and duplicated local/class variables. If there was a good way to push this all the way into FennecNativeActions.java or RobocopAPI.java I would prefer that but I can't think of anything.
Attachment #794263 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 22•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1117002f074f https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe519d90c1a https://hg.mozilla.org/integration/mozilla-inbound/rev/dc998f5fe868 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d806b7e8050 https://hg.mozilla.org/integration/mozilla-inbound/rev/721318716869 https://hg.mozilla.org/integration/mozilla-inbound/rev/671bb3cfad3f https://hg.mozilla.org/integration/mozilla-inbound/rev/c03239a1493a Try run is here: https://tbpl.mozilla.org/?tree=Try&rev=a2b0edb5c306 It's mostly green, with some occasional testHistory orange. IMHO, that orange looks like the revenge of bug 907624 and not problems with my patch. I'll let the sheriffs decide...
Flags: in-testsuite+
Assignee | ||
Comment 23•11 years ago
|
||
I'll also file followup bugs for mfinkle's comment 10 and kats's comment 21 and address those early next week.
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #22) > Try run is here: https://tbpl.mozilla.org/?tree=Try&rev=a2b0edb5c306 > > It's mostly green, with some occasional testHistory orange. IMHO, that > orange looks like the revenge of bug 907624 and not problems with my patch. > I'll let the sheriffs decide... Should also say that my try push was with a slightly older try, so that may have been the problem too...
Comment 25•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1117002f074f https://hg.mozilla.org/mozilla-central/rev/1fe519d90c1a https://hg.mozilla.org/mozilla-central/rev/dc998f5fe868 https://hg.mozilla.org/mozilla-central/rev/3d806b7e8050 https://hg.mozilla.org/mozilla-central/rev/721318716869 https://hg.mozilla.org/mozilla-central/rev/671bb3cfad3f https://hg.mozilla.org/mozilla-central/rev/c03239a1493a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment 26•11 years ago
|
||
FYI, either this bug or bug 905759 (or the combination) appears to have reduced memory usage (resident memory 30s after startup) by 3.68 MiB. Nice! This is the exact pushlog range over which the improvement happened: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fa56d4c9e630&tochange=610f5f410e3b
Comment 27•11 years ago
|
||
(The above is based on the areweslimyet.com/mobile data)
Assignee | ||
Comment 28•11 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26) > FYI, either this bug or bug 905759 (or the combination) appears to have > reduced memory usage (resident memory 30s after startup) by 3.68 MiB. Nice! =/ I was thinking about backing this bug out today after investigating bug 908919. I'm not sure whether the Talos test is no good or whether this bug did actually slow things down.
Comment 29•11 years ago
|
||
This was backed out due to bug 908919. https://hg.mozilla.org/mozilla-central/rev/8df4a9443d38
Assignee: nobody → nfroyd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 26 → ---
Assignee | ||
Comment 30•11 years ago
|
||
So, I did a little benchmarking on Try. An example push from m-i, selected because it was recent and had green Android 4.0 ts: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4dceda951fba ts has a score of 3928.79. Now, Try with the sequence of patches from this bug: https://tbpl.mozilla.org/?tree=Try&rev=9975bfb8b873 ts has a score of 4366.95. A large increase, and the reason we backed this out. However, a small reordering of switch cases: https://tbpl.mozilla.org/?tree=Try&rev=113243a9d74f ts has a score of 3949.95 -- nearly the same as inbound. I really don't understand this. I thought this was something to do with code locality, hence the reason I moved the cases around. But AFAICS, the switch is implemented as a jump table, so the ordering of the cases shouldn't really matter. 2.2 scores are all over the map: Inbound at 4249.58, these patches at 4757.16, and small tweak at 3782 (!). I feel like the changes are trivial enough that they can be checked in unreviewed. But I'd really like to understand *why* they causes these changes. Any suggestions?
Assignee | ||
Comment 31•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2910cc092c19 https://hg.mozilla.org/integration/mozilla-inbound/rev/01d6b977021f https://hg.mozilla.org/integration/mozilla-inbound/rev/8671264a5aaa https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1603d47f6c https://hg.mozilla.org/integration/mozilla-inbound/rev/fcff9ef5cf7d https://hg.mozilla.org/integration/mozilla-inbound/rev/a498d1641c16 https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b4781bd918 I relanded this, since Talos now says there's little or no change relative to inbound: https://tbpl.mozilla.org/?tree=Try&rev=50996918f565 (from some time ago) https://tbpl.mozilla.org/?tree=Try&rev=69a38d811cf3 (based off m-c as of this morning) Not sure whether that's better or worse than moving small amounts of code around to no discernible codegen changes...
Assignee | ||
Comment 32•11 years ago
|
||
Backed out for busting robocop tests with menus, among other things: https://hg.mozilla.org/integration/mozilla-inbound/rev/67646b378722
Assignee | ||
Comment 33•11 years ago
|
||
Turns out Preferences:RemoveObservers needs exactly the same datatype for its requestId as the original observe request. Might as well just move it over to XPConnect, too, though it's not used anywhere but the tests. Asking for re-review, but the changes are straightforward.
Attachment #794262 -
Attachment is obsolete: true
Attachment #804393 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 34•11 years ago
|
||
Updating the patch to plumb the new removeObservers all the way through. Try shows green: https://tbpl.mozilla.org/?tree=Try&rev=c34bad9b3956
Attachment #794263 -
Attachment is obsolete: true
Attachment #804394 -
Flags: review?(bugmail.mozilla)
Comment 35•11 years ago
|
||
I suspect this patch caused a regression in "time to throbber stop". Numbers went back to normal after these patches got backed out. More details here: https://bugzilla.mozilla.org/show_bug.cgi?id=909380#c1
Assignee | ||
Comment 36•11 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #35) > I suspect this patch caused a regression in "time to throbber stop". Numbers > went back to normal after these patches got backed out. > > More details here: https://bugzilla.mozilla.org/show_bug.cgi?id=909380#c1 That seems likely, since it regressed Talos Ts. However, the new crop of patches shows no regressions on talos relative to m-c or m-i. So I feel pretty confident about landing these.
Comment 37•11 years ago
|
||
Comment on attachment 804394 [details] [diff] [review] part 6 - update tests to use the new API Review of attachment 804394 [details] [diff] [review]: ----------------------------------------------------------------- r=me with indentation fixed ::: build/mobile/robocop/FennecNativeActions.java.in @@ +45,5 @@ > private Method mUnregisterEventListener; > private Method mBroadcastEvent; > + private Method mPreferencesGetEvent; > + private Method mPreferencesObserveEvent; > + private Method mPreferencesRemoveObserversEvent; indentation @@ +291,5 @@ > + > + public void sendPreferencesRemoveObserversEvent(int requestId) { > + try { > + mPreferencesRemoveObserversEvent.invoke(mRobocopApi, requestId); > + } catch (IllegalAccessException e) { woah crazy indentation
Attachment #804394 -
Flags: review?(bugmail.mozilla) → review+
Updated•11 years ago
|
Attachment #804393 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 38•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9de79eb78556 https://hg.mozilla.org/integration/mozilla-inbound/rev/0482a7192d05 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b551b5c270f https://hg.mozilla.org/integration/mozilla-inbound/rev/460a4dccb18b https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ccee9586b6 https://hg.mozilla.org/integration/mozilla-inbound/rev/b90df20f637f https://hg.mozilla.org/integration/mozilla-inbound/rev/13b45cdfd6ed Third time's the charm. Green try run in comment 34.
Comment 39•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9de79eb78556 https://hg.mozilla.org/mozilla-central/rev/0482a7192d05 https://hg.mozilla.org/mozilla-central/rev/1b551b5c270f https://hg.mozilla.org/mozilla-central/rev/460a4dccb18b https://hg.mozilla.org/mozilla-central/rev/d8ccee9586b6 https://hg.mozilla.org/mozilla-central/rev/b90df20f637f https://hg.mozilla.org/mozilla-central/rev/13b45cdfd6ed
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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
•