Closed Bug 1342462 Opened 3 years ago Closed 3 years ago

'browser' is undefined in UITour.jsm#startSubTour

Categories

(Firefox :: Tours, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: standard8, Assigned: MattN)

References

Details

Attachments

(1 file)

On working through ESLint, I've found that browser is undefined here:

browser/components/uitour/UITour.jsm
  1948:44  error  'browser' is not defined.  no-undef (eslint)

https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/browser/components/uitour/UITour.jsm#1948

Matt, I can see this would throw, but I don't know if this is a path that is currently used or not, and I'm not sure how we'd fix it.
Flags: needinfo?(MattN+bmo)
(In reply to Mark Banner (:standard8) from comment #0)
> Matt, I can see this would throw, but I don't know if this is a path that is
> currently used or not, and I'm not sure how we'd fix it.

So from what I can tell this was broken from the initial landing yet this was uplifted all the way to beta at the time… what a waste :( 

Since https://www.mozilla.org/en-US/firefox/reading/start/ is now a 404 I believe this code isn't used in practice anymore anyways so we should back out the broken/unused portions of bug 1134501 which may also fix bug 1157966.

I'll try take a quick stab at this.
Assignee: nobody → MattN+bmo
Blocks: 1134501, 1157966
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
Priority: -- → P3
Comment on attachment 8842292 [details]
Bug 1342462 - UITour: Remove automatic reader view tour based on a URL regex.

https://reviewboard.mozilla.org/r/116156/#review117758

Good riddance. Also, yuck, no tests for this? Even better riddance, I guess.

Might be worth just pinging alex gibson or someone else on the www team to triple-check this is dead.
Attachment #8842292 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #1)
> Since https://www.mozilla.org/en-US/firefox/reading/start/ is now a 404 I
> believe this code isn't used in practice anymore anyways so we should back
> out the broken/unused portions of bug 1134501 which may also fix bug 1157966.

(In reply to :Gijs from comment #3)
> Might be worth just pinging alex gibson or someone else on the www team to
> triple-check this is dead.

Alex, can you confirm that URLs that match `^https:\/\/www\.mozilla\.org\/[^\/]+\/firefox\/reading\/start` are no longer expected to open a reader view tour panel?
Flags: needinfo?(agibson)
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #4)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #1)
> > Since https://www.mozilla.org/en-US/firefox/reading/start/ is now a 404 I
> > believe this code isn't used in practice anymore anyways so we should back
> > out the broken/unused portions of bug 1134501 which may also fix bug 1157966.
> 
> (In reply to :Gijs from comment #3)
> > Might be worth just pinging alex gibson or someone else on the www team to
> > triple-check this is dead.
> 
> Alex, can you confirm that URLs that match
> `^https:\/\/www\.mozilla\.org\/[^\/]+\/firefox\/reading\/start` are no
> longer expected to open a reader view tour panel?

> Might be worth just pinging alex gibson or someone else on the www team to triple-check this is dead.

As far as I remember the UITour implementation for this feature was halted mid-way when the original Pocket integration took over. This URL never made it to production, so it should be fine to remove the related code.
Flags: needinfo?(agibson)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/8d7b7fa77365
UITour: Remove automatic reader view tour based on a URL regex. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/8d7b7fa77365
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(MattN+bmo)
For what reason? Is bug 1342459 going to get uplifted? This is essentially dead code removal AFAICT and 53 will likely go to beta before this gets approved.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8842292 [details]
Bug 1342462 - UITour: Remove automatic reader view tour based on a URL regex.

I guess a slight reduction in parent process memory could be useful but 154KB probably isn't noticeable.

Approval Request Comment for 53
[Feature/Bug causing the regression]: bug 1134501 added this broken but no unused code
[User impact if declined]: Slightly less (~154KB on OS X) main process memory due to UITour.jsm not being loaded, otherwise no user impact. Mostly requesting uplift because Ryan asked for it.
[Is this code covered by automated tests?]: The removed code didn't have tests but most other parts of UITour do.
[Has the fix been verified in Nightly?]: I verified bug 1157966 and a trivial `Mozilla.UITour.showHighlight("urlbar")` call from about:home since we don't have other active tour AFAIK.
[Needs manual test from QE? If yes, steps to reproduce]: Probably not since this was a fairly straightforward removal.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Fairly isolated/simple (supposedly unused) broken code removal
[String changes made/needed]: None
Attachment #8842292 - Flags: approval-mozilla-beta?
Comment on attachment 8842292 [details]
Bug 1342462 - UITour: Remove automatic reader view tour based on a URL regex.

This doesn't seem urgent to uplift, but we can certainly try it for beta 2.
Attachment #8842292 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting qe-verify- based on Matthew's assessment on manual testing needs (see Comment 10).
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.