Settings button on about:jetpack enabled only after initial install

RESOLVED FIXED in 0.8

Status

Mozilla Labs
Jetpack Prototype
P1
critical
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: adw, Assigned: myk)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

Refreshing about:jetpack and restarting the browser leaves my feature's settings button on about:jetpack disabled.  The only time it's not disabled is right after I've installed it.
(Assignee)

Comment 1

8 years ago
Some users may well want to access settings on second and subsequent runs.
Assignee: nobody → myk
Severity: normal → critical
Status: NEW → ASSIGNED
Target Milestone: -- → 0.8
(Assignee)

Comment 2

8 years ago
Hmm, I'm unable to reproduce this problem with the jetpack I use to test settings <http://mykzilla.org/jetpack/settings-test.html>.  I tried on both Linux and Mac OS X.  Drew: can you point me to the jetpack with which you're having this problem?
http://hg.mozilla.org/users/dwillcoxon_mozilla.com/jetpacks/file/93cf0e4c45f5/settings

If about:jetpack is closed when I install, there's never a time when I see the settings button enabled.  Other people have reported the problem on the mailing list and through Twitter, FWIW.

One difference between yours and mine is that mine imports some stuff and does other things, but even with all that commented out the problem still happens.
Oh, and yours works OK here.

Comment 5

8 years ago
I have this one http://git.fedorahosted.org/git/?p=triage.git;a=blob_plain;f=jetpack/bugzillaBugTriage.js;hb=895a47be95d62debbfbeba23dd842becea9bbb52 which seems to present exactly the same problem. Even when settings are enabled, it has not effect on about:jetpack.
(Assignee)

Comment 6

8 years ago
Drew: thanks for the reduced testcase!  Unfortunately, I still can't reproduce the problem with it.

I tried on both Linux and Mac OS X and with both Firefox 3.5.7 and the latest Namoroka nightly build.  I also tried with a fresh profile in case that made a difference.  What OS/Firefox are you seeing this on, and are there any errors in the console when it happens?
Oh!  This is cool, when installed from the remote URL, the test case works.  When installed locally from my disk, it doesn't.  Both Shiretoko and Namoroka nightlies, OS X 10.5, fresh profiles, Jetpack pulled from the repo, no errors in the console.

Comment 8

8 years ago
OK, confirmed ... when loading from the remote URL, settings are visible, when loading from file:/// they are not.

Also, two more comments on the settings generally:

- some [OK], [Submit], [Close] button would be helpful ... closing a dialog by clicking on the top-right corner is in my books "close without changing anything".
- in about:jetpack those jetpacks which shouldn't have any settings (i.e., no manifest) shouldn't have the settings link

Comment 9

8 years ago
I am probably hijacking this bug for "whatever is wrong with jetpack.settings", but if I try this minimalist example (http://www.ceplovi.cz/matej/jetpack/install-test-settings.html), then dialog opens all right, but then I get error

"chrome://jetpack/content/index.html -> http://www.ceplovi.cz/matej/jetpack/jetpack-settings.js"
lineNumber	22
message	"jetpack.settings is undefined"
name	"TypeError"
stack	"@chrome://jetpack/content/index.html -> \
   http://www.ceplovi.cz/matej/jetpack/jetpack-settings.js:22\n"

Comment 10

8 years ago
sorry, this should go to bug 526692
(Assignee)

Comment 11

8 years ago
I'm still not sure what's going on here, but a couple thoughts:

1. Jetpack retrieves "local" feeds differently than remote ones; there could be some bug in the code that retrieves local feeds.

2. Firebug throws the following exception when it parses the feature's code: FTS0: Error in hook: [Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///home/myk/Profiles/jetpack-dev/extensions/firebug@software.joehewitt.com/components/firebug-service.js :: getFrameGlobal :: line 2247"  data: no] fn=

The file jetpack/extension/modules/simple-storage.js is at the top of the stack (three times), but unfortunately the function and line number are listed as "@undefined;".  The next item in the stack is chrome://jetpack/content/js/settings-store.js, again with function/line number "@undefined;", but this is still a clue: maybe the simple storage module is throwing an exception when trying to init/read the feature's storage, and that's causing the problem.
Sorry I haven't debugged this.  I'm not sure how to fix it, but here's the epic story.

When I reload about:jetpack, my local feature actually loads twice.  That is, its context is created twice.  The first time, its code is completely empty.  Second time, it's OK.  Problem is, the settings button is set up after the first load but before the second.  The code is empty, there's no manifest, so no button.

The first context creation comes from the window's ready handler, at the bottom of jetpack-runtime.js, which calls JetpackRuntime.loadJetpacks().  So far so good.

loadJetpacks() sets the feed's code to the empty string if the feed is local.  Keep following loadJetpacks() and you get to _getLocalFeed(), which sets up an async XHR to load the local source code.  When the XHR completes, the feed fires a "feed-change" event, which triggers a feed reload, which triggers the second context creation, which now has the feature's code.

Meanwhile, the window ready handler in app.js has already run, and my feature's entry on Installed Features has already been created, its settings button disabled.

Inner function onFeedEvent() in app.js's ready handler already listens for subscription events; maybe it could listen for "feed-change" events too?
(Assignee)

Comment 13

8 years ago
Created attachment 421367 [details] [diff] [review]
patch v1: fixes problem

(In reply to comment #12)
> Inner function onFeedEvent() in app.js's ready handler already listens for
> subscription events; maybe it could listen for "feed-change" events too?

That seems reasonable.  This implementation is a bit hacky (in particular, it may be expensive to call getSubscribedFeeds once for every local feature), but given the reboot, I think it's ok as a short-term fix.
Attachment #421367 - Flags: review?(adw)
(Assignee)

Comment 14

8 years ago
Created attachment 421374 [details] [diff] [review]
patch v2: better architecture

Based on a conversation with Drew in IRC, this patch has a better architecture.  In particular, it adds an updateLinkForJetpack method (like the add|removeLinkForJetpack ones) that only updates the settings button instead of recreating the whole entry.
Attachment #421367 - Attachment is obsolete: true
Attachment #421374 - Flags: review?(adw)
Attachment #421367 - Flags: review?(adw)
Comment on attachment 421374 [details] [diff] [review]
patch v2: better architecture

If it weren't for the reboot I'd like a _getButton() to go along with _addButton(), but no worries, r=adw.
Attachment #421374 - Flags: review?(adw) → review+
(Assignee)

Comment 16

8 years ago
Fixed by changeset http://hg.mozilla.org/labs/jetpack/rev/847ad307af6e.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.