536780 - Settings button on about:jetpack enabled only after initial install

Status: RESOLVED FIXED

(Mozilla Labs :: Jetpack Prototype, defect, P1)

Description

[Drew Willcoxon :adw]

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.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Some users may well want to access settings on second and subsequent runs.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 3

[Drew Willcoxon :adw]

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.

Comment 4

[Drew Willcoxon :adw]

Oh, and yours works OK here.

Comment 5

[Matěj Cepl]

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.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 7

[Drew Willcoxon :adw]

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

[Matěj Cepl]

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

[Matěj Cepl]

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

[Matěj Cepl]

sorry, this should go to bug 526692

Comment 11

[Myk Melez [:myk] [@mykmelez]]

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/[e-mail]/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.

Comment 12

[Drew Willcoxon :adw]

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?

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: 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.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: 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.

Comment 15

[Drew Willcoxon :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.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Fixed by changeset http://hg.mozilla.org/labs/jetpack/rev/847ad307af6e.