Closed Bug 927946 Opened 6 years ago Closed 5 years ago

Add all APZC preferences to all.js

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: botond, Assigned: hharchani, Mentored)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 2 obsolete files)

Some of the APZC preferences (http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#239) are not in all.js (for example, gfx.azpc.x_skate_size_multiplier isn't), so we can't change them without manually adding them in about:config. I think we should add them all to all.js.
I'm not sure that some of these prefs get tweaked often enough to be worth adding to all.js. But the ones we expect users to tweak, sure.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> I'm not sure that some of these prefs get tweaked often enough to be worth
> adding to all.js. But the ones we expect users to tweak, sure.

Developers may want to tweak them when testing. On a mobile device, it's annoying to have to type "gfx.azpc.x_skate_size_multiplier". Is there any cost to adding them to all.js?
I was thinking of the performance cost of the extra JS, but perhaps I'm jumping to premature optimization here. There is also the cognitive cost of looking through the list of prefs when you load about:config but I guess that list is so long that it doesn't matter now anyway.

The other thing I was thinking of but forgot to mention in comment 1 is that all.js gets picked up by desktop platforms too, where APZ isn't used. So the prefs would show up in desktop builds but have no effect. I don't know if that matters though.
Attached patch Prefs added (obsolete) — Splinter Review
Pref entries have been made in all.js for all Preferences listed in AsyncPanZoomController.cpp.
Attachment #8384987 - Flags: review?(botond)
Comment on attachment 8384987 [details] [diff] [review]
Prefs added

Review of attachment 8384987 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good, thanks! A few comments:

Let's add the comment at the top of this block of prefs saying something like "For details on what these prefs do, see gfx/layers/ipc/AsyncPanZoomController.cpp."

For completeness, let's also include apz.allow-checkerboarding [1] and apz.enlarge_displayport_when_only_scrollable [2].

Finally, let's take this opportunity to rename "apz.allow-checkerboarding" to "apz.allow_checkerboarding" (underscore instead of dash) to be consistent with all the other prefs. Note that this pref is mentioned in [3] and [4], so those places will need to be updated as well.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#421
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#422
[3] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#689
[4] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#709

::: modules/libpref/src/init/all.js
@@ +297,5 @@
>  
>  // Whether to lock touch scrolling to one axis at a time
>  // 0 = FREE (No locking at all)
>  // 1 = STANDARD (Once locked, remain locked until scrolling ends)
>  // 2 = STICKY (Allow lock to be broken, with hysteresis)

This comment pertains to "apz.axis_lock_mode", let's keep it together with that.

@@ +303,5 @@
> +pref("apz.pan_repaint_interval", 250);
> +pref("apz.min_skate_speed", "1.0");
> +pref("apz.content_response_timeout", 300);
> +pref("apz.num_paint_duration_samples", 3);
> +pref("apz.touch_start_tolerance", "1.0/4.5");

Unfortunately, you can't do calculations like "1.0/4.5" in a pref string. You have to use the result of the division, and maybe mention in a comment that it came from 1.0 / 4.5.
Comment on attachment 8384987 [details] [diff] [review]
Prefs added

Review of attachment 8384987 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flag. Please re-request review after addressing the feedback in comment 5.
Attachment #8384987 - Flags: review?(botond)
Snighda, do you plan to continue working on this?
Flags: needinfo?(snigdhaagarwal93)
I have already made the changes requested in the comment. Unable to upload the patch due to some problems with another bug occurring earlier in the queue. Will upload the patch soon.
Flags: needinfo?(snigdhaagarwal93)
It doesn't look like Snigdha is still working on this and the list of APZ prefs has probably changed since then. Throwing this back on the pool of mentored bugs available to be worked on.
Mentor: bugmail.mozilla, botond
Whiteboard: [lang=js][good first bug]
I want to work on this Bug. Assign this to me and Help me. I'm completely new here.
(In reply to hharchani from comment #10)
> I want to work on this Bug. Assign this to me and Help me. I'm completely
> new here.

Great! I've assigned the bug to you.

The first step is to get a working Firefox build using the instructions here [1].

Once you've done that, you can start writing the patch. It will involve adding new preferences to modules/libpref/init/all.js. 

The existing APZ prefs in that file start here [2]. A list of all APZ prefs and their default values can be found in gfx/thebes/gfxPrefs.h [3]. (The APZ prefs are the ones whose names start with "apz.".)

For each APZ pref in gfxPrefs.h that is not already listed in all.js, you can add it there. You can look at surrounding entries in all.js to see what the correct format is.

For example, consider the following line in gfxPrefs.h:

  DECL_GFX_PREF(Live, "apz.min_skate_speed", APZMinSkateSpeed, float, 1.0f);

The corresponding line you'd add to all.js would be:

  pref("apz.min_skate_speed", "1.0");

(Notice how the floating-point constant becomes a string in all.js. This is how other floating-point-valued preferences are handled in all.js.)

Once you added the prefs to all.js, rebuild (with './mach build'), run your modified build, type 'about:config' in the address bar, and check that the newly added prefs are there.

If all went well, you can post your patch here for review.

Please let me know if you run into any issues, by posting here or emailing me, and I'll be happy to help!

[1] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[2] http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#406
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPrefs.h?rev=7aee66d772f4#132
Assignee: nobody → hharchani
Attached patch bug-927946.patch (obsolete) — Splinter Review
Attachment #8471157 - Flags: review?(botond)
Comment on attachment 8471157 [details] [diff] [review]
bug-927946.patch

Review of attachment 8471157 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks - the patch looks great!

I have a few small comments:

The APZ prefs are documented in gfx/layers/apz/src/AsyncPanZoomController.cpp [1]. Let's add a comment to all.js, at the top of the list of APZ prefs, pointing people to AsyncPanZoomController.cpp for documentation of the prefs.

Let's take this opportunity to rename "apz.allow-checkerboarding" to "apz.allow_checkerboarding" (i.e. change the dash to underscore) to be consistent with the other APZ prefs. This change needs to be made in three places: your patch to all.js, the declaration in gfxPrefs.h [2], and the documentation in AsyncPanZoomController.cpp [3].

Finally, were you able to build with your patch and check that the prefs show up in about:config?

If you could post an updated patch that makes these small changes, and check that the prefs show in about:config, that would be awesome!


[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=414eb2e95057#137
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPrefs.h?rev=7aee66d772f4#132
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=414eb2e95057#140

::: modules/libpref/init/all.js
@@ +406,5 @@
>  // Whether to lock touch scrolling to one axis at a time
>  // 0 = FREE (No locking at all)
>  // 1 = STANDARD (Once locked, remain locked until scrolling ends)
>  // 2 = STICKY (Allow lock to be broken, with hysteresis)
>  pref("apz.axis_lock_mode", 0);

Let's move this (along with the comment above it) underneath apz.asyncscroll.timeout so that it's in alphabetical order.

@@ +445,3 @@
>  
>  // Whether to print the APZC tree for debugging
>  pref("apz.printtree", false);

Let's move this (also along with its comment) into the list above in alpha order.
Attachment #8471157 - Flags: review?(botond) → review+
Attached patch bug-927946.patchSplinter Review
Fixed all issues and successfully built and run. All prefs are displayed in about:config.
Attachment #8471157 - Attachment is obsolete: true
Attachment #8471826 - Flags: review?(botond)
Comment on attachment 8471826 [details] [diff] [review]
bug-927946.patch

Review of attachment 8471826 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

I committed the patch to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/70b5978c6fb6

It will now run through our continuous integration system which does a build and runs tests on all platforms. You can follow its progress here: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=70b5978c6fb6

If it passes all the tests (which I expect it will), it will automatically be committed to mozilla-central in a day or so.

Thanks again for working on this!
Attachment #8471826 - Flags: review?(botond) → review+
hharchani, I just wanted to echo Botond's thanks. Congratulations on getting your first patch landed in the mozilla tree! If you're interested in some more challenging bugs let us know and we can definitely find something for you depending on your area of interest and which language(s) you're interested in working in.
https://hg.mozilla.org/mozilla-central/rev/70b5978c6fb6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.