Update default values for urlbar searches in Marionette

RESOLVED FIXED in Firefox 58

Status

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: whimboo, Assigned: skeletor, Mentored)

Tracking

Version 3
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [lang=py])

User Story

To get started with Marionette please read:
https://wiki.mozilla.org/User:Mjzffr/New_Contributors

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
(In reply to Marco Bonardo [::mak] from bug 1303346 comment #34)
> we enabled search suggestions by default. I suspect that's related.
> You want to set browser.urlbar.userMadeSearchSuggestionsChoice to true and
> browser.urlbar.suggest.searches to false.

It needs an update for:
* https://dxr.mozilla.org/mozilla-central/source/testing/marionette/server.js (recommended prefs)

* https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/geckoinstance.py (for Firefox)

Raajit, are you interested to work on this bug?
Flags: needinfo?(raajit.raj)
Yes I would love to. 
Can I get more information? Its not clear whats going on here.

Thanks
Flags: needinfo?(raajit.raj) → needinfo?(hskupin)
(Reporter)

Comment 2

a year ago
It's just that you would have to add those two preferences with their changed default values to the referenced places. More details can be found at bug 1303346 comment #34 if you are really interested in. Or better would be if Marco can give us references to those changes.
Flags: needinfo?(hskupin) → needinfo?(mak77)
(In reply to Henrik Skupin (:whimboo) from comment #2)
> Or better would
> be if Marco can give us references to those changes.

The bug implementing the new suggestions hint was bug 1344924, while bug 1344928 switched the default.
But both the prefs existed before, userMadeSearchSuggestionsChoice just makes us not show the hint, while suggest.searches is the pref controlling whether search.suggestions are enabled in the urlbar.
Flags: needinfo?(mak77)
Comment hidden (mozreview-request)

Updated

a year ago
Attachment #8872435 - Flags: review?(hskupin)
(Reporter)

Comment 5

a year ago
mozreview-review
Comment on attachment 8872435 [details]
Bug 1368034 - Disable the search suggestion for urlbar in Marionette

https://reviewboard.mozilla.org/r/143932/#review147776

Looks fine beside the one filed issue. Please have a look at it. Also we should improve the commit message, so it explains `what` in the summary and the `why` in the details.

::: testing/marionette/client/marionette_driver/geckoinstance.py:477
(Diff revision 1)
>  
>          # Disable the UI tour
>          "browser.uitour.enabled": False,
>  
> +        "browser.urlbar.userMadeSearchSuggestionsChoice": True,
> +        "browser.urlbar.suggest.searches": False,

Please have a look at the bug Marco referenced. It should help you to understand what will be changed by modifying the prefs. We would need a short comment above the blocks in both files.
Attachment #8872435 - Flags: review?(hskupin) → review+

Comment 6

a year ago
mozreview-review
Comment on attachment 8872435 [details]
Bug 1368034 - Disable the search suggestion for urlbar in Marionette

https://reviewboard.mozilla.org/r/143932/#review147784

::: testing/marionette/server.js:159
(Diff revision 1)
> +  ["browser.urlbar.userMadeSearchSuggestionsChoice", true],
> +  ["browser.urlbar.suggest.searches", false],

Please document.

Comment 7

a year ago
mozreview-review
Comment on attachment 8872435 [details]
Bug 1368034 - Disable the search suggestion for urlbar in Marionette

https://reviewboard.mozilla.org/r/143932/#review147788

::: testing/marionette/server.js:159
(Diff revision 1)
> +  ["browser.urlbar.userMadeSearchSuggestionsChoice", true],
> +  ["browser.urlbar.suggest.searches", false],

These also need to be added to testing/geckodriver/src/prefs.rs.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

a year ago
Attachment #8874187 - Flags: review?(hskupin)
(Reporter)

Comment 10

a year ago
Raajit, thank you for the update. But can you please make sure to no longer create new mozreview requests, but simply update the existing one? Thanks.
Assignee: nobody → raajit.raj
Status: NEW → ASSIGNED
(Reporter)

Comment 11

a year ago
So I have just seen that the second patch only includes the changes for geckodriver. Please make sure to provide that as part of your original review request.
(Reporter)

Comment 12

a year ago
mozreview-review
Comment on attachment 8874187 [details]
Bug 1368034 - Update default values for urlbar searches in Marionette

https://reviewboard.mozilla.org/r/145612/#review150264
Attachment #8874187 - Flags: review?(hskupin) → review-
Yeah I have no idea what went wrong there. I just updated the patch locally then pushed it and this happened. I was thinking to discard and submit a new patch but you told me not to do so before.
(Reporter)

Comment 14

a year ago
mozreview-review
Comment on attachment 8872435 [details]
Bug 1368034 - Disable the search suggestion for urlbar in Marionette

https://reviewboard.mozilla.org/r/143932/#review150268

I'm going to r- the patch until we have a final working solution. It will make it easier to spot, when the patch is ready to land.

::: commit-message-34ac1:1
(Diff revisions 1 - 2)
> -Bug 1368034 Update default values for urlbar searches in Marionette
> +Bug 1368034 - Disable the search suggestion for urlbar in Marionette

The commit message is still missing a more details description of why we have to do it. I referenced the bugs which changed the behavior of Firefox earlier in this bug. So please get an understanding of it, and add another paragraph.

For an example see:
https://hg.mozilla.org/mozilla-central/rev/ff3c813fcdea

::: testing/marionette/server.js
(Diff revisions 1 - 2)
>    ["browser.uitour.enabled", false],
>  
> +  // Disable the URL bar search suggestions
>    ["browser.urlbar.userMadeSearchSuggestionsChoice", true],
>    ["browser.urlbar.suggest.searches", false],
> -

Please do not remove this empty line.

::: testing/geckodriver/src/prefs.rs:111
(Diff revision 2)
>          // Disable the UI tour
>          ("browser.uitour.enabled", Pref::new(false)),
>  
> +        // Disable the URL bar search suggestions
> +        ("browser.urlbar.userMadeSearchSuggestionsChoice", Pref::new(true)),
> +        "browser.urlbar.suggest.searches", Pref::new(false),

There are missing braces.
Attachment #8872435 - Flags: review+ → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 17

a year ago
Raajit, the mozreview is still messed-up. Can you please contact me on IRC first before pushing another update? It has to get fixed first before I can do another review.
(Reporter)

Comment 18

a year ago
mozreview-review
Comment on attachment 8872435 [details]
Bug 1368034 - Disable the search suggestion for urlbar in Marionette

https://reviewboard.mozilla.org/r/143932/#review150944
Attachment #8872435 - Flags: review?(hskupin)
(Reporter)

Comment 19

a year ago
mozreview-review
Comment on attachment 8874187 [details]
Bug 1368034 - Update default values for urlbar searches in Marionette

https://reviewboard.mozilla.org/r/145612/#review150948
Attachment #8874187 - Flags: review?(hskupin)
Comment hidden (mozreview-request)
I dont think it worked.
What should I do now?
(Reporter)

Comment 22

a year ago
mozreview-review
Comment on attachment 8874187 [details]
Bug 1368034 - Update default values for urlbar searches in Marionette

https://reviewboard.mozilla.org/r/145612/#review151728

We didn't finish the discussion about your problem. So I didn't expect that you push once more an update to mozreview. Please lets get everything sorted out first.

::: testing/geckodriver/src/prefs.rs:111
(Diff revision 3)
>          // Disable the UI tour
>          ("browser.uitour.enabled", Pref::new(false)),
>  
>          // Disable the URL bar search suggestions
>          ("browser.urlbar.userMadeSearchSuggestionsChoice", Pref::new(true)),
> -        "browser.urlbar.suggest.searches", Pref::new(false),
> +        ("browser.urlbar.suggest.searches", Pref::new(false),)

This line is broken cause the comma is on the wrong position.
Attachment #8874187 - Flags: review?(hskupin)
(Reporter)

Comment 23

a year ago
Raajit, would you have the time so that we can finish off this bug? I would appreciate that.
(In reply to Henrik Skupin (:whimboo) from comment #22)
> We didn't finish the discussion about your problem. So I didn't expect that
> you push once more an update to mozreview. Please lets get everything sorted
> out first.

I will surely get this done once we rectify all the mistakes here.
Hi Henrik, 
Really sorry for the delay. Can you please help me finish this bug?
Flags: needinfo?(hskupin)
(Reporter)

Comment 26

a year ago
Raajit, I would propose that you get in contact to me on IRC and we walk through all again.
Flags: needinfo?(hskupin)
Priority: -- → P3
(Reporter)

Comment 27

a year ago
No response from Raajit over the last 2 month. Putting it back into the pool for someone else to finish it up.
Assignee: raajit.raj → nobody
Status: ASSIGNED → NEW
Comment hidden (mozreview-request)
(Reporter)

Comment 29

a year ago
mozreview-review
Comment on attachment 8911721 [details]
Bug 1368034 - Update default values for urlbar searches in Marionette

https://reviewboard.mozilla.org/r/183128/#review188326

Code-wise it looks fine but comments and the commits need an update.

::: testing/geckodriver/src/prefs.rs:112
(Diff revision 1)
>  
>          // Do not warn on quitting Firefox
>          ("browser.warnOnQuit", Pref::new(false)),
>  
> +        //Disable search suggestion hints, but keep search suggestions enabled
> +        ("browser.urlbar.userMadeSearchSuggestionsChoice", Pref::new(true)),

Please use the description from here:
https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#361

::: testing/geckodriver/src/prefs.rs:113
(Diff revision 1)
>          // Do not warn on quitting Firefox
>          ("browser.warnOnQuit", Pref::new(false)),
>  
> +        //Disable search suggestion hints, but keep search suggestions enabled
> +        ("browser.urlbar.userMadeSearchSuggestionsChoice", Pref::new(true)),
> +        ("browser.urlbar.suggest.searches", Pref::new(false)),

Please use the description from here:
https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#357

::: testing/marionette/client/marionette_driver/geckoinstance.py:478
(Diff revision 1)
>          "browser.tabs.warnOnOpen": False,
>  
>          # Disable the UI tour
>          "browser.uitour.enabled": False,
>  
> +        # Disable search suggestion hints, but keep search suggestions enabled

Please update with the above mentioned descriptions.

::: testing/marionette/server.js:165
(Diff revision 1)
>    // Disable the UI tour.
>    //
>    // Should be set in profile.
>    ["browser.uitour.enabled", false],
>  
> +  // Disable search suggestion hints, but keep search suggestions enabled

Please update with the above mentioned descriptions.
Attachment #8911721 - Flags: review?(hskupin) → review+
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8874187 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8872435 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Assignee: nobody → rubberdonkeysandwich
Status: NEW → ASSIGNED
(Reporter)

Comment 31

a year ago
mozreview-review
Comment on attachment 8911721 [details]
Bug 1368034 - Update default values for urlbar searches in Marionette

https://reviewboard.mozilla.org/r/183128/#review188334

::: commit-message-7e962:3
(Diff revisions 1 - 2)
>  Bug 1368034 - Update default values for urlbar searches in Marionette r=whimboo
>  
> -Search suggestions and an onboarding notification bar were added to Firefox. This patch updates the default settings in Marionette to enable the search suggestions but disable the onboarding notifications.
> +Search suggestions create unneccessary network requests and the suggestions opt-in notification interferes with tests that don't expect it to be there. So, this patch updates the default settings in Marionette to disable both.

Please limit the line length to 78 chars by doing manual wrapping.

::: testing/geckodriver/src/prefs.rs:117
(Diff revisions 1 - 2)
> +        // tests that don't expect it to be there.
>          ("browser.urlbar.userMadeSearchSuggestionsChoice", Pref::new(true)),
> +
> +        // Turn off search suggestions in the location bar so as not to trigger
> +        // network connections.
>          ("browser.urlbar.suggest.searches", Pref::new(false)),

nit: put this pref before the other one to keep alphabetical order. Please do that in all cases.

Also I don't see that the number of list items has been increased. Geckodriver should still not build.
Attachment #8911721 - Flags: review+ → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 33

a year ago
mozreview-review
Comment on attachment 8911721 [details]
Bug 1368034 - Update default values for urlbar searches in Marionette

https://reviewboard.mozilla.org/r/183128/#review188702

The latest changes look fine to me. I pushed a try build to ensure it works all fine.
Attachment #8911721 - Flags: review?(hskupin) → review+

Comment 34

a year ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/72ccf148b3e6
Update default values for urlbar searches in Marionette r=whimboo

Comment 35

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/72ccf148b3e6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.