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)
Tracking
()
People
(Reporter: MattN, Assigned: MattN)
References
Details
(Keywords: regression, Whiteboard: [comma-separated browser.uitour.testingOrigins])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Unfocused
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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-
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Assignee | ||
Comment 3•10 years ago
|
||
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)
Updated•10 years ago
|
Iteration: 35.3 → 36.1
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
(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
status-firefox36:
--- → fixed
Whiteboard: [comma-separated browser.uitour.testingOrigins]
Comment 6•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Assignee | ||
Comment 7•10 years ago
|
||
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?
Comment 8•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8504157 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 10•10 years ago
|
||
Flags: in-testsuite+
Comment 11•10 years ago
|
||
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.
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Comment 13•10 years ago
|
||
Thanks Matt - useful link will keep a check.
You need to log in
before you can comment on or make changes to this bug.
Description
•