Closed Bug 1366005 Opened 3 years ago Closed 3 years ago

Remove UITour.showHeartbeat

Categories

(Firefox :: Tours, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: mythmon, Assigned: mythmon)

References

Details

Attachments

(2 files)

Once SelfSupport is gone (bug 1361578), we can remove UITour.showHeartbeat, since it doesn't have any other users, and is replaced by Shield.
Depends on: 1366006
Assignee: nobody → mcooper
Comment on attachment 8869606 [details]
Bug 1366005 - Remove UITour.showHeartbeat.

https://reviewboard.mozilla.org/r/141192/#review144770

Thank for taking this :)

::: browser/components/uitour/UITour.jsm:16
(Diff revision 1)
>  Cu.import("resource://gre/modules/AppConstants.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Preferences.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource:///modules/RecentWindow.jsm");

You can remove this import now too I think

::: browser/components/uitour/UITour.jsm:40
(Diff revision 1)
>    "resource:///modules/ReaderParent.jsm");
>  
>  // See LOG_LEVELS in Console.jsm. Common examples: "All", "Info", "Warn", & "Error".
>  const PREF_LOG_LEVEL      = "browser.uitour.loglevel";
>  const PREF_SEENPAGEIDS    = "browser.uitour.seenPageIDs";
>  const PREF_SURVEY_DURATION = "browser.uitour.surveyDuration";

PREF_SURVEY_DURATION can be deleted I think… eslint no-unused should have caught that I guess

::: browser/themes/linux/browser.css
(Diff revision 1)
> -%include ../shared/UITour.inc.css
> -

You definitely don't want to be deleting this CSS file… just remove the heartbeat-specific stuff
Attachment #8869606 - Flags: review?(MattN+bmo)
Status: NEW → ASSIGNED
Component: General → Tours
Comment on attachment 8869606 [details]
Bug 1366005 - Remove UITour.showHeartbeat.

https://reviewboard.mozilla.org/r/141192/#review145328

Thanks

::: browser/themes/shared/UITour.inc.css
(Diff revision 2)
> -.heartbeat > #star-rating-container > #star2 {
> -  -moz-box-ordinal-group: 2;
> -}
> -
> -.heartbeat > #star-rating-container > .star-x  {
> -  background: url("chrome://browser/skin/heartbeat-star-off.svg");

I assume you are still going to use this and the 2 other svg files with SHIELD?
Attachment #8869606 - Flags: review?(MattN+bmo) → review+
> I assume you are still going to use this and the 2 other svg files with SHIELD?

Actually, that's an oversight. I don't see any other in-tree code using these svgs. I'll copy those svgs over to the SHIELD add-on as well and remove them here.
Depends on: 1367598
I've added a commit that adds the removed CSS to shield-recipe-client. Originally I was going to do this separately in bug 
1366006, but I wasn't able to get them to work separately. The second patch was reviewed by Gijs on Github: https://github.com/mozilla/normandy/pull/770.
Attachment #8871341 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8871341 [details]
Bug 1366005 - Move UITour CSS into shield-recipe-client.

I was hoping that by setting r=Gijs here, it would transfer to Reviewboard (it did not).

NB: Gijs approved that patch on Github, which is why I set r+ for him.
Attachment #8871341 - Flags: review+ → review?(gijskruitbosch+bugs)
Comment on attachment 8871341 [details]
Bug 1366005 - Move UITour CSS into shield-recipe-client.

https://reviewboard.mozilla.org/r/142818/#review146846
Attachment #8871341 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mcooper@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc2c82d8ba22
Remove UITour.showHeartbeat r=MattN
https://hg.mozilla.org/integration/autoland/rev/7662d766f166
Move UITour CSS into shield-recipe-client. r=Gijs
Pushed by mcooper@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48a6570b38cb
Remove UITour.showHeartbeat. r=MattN
https://hg.mozilla.org/integration/autoland/rev/9cdcc98ab24a
Move UITour CSS into shield-recipe-client. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/48a6570b38cb
https://hg.mozilla.org/mozilla-central/rev/9cdcc98ab24a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Heads-up: this bug (specifically, moving the "heartbeat > #star-rating-container > .star-x" CSS rules) caused breakage in the Pulse addon, which seems to have been sharing the star styling & images that this bug moved around.

I reported that issue here:
  https://github.com/mozilla/pulse/issues/179

(Not sure at this point whether the fix will be entirely in the Pulse addon itself vs. whether there are mozilla-central changes needed as well. Anyway, just noting the connection here, for the record.)
You need to log in before you can comment on or make changes to this bug.