Closed Bug 1398656 Opened 7 years ago Closed 7 years ago

Always build the form autofill system add-on but disable it by default on release

Categories

(Toolkit :: Form Manager, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
thunderbird_esr52 --- unaffected
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + verified
firefox57 --- fixed

People

(Reporter: lchang, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 file, 1 obsolete file)

The form autofill system add-on isn't currently built in release build. We should enable it once we get pre-release sign-off.
Assignee: nobody → lchang
[Tracking Requested - why for this release]: We will use Shield pref-flipping instead of balrog for the rollout and this makes that possible.
Assignee: lchang → MattN+bmo
Status: NEW → ASSIGNED
Priority: -- → P1
Summary: Enable form autofill system add-on in release build → Always build the form autofill system add-on but disable it by default on release
I still have to test the preprocessing
Comment on attachment 8907876 [details]
Bug 1398656 - Always build the form autofill system add-on but disable it by default on release.

https://reviewboard.mozilla.org/r/179556/#review184772

Thanks for taking this.
Attachment #8907876 - Flags: review?(lchang) → review+
Attachment #8906932 - Attachment is obsolete: true
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e5eab26b9de
Always build the form autofill system add-on but disable it by default on release. r=lchang
Comment on attachment 8907876 [details]
Bug 1398656 - Always build the form autofill system add-on but disable it by default on release.

Approval Request Comment
[Feature/Bug causing the regression]:
Feature Shipment.

[User impact if declined]:
Form Autofill won't be built in release build.

[Is this code covered by automated tests?]:
Unnecessary as it's build configuration.

[Has the fix been verified in Nightly?]:
The change is for release channel only.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It just builds the system add-on in release channel as it is in beta channel.

[String changes made/needed]:
N/A
Attachment #8907876 - Flags: approval-mozilla-beta?
Track 56+ as this feature is disabled by default.
https://hg.mozilla.org/mozilla-central/rev/5e5eab26b9de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8907876 [details]
Bug 1398656 - Always build the form autofill system add-on but disable it by default on release.

Given m-b is 57 and m-r is 56 now, I move approval‑mozilla‑beta to approval‑mozilla‑release. Autofill will be disabled dy default. This should be merged to m-r.
Attachment #8907876 - Flags: approval-mozilla-release+
Attachment #8907876 - Flags: approval-mozilla-beta?
Attachment #8907876 - Flags: approval-mozilla-beta-
Using Fx 56.0 RC-build 1, the prefcode "extensions.formautofill.available" has the value "staged-rollout", is this intended?
Flags: needinfo?(lchang)
Hi Gabi,

Yes, any value other than "on" and "detect" makes formautofill disable. We do intend to disable it by default and will turn it on gradually by Shield.
Flags: needinfo?(lchang)
Verified on Firefox 56.0 that autofill is disabled by default, under Windows 10x64, Ubuntu 16.04x86 and under macOS 10.12.
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #4)
> I still have to test the preprocessing

I guess it didn't get tested before landing…

(In reply to Gabi Cheta from comment #12)
> Using Fx 56.0 RC-build 1, the prefcode "extensions.formautofill.available"
> has the value "staged-rollout", is this intended?

No, it's only supposed to be staged-rollout if the channel is "release".

Let me see if there's an easy fix then we can decide what to do here…
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #15)
> (In reply to Gabi Cheta from comment #12)
> > Using Fx 56.0 RC-build 1, the prefcode "extensions.formautofill.available"
> > has the value "staged-rollout", is this intended?
> 
> No, it's only supposed to be staged-rollout if the channel is "release".
> Let me see if there's an easy fix then we can decide what to do here…

Hi Matt, I thought RC-build is using channel "release".
Flags: needinfo?(MattN+bmo)
Don't think so… and I tested the official beta build and it's also set to "staged-rollout".
I see. You mean the problem is that we falsely disable it on beta build, too.
So it looks like you may be right that the RC builds are built with the update channel of "release" but then the channel is switched back to "beta" afterwards through some magic. Not sure if this is documented anywhere…

(In reply to Luke Chang [:lchang] from comment #18)
> You mean the problem is that we falsely disable it on beta build, too.

Well I was more worried about release not getting the right value but yes, the other concern was that beta users lost autofill. I think that's actually fine so that the RC is testing what users will get before our staged rollout but I intentionally used an update channel check instead of an early beta check to avoid it getting disabled for beta users. I didn't realize there was some magic to build with one channel but ship with another.

In summary, I think everything is fine here but for the next beta cycle we should try to avoid this situation by doing the gradual rollout also for RC builds.
Flags: needinfo?(MattN+bmo)
Yeah, I just noticed the same thing that my beta build gets RC-build after updating and I didn't know that would happen before. I'll pay more attention once we are going to gradual rollout the credit card's part. However, I don't know what you meant about doing gradual rollout also for RC build. Does that mean using gradual rollout mechanism but enable 100% users directly on RC builds?

BTW, for address' part, I think it should be set back to "detect" in 57-release by default, right?
You need to log in before you can comment on or make changes to this bug.