Closed Bug 1331888 Opened 8 years ago Closed 8 years ago

Remove all undo notifications immediately after a user makes a choice and do not re-show them while undo is ongoing

Categories

(Firefox :: Migration, defect)

Product:
Firefox
Component:
Migration
Version:
51 Branch
Platform:
Unspecified
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1331888 - immediately remove (and don't reshow) notification bars once user chooses to undo,
59 bytes, text/x-review-board-request
dao
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
lizzard
: approval-mozilla-release+
Details 
+++ This bug was initially created as a clone of Bug #1331800 +++

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

see video below:

https://jwilliams-softvision.tinytake.com/sf/MTI3MzY4M180Nzc2OTEy

As you can see it takes about 1 minute for my tabs, browser history, etc. to be automigrated over from Chrome, which freezes my browser. Once it is automigrated, Opening about:newtab does produce the dont keep option. On Win 7 and 10 the dont keep button works as intended but on win 8.1 it only deletes the bookmarks. The browser history and history tiles stay. Opening about:newtab again produces the same prompt "We automatically imported your data from Google Chrome. Would you like to keep it?". This prompt shows over and over on win 8.1.


-----

We should not reshow the prompt after the user has made a choice on that run of Firefox.
Comment on attachment 8827849 [details]
Bug 1331888 - immediately remove (and don't reshow) notification bars once user chooses to undo,

Ugh, if I hardcode the skip animation in AutoMigrate.jsm the test stops working, so clearly waiting for the notification to be removed isn't working correctly because of the anon content. :-(
Attachment #8827849 - Flags: review?(dao+bmo)
(In reply to :Gijs from comment #2)
> Comment on attachment 8827849 [details]
> Bug 1331888 - immediately remove (and don't reshow) notification bars once
> user chooses to undo,
> 
> Ugh, if I hardcode the skip animation in AutoMigrate.jsm the test stops
> working, so clearly waiting for the notification to be removed isn't working
> correctly because of the anon content. :-(

Ah, but it wasn't because of that - it was because I was trying to use notification.parentNode after the notification was fully removed, which doesn't work because it gets set to null. Fixed in the upcoming revision.
Comment on attachment 8827849 [details]
Bug 1331888 - immediately remove (and don't reshow) notification bars once user chooses to undo,

https://reviewboard.mozilla.org/r/105446/#review106264

::: browser/components/migration/AutoMigrate.jsm:205
(Diff revision 2)
>        histogram.add(5);
>        throw new Error("Can't undo!");
>      }
>  
> +    this._pendingUndoTasks = true;
> +    this._removeNotificationBars();

Why are you calling _removeNotificationBars here? Isn't that already called via removeUndoOption further down?

::: browser/components/migration/AutoMigrate.jsm:237
(Diff revision 2)
>      let browserWindows = Services.wm.getEnumerator("navigator:browser");
>      while (browserWindows.hasMoreElements()) {
>        let win = browserWindows.getNext();
>        if (!win.closed) {
>          for (let browser of win.gBrowser.browsers) {
>            let nb = win.gBrowser.getNotificationBox(browser);

Ugh, why are we using browser-specific notification boxes for this rather than high-priority-global-notificationbox or global-notificationbox?
(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8827849 [details]
> Bug 1331888 - immediately remove (and don't reshow) notification bars once
> user chooses to undo,
> 
> https://reviewboard.mozilla.org/r/105446/#review106264
> 
> ::: browser/components/migration/AutoMigrate.jsm:205
> (Diff revision 2)
> >        histogram.add(5);
> >        throw new Error("Can't undo!");
> >      }
> >  
> > +    this._pendingUndoTasks = true;
> > +    this._removeNotificationBars();
> 
> Why are you calling _removeNotificationBars here? Isn't that already called
> via removeUndoOption further down?

Yes, but the point of this bug is that if the undo takes a while (like because there's a lot of history), we can reshow (or keep showing, if they had been opened on multiple tabs) the notification. So I'm removing them immediately.

> 
> ::: browser/components/migration/AutoMigrate.jsm:237
> (Diff revision 2)
> >      let browserWindows = Services.wm.getEnumerator("navigator:browser");
> >      while (browserWindows.hasMoreElements()) {
> >        let win = browserWindows.getNext();
> >        if (!win.closed) {
> >          for (let browser of win.gBrowser.browsers) {
> >            let nb = win.gBrowser.getNotificationBox(browser);
> 
> Ugh, why are we using browser-specific notification boxes for this rather
> than high-priority-global-notificationbox or global-notificationbox?

That was the request for the original funnelcakes / beta experiments, to show the notification on about:home but not elsewhere, and then for this funnelcake to extend that to about:newtab.
(In reply to :Gijs from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > Comment on attachment 8827849 [details]
> > Bug 1331888 - immediately remove (and don't reshow) notification bars once
> > user chooses to undo,
> > 
> > https://reviewboard.mozilla.org/r/105446/#review106264
> > 
> > ::: browser/components/migration/AutoMigrate.jsm:205
> > (Diff revision 2)
> > >        histogram.add(5);
> > >        throw new Error("Can't undo!");
> > >      }
> > >  
> > > +    this._pendingUndoTasks = true;
> > > +    this._removeNotificationBars();
> > 
> > Why are you calling _removeNotificationBars here? Isn't that already called
> > via removeUndoOption further down?
> 
> Yes, but the point of this bug is that if the undo takes a while (like
> because there's a lot of history), we can reshow (or keep showing, if they
> had been opened on multiple tabs) the notification. So I'm removing them
> immediately.

And, to be clear, the undo option can be removed in a number of different ways, so we do still need to remove them in `removeUndoOption`, too.
(In reply to :Gijs from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > Why are you calling _removeNotificationBars here? Isn't that already called
> > via removeUndoOption further down?
> 
> Yes, but the point of this bug is that if the undo takes a while (like
> because there's a lot of history), we can reshow (or keep showing, if they
> had been opened on multiple tabs) the notification. So I'm removing them
> immediately.

Ah, I missed that the 'undo' method is async. Then my question becomes, why call _removeNotificationBars twice rather than only in the beginning? Can you remove it from removeUndoOption, and also rename the latter to something more adequate?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #7)
> And, to be clear, the undo option can be removed in a number of different
> ways, so we do still need to remove them in `removeUndoOption`, too.

I see two other call sites. These could explicitly call _removeNotificationBars as well.
(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to :Gijs from comment #7)
> > And, to be clear, the undo option can be removed in a number of different
> > ways, so we do still need to remove them in `removeUndoOption`, too.
> 
> I see two other call sites. These could explicitly call
> _removeNotificationBars as well.

Yeah, there used to be more. I guess this works, though we'll need to remember to do the same thing for any future callers, which seems a bit meh. Anyway, done this as per your request.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8827849 [details]
Bug 1331888 - immediately remove (and don't reshow) notification bars once user chooses to undo,

https://reviewboard.mozilla.org/r/105446/#review106294
Attachment #8827849 - Flags: review?(dao+bmo) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/276202b20683
immediately remove (and don't reshow) notification bars once user chooses to undo, r=dao
Just a not Gijs, I do not think that the undo is taking a lot of time because of the size of the history. I used the same google chrome profile on win 7, 8.1, & 10. Win 7 & 10 did it in milliseconds. Win 8.1 did not do it at all.
note*
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Justin [:JW_SoftvisionQA] from comment #15)
> Just a not Gijs, I do not think that the undo is taking a lot of time
> because of the size of the history. I used the same google chrome profile on
> win 7, 8.1, & 10. Win 7 & 10 did it in milliseconds. Win 8.1 did not do it
> at all.

That's surprising. There isn't really OS-specific code involved here, so I don't know why the behaviour would be different on Windows 8. Let's look at this in bug 1331800.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8827849 [details]
Bug 1331888 - immediately remove (and don't reshow) notification bars once user chooses to undo,

Approval Request Comment
[Feature/Bug causing the regression]: funnelcake 99 for automigration
[User impact if declined]: if removing imported data (clicking "Don't keep" on the notification bar) takes a long time or breaks halfway through, users could get prompted multiple times. This is a very confusing user experience.
[Is this code covered by automated tests?]: yes, this patch includes a test.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Using the machine with which we're reproducing the issue in bug 1331800 we should be able to verify that after clicking "Don't keep", the notification bar does not then show up again on new tabs.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky, and why?]: not for 51, as the only code changes are to Automigration 'undo' code, which will only run on the funnelcake. Relatively low risk for the funnelcake given that the patch has tests and the issue at hand can't really get a lot worse...
[String changes made/needed]: no
Attachment #8827849 - Flags: approval-mozilla-release?
Attachment #8827849 - Flags: approval-mozilla-beta?
Attachment #8827849 - Flags: approval-mozilla-aurora?
Comment on attachment 8827849 [details]
Bug 1331888 - immediately remove (and don't reshow) notification bars once user chooses to undo,

Should only affect onboarding funnelcake, let's uplift this for the RC2 build today.
Attachment #8827849 - Flags: approval-mozilla-release?
Attachment #8827849 - Flags: approval-mozilla-release+
Attachment #8827849 - Flags: approval-mozilla-beta?
Attachment #8827849 - Flags: approval-mozilla-beta+
Attachment #8827849 - Flags: approval-mozilla-aurora?
Attachment #8827849 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/276202b20683
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Let's try to verify this on our test machines. If that doesn't work, we'll reach out to Justin for a spot-check (per Comment 18).
Flags: qe-verify+
Justin, can you please verify this bug across branches, since it only affects funnelcake? Thanks.
Flags: needinfo?(jwilliams)
I can verify that this bug has been fixed across all branches.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jwilliams)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: