Closed Bug 1081772 Opened 10 years ago Closed 10 years ago

Add back a way to test UITour on origins not whitelisted by default

Categories

(Firefox :: General, defect)

35 Branch
defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.1
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Keywords: regression, Whiteboard: [comma-separated browser.uitour.testingOrigins])

Attachments

(1 file, 1 obsolete file)

Since bug 1050080 landed, browser.uitour.whitelist.add.testing no longer works which makes it hard for Firefox and mozilla.org developers to test UITour functionality.
Attachment #8503843 - Flags: review?(bmcbride)
Flags: qe-verify-
Flags: firefox-backlog+
Comment on attachment 8503843 [details] [diff] [review]
v.1 Bring back browser.uitour.whitelist.add.testing (with test)

Review of attachment 8503843 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/UITour.jsm
@@ +149,5 @@
>      XPCOMUtils.defineLazyGetter(this, "url", function () {
>        return Services.urlFormatter.formatURLPref("browser.uitour.url");
>      });
>  
> +    this.loadTestingOrigins();

Given the usage of this, I'd rather see any change reflected live, without needing a restart (having to restart is an impediment to a nice development/testing experience). If we just check the pref on each event, we'd not only have the changes be live, but we'd also avoid polluting the permissions DB with what was potentially meant as a temporary override.
Attachment #8503843 - Flags: review?(bmcbride) → review-
(In reply to Blair McBride [:Unfocused] from comment #1)
> Given the usage of this, I'd rather see any change reflected live, without
> needing a restart (having to restart is an impediment to a nice
> development/testing experience). If we just check the pref on each event,
> we'd not only have the changes be live, but we'd also avoid polluting the
> permissions DB with what was potentially meant as a temporary override.

In case you didn't notice, I specified that the permission was for the session so it wouldn't stay in the database. I didn't check the permission for each page event since we used to avoid checking prefs on repeated actions like that. If you're fine with it then I'll do that.
I moved the check for this after the permission check so the common case is still the same speed.
Attachment #8503843 - Attachment is obsolete: true
Attachment #8504157 - Flags: review?(bmcbride)
Iteration: 35.3 → 36.1
Comment on attachment 8504157 [details] [diff] [review]
v.2 Make browser.uitour.whitelist.add.testing live

Review of attachment 8504157 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/UITour.jsm
@@ +24,5 @@
>    "resource:///modules/BrowserUITelemetry.jsm");
>  
>  
>  const UITOUR_PERMISSION   = "uitour";
> +const PREF_TEST_WHITELIST = "browser.uitour.whitelist.add.testing";

I do kinda wonder if we should just rename this, given the old system would have reset it to "" anyway - so anyone using this will need to set it again. Something like browser.uitour.testingDomains
Attachment #8504157 - Flags: review?(bmcbride) → review+
(In reply to Blair McBride [:Unfocused] from comment #4)
> > +const PREF_TEST_WHITELIST = "browser.uitour.whitelist.add.testing";
> 
> I do kinda wonder if we should just rename this, given the old system would
> have reset it to "" anyway - so anyone using this will need to set it again.
> Something like browser.uitour.testingDomains

Good point. I forgot about it getting blanked upon import. After talking to Alex Gibson about it, I realized the resetting in old builds would make this painful if your testing profile is switching between builds before and after bug 1050080. Alex was fine with renaming as long as we told him so he could update his documentation. I went with browser.uitour.testingOrigins.

https://hg.mozilla.org/integration/fx-team/rev/d04c14e7d54e
Whiteboard: [comma-separated browser.uitour.testingOrigins]
https://hg.mozilla.org/mozilla-central/rev/d04c14e7d54e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment on attachment 8504157 [details] [diff] [review]
v.2 Make browser.uitour.whitelist.add.testing live

Approval Request Comment
[Feature/regressing bug #]: bug 1050080 
[User impact if declined]: Developers/QE won't be able to test tours in 35 (when we will be doing a Loop tour)
[Describe test coverage new/current, TBPL]: a new test was added. manual testing was done too.
[Risks and why]: Low risk isolated to UITour. The new code doesn't get hit in the common case where the website is whitelisted by default e.g. for non-testing websites.
[String/UUID change made/needed]: None
Attachment #8504157 - Flags: approval-mozilla-aurora?
Had a few people report that they can't whitelist https://www-demo2.allizom.org/en-US/firefox/33.1/whatsnew/ in the latest Nightly for testing.

They say they have tried both:

browser.uitour.testingOrigins

and 

browser.uitour.whitelist.add.testing

Any ideas Matt?
Flags: needinfo?(MattN+bmo)
False alarm,

It seems with the new `browser.uitour.testingOrigins` pref you need to include the protocol (e.g. https://), whereas with the old pref you needed to exclude it.

Our localizers have it working ok now, I'll just make a note to update our documentation on this.
Flags: needinfo?(MattN+bmo)
Attachment #8504157 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Has this fix been added to the gum/dev build? trying the latest build (21 Oct), I can't seem to add to whitelist for local development.
(In reply to Alex Gibson [:agibson] from comment #11)
> Has this fix been added to the gum/dev build? trying the latest build (21
> Oct), I can't seem to add to whitelist for local development.

Doesn't look like it: https://hg.mozilla.org/projects/gum/log/tip/browser/modules/UITour.jsm

If that's tracking Aurora it will probably get merged there soon as it only landed on Aurora 15 hours ago. I don't know much about Gum though.
Thanks Matt - useful link will keep a check.
You need to log in before you can comment on or make changes to this bug.