Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #8795: remove redundant ConstraintLayout around BrowserToolbar. #12014

Merged
merged 1 commit into from
Jun 27, 2020

Conversation

mcomella
Copy link
Contributor

@mcomella mcomella commented Jun 26, 2020

This is functionally equivalent to the code before this patch but should
be slightly more performant in theory because ConstraintLayout is
expensive to inflate.

The elevation and layoutParams set dynamically appeared to have no effect
with the wrapping view but broke the view when used by itself so I had
to remove them. I also updated a few other unnecessary params.

Theoretically this may have some perf benefits but I didn't see anything
outside noise levels after I took the numbers (but I didn't try very hard).

These were the perf results:


NO CHANGES

first click
5 runs, average: 470.4
[432, 493, 504, 484, 439]

subsequent
10 runs, average: 346.7
[352, 390, 362, 337, 334, 344, 305, 315, 398, 330]


REMOVE TOOLBAR_WRAPPER

first click
5 runs, average: 496.6
[490, 461, 443, 573, 516]

subsequent
10 runs, average: 344
[312, 352, 375, 342, 322, 380, 337, 333, 346, 341]

I suspect the first click is higher before because of the 573 outlier rather than a true regression.


Before:
device-2020-06-26-093810

After:
device-2020-06-26-093706

Scroll before:
rm-toolbar-wrapper-master-scroll

Scroll after:
rm-toolbar-wrapper-patch-scroll

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
    • UI change
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.
    • 😬 I'll try to test if I have time today

After merge

  • Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

…wserToolbar.

This is functionally equivalent to the code before this patch but should
be slightly more performant in theory because ConstraintLayout is
expensive to inflate.

The elevation and layoutParams set dynamically appeared to have no effect
with the wrapping view but broke the view when used by itself so I had
to remove them. I also updated a few other unnecessary params.

Theoretically this may have some perf benefits but I didn't see anything
outside noise levels after I took the numbers (but I didn't try very
hard).
@gabrielluong gabrielluong merged commit 5ef6b49 into mozilla-mobile:master Jun 27, 2020
@liuche liuche mentioned this pull request Jun 27, 2020
12 tasks
@mcomella mcomella deleted the 8795-url-bar branch June 29, 2020 23:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants