browser.search.widget.inNavBar isn't set correctly on startup

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Preferences
P1
normal
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
Bug 1387416 changed the browser.search.widget.inNavBar to default to false, and it also attempted to change it to be set correctly on start up according to its state in CustomizableUI.

However, looking into bug 1393437 comment 24, I've found that the pref is always set to false on startup, regardless of if the search box is in toolbar or not.
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 2

7 months ago
(In reply to Mark Banner (:standard8) from comment #0)
> Bug 1387416 changed the browser.search.widget.inNavBar to default to false,
> and it also attempted to change it to be set correctly on start up according
> to its state in CustomizableUI.
> 
> However, looking into bug 1393437 comment 24, I've found that the pref is
> always set to false on startup, regardless of if the search box is in
> toolbar or not.

I'm confused, bug 1393437 comment 24 looks like it would happen if the pref was set to true, not false... Can you clarify how this patch addresses things? Do we somehow init the tracker before we've loaded user prefs or something? I don't really understand why the prefs would mismatch...
Flags: needinfo?(standard8)
(Assignee)

Comment 3

7 months ago
(In reply to :Gijs from comment #2)
> (In reply to Mark Banner (:standard8) from comment #0)
> > Bug 1387416 changed the browser.search.widget.inNavBar to default to false,
> > and it also attempted to change it to be set correctly on start up according
> > to its state in CustomizableUI.
> > 
> > However, looking into bug 1393437 comment 24, I've found that the pref is
> > always set to false on startup, regardless of if the search box is in
> > toolbar or not.
> 
> I'm confused, bug 1393437 comment 24 looks like it would happen if the pref
> was set to true, not false... Can you clarify how this patch addresses
> things? Do we somehow init the tracker before we've loaded user prefs or
> something? I don't really understand why the prefs would mismatch...

At startup SearchWidgetTracker.init() is called before the toolbar elements have been initialised, so when it calls SearchWidgetTracker.syncPreferenceWithWidget() -> widgetIsInNavBar() then 'placement' comes back as null, so the preference is always set to false on startup.

Later on, toolbar.xml kicks in and initialises the toolbar, and then widgetIsInNavBar will return the right value. Hence the move to waiting for the onAreaNodeRegistered event.

Bug 1393437 comment 24 has the search box in the toolbar, so the pref value should be "true", but due to the above startup sequence, it actually gets set to "false". The preferences display correctly displays the incorrectly set value.
Flags: needinfo?(standard8)

Comment 4

7 months ago
mozreview-review
Comment on attachment 8908316 [details]
Bug 1399968 - Ensure the inNavBar search pref really does get set correctly on startup.

https://reviewboard.mozilla.org/r/179944/#review185396
Attachment #8908316 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 5

7 months ago
(In reply to Mark Banner (:standard8) from comment #3)
> At startup SearchWidgetTracker.init() is called before the toolbar elements
> have been initialised, so when it calls
> SearchWidgetTracker.syncPreferenceWithWidget() -> widgetIsInNavBar() then
> 'placement' comes back as null, so the preference is always set to false on
> startup.
> 
> Later on, toolbar.xml kicks in and initialises the toolbar, and then
> widgetIsInNavBar will return the right value. Hence the move to waiting for
> the onAreaNodeRegistered event.
> 
> Bug 1393437 comment 24 has the search box in the toolbar, so the pref value
> should be "true", but due to the above startup sequence, it actually gets
> set to "false". The preferences display correctly displays the incorrectly
> set value.

This makes sense, thanks for the explanations!

Comment 6

7 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/956bc408307c
Ensure the inNavBar search pref really does get set correctly on startup. r=Gijs

Comment 7

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/956bc408307c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Comment 8

3 months ago
I think the bug is not fixed yet. I use the "general.config.filename" to provide users with a centralized configuration. Here I have defined the value: pref("browser.search.widget.inNavBar", true); --> However, this value is reset to false at startup.

I also tested: lockPref("browser.search.widget.inNavBar", true); --> The value remains correctly in the configuration, but it is not effective. The search window is not displayed.

Test environment: Firefox Quantum 58.0 (64-Bit)

Comment 9

3 months ago
(In reply to Susi from comment #8)
> I think the bug is not fixed yet. I use the "general.config.filename" to
> provide users with a centralized configuration. Here I have defined the
> value: pref("browser.search.widget.inNavBar", true); --> However, this value
> is reset to false at startup.
> 
> I also tested: lockPref("browser.search.widget.inNavBar", true); --> The
> value remains correctly in the configuration, but it is not effective. The
> search window is not displayed.
> 
> Test environment: Firefox Quantum 58.0 (64-Bit)

I don't think locking the pref or generally using autoconfig with it is a supported usecase. Paolo, can you clarify?
Flags: needinfo?(paolo.mozmail)

Comment 10

3 months ago
Correct, that's bug 1425199.
Flags: needinfo?(paolo.mozmail)

Comment 11

3 months ago
Thank you for your prompt reply! Do I understand the article correctly, that the "Policy Engine" is only supported from Firefox 60 ESR? Not with the current 58.0.x - non ESR version?

Comment 12

3 months ago
(In reply to Susi from comment #11)
> the "Policy Engine" is only supported from Firefox 60 ESR?

Yes, and it's still in development (Firefox Nightly is currently version 60). The comment in bug 1425199 says that when working on the policy engine we *could* look into having a setting for this. It's up to the team working on the policy engine whether they add such a setting. Felipe, is this on your radar/roadmap?

> Not with the current 58.0.x - non ESR version?

Correct.
Flags: needinfo?(felipc)
(In reply to :Gijs from comment #12)
> (In reply to Susi from comment #11)
> > the "Policy Engine" is only supported from Firefox 60 ESR?
> 
> Yes, and it's still in development (Firefox Nightly is currently version
> 60). The comment in bug 1425199 says that when working on the policy engine
> we *could* look into having a setting for this. It's up to the team working
> on the policy engine whether they add such a setting. Felipe, is this on
> your radar/roadmap?

This request hadn't come up to us before, so it's not on our list of policies to support in 60. But it is a reasonable request. I suggest filing a bug in Firefox :: Enterprise Policies. I don't think we can work on this on the timeline for 60, but I'd happily accept a patch for it.
Flags: needinfo?(felipc)
You need to log in before you can comment on or make changes to this bug.