Closed
Bug 1361286
Opened 8 years ago
Closed 8 years ago
support in-page messages for Automigration module
Categories
(Firefox :: New Tab Page, enhancement, P1)
Firefox
New Tab Page
Tracking
()
VERIFIED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: gasolin, Assigned: gasolin)
References
Details
(Whiteboard: [photon-onboarding])
Attachments
(3 files, 3 obsolete files)
Based on UI spec in Bug 1360109, We need add 3 new message:
* message to receive auto imported(to show the message in page),
* message to trigger the undo action(and might need receive `undo is done` message),
* message to trigger the manual import dialog
Assignee | ||
Comment 1•8 years ago
|
||
Hi gijs,
Targeting on 57 for Photon onboarding project, UX just provided the new UI spec around the automigration UI on the new tab. Based on that spec, there are several changes:
* Instead of using doorhanger, new spec show the imported message inside of the new tab page
* Instead of using doorhanger, new spec show `undo` button inside of the new tab page
* Instead of using doorhanger, new spec provide a `manual import` button at the bottom of the new tab page.
(And we won't touch about:home page since UX plan to replace about:home with new tab.)
(All above changes should be able to pref off.)
Based on the UI spec, we need to have more communication way between automigration and new-tab.
It will be nice if you can give us some feedback if add new messages in automigrate.jsm is the right way to go, and some implement hints is more than welcome.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 2•8 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #1)
> Hi gijs,
>
> Targeting on 57 for Photon onboarding project, UX just provided the new UI
> spec around the automigration UI on the new tab. Based on that spec, there
> are several changes:
Which spec? Is it this one: https://mozilla.invisionapp.com/share/Y3BGDY88H#/screens/230329192 or not?
Generally, please provide links like this when asking for needinfo based on a spec (or other external document that isn't already on the bug). Now I have to go look for it on other bugs, and even when I find something I don't really know if it's the right thing.
> * Instead of using doorhanger, new spec show the imported message inside of
> the new tab page
By "doorhanger", do you mean "notification bar" ? Generally, "doorhanger" means a panel with an arrow pointing to an anchor in the toolbar.
> * Instead of using doorhanger, new spec show `undo` button inside of the new
> tab page
By "undo button" do you mean the "Don't keep" link? Or are we looking at different specs?
> * Instead of using doorhanger, new spec provide a `manual import` button at
> the bottom of the new tab page.
>
> (And we won't touch about:home page since UX plan to replace about:home with
> new tab.)
OK, but we should probably remove the old UI and strings, as a separate patch, including any relevant code in about:home land.
> (All above changes should be able to pref off.)
There is an extant pref for this that you should reuse.
> Based on the UI spec, we need to have more communication way between
> automigration and new-tab.
>
> It will be nice if you can give us some feedback if add new messages in
> automigrate.jsm is the right way to go, and some implement hints is more
> than welcome.
I think you should add APIs to AboutNewTab.jsm, and keep communication with the actual page in there. AutoMigrate.jsm shouldn't have to deal with the about:newtab page (html / channel stuff) directly. This will also make it easier to port to activity stream later. AboutNewTab.jsm can call AutoMigrate.jsm for you, which already has all the methods you need to actually do things (keep/don't keep), except for showing the dialog for which you will want to use the relevant method on MigrationUtils.jsm (lazily imported). For displaying the message, there should be code in AboutNewTab.jsm and the about:newtab code itself that is activated via the API in AboutNewTab that AutoMigrate.jsm can call into.
Does that help?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gasolin)
Assignee | ||
Comment 3•8 years ago
|
||
> Which spec? Is it this one:
> https://mozilla.invisionapp.com/share/Y3BGDY88H#/screens/230329192 or not?
Thank you for quick response, yes it's the right spec. Sorry for inconvinient, I'll do f? on the spec next time.
> > * Instead of using doorhanger, new spec show the imported message inside of
> > the new tab page
>
> By "doorhanger", do you mean "notification bar" ? Generally, "doorhanger"
> means a panel with an arrow pointing to an anchor in the toolbar.
>
Yes, I mean the notification bar.
> By "undo button" do you mean the "Don't keep" link? Or are we looking at
> different specs?
Yes its "Don't keep" link which will trigger the undo action to reverse the autoimport.
> > * Instead of using doorhanger, new spec provide a `manual import` button at
> > the bottom of the new tab page.
> >
> > (And we won't touch about:home page since UX plan to replace about:home with
> > new tab.)
>
> OK, but we should probably remove the old UI and strings, as a separate
> patch, including any relevant code in about:home land.
We could do this once we did replace the whole about:home.
> > (All above changes should be able to pref off.)
>
> There is an extant pref for this that you should reuse.
> > Based on the UI spec, we need to have more communication way between
> > automigration and new-tab.
> >
> > It will be nice if you can give us some feedback if add new messages in
> > automigrate.jsm is the right way to go, and some implement hints is more
> > than welcome.
>
> I think you should add APIs to AboutNewTab.jsm, and keep communication with
> the actual page in there. AutoMigrate.jsm shouldn't have to deal with the
> about:newtab page (html / channel stuff) directly. This will also make it
> easier to port to activity stream later. AboutNewTab.jsm can call
> AutoMigrate.jsm for you, which already has all the methods you need to
> actually do things (keep/don't keep), except for showing the dialog for
> which you will want to use the relevant method on MigrationUtils.jsm (lazily
> imported). For displaying the message, there should be code in
> AboutNewTab.jsm and the about:newtab code itself that is activated via the
> API in AboutNewTab that AutoMigrate.jsm can call into.
>
> Does that help?
Thanks that's helpful!
According to the above comment I'll move this bug from the `Migration` to the `New Tab` module.
Component: Migration → New Tab Page
Flags: needinfo?(gasolin)
Comment 4•8 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #3)
> > > * Instead of using doorhanger, new spec provide a `manual import` button at
> > > the bottom of the new tab page.
> > >
> > > (And we won't touch about:home page since UX plan to replace about:home with
> > > new tab.)
> >
> > OK, but we should probably remove the old UI and strings, as a separate
> > patch, including any relevant code in about:home land.
>
> We could do this once we did replace the whole about:home.
I think we should remove the existing notification bar entirely. There's nobody who will be interested in having it appear on about:home, so it's dead code (that causes startup slowdown - bug 1360253), so we should remove it.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Priority: -- → P1
Updated•8 years ago
|
Whiteboard: [photon-onboarding]
Updated•8 years ago
|
Flags: qe-verify+
QA Contact: jwilliams
Assignee | ||
Comment 5•8 years ago
|
||
In test it works fine to send/receive message between page.js and AboutNewTab.jsm with RemotePages.
And here's what the sample UI in HTML looks like http://codepen.io/gasolin/pen/aWLVYL
I'd like to land the basic description string and the function buttons in this bug (without tooltip, image, and the detailed style since the UI spec is not ready), and do the detail UI update at the followup bugs.
Comment 6•8 years ago
|
||
(In reply to :Gijs from comment #2)
> (In reply to Fred Lin [:gasolin] from comment #1)
> > (All above changes should be able to pref off.)
>
> There is an extant pref for this that you should reuse.
Just so I'm being clear here... the meeting request I got today suggested using browser.newtabpage.compact. I don't think that's the right pref. I think we should use browser.migrate.automigrate.ui.enabled and browser.migrate.automigrate.enabled, which already exist. The former should gate the UI (and is set to true by default already) and the latter should gate whether we actually automigrate (which gates whether the UI ever gets shown, of course, because we only show it if automigration happened).
Assignee | ||
Comment 7•8 years ago
|
||
per offline discussion with gijs, the UX prefer to pref off all new automigration UI in newtab page so they can pref it on in certain build for testing. Therefore gijs suggest add a new pref instead of reuse existing prefs.
Flags: needinfo?(bmao)
Assignee | ||
Comment 8•8 years ago
|
||
I doubled confirmed with UX (michael & Bryant). They want:
1. able to pref on/off the automigration feature (which is default on in nightly but default off in release)
2. to replace old ui (notification) to the new one
So we can use pref `browser.migrate.automigrate.ui.enabled` and `browser.migrate.automigrate.enabled` as gijs suggested. And we can remove the whole notification here.
Flags: needinfo?(bmao)
Assignee | ||
Comment 9•8 years ago
|
||
The main UX flow is changed in several places based on my prototype and recent offline discussions.
Bryant is working on the new UI spec. Here is a quick summary of the new flow, which affect how we implement the related issues: (item with ? ahead is my suggestion that is not confirmed yet with UX, I'd like write them as a base to double check these flow details)
+ The automigration message is shown only for new user without any Firefox profile in his/her device (main mechanism is implemented)
* In first page
* Show the first page message with `Keep ${browserName} Data` button and 'Don't Keep' link
* highlight tiles background
* When user click 'Don't Keep' link
+ do the actual 'undo' action (as we've tested in the funnel cake)
+ should be switched to second page in all opened newtab (not just this newtab)
+ first page message should be shown when user add another new tab
* hide the augomigrate message after 3 days if user did not interact with that (implemented)
* In second page
+ show the second page message with a single `OK, Got it` button
- ('Go back' link in previous spec is removed)
+ removed tiles background highlight
? show the manual import link at all opened newtab
? change 'Your Top Site' back to 'Top site' at all opened newtab
* When user click `OK, Got it` button
+ hide augomigrate messages in all opened newtab
+ if user try to add another newtab when second page message is shown
? the second page message is still shown at all existing newtab (not deal with them)
* but not show the second page message in the newly added newtab
Bryant, can you help confirm if the above flow meet what we've discussed this week? Then I can create bugs, breakdown tasks, and update the prototype based on the new flow before the new UX spec is ready.
Flags: needinfo?(bmao)
Assignee | ||
Comment 10•8 years ago
|
||
As the discussion result last week, we'll put the automigration message into activity stream
* This patch is the core part for AutoMigration.jsm , which is similar to maybeShowUndoNotification but only returns the browser name when its migrated.
* UX are working on the new UX spec for activity stream, Bryant told me the flow will be very similar to comment 9.
* The activity stream part will be landed to activity stream repo in github
* The newtab part WIP is attached to help the review process, but it won't be landed. (The message is not in the separate l10n file, no test...etc.)
prerequisite:
* preferences in about:config
```
browser.migrate.automigrate.enabled
browser.migrate.automigrate.ui.enabled
browser.newtabpage.compact = true;
browser.newtabpage.columns = 6;
```
* run with new profile
```
./mach run -P
```
For maybeGetMigratedBrowser method, I bypass the `canUndo` function(don't know how to test it, seems it always return false) and test it with following settings:
```
async maybeGetMigratedBrowser(target) {
// if (!(await this.canUndo())) {
// return "";
// }
// The tab might have navigated since we requested the undo state:
let canUndoFromThisPage = ["about:home", "about:newtab"].includes(target.currentURI.spec);
if (!canUndoFromThisPage ||
!Preferences.get(kUndoUIEnabledPref, false)) {
return "";
}
if (!this.shouldStillShowUndoPrompt()) {
this._purgeUndoState(this.UNDO_REMOVED_REASON_OFFER_EXPIRED);
return "";
}
let remainingDays = Preferences.get(kAutoMigrateDaysToOfferUndoPref, 0);
Services.telemetry.getHistogramById("FX_STARTUP_MIGRATION_UNDO_OFFERED").add(4 - remainingDays);
return "Chrome";
},
```
Please kindly provide feedback if this is in the right direction.
Attachment #8868371 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 11•8 years ago
|
||
The WIP patch that should be applied after Bug-1361286.patch
This patch is just for evaluation, not intended for review.
Comment 12•8 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #9)
> The main UX flow is changed in several places based on my prototype and
> recent offline discussions.
>
> Bryant is working on the new UI spec. Here is a quick summary of the new
> flow, which affect how we implement the related issues: (item with ? ahead
> is my suggestion that is not confirmed yet with UX, I'd like write them as a
> base to double check these flow details)
>
> + The automigration message is shown only for new user without any Firefox
> profile in his/her device (main mechanism is implemented)
> * In first page
> * Show the first page message with `Keep ${browserName} Data` button and
> 'Don't Keep' link
After copy review, we will simplify the button string to "Don't Import" and "Import" which means there will be no variable in the button string.
> * highlight tiles background
We are still discussing if we want to keep the background or not, I'll update the discussion once we've reached a consesus
> * When user click 'Don't Keep' link
> + do the actual 'undo' action (as we've tested in the funnel cake)
> + should be switched to second page in all opened newtab (not just this
> newtab)
> + first page message should be shown when user add another new tab
> * hide the augomigrate message after 3 days if user did not interact with
> that (implemented)
> * In second page
> + show the second page message with a single `OK, Got it` button
> - ('Go back' link in previous spec is removed)
We will drop the "Ok, Got it" button as well, only show the message with a link inside the text to call out the import wizard. Also, dismiss the message after 3 active days if the user did not interact with the link.
> + removed tiles background highlight
> ? show the manual import link at all opened newtab
> ? change 'Your Top Site' back to 'Top site' at all opened newtab
We intend to show the string "Top Sites" either user import the message or not, but it's still under discussion.
> * When user click `OK, Got it` button
> + hide augomigrate messages in all opened newtab
> + if user try to add another newtab when second page message is shown
> ? the second page message is still shown at all existing newtab (not
> deal with them)
> * but not show the second page message in the newly added newtab
We've integrated the import link with the second page message, so they will dismiss altogether
>
>
> Bryant, can you help confirm if the above flow meet what we've discussed
> this week? Then I can create bugs, breakdown tasks, and update the prototype
> based on the new flow before the new UX spec is ready.
Flags: needinfo?(bmao)
Assignee | ||
Comment 13•8 years ago
|
||
Here's the main message flow between page.js and aboutNewTab.jsm
Assignee | ||
Comment 14•8 years ago
|
||
Here's the extra message flow (when user doesn't want to automigrate) between page.js and aboutNewTab.jsm
Updated•8 years ago
|
Target Milestone: --- → Firefox 56
Comment 15•8 years ago
|
||
Comment on attachment 8868371 [details] [diff] [review]
Bug-1361286.patch
Review of attachment 8868371 [details] [diff] [review]:
-----------------------------------------------------------------
Please use mozreview for future requests. If that is problematic for some reason, please ensure patches you export use appropriate amounts of context. 3 lines is far too little - use 10.
If canUndo() always returns false, it sounds like automigration is not running. Have you tried using the browser debugger to find out where it fails?
The extra pref is unnecessary - the presence or absence of the migrated browser name pref is sufficient to indicate whether a migration occurred.
It seems in the new strings for the buttons you no longer need the browser name. Do we still need the browser name elsewhere?
I don't see any new 'messages' added by this patch - only additional exposed APIs on AutoMigrate.jsm . If we need a messaging/observer API, then this patch should add one. However, if we need a public API that shares the "after 3 days, remove the possibility of using undo" stuff between the existing notification bar code and the new code, then encapsulating all of that + the canUndo() call into a new method would make sense, but it should then also change the existing code, not just copy/paste the relevant bits.
::: browser/components/migration/AutoMigrate.jsm
@@ +10,4 @@
>
> const kAutoMigrateEnabledPref = "browser.migrate.automigrate.enabled";
> const kUndoUIEnabledPref = "browser.migrate.automigrate.ui.enabled";
> +const kAutoMigrateDonePref = "browser.migrate.automigrate.done";
This pref seems unused. I don't know why you're adding it.
@@ +409,5 @@
> + return "";
> + }
> +
> + let remainingDays = Preferences.get(kAutoMigrateDaysToOfferUndoPref, 0);
> + Services.telemetry.getHistogramById("FX_STARTUP_MIGRATION_UNDO_OFFERED").add(4 - remainingDays);
This is duplicating logic from another method, which I can't quickly identify because you're not using mozreview. Please factor out the shared logic.
@@ +690,5 @@
> QueryInterface: XPCOMUtils.generateQI(
> [Ci.nsIObserver, Ci.nsINavBookmarkObserver, Ci.nsISupportsWeakReference]
> ),
> +
> + undoAutoMigrate(chromeWindow) {
Why are you moving this?
Attachment #8868371 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 16•8 years ago
|
||
Thanks for giving feedback!
> Please use mozreview for future requests.
Sure!
> It seems in the new strings for the buttons you no longer need the browser
> name. Do we still need the browser name elsewhere?
>
No, if UX doesn't change their mind, we can construct and return the message from AutoMigrate.jsm
> I don't see any new 'messages' added by this patch - only additional exposed
> APIs on AutoMigrate.jsm .
New messages is constructed by the newtab page, see the newtab WIP patch.
> ::: browser/components/migration/AutoMigrate.jsm
> @@ +10,4 @@
> >
> > const kAutoMigrateEnabledPref = "browser.migrate.automigrate.enabled";
> > const kUndoUIEnabledPref = "browser.migrate.automigrate.ui.enabled";
> > +const kAutoMigrateDonePref = "browser.migrate.automigrate.done";
>
> This pref seems unused. I don't know why you're adding it.
>
kAutoMigrateDonePref is used to denote Migration:Ended / UNDO-ed. I'll check if it can be replaced by the browser name pref.
>
> @@ +690,5 @@
> > QueryInterface: XPCOMUtils.generateQI(
> > [Ci.nsIObserver, Ci.nsINavBookmarkObserver, Ci.nsISupportsWeakReference]
> > ),
> > +
> > + undoAutoMigrate(chromeWindow) {
>
> Why are you moving this?
Instead of doing within the notification, This method is called by the newtab page to undo the AutoMigration.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8868904 [details]
Bug 1361286 - return message instead of showing notification in Automigration module
I've update the patch and use mozreview push.
Main changes are:
* reuse maybeShowUndoNotification method, but tweak it to return message if new pref `browser.migrate.automigrate.inpage.ui.enabled` is set.
* update locale messages to new string
Other changes: (should not be landed)
* changed the rule to display second message in the newTabPage
* import Log.jsm for debugging
By add logging message, I can confirm `canUndo` method is always returning `false`. And I found browser console shows error "this[module] is undefined" in nsBrowserGlue.js:177 even before I made any changes. That means in this feedback patch I still need put some workaround to make it work.
I'll start trace above mentioned issues today.
Attachment #8868904 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 20•8 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #19)
> And I found browser console shows error "this[module] is undefined"
> in nsBrowserGlue.js:177 even before I made any changes.
This is unrelated. bug 1363155
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8868904 [details]
Bug 1361286 - return message instead of showing notification in Automigration module
https://reviewboard.mozilla.org/r/139592/#review144046
::: browser/components/migration/AutoMigrate.jsm:329
(Diff revision 1)
> * Show the user a notification bar indicating we automatically imported
> * their data and offering them the possibility of removing it.
> * @param target (xul:browser)
> * The browser in which we should show the notification.
> */
> async maybeShowUndoNotification(target) {
I don't think it's a good idea to have a method called "maybeShowUndoNotification" that then doesn't show a notification but returns a string instead.
We should have 1 utility method (maybe called `shouldShowUndoPrompt`) that checks whether we could/should show the notification, that calls canUndo(), that checks the current page in the browser, that (in the non-in-page case) checks for an existing notification, and that calls the existing method `shouldStillShowPrompt` (which should be renamed to 'isUndoPromptExpired()' or something to make it clear that it specifically takes care of the pref changes that expire the prompt).
Then you write 2 smaller methods. One just fetches the string we want (so maybe call it `getUndoMigrationMessage`), if the utility method from the previous para indicates we should show the notification, and null otherwise (make that clear in the method's docstring comment).
The other one can be `maybeShowUndoNotificationBar` and it can call `getUndoMigrationMessage`, check if it has a non-null result, and then create the notification bar if so.
This way, every method's responsibilities are clear and match their function name, and we don't duplicate any code. Your new code in about:newtab code can simply call the `getUndoMigrationMessage` code directly.
The telemetry can go in the `shouldShowUndoPrompt` method.
::: browser/locales/en-US/chrome/browser/migration/migration.properties:76
(Diff revision 1)
>
> 128_firefox=Windows and Tabs
>
> # Automigration undo notification.
> # %1$S will be replaced with the name of the browser we imported from, %2$S will be replaced with brandShortName
> -automigration.undo.message.all = Pick up where you left off. We’ve imported these sites and your bookmarks, history and passwords from %1$S into %2$S.
> +automigration.undo.message2.all = Dive right in. Try %1$S with your top sites, bookmarks, history and passwords from %2$S.
Here and for the other strings, I don't really understand why we list 'top sites' and 'history' separately.
::: browser/locales/en-US/chrome/browser/migration/migration.properties:77
(Diff revision 1)
> 128_firefox=Windows and Tabs
>
> # Automigration undo notification.
> # %1$S will be replaced with the name of the browser we imported from, %2$S will be replaced with brandShortName
> -automigration.undo.message.all = Pick up where you left off. We’ve imported these sites and your bookmarks, history and passwords from %1$S into %2$S.
> -automigration.undo.message.bookmarks = Pick up where you left off. We’ve imported these sites and your bookmarks from %1$S into %2$S.
> +automigration.undo.message2.all = Dive right in. Try %1$S with your top sites, bookmarks, history and passwords from %2$S.
> +automigration.undo.message2.bookmarks = Dive right in. Try %1$S with your top sites, bookmarks from %2$S.
This should have 'and' between 'sites' and 'bookmarks'.
Comment 22•8 years ago
|
||
Comment on attachment 8868904 [details]
Bug 1361286 - return message instead of showing notification in Automigration module
(In reply to Fred Lin [:gasolin] from comment #10)
> * run with new profile
>
> ```
> ./mach run -P
> ```
>
>
I think the reason you're not able to run this is that you're not actually running automigration. Try running:
./mach run -P
and then creating a profile called 'test-automigrate' in the profile manager, then closing the profile manager (don't start anything).
Then run:
./mach run -P test-automigrate --migration
(you might need to use '-migration' with just 1 dash (-) on Windows, I'm not sure)
Attachment #8868904 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 23•8 years ago
|
||
Thanks, I can run `./mach run -P test-automigrate --migration` on Mac, and now `canUndo` method returns true.
Assignee | ||
Comment 24•8 years ago
|
||
> ::: browser/locales/en-US/chrome/browser/migration/migration.properties:76
> (Diff revision 1)
> >
> > 128_firefox=Windows and Tabs
> >
> > # Automigration undo notification.
> > # %1$S will be replaced with the name of the browser we imported from, %2$S will be replaced with brandShortName
> > -automigration.undo.message.all = Pick up where you left off. We’ve imported these sites and your bookmarks, history and passwords from %1$S into %2$S.
> > +automigration.undo.message2.all = Dive right in. Try %1$S with your top sites, bookmarks, history and passwords from %2$S.
>
> Here and for the other strings, I don't really understand why we list 'top
> sites' and 'history' separately.
>
Hi Bryant, here are variants of strings we will show for automigration (based on the source set(bookmarks, history, passwords) we can convert).
http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/migration/migration.properties#76
Since `Top sites` are generated via history by Firefox, could you help figure out "do we need to list 'top sites' and 'history' separately?"
Flags: needinfo?(bmao)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Summary: Introduce new send/receive messages for Automigration → support in-page messages for Automigration module
Assignee | ||
Comment 27•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868904 [details]
Bug 1361286 - return message instead of showing notification in Automigration module
https://reviewboard.mozilla.org/r/139592/#review144046
> I don't think it's a good idea to have a method called "maybeShowUndoNotification" that then doesn't show a notification but returns a string instead.
>
> We should have 1 utility method (maybe called `shouldShowUndoPrompt`) that checks whether we could/should show the notification, that calls canUndo(), that checks the current page in the browser, that (in the non-in-page case) checks for an existing notification, and that calls the existing method `shouldStillShowPrompt` (which should be renamed to 'isUndoPromptExpired()' or something to make it clear that it specifically takes care of the pref changes that expire the prompt).
>
> Then you write 2 smaller methods. One just fetches the string we want (so maybe call it `getUndoMigrationMessage`), if the utility method from the previous para indicates we should show the notification, and null otherwise (make that clear in the method's docstring comment).
>
> The other one can be `maybeShowUndoNotificationBar` and it can call `getUndoMigrationMessage`, check if it has a non-null result, and then create the notification bar if so.
>
> This way, every method's responsibilities are clear and match their function name, and we don't duplicate any code. Your new code in about:newtab code can simply call the `getUndoMigrationMessage` code directly.
>
> The telemetry can go in the `shouldShowUndoPrompt` method.
Since both notification and in-page message should check all of these conditions, I choose `maybeShowUndoMessage` instead of `shouldShowUndoPrompt` and calling `getUndoMigrationMessage` or `showUndoNotificationBar` based on pref.
The remote page message `NewTab:MaybeShowAutoMigrationUndoNotification` are renamed to `NewTab:MaybeShowUndoMessage` accordingly.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8868904 -
Attachment is obsolete: true
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
Here's the fully functional feedback-only patch that
* address mentioned issues
* split `maybeShowAutoMigrationUndoNotification` to 3 methods
* fix the logic to show/hide the manual import message (2nd message)
If the AutoMigration.jsm part looks fine I'll add a patch without changes in newTabPage.jsm and page.js.
Attachment #8868905 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
https://reviewboard.mozilla.org/r/140532/#review144572
::: browser/components/migration/AutoMigrate.jsm:307
(Diff revision 3)
> + // keep browserName to help
> + // if we should show the inpage manual import message
Nit: on one line please.
::: browser/components/migration/AutoMigrate.jsm:319
(Diff revision 3)
> Services.telemetry.getKeyedHistogramById("FX_STARTUP_MIGRATION_UNDO_REASON");
> histogram.add(migrationBrowser, reason);
> },
>
> getBrowserUsedForMigration() {
> - let browserId = Services.prefs.getCharPref(kAutoMigrateBrowserPref);
> + let browserId = Services.prefs.getStringPref(kAutoMigrateBrowserPref);
Why the switch to getStringPref?
::: browser/components/migration/AutoMigrate.jsm:332
(Diff revision 3)
> - * Show the user a notification bar indicating we automatically imported
> - * their data and offering them the possibility of removing it.
> + * Show the user message indicating we automatically imported
> + * their data via notification bar or the in-page message.
> * @param target (xul:browser)
> * The browser in which we should show the notification.
> */
> - async maybeShowUndoNotification(target) {
> + async maybeShowUndoMessage(target) {
But this still doesn't do what it says on the tin... it sometimes shows something, and sometimes it returns a string. That doesn't make sense. That's why I suggested splitting it up, which still hasn't really happened because this code is still both returning strings and maybe showing a notification bar.
::: browser/components/migration/AutoMigrate.jsm:334
(Diff revision 3)
> * @param target (xul:browser)
> * The browser in which we should show the notification.
> */
> - async maybeShowUndoNotification(target) {
> + async maybeShowUndoMessage(target) {
> + let canUndo = await this.canUndo();
> + log.debug(">>> canUndo:", canUndo);
Please remove all the debug logging for the next iteration.
::: browser/modules/AboutNewTab.jsm:49
(Diff revision 3)
> + this.doneAutoMigration.bind(this))
> + },
> +
> + async maybeShowUndoMessage({ target }) {
> + let browserName = Services.prefs.getStringPref("browser.migrate.automigrate.browser", "");
> + if (!AutoMigrate.isUndoPromptExpired() && browserName == "") { // show manual import message
This check doesn't match the other code, where you're keeping the browser name pref.
Comment 31•8 years ago
|
||
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
I'm not picky about the exact method names I'm suggesting, but I really don't want a method that is trying to do two things: (1) return a string that gets used somewhere else and (2) show the old-style notification bar. It makes the code hard to read.
Attachment #8868905 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 32•8 years ago
|
||
mozreview-review |
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
https://reviewboard.mozilla.org/r/140532/#review144976
::: browser/components/migration/AutoMigrate.jsm:332
(Diff revision 3)
> - * Show the user a notification bar indicating we automatically imported
> - * their data and offering them the possibility of removing it.
> + * Show the user message indicating we automatically imported
> + * their data via notification bar or the in-page message.
> * @param target (xul:browser)
> * The browser in which we should show the notification.
> */
> - async maybeShowUndoNotification(target) {
> + async maybeShowUndoMessage(target) {
I'll make this method as a conditional checking function, it will always return a string ("inpage", "notification", or "") and the followup action can be handled in aboutNewPage or aboutHome.
::: browser/modules/AboutNewTab.jsm:49
(Diff revision 3)
> + this.doneAutoMigration.bind(this))
> + },
> +
> + async maybeShowUndoMessage({ target }) {
> + let browserName = Services.prefs.getStringPref("browser.migrate.automigrate.browser", "");
> + if (!AutoMigrate.isUndoPromptExpired() && browserName == "") { // show manual import message
To make this check work, I added then additional check in _purgeUndoState to not clear the browser name pref while work in in-page mode.
Not sure if its the best way, but its flexiable enough. We can set the browser name perf while user complete the manual import, so the message will disapear after user manual import.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8868371 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8868372 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8868905 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 35•8 years ago
|
||
Gif screencast from the feedback PR http://g.recordit.co/DdA6RDBlrF.gif
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
https://reviewboard.mozilla.org/r/140532/#review145044
::: browser/components/migration/AutoMigrate.jsm:706
(Diff revision 5)
> + /**
> + * Keep the automigration result and not prompt anymore
> + */
> + keepAutoMigration() {
> + this._purgeUndoState(this.UNDO_REMOVED_REASON_OFFER_REJECTED);
> + Preferences.set(kAutoMigrateDaysToOfferUndoPref, 0);
Why do we need to do this? We don't do this in the notification bar case. Should that be calling this new method instead?
::: browser/modules/AboutHome.jsm:13
(Diff revision 5)
> -Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> -Components.utils.import("resource://gre/modules/Services.jsm");
> +cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +cu.import("resource://gre/modules/Services.jsm");
This is broken?
::: browser/modules/AboutHome.jsm:153
(Diff revision 5)
> - AutoMigrate.maybeShowUndoNotification(aMessage.target);
> + switch (await AutoMigrate.shouldShowUndoPrompt(aMessage.target)) {
> + case "notification":
This should just use an if statement, and if you make `shouldShowUndoPrompt` return a bool, it should check the UI pref itself. If you think that's ugly, encapsulate the UI pref into AutoMigrate.jsm, or rename the method so that it makes it clear it's going to return a string.
::: browser/modules/AboutNewTab.jsm:39
(Diff revision 5)
> }
> this.pageListener = new RemotePages("about:newtab");
> this.pageListener.addMessageListener("NewTab:Customize", this.customize.bind(this));
> - this.pageListener.addMessageListener("NewTab:MaybeShowAutoMigrationUndoNotification",
> - (msg) => AutoMigrate.maybeShowUndoNotification(msg.target.browser));
> + this.pageListener.addMessageListener("NewTab:MaybeShowUndoMessage",
> + this.maybeShowUndoMessage.bind(this));
> + this.pageListener.addMessageListener("NewTab:undoAutoMigration",
Nit: here and elsewhere, InitialCasing (not camelCase) for the message names, please.
::: browser/modules/AboutNewTab.jsm:49
(Diff revision 5)
> + if (!AutoMigrate.isUndoPromptExpired() && browserName == "") { // show manual import message
> + this.pageListener.sendAsyncMessage("NewTab:migrationIsReverted");
> + } else { // show migrated message
> + this.window = target.browser.ownerGlobal;
> + switch (await AutoMigrate.shouldShowUndoPrompt(target.browser)) {
This calls isUndoPromptExpired() twice - once directly, and once via shouldShowUndoPrompt.
I think this should just call shouldShowUndoPrompt to determine whether or not the prompt should be shown.
`shouldShowUndoPrompt` sounds like it should return a bool. We can check the type of UI that we should be showing here, if indeed `shouldShowUndoPrompt` returns true.
Then you can have a separate check for `browserName` (and the UI pref, which you're not checking right now!) if `shouldShowUndoPrompt` returns false, to see if you need to show the 'would you like to manually import' message.
::: browser/modules/AboutNewTab.jsm:68
(Diff revision 5)
> + }
> + }
> + },
> +
> + undoAutomigration({ target }) {
> + // show the message since we don't need to wait undo is actually done
Nit: ... don't need to wait until undo has finished.
Comment 37•8 years ago
|
||
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
This looks better but I still have quite some comments.
Besides the earlier comment: aren't we supposed to expire the manual import message after a while, too? Where does that happen? It seems like that should be clearing the automigrate pref that stores the browser.
Attachment #8868905 -
Flags: feedback?(gijskruitbosch+bugs) → feedback+
Assignee | ||
Comment 38•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
https://reviewboard.mozilla.org/r/140532/#review145044
Thanks for feedback!
> Why do we need to do this? We don't do this in the notification bar case. Should that be calling this new method instead?
We need that to check if we need to show the manual import message(the 2nd message). That change does not effect the notification bar case.
> This is broken?
just a stop-by change to align with AboutNewTab
> This should just use an if statement, and if you make `shouldShowUndoPrompt` return a bool, it should check the UI pref itself. If you think that's ugly, encapsulate the UI pref into AutoMigrate.jsm, or rename the method so that it makes it clear it's going to return a string.
I'll make it return a bool and do the UI pref check in AboutNewTab/Home
Comment 39•8 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #38)
> > Why do we need to do this? We don't do this in the notification bar case. Should that be calling this new method instead?
>
> We need that to check if we need to show the manual import message(the 2nd
> message). That change does not effect the notification bar case.
But why do we need to change the 'how many days to show this notification' pref? Overloading the pref like that doesn't seem like a good idea. If the new tab page needs to display something specific, it should have its own pref.
> > This is broken?
>
> just a stop-by change to align with AboutNewTab
Sure, but it should be "Cu" not "cu".
Assignee | ||
Comment 40•8 years ago
|
||
That done by reuse the `isUndoPromptExpired` check in AboutNewTab.jsm
The manual import message is only shown when !isUndoPromptExpired() & browserName is exist
Thanks for remind, I forget to reuse `automigrate.ui.enabled`, by set `automigrate.ui.enabled` to false, we don't need to do the hack like `Preferences.set(kAutoMigrateDaysToOfferUndoPref, 0);` to hide the prompt
Comment 41•8 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #40)
> That done by reuse the `isUndoPromptExpired` check in AboutNewTab.jsm
> The manual import message is only shown when !isUndoPromptExpired() &
> browserName is exist
>
> Thanks for remind, I forget to reuse `automigrate.ui.enabled`, by set
> `automigrate.ui.enabled` to false, we don't need to do the hack like
> `Preferences.set(kAutoMigrateDaysToOfferUndoPref, 0);` to hide the prompt
Flipping the UI pref also seems wrong. Why do we need to reuse anything? Why can't we set a separate pref? We only show the manual import message if the user chooses to remove (not keep) the imported data from the new tab page, so the new tab page can take care of everything itself without delegating to the automigrate code.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•8 years ago
|
||
I add `browser.migrate.done` pref to control if the automigrate or wizard actually done all migration work, so we don't have change other prefs to find out if we actually done the migration. That makes the patch looks more like a refactor patch.
The message2 is updated based on revised UX spec https://mozilla.invisionapp.com/share/Y3BGDY88H#/screens (According to UX, the workflow is final, but UI details & l10n are still subject to change)
This patch doesn't set the `done` pref on the manual wizard (haven't trace that part).
Since the manual import link as the 2nd message could be added in bug 1361294, we can revisit the messages and the rest stuff at that time.
Comment 44•8 years ago
|
||
mozreview-review |
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
https://reviewboard.mozilla.org/r/140532/#review145948
::: browser/components/migration/AutoMigrate.jsm:15
(Diff revision 6)
>
> const kAutoMigrateEnabledPref = "browser.migrate.automigrate.enabled";
> const kUndoUIEnabledPref = "browser.migrate.automigrate.ui.enabled";
>
> +const kInPageUIEnabledPref = "browser.migrate.automigrate.inpage.ui.enabled";
> +const kMigrateDonePref = "browser.migrate.done";
I don't understand what the pref means and why we need it. When the user selects 'keep' we remove the backup file from disk and because of that, `shouldShowMigratePrompt` will return false anyway. We don't need an extra pref for that. My understanding is that we show the alternative ("hey, maybe you want to import manually") message when the user *doesn't* select `keep`, so it doesn't seem to do with that, either... I checked the spec, and I don't see anything else that depends on this step having been completed. What am I missing?
Attachment #8868905 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 45•8 years ago
|
||
mozreview-review |
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
https://reviewboard.mozilla.org/r/140532/#review146306
::: browser/components/migration/AutoMigrate.jsm:15
(Diff revision 6)
>
> const kAutoMigrateEnabledPref = "browser.migrate.automigrate.enabled";
> const kUndoUIEnabledPref = "browser.migrate.automigrate.ui.enabled";
>
> +const kInPageUIEnabledPref = "browser.migrate.automigrate.inpage.ui.enabled";
> +const kMigrateDonePref = "browser.migrate.done";
Yeah, since all followup actions are sheltered under `shouldShowMigratePrompt`, we don't need extra pref \o/ Thanks for point it out!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 47•8 years ago
|
||
Notes:
* The `MIGRATION_ENTRYPOINT_NEWTAB` constant in `migrationUtil.jsm` is added for calling Import Wizard from the NewTab page.
* You can also use mozreview https://reviewboard.mozilla.org/r/140532/diff/5-7/ to see what's changed in automigrate module from the feedback-only patch to the review patch.
Comment 48•8 years ago
|
||
mozreview-review |
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
https://reviewboard.mozilla.org/r/140532/#review146398
::: browser/components/migration/AutoMigrate.jsm:320
(Diff revision 7)
> }
> return null;
> },
>
> /**
> - * Show the user a notification bar indicating we automatically imported
> + * Decide if need to show the user prompt indicating we automatically imported
Nit: Decide if we need to show [the user] a prompt ...
Attachment #8868905 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 49•8 years ago
|
||
mozreview-review |
Comment on attachment 8868905 [details]
Bug 1361286 - add in-page messages support for Automigration module;
https://reviewboard.mozilla.org/r/140532/#review146402
::: browser/components/migration/AutoMigrate.jsm:14
(Diff revision 7)
> const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
>
> const kAutoMigrateEnabledPref = "browser.migrate.automigrate.enabled";
> const kUndoUIEnabledPref = "browser.migrate.automigrate.ui.enabled";
>
> +const kInPageUIEnabledPref = "browser.migrate.automigrate.inpage.ui.enabled";
Can you add this to firefox.js ?
Also, this code is tested by xpcshell-tests, but your trypush didn't run any. You probably want to make sure that happens (and there are no issues) before landing.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 51•8 years ago
|
||
added pref to firefox.js (default = false)
test green, thanks!
Keywords: checkin-needed
Comment 52•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2612892f2785
add in-page messages support for Automigration module; r=Gijs
Keywords: checkin-needed
Comment 53•8 years ago
|
||
bugherder |
Comment 54•8 years ago
|
||
Backed out (as requested by flod) for typos in the patch's strings: https://hg.mozilla.org/integration/autoland/rev/1f86efc3d22fb693317cb41bf7c5363909b61e91
Status: RESOLVED → REOPENED
Flags: needinfo?(gasolin)
Resolution: FIXED → ---
Comment 55•8 years ago
|
||
I don't like being a pain for devs more than I already am, but this landed in m-c right before the weekend, and 6 out of 7 strings contain the same typo ("favorate"). This is also blocking exposure of new strings to localization tools.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 59•8 years ago
|
||
sorry for miss-spelling, fixed and land again
Flags: needinfo?(gasolin)
Keywords: checkin-needed
Comment 60•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/169bd9085780
add in-page messages support for Automigration module; r=Gijs
Keywords: checkin-needed
Comment 61•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 62•8 years ago
|
||
Are there any prefs to set for me to verify this issue? thanks
Flags: needinfo?(gasolin)
Assignee | ||
Comment 63•8 years ago
|
||
Hi justin, currently there's no new UI related change in this issue (except the string)
you can follow comment 22 to test if the automigrate notification bar still works normally
Flags: needinfo?(gasolin)
Comment 64•8 years ago
|
||
Thanks Fred. I can verify that the Automigrate notification bar still works as expected.
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bmao)
You need to log in
before you can comment on or make changes to this bug.
Description
•