1859583 - [Easy Setup Feature] - Clean up test cases and unused logic

Status: RESOLVED FIXED

(Firefox :: Messaging System, task, P1)

Description

[Negin Sauermann]

With 1855031, easy setup screens will use targeting to render screens. Remove/refactor any used or redundant test cases (currently browser_aboutwelcome_multistage_mr.js and browser_aboutwelcome_screen_targeting.js test similar logic) as well as remove respective calls from AboutWelcomeParent and getAWContent

Comment 1

[Negin Sauermann]

Attachment: Bug 1859583 - [Easy Setup Feature] - Clean up test cases

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bda9015bace
[Easy Setup Feature] - Clean up test cases and unused logic r=omc-reviewers,pdahiya

Comment 3

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0bda9015bace

Comment 4

[Noemi Erli[:noemi_erli]]

Backed out changeset 0bda9015bace (Bug 1859583) as requested in Bug 1864194 CLOSED TREE
Link: https://hg.mozilla.org/integration/autoland/rev/c86be1bd3b8dac7ea84d676ccbbf25ffd5d09a77

Comment 5

[Negin Sauermann]

I can see a regression in the load time on try with the reverted and base patch most evident when looking at welcome loadtime opt cold fission webrender:
original patch - 144.5ms
completely reverted try run - 70.1ms

Culprit seems to be the changes to removing ShellServices calls as this try run shows reduction in ms back to before values

Doing one more try run to confirm this with the ShellService calls reverted.

I'm not sure what the best way to proceed is since we no longer require those calls? NI'ing Ed/Punam for feedback.

Comment 6

[Shane Hughes [:aminomancer]]

It may have something to do with removing the lazy resource loading. You could try just statically importing ShellService at the top of AboutWelcomeParent (disabling the eslint warning for unused variables). It is going to get imported anyway by ASRouterTargeting, and is needed for the screen targeting expressions, but higher up in the stack. If the performance is not the same as the reverted try run, you could rule that out as a possibility.

Comment 7

[Negin Sauermann]

(In reply to Shane Hughes [:aminomancer] from comment #6)

It may have something to do with removing the lazy resource loading. You could try just statically importing ShellService at the top of AboutWelcomeParent (disabling the eslint warning for unused variables). It is going to get imported anyway by ASRouterTargeting, and is needed for the screen targeting expressions, but higher up in the stack. If the performance is not the same as the reverted try run, you could rule that out as a possibility.

Oh that's smart, good call! Let me give that a shot. Thanks Shane

Comment 8

[Negin Sauermann]

Hm, no difference in ms when statically importing ShellService.
https://treeherder.mozilla.org/jobs?repo=try&author=nsauermann%40mozilla.com&selectedTaskRun=YboFCim9Sx2WRsM2QzCY1w.0

Comment 9

[Shane Hughes [:aminomancer]]

Maybe ShellService does some kind of value caching. The other point of interest in your patch is the removal of calls to ShellService for needPin/needDefault, these values are later used in the screen targeting expressions. I guess it's conceivable that the ESM calls early in startup previously caused ShellService to cache something, so it didn't need to be recomputed during the page load test when screen targeting was evaluated (saving ~75ms). So, you could try making dummy ShellService calls somewhere in AboutWelcomeParent, as you did with the static import.

Comment 10

[Punam Dahiya [:pdahiya]]

Culprit seems to be the changes to removing ShellServices calls as this try run shows reduction in ms back to before values

As discussed , let's split the cleanup of test cases and shell service removal in their own respective patches to unblock this bug. Shell Service call removal can be done with fix of bug 1864194 proposing new baseline for load time and better highlight how ShellService API possible I/O interaction with an OS impacts welcome loadtime raptor test

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efe09552a4d3
[Easy Setup Feature] - Clean up test cases r=omc-reviewers,pdahiya

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/efe09552a4d3