903039 - native options - inline options should be dynamically generated

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

We can add the options.xul xul elements dynamically, instead of generating this file with python code.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Erik, I assume you want to do that to simplify cfx-js work. However I'm not sure it's a good idea, mainly because we'll have to pay a cost at runtime instead of
compile time and also this will introduce timing constraints. Also I'm not sure
if options.xul is loaded only once.

I think I'd rather keep as much work as we can do at compile time in cfx as it's a lot easier to test and there is no runtime cost. That being said cfx.py uses full blown xml parser and dom library to manipulate it, I do think we can keep it a lot
more simple in cfx-js & use a simple template (mustachejs or something) to generate `options.xul`.

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

Note we can dynamically add options with this code snippet https://developer.mozilla.org/en-US/Add-ons/Inline_Options?redirectlocale=en-US&redirectslug=Extensions%2FInline_Options#Display_notifications

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

Note: the "addon-options-displayed" notification may not be fired atm if there is no options.xul tho so we'd need to file a AOM bug if that is the case, and use a blank options.xul in the meantime

Comment 4

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368

Erik, here is the initial pass: no tests yet, and several TODO items remain, but it "works".

i'm of the opinion that this might not be the best approach. the AOM currently doesn't simply "import" <setting> elements from options.xul, it has a non-insignificant amount of logic in extensions.js.

http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#3003

obviously, we can duplicate that logic in the SDK, but that would mean we would have to track changes and "port" them over (not ideal).

since we are already modifying the AOM for the purposes of native jetpack (bug 915376), it might be a better idea to generate options.xul at the time the addon is installed, and save that (in the form of a data: uri) in the addon store.

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tomislav Jovanovic [:zombie] from comment #4)
> Created attachment 8366435 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368
> 
> Erik, here is the initial pass: no tests yet, and several TODO items remain,
> but it "works".
> 
> i'm of the opinion that this might not be the best approach. the AOM
> currently doesn't simply "import" <setting> elements from options.xul, it
> has a non-insignificant amount of logic in extensions.js.
> 
> http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> content/extensions.js#3003
> 
> obviously, we can duplicate that logic in the SDK, but that would mean we
> would have to track changes and "port" them over (not ideal). 

We should probably export that logic to a jsm where both extensions.js and our module can utilize it.  That can be a follow-up bug though.

> since we are already modifying the AOM for the purposes of native jetpack
> (bug 915376), it might be a better idea to generate options.xul at the time
> the addon is installed, and save that (in the form of a data: uri) in the
> addon store.

That sounds ugly from maintenance point of view, and I don't think it would gain us much in terms of performance, and if a user rarely looks at the options for add-ons then it is a net loss.

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8366435 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368

This looks good to me!  We added a `isNative` option that is used with Loader in bug 935109 which would be the flag that should be used to determine if the code introduced here should run or not though, instead of a cfx flag.

Comment 7

[Tomislav Jovanovic :zombie]

Comment on attachment 8366435 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368

i think this is done. all existing and new test are passing. a few things left to do on the firefox side (bug 971249), but we have workarounds, and they are, well, working.

i left removing all the dead code for the last step, after i address any review comments (all but guaranteed with a PR this big :)

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8366435 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368

This all looks great!  just need a regression test.  IIRC we decided using a old school xpi, and using the AddonManager to install it is the best solution we had for this atm.

Comment 9

[Tomislav Jovanovic :zombie]

> This all looks great!  just need a regression test. 

to my best understanding, the existing simple-prefs-regression test addon already serves this purpose. it has a custom "bootstrap.js" and "options.xul" that simulate an old-school xpi, and doesn't use the native options.


> IIRC we decided using a old school xpi, and using the AddonManager
> to install it is the best solution we had for this atm.

i only suggested this before i figured out how to make that addon work with a workaround:
https://github.com/mozilla/addon-sdk/pull/1368/files#diff-2badb386d5f1e68d794695d6c18c28ecR156


in any case, i don't know what else is needed for regression test that this doesn't already cover? only thing i can think of might be adding another <setting> element to "options.xul", but _without_ adding another preference to "package.json", and testing that it is added in the UI, thus proving that old-style, non-native preferences work?

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8366435 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368

I stand corrected, we do have a regression test that should be enough.  Just clean up that commented out code and I'll pull it.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

Hey zombie, can we remove the changes related to bug 983322 and make a separate patch for that?

Comment 12

[Tomislav Jovanovic :zombie]

done

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8366435 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368

I'm going to take the r+ back now after the discussion with Irakli in irc that zombie and I had with him.  He wants to avoid changes to bootstrap.js.

Comment 14

[Tomislav Jovanovic :zombie]

managed to avoid modifying bootstrap.js, along with several other fixes..

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8366435 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368

This is really close!  I just need to have a few more tests added:

1. test that the listener which updates the inline options document is actually removed on unload, so that it is not run when the add-on is upgraded/downgraded
2. tests that the validation throws error when it is supposed to
3. tests that the `boolint`, `color`, `file`, `directory` pref types all do not cause errors (these should have already existed, but appear to not which would be my bad)
4. test that invalid pref types are handled correctly (we can either ignore them or throw, I think we should do the latter)
5. I'm also concerned that the AOM could change one day by caching these inline option documents and still fire the event which we use to update them, in which case we wouldn't have any tests fail but there would be duplicate <setting> nodes, so we need some tests to protect ourselves against this.  Either we can handle that possible case, or we can add a test that this is not the case, which would fail if this assumption changes.

Also mentioned a few style nits that should be changed just to keep the code base more consistent.

I'm sorry that I didn't notice this stuff sooner :(

Comment 16

[Tomislav Jovanovic :zombie]

Comment on attachment 8366435 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368

1. i explicitly changed this code [1] after i fixed bug 807382. that fix has already landed, alongside tests for exactly this, so i don't think this is needed.

2. done

3. done

4. this is the same as (2), see [2]

5. this seemed unlikely, so i opted for the test


boy our style guide is exhaustive and irregular..


[1] https://github.com/mozilla/addon-sdk/pull/1368/files#r11456848
[2] https://github.com/mozilla/addon-sdk/pull/1368/files#r11569846

Comment 17

[Tomislav Jovanovic :zombie]

stupid bugzilla url parsing rules..

[1] https://github.com/mozilla/addon-sdk/pull/1368/files#%7211456848
[2] https://github.com/mozilla/addon-sdk/pull/1368/files#%7211569846

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tomislav Jovanovic [:zombie] from comment #16)
> Comment on attachment 8366435 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368
> 
> 1. i explicitly changed this code [1] after i fixed bug 807382. that fix has
> already landed, alongside tests for exactly this, so i don't think this is
> needed.

we should still do this test, because one day someone could change the implementation here, break this, and we wouldn't know without a test for it.

Comment 19

[Tomislav Jovanovic :zombie]

Erik, either i don't understand exactly what you are asking for here, or i actually don't know how to write a test for this. after landing bug 807382, all system/events listeners still alive at addon unload time are removed. there is no way to break this guarantee from an outside module. so apart from using a different module to register listeners, i don't understand what change in implementation here could break this.

in essence, i don't know how to write a unit test for this without it boiling down to a semantically equivalent test for system/events, which doesn't sound like the right thing to do here (in a unit test for this module).

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tomislav Jovanovic [:zombie] from comment #19)
> Erik, either i don't understand exactly what you are asking for here, or i
> actually don't know how to write a test for this. after landing bug 807382,
> all system/events listeners still alive at addon unload time are removed.
> there is no way to break this guarantee from an outside module. so apart
> from using a different module to register listeners, i don't understand what
> change in implementation here could break this.

So in very simple terms, if module A depends on module B, where they basically do the same thing, and module B has tests, that doesn't mean module A should not also have tests, because the implementation of module A could change, such that it depends on module C, or nothing.

Applied to this case we have a simple-prefs module that is supposed to be dead after unload, and the simple-prefs module uses a system/events module which is supposed to be dead after unload.  So having tests for the system/events module is not sufficient for testing the simple-prefs module because the simple-prefs may not use the system/events module at some point in the future.

In test driven development, tests should be written without knowledge of the implementation.  This is accomplished by writing the tests first, but that is usually unpractical in my experience.

> in essence, i don't know how to write a unit test for this without it
> boiling down to a semantically equivalent test for system/events, which
> doesn't sound like the right thing to do here (in a unit test for this
> module).

I think the simplest way to do this will be change the `enable(preferences)` signature to be `enable({ preferences, id })` and use a simple bootstrap add-on as a fixture (by which I mean add it here https://github.com/mozilla/addon-sdk/tree/master/test/fixtures) which would be installed, then call `enable(..)`, verify the `prefs` are added correctly, then disable and re-enable the add-on, at which point you can check if the prefs are still added (they should not be), then uninstall the fixture add-on.

I can do this if you'd like.

Comment 21

[Tomislav Jovanovic :zombie]

i don't remember the last time i was this frustrated while hacking on something.

the final code isn't that bad, except for the one seemingly unnecessary timeout (commented).

it took me hours to pinpoint the exact place between the message passing steps that causes the problem. and i still don't understand why it's required.


> which would be installed, then call `enable(..)`, verify the `prefs` are added
> correctly, then disable and re-enable the add-on, at which point you can
> check if the prefs are still added (they should not be), then uninstall the
> fixture add-on.

this doesn't work with an empty bootstrap addon, as enabling/disabling it does nothing, so i used a simple jetpack addon with a single pref instead, which makes the test even closer to the real thing.


Erik, note that if you try to run this test locally, it will fail because the xpi in fixtures requires changes from this PR landed in firefox in order to work correctly. i expect it to work on tinderbox since that will build firefox with my changes. 

i have run the tests with another version of the same addon packaged with --no-strip-xpi, but that one is 400k, and defeats the goal of testing whatever modules are shipping in firefox. so, you can either trust me ;), or i can provide you with that other version of the xpi.

Comment 22

[Tomislav Jovanovic :zombie]

> and i still don't understand why it's required.

ignore this, i figured it out, it was my fault all along.. :/

Comment 23

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8366435 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1368

Nice work zombie!  let's merge this.

Comment 24

[Tomislav Jovanovic :zombie]

(In reply to Tomislav Jovanovic [:zombie] from comment #21)
> Erik, note that if you try to run this test locally, it will fail because
> the xpi in fixtures requires changes from this PR landed in firefox in order
> to work correctly. i expect it to work on tinderbox since that will build
> firefox with my changes. 

unfortunately, i found out my assumptions about this were wrong. our CI just takes a firefox binary from m-c (or was it fx-team?) and runs the cfx tests, so this wouldn't work.

i think i will break out this test addon as a fallow-up, land the rest of this PR, wait for the changes to get uplifted, and then land bug 1011813 next week.

sounds like a good plan Erik?

Comment 25

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tomislav Jovanovic [:zombie] from comment #24)
> (In reply to Tomislav Jovanovic [:zombie] from comment #21)
> > Erik, note that if you try to run this test locally, it will fail because
> > the xpi in fixtures requires changes from this PR landed in firefox in order
> > to work correctly. i expect it to work on tinderbox since that will build
> > firefox with my changes. 
> 
> unfortunately, i found out my assumptions about this were wrong. our CI just
> takes a firefox binary from m-c (or was it fx-team?) and runs the cfx tests,
> so this wouldn't work.
> 
> i think i will break out this test addon as a fallow-up, land the rest of
> this PR, wait for the changes to get uplifted, and then land bug 1011813
> next week.
> 
> sounds like a good plan Erik?

That sounds good, I'll make the test add-on in the follow up bug.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Just to confirm, tests pull the tip-most Firefox build from fx-team.

Comment 27

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/40ee655d79d9f21c34d96828cd9fe48bc8e66b15
bug 903039 - jetpack native options

- work without changing bootstrap.js
- add validate, type and caching tests
- exhaustive and irregular style guide
- test addon disable
- Revert "test addon disable"

https://github.com/mozilla/addon-sdk/commit/c73d7c97183ddb2130f60849fda526e042d5d0eb
Merge pull request #1368 from zombie/903039-native-options

bug 903039 - jetpack native options, r=@erikvold

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merging this broke tests on all platforms: https://tbpl.mozilla.org/php/getParsedLog.php?id=39878420&tree=Jetpack

I'm not near a computer that has a copy of the SDK or git handy, so if someone else could run this command to revert the bad commit, that'd be great.

git revert -m 1 c73d7c9718 && git push

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Jordan reverted it in https://github.com/mozilla/addon-sdk/commit/21bb6639fc2546cc2e466532edbc988453a372d0

Zombie, you should be able to do a | git revert 21bb6639fc | to un-revert your code, and then create a new pull request from that to work on fixing the broken test.

Comment 30

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/db5d98b39f9014bb4b72546b56a26c21b66bfd54
bug 903039 - native options, test cleanup

https://github.com/mozilla/addon-sdk/commit/2bcbd9d482c9d4e1182978c39a0f4ad9e2c70f4e
Merge pull request #1491 from zombie/903039-native-options-test-clean

bug 903039 - native options, test cleanup, a=@erikvold

Comment 31

[Tomislav Jovanovic :zombie]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #25)
> That sounds good, I'll make the test add-on in the follow up bug.

i attached a PR with my original patch to bug 1011813, but if you wanted to modify that or do something different, feel free..


(In reply to Wes Kocher (:KWierso) from comment #28)
> Merging this broke tests on all platforms:

the problem was that a few simple-prefs tests got messed up because i didn't clear prefs after a test (partly because default prefs can't be easily cleared, filed bug 1012231).


(In reply to [github robot] from comment #30)
> bug 903039 - native options, test cleanup, a=@erikvold

it seems linux runs test files in reverse order, so now a native-options test gets messed up because some simple-prefs tests don't clean up after themselves. 

https://tbpl.mozilla.org/php/getParsedLog.php?id=39904738&tree=Jetpack

i filed bug 1012361 and attached a PR (quick review so i don't have to revert again?)