move preferences get/observe from java into native-ish code

RESOLVED FIXED in Firefox 26

Status

()

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

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

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
Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 791325 [details] [diff] [review]
part 1 - pass String[] to PrefsHelper.getPrefs wherever possible

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

4 years ago
Created attachment 791327 [details] [diff] [review]
part 1b - add ArrayList getPrefs method to PrefsHelper

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

4 years ago
Created attachment 791329 [details] [diff] [review]
part 2 - factor out reading an nsString from a Java string in AndroidJavaWrappers.cpp

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

4 years ago
Created attachment 791330 [details] [diff] [review]
part 3 - add Preferences events to AndroidGeckoEvent

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

4 years ago
Created attachment 791331 [details] [diff] [review]
part 4 - refactor browser.js's getPreferences function into a JSON frontend and a "raw" backend

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

4 years ago
Created attachment 791332 [details] [diff] [review]
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON

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+
(Assignee)

Comment 11

4 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 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.
(Assignee)

Comment 14

4 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 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

4 years ago
Created attachment 794182 [details] [diff] [review]
part 6 - update tests to use the new API

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+
(Assignee)

Comment 18

4 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

4 years ago
Created attachment 794262 [details] [diff] [review]
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON

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

4 years ago
Created attachment 794263 [details] [diff] [review]
part 6 - update tests to use the new API

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+
(Assignee)

Comment 22

4 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

4 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

4 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...
(Assignee)

Updated

4 years ago
Blocks: 908675
(Assignee)

Updated

4 years ago
Blocks: 908676
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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)
(Assignee)

Comment 28

4 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.
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

4 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

4 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

4 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

4 years ago
Created attachment 804393 [details] [diff] [review]
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON

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

4 years ago
Created attachment 804394 [details] [diff] [review]
part 6 - update tests to use the new API

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
(Assignee)

Comment 36

4 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 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+
(Assignee)

Comment 38

4 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.
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
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.