Closed
Bug 604253
Opened 14 years ago
Closed 13 years ago
Use beta channel to decide UI to show
Categories
(Mozilla Labs Graveyard :: Test Pilot, defect, P1)
Mozilla Labs Graveyard
Test Pilot
Tracking
(Not tracked)
VERIFIED
FIXED
1.1
People
(Reporter: jono, Assigned: jono)
Details
(Whiteboard: needs-integration, ui, beta)
Attachments
(3 files, 2 obsolete files)
15.45 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
1.11 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
mossop
:
review+
mossop
:
approval2.0+
|
Details | Diff | Splinter Review |
Currently the extension is deciding whether to show Feedback UI or Test Pilot UI based on the Firefox version. Instead, it should decide this based on whether the user is in the Beta Channel or not.
Priority: -- → P1
Whiteboard: beta
Target Milestone: -- → 1.1
As of http://hg.mozilla.org/labs/testpilot/rev/594472d72b3e we are now using the channel setting in method _isFfx4BetaVersion(). Assuming for now that nightly channel should be treated the same as beta channel. Still need to find a way of switching the overlay based on the channel, though.
Done in http://hg.mozilla.org/labs/testpilot/rev/0a2061bc092f http://hg.mozilla.org/labs/testpilot/rev/cfb09bdcaa07 http://hg.mozilla.org/labs/testpilot/rev/c3989950978f http://hg.mozilla.org/labs/testpilot/rev/ee5d7f7e26ab http://hg.mozilla.org/labs/testpilot/rev/2d24615e4f3e http://hg.mozilla.org/labs/testpilot/rev/586b2c4965d9 and http://hg.mozilla.org/labs/testpilot/rev/546b286cda6d It turned out to require a pretty major rewrite of how the UI is constructed. In the process, these changes also fixed bug https://bugzilla.mozilla.org/show_bug.cgi?id=593949 Need to thoroughly test this and integrate to bundled Feedback add-on before closing the bug.
Whiteboard: beta → needs-integration, ui, beta
This patch fixes this bug as well as bug 593949 and bug 577243 . Dave, can you please review for inclusion into Feedback in Firefox 4?
Attachment #500436 -
Flags: review?(dtownsend)
Comment 4•14 years ago
|
||
Comment on attachment 500436 [details] [diff] [review] Patch fixing this as well as related bugs Not going to do a deep review on this just yet. As a rule I dislike manually building so much UI like this so I'm hopeful there is an easier way (also one that doesn't require so much string churn). You may have already looked into this but do you know about document.loadOverlay to dynamically load overlays? In theory you would be able to keep the bulk of feedback-browser.xul and tp-browser.xul and just load the appropriate one in your JS code. I know there are a couple of bugs with dynamic overlays though so perhaps you've already tried this and hit one of them? Re-request review if that doesn't work out (letting me know what the bug was you were hitting would be useful as we want to get dynamic overlays working well really). >diff --git a/extension/content/browser.js b/extension/content/browser.js >--- a/extension/content/browser.js >+++ b/extension/content/browser.js >@@ -141,71 +141,40 @@ > > menuPopup.openPopup(menuButton, alignment, 0, 0, true); > } > }; > > > var TestPilotWindowHandlers = { > onWindowLoad: function() { >+ dump("onWindowLoad called.\n"); Drop the dump statement please.
Attachment #500436 -
Flags: review?(dtownsend)
Here is a revised version of the patch. It uses loadOverlay() to get the UI in, and does not have any string changes. It also fixes bug 577243 (feedback button custimizability) but does not attempt to fix bug 593949 (put test pilot icon in add-on bar) - i will do that in a separate patch.
Attachment #502976 -
Flags: review?(dtownsend)
Comment 6•13 years ago
|
||
Comment on attachment 502976 [details] [diff] [review] Revised version of patch, incorporating review comments Looks good, thanks for making all those changes. Just one issue to be fixed then we can land this. >diff --git a/extension/modules/interface.js b/extension/modules/interface.js >+ buildFeedbackInterface: function(window) { >+ /* If this is first run, and it's ffx4 beta version, and the feedback >+ * button is not in the expected place, put it there! >+ * (copied from MozReporterButtons extension) */ >+ >+ /* Check if we've already done this customization -- if not, don't do it >+ * again (don't want to put it back in after user explicitly takes it out- >+ * bug 577243 )*/ >+ let firefoxnav = window.document.getElementById("nav-bar"); >+ let pref = "extensions.testpilot.alreadyCustomizedToolbar"; Doesn't look like the pref is already set anywhere else so the first access will throw an exception. Either add the pref to the default preferences file or wrap this in a try...catch block.
Attachment #502976 -
Flags: review?(dtownsend) → review-
Thanks for catching that, Dave! It looks like I just forgot to include preferences.js when generating the patch file. This new patch includes the pref default.
Attachment #500436 -
Attachment is obsolete: true
Attachment #502976 -
Attachment is obsolete: true
Has this been merged into Feedback yet? Can I close the bug?
Updated•13 years ago
|
Attachment #503560 -
Flags: review?(dtownsend)
Updated•13 years ago
|
Attachment #503560 -
Flags: review?(dtownsend) → review+
Comment 9•13 years ago
|
||
Landed: http://hg.mozilla.org/mozilla-central/rev/a47344f024ee
Assignee: nobody → jdicarlo
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•13 years ago
|
||
Oops, Jinghua just found a bug in the patch when testing on Firefox 3.6. It required a one-line change. This is a patch on the previous patch, so it should be applied after that one. It's not a replacement.
Attachment #511460 -
Flags: review?(dtownsend)
Comment 11•13 years ago
|
||
Attachment #511509 -
Flags: review+
Attachment #511509 -
Flags: approval2.0+
Comment 12•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/00678ee61f4a
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Attachment #511460 -
Flags: review?(dtownsend) → review+
Updated•8 years ago
|
Product: Mozilla Labs → Mozilla Labs Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•