Closed Bug 1167096 Opened 5 years ago Closed 5 years ago

Pocket Icon reappears in the toolbar after removal on a clean profile (or at least one with no saved state)

Categories

(Firefox :: Toolbars and Customization, defect, P2)

x86
Windows
defect
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
firefox38.0.5 + fixed
firefox39 --- verified
firefox40 --- verified
firefox41 --- verified

People

(Reporter: gcp, Assigned: Gijs)

References

Details

Attachments

(1 file, 1 obsolete file)

STR:

1) Launch fresh Firefox installation, fresh profile.
2) Customize->Remove Pocket icon from toolbar.
3) Set up sync, change some preferences (I don't sync prefs), potentially get a Firefox update. <- Not sure which parts here are critical.
4) Relaunch Firefox

Expected behavior:
Customization stays, Pocket is gone.

Actual behavior:
Pocket reappears.
Observed with recently Nightlies, once installing in Windows 7, once in Windows 8.1
Reproduced again, without step 3. Should be trivial to verify now.
Blocks: 1164302
Attached file MozReview Request: bz://1167096/Gijs (obsolete) —
/r/9183 - Bug 1167096 - flip introductory prefs if there's no saved state, r?jaws

Pull down this commit:

hg pull -r 74f7a3070718c4a018bb43f69cfcd73968d26ccb https://reviewboard-hg.mozilla.org/gecko/
Attachment #8608686 - Flags: review?(jaws)
I don't understand why QA didn't see this.

Also, ffffffffffff.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
Points: --- → 2
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
(the good news being, if you remove it *a second time* it'll stay removed, even on current code)
Summary: Pocket Icon reappears in the toolbar after removal. → Pocket Icon reappears in the toolbar after removal on a clean profile (or at least one with no saved state)
Comment on attachment 8608686 [details]
MozReview Request: bz://1167096/Gijs

Approval Request Comment
[Feature/regressing bug #]: new button introduction code / pocket feature
[User impact if declined]: new profiles will have trouble removing their pocket button
[Describe test coverage new/current, TreeHerder]: no :-(
[Risks and why]: medium-low. Change is specific to the "no saved state" (ie new profile / deleted prefs.js) case, but we all know that we're doing an RC build and that that's not a great time to land pretty much anything...
[String/UUID change made/needed]: nope
Attachment #8608686 - Flags: approval-mozilla-release?
Attachment #8608686 - Flags: approval-mozilla-beta?
Attachment #8608686 - Flags: approval-mozilla-aurora?
(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 8608686 [details]
> MozReview Request: bz://1167096/Gijs
> 
> Approval Request Comment
> [Feature/regressing bug #]: new button introduction code / pocket feature
> [User impact if declined]: new profiles will have trouble removing their
> pocket button
It is a sensitive feature. So, we should be careful with the user perception of this bug.


> [Describe test coverage new/current, TreeHerder]: no :-(
It is not the first time that we have a bug like this. We should have testing for these cases.


> [Risks and why]: medium-low. Change is specific to the "no saved state" (ie
> new profile / deleted prefs.js) case, but we all know that we're doing an RC
> build and that that's not a great time to land pretty much anything...
What is the worst case scenario here?
(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > Comment on attachment 8608686 [details]
> > MozReview Request: bz://1167096/Gijs
> > 
> > Approval Request Comment
> > [Feature/regressing bug #]: new button introduction code / pocket feature
> > [User impact if declined]: new profiles will have trouble removing their
> > pocket button
> It is a sensitive feature. So, we should be careful with the user perception
> of this bug.
> 
> 
> > [Describe test coverage new/current, TreeHerder]: no :-(
> It is not the first time that we have a bug like this. We should have
> testing for these cases.

There is testing for this feature in general (automatically inserting buttons) but it is not really easily possible to test all the different cases of profile use (or lack thereof) in an automated test, especially not when they require restarts, which is why the test did not catch this. The tests are already pretty hacky, I don't think it's worth adding another hack to simulate a new profile or a restart on something that clearly isn't a new profile.

> > [Risks and why]: medium-low. Change is specific to the "no saved state" (ie
> > new profile / deleted prefs.js) case, but we all know that we're doing an RC
> > build and that that's not a great time to land pretty much anything...
> What is the worst case scenario here?

I don't really know how to answer this question. I ran a build on a clean profile with this change (obviously) and the pocket button still shows up. The change only affects what happens when you start the browser with a clean profile (which then itself influences what happens if you start it again later and the profile is no longer "clean").

I expect that the worst case scenario is that the patch doesn't fix the bug or that the button disappears automatically on (second/third) startup. Those are easily testable, though, and I did, and didn't see this happening. Of course, it's possible my logic is wrong and/or my testing was deficient...

Does this help?
Flags: needinfo?(sledru)
Comment on attachment 8608686 [details]
MozReview Request: bz://1167096/Gijs

It does.
I think it is break in an obvious way, we will see it quickly.

Anyway, I am taking it because:
* pocket is a sensitive project and I would be sad to see on HN a thread "Firefox prevents you to remove the Pocket icon"
* I trust your judgment & skills
* Jaws reviewed it
Flags: needinfo?(sledru)
Attachment #8608686 - Flags: approval-mozilla-release?
Attachment #8608686 - Flags: approval-mozilla-release+
Attachment #8608686 - Flags: approval-mozilla-beta?
Attachment #8608686 - Flags: approval-mozilla-beta+
Attachment #8608686 - Flags: approval-mozilla-aurora?
Attachment #8608686 - Flags: approval-mozilla-aurora+
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/f5a31d211e41
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Reproduced the initial issue using Firefox 38.0.5 beta 1 build 2 on Windows 7 64-bit, verified that the issue is fixed using Firefox 39 beta 1 build 2 on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.
Removing qe-verify+ since verification on Firefox 39 Beta should suffice.
Flags: qe-verify+
Attachment #8608686 - Attachment is obsolete: true
Attachment #8620344 - Flags: review+
I have reproduced this bug with Firefox Nightly 41.0a1 (Build ID: 20150521030204) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 43.0a1(Build ID: 20150908030203)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0

Verified as fixed with Firefox beta 41.0b8 (Build ID: 20150907144446)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0

Verified as fixed with Firefox aurora 42.0a2 (Build ID: 20150908004020)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0

[testday-20150904]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.