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

for #10568 Make Search Fragment xml include toolbar and AwesomeBar #11744

Merged
merged 1 commit into from
Jun 21, 2020

Conversation

sraturi
Copy link
Contributor

@sraturi sraturi commented Jun 18, 2020

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • 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.

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

@sraturi sraturi marked this pull request as draft June 18, 2020 18:51
@sraturi
Copy link
Contributor Author

sraturi commented Jun 18, 2020

Run 1 is a fresh install. Run 2 is stopping the application and restarting in 10 seconds.

PROFILES
Run 1 with changes: https://share.firefox.dev/37E8Tb2
Run 2 with changes https://share.firefox.dev/30VHpMw

Run 1 Master: https://share.firefox.dev/3ehngV9
Run 2 Master: https://share.firefox.dev/3fBihij

STATS
EDIT: All numbers are in MS, Ran each of the runs 3 times to get average.

  Run 1 (Master) Run 1 (This Branch) Run 2 (Master) Run 2 (This Branch)
Inflating search frag 30, 42,43 = 38.3333 120,142,101= 121 26,27,28 = 27 88,76,88 = 84
Init awesome bar 12, 14,13 = 13 1, 1,1=1 12,14,16=14 1, 1,1 = 1
Init toolbar 69, 84,85 = 79.33 0 0,1= .333 44, 68,59 = 57 0,0,0
total(of the avg) 130.666 122.333 98 85
         

TLDR: improvement

Fresh install inflation time = 130.666 (Master) - 122.333(Changes) = inflation time reduce by 8.336 MS

Second Run inflation time = 98 (Master) - 85(Changes) = inflation time reduced by 13 MS

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2020

Codecov Report

Merging #11744 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11744      +/-   ##
============================================
- Coverage     21.81%   21.80%   -0.01%     
  Complexity      716      716              
============================================
  Files           375      375              
  Lines         15048    15063      +15     
  Branches       1952     1956       +4     
============================================
+ Hits           3283     3285       +2     
- Misses        11482    11495      +13     
  Partials        283      283              
Impacted Files Coverage Δ Complexity Δ
...in/java/org/mozilla/fenix/search/SearchFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../mozilla/fenix/search/awesomebar/AwesomeBarView.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...va/org/mozilla/fenix/search/toolbar/ToolbarView.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...pp/src/main/java/org/mozilla/fenix/HomeActivity.kt 8.67% <0.00%> (+<0.01%) 10.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82cbafa...1570f14. Read the comment docs.

@sblatz sblatz self-assigned this Jun 18, 2020
@sraturi sraturi marked this pull request as ready for review June 18, 2020 19:25
@sraturi sraturi force-pushed the #10568 branch 2 times, most recently from 70d79b9 to 9fa0289 Compare June 18, 2020 20:18
@@ -82,14 +79,9 @@ interface AwesomeBarInteractor {
*/
class AwesomeBarView(
private val container: ViewGroup,
val interactor: AwesomeBarInteractor
) : LayoutContainer {
val view: BrowserAwesomeBar = LayoutInflater.from(container.context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, we don't need this any more because we have this logic here

Copy link
Contributor

@sblatz sblatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in regular tabs & custom tabs, didn't seem to have any issues. Code looks good 😄

Copy link
Contributor

@MarcLeclair MarcLeclair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested too and looks good on my end too

@sraturi sraturi merged commit 988513a into mozilla-mobile:master Jun 21, 2020
@liuche liuche mentioned this pull request Jun 27, 2020
12 tasks
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.

Make Search Fragment include AppBarLayout and AwesomeBar in the same XML
4 participants