Closed Bug 1399968 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Settings UI, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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
(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)
(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 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+
(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!
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
https://hg.mozilla.org/mozilla-central/rev/956bc408307c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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)
(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)
Correct, that's bug 1425199.
Flags: needinfo?(paolo.mozmail)
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?
(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.

Attachment

General

Created:
Updated:
Size: