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)

defect
Not set
normal

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.
We want JSONArray to go away, since we're going to be transferring strings via JNI.
Attachment #791325 - Flags: review?(blassey.bugs)
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?
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)
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)
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)
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 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 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+
Don't forget to also update the tests in mobile/android/base/tests. Some of that code also reads prefs via Preferences:Get.
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+
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 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+
Attachment #791330 - Flags: review?(blassey.bugs) → review+
Attachment #791329 - Flags: review?(blassey.bugs) → review+
Attachment #791325 - Flags: review?(blassey.bugs) → review+
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.
(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 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+
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 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+
(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)...
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+
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 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+
I'll also file followup bugs for mfinkle's comment 10 and kats's comment 21 and address those early next week.
(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...
Blocks: 908675
Blocks: 908676
Depends on: 908919
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
(The above is based on the areweslimyet.com/mobile data)
(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.
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 → ---
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?
Backed out for busting robocop tests with menus, among other things:

https://hg.mozilla.org/integration/mozilla-inbound/rev/67646b378722
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)
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)
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
Depends on: 909380
(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 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+
Attachment #804393 - Flags: review?(mark.finkle) → review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.