Closed Bug 1699601 Opened 7 months ago Closed 7 months ago

Make the distinction between promiseApzRepaintsFlushed and promiseApzFlushedRepaints more evident

Categories

(Core :: Panning and Zooming, task)

task

Tracking

()

RESOLVED FIXED
89 Branch
Tracking Status
firefox88 --- fixed
firefox89 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file)

As Botond pointed out here we have two functions, promiseApzRepaintsFlushed and promiseApzFlushedRepaints which do slightly different things but have confusingly similar names.

Naming being one of the hardest problems I'm not entirely sure what to do here. My inclination is to rename promiseApzRepaintsFlushed something more along the lines of promiseApzControllerFlushed since it only flushes the APZ side of things, and can leave a paint pending on the main thread.

Also promiseApzRepaintsFlushed is the one with fewer callers, and therefore fewer call sites to change. Many of those call sites could probably actually be changed to call the other one which should be safe unless the test in question is specifically want to exercise behaviour with a pending paint.

Feedback on the naming is welcome! I probably won't actually start work on it for a few days.

(In reply to Kartikaya Gupta (email:kats@mozilla.staktrace.com) from comment #0)

Also promiseApzRepaintsFlushed is the one with fewer callers, and therefore fewer call sites to change. Many of those call sites could probably actually be changed to call the other one which should be safe unless the test in question is specifically want to exercise behaviour with a pending paint.

Based on this, would it be fair to say In terms of usage guidance that promiseApzFlushedRepaints() is the one to reach for by default?

If so, we could stick the word Only into the name of the other one, which would both be descriptive (it does fewer things), and make it so that the more-frequently used one has the shorter name.

(In reply to Botond Ballo [:botond] from comment #1)

Based on this, would it be fair to say In terms of usage guidance that promiseApzFlushedRepaints() is the one to reach for by default?

Yep.

If so, we could stick the word Only into the name of the other one, which would both be descriptive (it does fewer things), and make it so that the more-frequently used one has the shorter name.

Fair enough. How about promiseOnlyApzControllerFlushed? I still feel it's valuable to move this function away from the "repaint" terminology to reduce ambiguity about what it does.

(In reply to Kartikaya Gupta (email:kats@mozilla.staktrace.com) from comment #2)

Fair enough. How about promiseOnlyApzControllerFlushed? I still feel it's valuable to move this function away from the "repaint" terminology to reduce ambiguity about what it does.

Sounds good!

https://treeherder.mozilla.org/jobs?repo=try&revision=df077e4c77815638602c51b169372e214ac30936

I stuck to just doing a rename here. Some of these sites could be migrated to using promiseApzFlushedRepaints() but it would require going through each test to figure out if changing it will invalidate the scenario being tested. Since I don't want to spend the time to do that I figured a simple rename would be the safer approach. Certainly if a test using promiseOnlyApzControllerFlushed is exhibiting intermittent failures, then it would be worth examining changing it to a promiseApzFlushedRepaints as it might help resolve the intermittent failure.

This was a mechanical search-and-replace operation, plus adding some docs on
renamed function.

Attachment #9210620 - Attachment description: Bug 1699601 - Rename promiseApzRepaintsFlushed to promiseApzControllerFlushed. r?botond → Bug 1699601 - Rename promiseApzRepaintsFlushed to promiseOnlyApzControllerFlushed. r?botond
Pushed by kgupta@mozilla.staktrace.com:
https://hg.mozilla.org/integration/autoland/rev/ed5c38a59629
Rename promiseApzRepaintsFlushed to promiseOnlyApzControllerFlushed. r=botond
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
You need to log in before you can comment on or make changes to this bug.