Closed Bug 1443572 Opened 6 years ago Closed 2 years ago

Disable onboarding when using Marionette

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: davehunt, Unassigned)

Details

User Story

Please read the following document in how to get started:

https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html

Attachments

(3 files)

bug 1443572
59 bytes, text/x-review-board-request
Details
Bug 1443572 - Disable Photon onboarding experience for Marionette and geckodriver
59 bytes, text/x-review-board-request
whimboo
: review-
Details
firefox 60 newtab onboarding extension
121.37 KB, image/png
Details 
The browser onboarding can be disabled by setting the 'browser.onboarding.enabled' pref to false. This can be added to the RECOMMENDED_PREFS map in testing/marionette/components/marionette.js.
Mentor: dave.hunt
Keywords: good-first-bug
:davehunt May I take this up? Thanks!
Hello,
Is this bug open for solving?
> Is this bug open for solving?

Yes! If you're interested in working on it I can assign it to you. Have you contributed to mozilla-central previously? If not, you may find https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction a useful place to get started.
No, This will be my first time and I would like to work on this bug.
Thank you
Assignee: nobody → akshitadvlp
Status: NEW → ASSIGNED
I have assigned the bug to you. Please comment here if you need any assistance.
Mentor: dave.hunt → hskupin
Thank you for assigning the issue.
Hello,
I had started with the issue and understood the changes I need to make to the repo.
But I got stuck with part of how to implement the changes with the Mozilla-central and also studied the page directed to me but :(
It would be great if I could get a starting on how to proceed.

Thank you and sorry for the convience :)
akshitadvlp, I added a link to our documenation to the user story section. Please read it to know how to get started.
User Story: (updated)
Otherwise you can also reach out on the #introduction or #ateam IRC channels.
akshitadvlp, please see my last comment and the URL I added in the user story section of this bug. When you open it you will find all details under `Work on bugs and get code review`. Just follow the links and read on.
Hello Henrik,
I am following the documentation, thank you for linking that.
I just thought to get confirmation about the changes before pushing them now I will upload it through the review part.
Thank you and sorry for the inconvenience :)
We do reviews through our review tools, just to make sure to track remaining issues correctly. So thanks for uploading now.
Hello,
I have made all the changes as mentioned but when pushing to review through ssh. It is giving me an error.
remote: Permission denied (public key).
abort: no suitable response from remote hg!
Can I get a little suggestion on how to move forward?
Thank you :)
Can you please tell me which commands you used to push the changes? Make sure that you run `hg push review`. I assume you missed to specify `review` which indeed will not work. To solve problems like that it is better to reach us on IRC so that we can clarify this directly.
Hello,
I have submitted a review :)
Comment on attachment 8961996 [details]
bug 1443572

https://reviewboard.mozilla.org/r/230844/#review236332


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Invalid syntax [py3: is-parseable]

::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Invalid syntax [py2: is-parseable]

::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Syntaxerror: invalid syntax [flake8: E999]
Comment on attachment 8961996 [details]
bug 1443572

https://reviewboard.mozilla.org/r/230844/#review236334


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Invalid syntax [py3: is-parseable]

::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Invalid syntax [py2: is-parseable]

::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Syntaxerror: invalid syntax [flake8: E999]
Comment on attachment 8961996 [details]
bug 1443572

https://reviewboard.mozilla.org/r/230844/#review236336


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Invalid syntax [py3: is-parseable]

::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Invalid syntax [py2: is-parseable]

::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Syntaxerror: invalid syntax [flake8: E999]
Comment on attachment 8961996 [details]
bug 1443572

https://reviewboard.mozilla.org/r/230844/#review236338


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Invalid syntax [py3: is-parseable]

::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Invalid syntax [py2: is-parseable]

::: testing/marionette/client/marionette_driver/geckoinstance.py:369
(Diff revision 1)
>          "browser.snippets.enabled": False,
>          "browser.snippets.syncPromo.enabled": False,
>          "browser.snippets.firstrunHomepage.enabled": False,
>  
> +        # (bug 1443572)
> +        "browser.onboarding.enabled", False,

Error: Syntaxerror: invalid syntax [flake8: E999]
akshitadvlp, please check the review comments from the code review bot which you want to fix. Also make sure that you run the Marionette unit tests locally to make sure that the code is working. Right now you will see a couple of exceptions.

Once it has been done and you updated the mozreview patch, make also sure to flag me as reviewer of the patch inside of mozreview. Thanks.
Thank you the guidance. I will implement the changes and revert you asap.
Flags: needinfo?(akshitadvlp)
Priority: -- → P3
The system I had made the setup for Mozilla-central crashed and I have been waiting for it to get repaired to complete the bug.
Flags: needinfo?(akshitadvlp)
I wonder if you will have the time to finish your patch. If not please let us know. Thanks.
Flags: needinfo?(akshitadvlp)
Assignee: akshitadvlp → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(akshitadvlp)
As @whimboo suggested, I'll start working on this bug as soon as I can.
Comment on attachment 8983511 [details]
Bug 1443572 - Disable Photon onboarding experience for Marionette and geckodriver

https://reviewboard.mozilla.org/r/249362/#review255538

I did not know what tests to run, so I was about to run the unit tests, but I needed a build and it was taking a lot of time, so I pushed this to review, so that the try server can find all the errors.

::: testing/geckodriver/src/prefs.rs:47
(Diff revision 1)
>  
>          // Never start the browser in offline mode
>          ("browser.offline", Pref::new(false)),
>  
> +        // Disable browser onboarding
> +        // (bug 1443572)

I am doubtful that this comment is needed, but I saw a similar comment on line 28 and kept it.
Comment on attachment 8983511 [details]
Bug 1443572 - Disable Photon onboarding experience for Marionette and geckodriver

https://reviewboard.mozilla.org/r/249364/#review255718

::: commit-message-89d79:1
(Diff revision 1)
> +Bug 1443572 - Disabled browser onboarding when using Marionette r?whimboo

Please adjust so it looks like:

`Bug 1443572 - Disable Photon onboarding experience for Marionette and geckodriver.`

::: testing/geckodriver/src/prefs.rs:11
(Diff revision 1)
>  // channel builds of Firefox.  Removing a preference from this file
>  // will cause regressions, so please be careful and get review from
>  // a Testing :: Marionette peer before you make any changes to this file.
>  
>  lazy_static! {
>      pub static ref DEFAULT: [(&'static str, Pref); 80] = [

You added a pref so the length of the list also needs to be bumped up.

Please note that I landed huge changes to that file yesterday, which means you have to rebase your bookmark against central, after pulling from it.

Currently you wont be able to compile geckodriver. Try it out yourself by changing into `testing/geckodriver` and call `cargo build`.

::: testing/geckodriver/src/prefs.rs:47
(Diff revision 1)
>  
>          // Never start the browser in offline mode
>          ("browser.offline", Pref::new(false)),
>  
> +        // Disable browser onboarding
> +        // (bug 1443572)

Lets name it "Disable Photon onboarding experience". Also there is no need to add the bug number here because there is nothing left to do.

::: testing/marionette/client/marionette_driver/geckoinstance.py:381
(Diff revision 1)
>      fennec_prefs = {
>          # Enable output of dump()
>          "browser.dom.window.dump.enabled": True,
>  
> +        # Disable browser onboarding
> +        # (bug 1443572)

Photon is Firefox only. So we don't need it for the Fennec instance.

::: testing/marionette/client/marionette_driver/geckoinstance.py:523
(Diff revision 1)
>          # Do not show the EULA notification which can interfer with tests
>          "browser.EULA.override": True,
>  
> +        # Disable browser onboarding
> +        # (bug 1443572)
> +        "browser.onboarding.enabled": False,

Same as for my comment for prefs.rs. Please update.

::: testing/marionette/components/marionette.js:97
(Diff revision 1)
>    // as it is picked up at runtime.
>    ["browser.offline", false],
>  
> +  // Disable browser onboarding
> +  // (bug 1443572)
> +  ["browser.onboarding.enabled", false],

Same as above.
Attachment #8983511 - Flags: review?(hskupin) → review-
As I mentioned on IRC, I do not see "browser.onboarding.enabled" as false, which I have set in the files. I believe I checked what bookmark I was on, and also verified that the files had it set as false. @whimboo mentioned about using unpatched Marionette, but I do not understand how to do that.
Flags: needinfo?(hskupin)
Comment on attachment 8983511 [details]
Bug 1443572 - Disable Photon onboarding experience for Marionette and geckodriver

https://reviewboard.mozilla.org/r/249364/#review256468

::: testing/marionette/client/marionette_driver/geckoinstance.py:518
(Diff revision 2)
>  
>          # Do not show the EULA notification which can interfer with tests
>          "browser.EULA.override": True,
>  
> +        # Disable Photon onboarding experience
> +        "broswer.onboarding.enabled": False,

This should be `browser`.
Attachment #8983511 - Flags: review?(hskupin) → review-
(In reply to Aditya Khadse from comment #32)
> As I mentioned on IRC, I do not see "browser.onboarding.enabled" as false,
> which I have set in the files. I believe I checked what bookmark I was on,
> and also verified that the files had it set as false. @whimboo mentioned
> about using unpatched Marionette, but I do not understand how to do that.

As noted in my review there is a spelling mistake which causes the appropriate pref not to appear as false when testing with Marionette. Anyway, in your case you should always see the blue box at the top-left corner when opening a new tab.
Assignee: nobody → akk5597
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
Aditya, are you going to finish off this patch? Please let us know. Thanks.
Flags: needinfo?(akk5597)
I'm extremely sorry but I have been busy with my college and was unable to continue working on this. I'll need some time to get back to this and will try to upload a patch within a few hours. Sorry for the inconvenience.
Flags: needinfo?(akk5597)
Comment on attachment 8983511 [details]
Bug 1443572 - Disable Photon onboarding experience for Marionette and geckodriver

https://reviewboard.mozilla.org/r/249364/#review269400

::: testing/geckodriver/src/prefs.rs:11
(Diff revision 3)
>  // channel builds of Firefox.  Removing a preference from this file
>  // will cause regressions, so please be careful and get review from
>  // a Testing :: Marionette peer before you make any changes to this file.
>  
>  lazy_static! {
> -    pub static ref DEFAULT: [(&'static str, Pref); 52] = [
> +    pub static ref DEFAULT: [(&'static str, Pref); 53] = [

This change cannot be applied anymore because your local checkout of mozilla-central is too old. Please run a pull -u on central, and fix this merge conflict.

::: testing/marionette/client/marionette_driver/geckoinstance.py:518
(Diff revision 3)
>  
>          # Do not show the EULA notification which can interfer with tests
>          "browser.EULA.override": True,
>  
> +        # Disable Photon onboarding experience
> +        "broswer.onboarding.enabled": False,

As mentioned already last time there is a spelling error. That's why onboarding is not getting disabled.
Attachment #8983511 - Flags: review?(hskupin) → review-
It looks like that in recent versions of Nightly the notification isn't shown anymore. Instead there is an indication with a small blue dot on the Nightly logo of `about:newtab`. Anyway, I think it's still worth fixing it.
(In reply to Henrik Skupin (:whimboo) from comment #39)
> It looks like that in recent versions of Nightly the notification isn't
> shown anymore. Instead there is an indication with a small blue dot on the
> Nightly logo of `about:newtab`. Anyway, I think it's still worth fixing it.

Then it should be added with a comment saying when it can be removed.
(In reply to Andreas Tolfsen 「:ato」 from comment #40)
> Then it should be added with a comment saying when it can be removed.

No, we should keep it until it completely got removed. Also as for any guided tour this can change again over time.
Assignee: akk5597 → nobody
Status: ASSIGNED → NEW

The browser.onboarding.enabled pref doesn't exist anymore. We should either update this bug to reflect whatever current prefs need changing (if any) or close this out if there's nothing left to do.

Flags: needinfo?(hskupin)

Thanks for letting us know Ryan. I had a look and found bug 1462415 which removed the onboarding system addon in favor of a more simplified version in Firefox 64. But I'm not sure if there are any preferences that we may have to disable for the new onboarding experience.

Gijs, is there anything you would have in mind? If not we probably should close this old bug. Thanks.

Flags: needinfo?(hskupin) → needinfo?(gijskruitbosch+bugs)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #43)

Thanks for letting us know Ryan. I had a look and found bug 1462415 which removed the onboarding system addon in favor of a more simplified version in Firefox 64. But I'm not sure if there are any preferences that we may have to disable for the new onboarding experience.

Gijs, is there anything you would have in mind? If not we probably should close this old bug. Thanks.

I don't work on onboarding, 302 Mardak.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(edilee)

I had to grab an old build to remind myself what this was. Did it need to be disabled to avoid changing the interface? Looks like it decides to show a message on about:newtab potentially covering the search box or other UI.

There is a browser.aboutwelcome.enabled pref but toggling that will result in it opening about:newtab instead of about:welcome.

New user onboarding now is only shown for a brand new profile and doesn't change other UIs like about:newtab, so I don't think a new pref needs to be set (as has been the case for the past 40+ versions ?), so the outdated pref can be removed.

Flags: needinfo?(edilee)

Thanks Ed! So basically there was the request to get this preference added but we actually never did. At the end we weren't that much affected at least in CI because new tabs are getting opened with about:blank by default. Only users who are modifying the pref which controls if the new tab page should be opened might have seen it.

I think that the only remaining question here would be if there is some other pref we would like to have to set? Which affect das browser.aboutwelcome.enabled have when the new tab page is enabled/disabled? Also what does the welcome page do? For us it would be important because in most of the cases Marionette/geckodriver will use a fresh Firefox profile for the tests. And we don't want to have some overlays covering parts of the page as shown in the currently selected tab.

Flags: needinfo?(edilee)

about:welcome disabled via the pref ends up loading the contents of about:home/newtab. In either case, the content loaded stays within the tab without overlaying outside of its page.

Flags: needinfo?(edilee)

Ok, so it sounds like as long as we do not open about:welcome we won't be affected with any onboarding experience, and as such the following preferences that we set should be enough:

https://searchfox.org/mozilla-central/rev/cc98a15c7327d742d283cddddde712a8a3165006/remote/shared/RecommendedPreferences.jsm#264-267

Closing as invalid given that there is nothing to fix.

Mentor: hskupin
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: good-first-bug
Resolution: --- → INVALID
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: