1370077 - Don't log warning when `background.persistent` is set to `true`

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P5)

Description

[ExE Boss]

Attachment: `background.persistent` test extension v1

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170518000419

Steps to reproduce:

Create and load a WebExtension with a background page that has the `background.persistent` property set to `true`.

A test extension is attached as an attachment.


Actual results:

The following warning message was printed into the console:
```
addons.webextension.persistent-background-page@bmo-test-extension	WARN	Loading extension 'persistent-background-page@bmo-test-extension': Reading manifest: Error processing background.persistent: Event pages are not currently supported. This will run as a persistent background page.
```


Expected results:

No warning messages should be printed when `background.persistent` is set to `true`, as persistent background pages are fully supported.

Comment 1

[[:Towkir] Ahmed]

Hi, I am interested in fixing this one.
How do you suggest I should approach for the fix ? 
Where do the functions for this live ?

Comment 2

[Bob Silverberg [:bsilverberg]]

Thanks for your interest in taking this bug on, Ahmed.

I am going to be away for the next couple of weeks, so I'm going to ask Shane if he can take over for me as mentor in my absence.

I took a quick look at the code and I'm not sure if the change is as simple as may have originally been thought, but I'll put my observations in here and maybe you and Shane can work out if it's still a good first bug.

The warning is being generated because the `persistent` property is marked as `deprecated` in manifest.json at [1] and [2] via the `PersistentBackgroundProperty` id [3].

The code that issues warnings for deprecated entries can be found in Schemas.jsm at [4], which is called by `checkDeprecated` [5].

This code is generic in that it will issue a warning for any deprecated entry, regardless of the value of that entry, so to fix this bug we will need to _not_ use `deprecated` for the `persistent` property. We could add some special-cased code into Schemas.jsm to deal with the `persistent` property and issue a warning if its value is not `true`, although I don't really see much special casing in there currently to deal with specific properties. Maybe a new type in manifest.json for something like `DeprecatedIfNotTrue` would work, but that seems pretty ugly.

Shane, would you be able to help Ahmed over the next couple of weeks with this bug, starting with a suggestion about how we can approach a fix?

[1] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/schemas/manifest.json#102-105
[2] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/schemas/manifest.json#116-119
[3] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/schemas/manifest.json#436-438
[4] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#1017-1039
[5] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#1041-1052

Comment 3

[Shane Caraveo (:mixedpuppy)]

I'm wondering if a new type, deprecatedValues, would be appropriate.  A list of values should be allowed as the value for it.  Then the code Schemas.jsm pointed to in comment 2 can be modified to also check the values against this.

You probably need to look through Schemas.jsm[1] for where "deprecated" is defined and add similar code for "deprecatedValues".

After that, the schema could look like:

{
 "id": "PersistentBackgroundProperty",
 "type": "boolean",
 "deprecated": "Event pages are not currently supported. This will run as a persistent background page.",
 "deprecatedValues": ["false"]
}


[1] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#1080
    http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#943-959

Let me know if you need any further guidance.

Comment 4

[apoorvasingh2811]

Hi, can I take this up?

Comment 5

[[:Towkir] Ahmed]

Yes please, I am a bit busy now.
For instructions, see comment 2 and comment 3

Comment 6

[apoorvasingh2811]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> I'm wondering if a new type, deprecatedValues, would be appropriate.  A list
> of values should be allowed as the value for it.  Then the code Schemas.jsm
> pointed to in comment 2 can be modified to also check the values against
> this.
> 
> You probably need to look through Schemas.jsm[1] for where "deprecated" is
> defined and add similar code for "deprecatedValues".
> 
> After that, the schema could look like:
> 
> {
>  "id": "PersistentBackgroundProperty",
>  "type": "boolean",
>  "deprecated": "Event pages are not currently supported. This will run as a
> persistent background page.",
>  "deprecatedValues": ["false"]
> }
> 
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#1080
>    
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#943-959
> 
> Let me know if you need any further guidance.

So, I only have to make the changes in
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#1080 .  and   > http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#943-959 ?

Comment 7

[apoorvasingh2811]

^  http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#1080 and http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#943-959 ?

Comment 8

[Bob Silverberg [:bsilverberg]]

(In reply to apoorvasingh2811 from comment #7)
> ^ 
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#1080 and
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#943-959 ?

That looks right, according to what Shane suggested, but I think you'll also want to change checkDeprecated at [1] as well to deal with the new deprecatedValues logic. Perhaps that can be avoided, so you'll want to experiment a bit to see what works best.

[1] http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/toolkit/components/extensions/Schemas.jsm#1079

Comment 9

[Firefox Bug Husbandry Bot]

Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958

Comment 11

[Caitlin Neiman [:caitmuenster]]

> The following warning message was printed into the console:
> ```
> addons.webextension.persistent-background-page@bmo-test-extension	WARN
> Loading extension 'persistent-background-page@bmo-test-extension': Reading
> manifest: Error processing background.persistent: Event pages are not
> currently supported. This will run as a persistent background page.
> ```

> 
> Expected results:
> 
> No warning messages should be printed when `background.persistent` is set to
> `true`, as persistent background pages are fully supported.

I think we actually do need to generate a warning for background.persistent=true, but because it doesn't affect the extension's ability to run, it shouldn't come across as an error message. 
 
The message text should be changed to read: Reading manifest: Warning: Event pages are not currently supported. This will run as a persistent background page."

Comment 12

[Caitlin Neiman [:caitmuenster]]

If this is your first contribution to Firefox, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for how to get started. 

Please needinfo the bug's mentor if you have any questions.

Comment 13

[Mike Conca [:mconca]]

(In reply to Caitlin Neiman [:caitmuenster] from comment #11)
> The message text should be changed to read: Reading manifest: Warning: Event
> pages are not currently supported. This will run as a persistent background
> page.

I agree that 1) we should still surface a message until such time as we support event pages, and 2) changing this to a warning would make more sense.

Comment 15

[edward.i.wu]

Hi I am completely new to contributing, but would like to give this a try.

Comment 16

[edward.i.wu]

is there any extension with background persistence to use as a test? the one attached is unverified

Comment 17

[Caitlin Neiman [:caitmuenster]]

Hi Edward, and welcome! You can download the attached example as a test (it's ok if it's unverified - you can see the error message when you temporarily install it through about:debugging). 

Please feel welcome to needinfo :robwu if you need any help!

Comment 18

[edward.i.wu]

(In reply to Caitlin Neiman [:caitmuenster] from comment #17)
> Hi Edward, and welcome! You can download the attached example as a test
> (it's ok if it's unverified - you can see the error message when you
> temporarily install it through about:debugging). 
> 
> Please feel welcome to needinfo :robwu if you need any help!

Thanks Caitlin :D

Comment 19

[edward.i.wu]

Hi Rob,
In addition to the highlighted code sections above, I found the error and makeError functions of the context class of Schemas.jsm that print out the error text. However if the solution is the print a Warning(instead of "Error processing..."), where could that be done? Also do the discussions about adding a new deprecatedValue type still apply if we want a warning? Any tips on how to get started, or better understand the code would be very much appreciated.

Comment 20

[Rob Wu [:robwu]]

"Error processing ..." is auto-generated at [1], and its purpose is to show the source of the error/warning.

Since the message is non-fatal (and doesn't prevent the extension from loading), I suggest to not bother with introducing a distinction between warning and error.


The previously suggested "defaultValues" property is a bit limited, because there is no way to associate a deprecation message with a value.

So instead of introducing a new "defaultValues" property, I recommend re-using the existing "choices" property. The "choices" property in the schema can be used to define an union of types. We want to be able to define a boolean type with value true (without deprecation message) and another boolean type with value false (with the existing deprecation message).
Currently, there is no way to define a boolean type with a specific value, so this is where you need to introduce new logic to Schemas.jsm

The existing schema format already has a concept for limiting the number of values: String types can have an "enum" property to limit to a specific range of choices. So let's introduce this to the boolean type too, so that the definition of "persistent" (at [2]) can be changed to something like this:

      {
        "id": "PersistentBackgroundProperty",
        "choices": [
          {
            "type": "boolean",
            "enum": [true]
          },
          {
            "type": "boolean",
            "enum": [false],
            "deprecated": "Event pages are not currently supported. This will run as a persistent background page."
          }
        ]
      }

> Any tips on how to get started, or better understand the code would be very much appreciated.

If you haven't read it before, I suggest to start with reading https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Then, once you are somewhat familiar with the development process, use search for "enum" in Schemas.jsm to see how StringType supports enumerations. The actual implementation for booleans (to be added to the existing BooleanType class) is much simpler, and you can probably figure it out.

While you can try to re-compile Firefox and do manual testing, it is probably much faster to write a unit test and run that test to see if your code works as expected. So start off by changing Schemas.jsm and adding a unit test to toolkit/components/extensions/test/xpcshell/test_ext_schemas.js (this test should verify that "enum" is supported in BooleanType).

Once that works, you can update the schema for PersistentBackgroundProperty as shown above (at [2]), and add a new test to toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js to confirm that no message is displayed [4].

Once that works, the patch is ready for review (and just to be sure you can try to manually test your feature by loading the extension from comment 0 at about:debugging and confirming that no error message appears).

[1] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/Schemas.jsm#431
[2] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/schemas/manifest.json#579-583
[3] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
[4] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js#63

Comment 21

[edward.i.wu]

(In reply to Rob Wu [:robwu] from comment #20)
> "Error processing ..." is auto-generated at [1], and its purpose is to show
> the source of the error/warning.
> 
> Since the message is non-fatal (and doesn't prevent the extension from
> loading), I suggest to not bother with introducing a distinction between
> warning and error.
> 
> 
> The previously suggested "defaultValues" property is a bit limited, because
> there is no way to associate a deprecation message with a value.
> 
> So instead of introducing a new "defaultValues" property, I recommend
> re-using the existing "choices" property. The "choices" property in the
> schema can be used to define an union of types. We want to be able to define
> a boolean type with value true (without deprecation message) and another
> boolean type with value false (with the existing deprecation message).
> Currently, there is no way to define a boolean type with a specific value,
> so this is where you need to introduce new logic to Schemas.jsm
> 
> The existing schema format already has a concept for limiting the number of
> values: String types can have an "enum" property to limit to a specific
> range of choices. So let's introduce this to the boolean type too, so that
> the definition of "persistent" (at [2]) can be changed to something like
> this:
> 
>       {
>         "id": "PersistentBackgroundProperty",
>         "choices": [
>           {
>             "type": "boolean",
>             "enum": [true]
>           },
>           {
>             "type": "boolean",
>             "enum": [false],
>             "deprecated": "Event pages are not currently supported. This
> will run as a persistent background page."
>           }
>         ]
>       }
> 
> > Any tips on how to get started, or better understand the code would be very much appreciated.
> 
> If you haven't read it before, I suggest to start with reading
> https://wiki.mozilla.org/WebExtensions/Contribution_Onramp
> 
> Then, once you are somewhat familiar with the development process, use
> search for "enum" in Schemas.jsm to see how StringType supports
> enumerations. The actual implementation for booleans (to be added to the
> existing BooleanType class) is much simpler, and you can probably figure it
> out.
> 
> While you can try to re-compile Firefox and do manual testing, it is
> probably much faster to write a unit test and run that test to see if your
> code works as expected. So start off by changing Schemas.jsm and adding a
> unit test to toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
> (this test should verify that "enum" is supported in BooleanType).
> 
> Once that works, you can update the schema for PersistentBackgroundProperty
> as shown above (at [2]), and add a new test to
> toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js to
> confirm that no message is displayed [4].
> 
> Once that works, the patch is ready for review (and just to be sure you can
> try to manually test your feature by loading the extension from comment 0 at
> about:debugging and confirming that no error message appears).
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/
> Schemas.jsm#431
> [2]
> https://searchfox.org/mozilla-central/rev/
> 232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/
> schemas/manifest.json#579-583
> [3]
> https://searchfox.org/mozilla-central/rev/
> 232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/
> xpcshell/test_ext_schemas.js
> [4]
> https://searchfox.org/mozilla-central/rev/
> 232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/
> xpcshell/test_ext_eventpage_warning.js#63

Thank you Rob, I'll try to digest all this and let you know if I have any questions

Comment 22

[edward.i.wu]

I've made the changes to Schema.jsm and added the changes you wrote to manifest.json, which removes the warning when adding the example extension in about:debugging manually, but I am having trouble adding unit tests to text_ext_schemas.js. 

Trying to add just a equals test, similar to how they do at: 

https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js#522-524

but the json type is not processing the way I think it would. 


       let booleanEnumJson = [{
          namespace: "booleanEnum",

          types: [
            {
             "id": "enumTrue",
             "type": "boolean",
             "enum": [true],
            },
            {
             "id": "enumFalse",
             "type": "boolean",
             "enum": [false],
            },
        ],
      }];


using

 ok(root.booleanEnum, "namespace exists");

in an add_task function shows that the json is getting loaded as:

       {"enumTrue":"undefined","enumFalse":"undefined"}

The same happens if I remove enum, and if I add a default value of true. 
Is this the right way to approach the test? Which pieces am I missing?

Comment 23

[Rob Wu [:robwu]]

Could you put up your patch to Schemas.jsm? It's difficult to tell what's going on without seeing your changes.

Comment 24

[edward.i.wu]

These were the changes to the BooleanType class: 


class BooleanType extends Type {
  static get EXTRA_PROPERTIES() {
    return ["enum", ...super.EXTRA_PROPERTIES];
  }

  static parseSchema(root, schema, path, extraProperties = []) {
    this.checkSchemaProperties(schema, path, extraProperties);
    let enumeration = schema.enum || null;
    return new this(schema, enumeration);
  }

  constructor(schema, enumeration) {
    super(schema);
    this.enumeration = enumeration;
  }

  normalize(value, context) {
    return this.normalizeBase("boolean", value, context);
  }

  checkBaseType(baseType) {
    return baseType == "boolean";
  }

  checkDeprecated(context, value = null) {
    if (this.deprecated && this.enumeration && !this.enumeration[0]) {
      this.logDeprecation(context, value);
    }
  }
}

Comment 25

[Rob Wu [:robwu]]

Regarding comment 22: I think that you were expecting root.booleanEnum.enumTrue to be true, but it is actuall undefined. This is fine; we are merely using "enum" as a way to limit the range of values for a boolean. With StringType, you can see more involved logic in the getDescriptor method, but that only exists because the range of possible string values is large, so it makes sense to export the possible values of a (string) enumeration to the namespace.
A boolean type can only have two values, true and false. I don't think that it is necessary to have StringType's behavior for generating the descriptor (i.e. letting root.booleanEnum be {ENUMTRUE: true, ENUMFALSE: false}).
If you want to test the value at all, then I would verify that the value is undefined.

Your current change ensures that the enumeration (if any) is attached to the BooleanType, but it is not used anywhere.
As you can see at the existing StringType implementation [1], the `normalize` method is responsible for generating the expected value, and you need to add validation if needed.

To test that your code works, write a test where a function requires a parameter of the booleanEnum.enumTrue. The test should verify that calling the method with boolean true succeeds, and boolean false fails.


( The desired use case (from my comment 20) is using the boolean+enum in a "choices" array. In that case, the BooleanType's normalize method will be called by the ChoiceType's normalize method [2]. )


[1] https://searchfox.org/mozilla-central/rev/70a4778dd84ec94b4b3a2537fd28482de45b9a00/toolkit/components/extensions/Schemas.jsm#1470-1486
[2] https://searchfox.org/mozilla-central/rev/70a4778dd84ec94b4b3a2537fd28482de45b9a00/toolkit/components/extensions/Schemas.jsm#1333

Comment 26

[edward.i.wu]

Ah ok after rereading your comments, I think I understand now. Usually the value parameter in the BooleanType normalize function is undefined(because there is no way to define a boolean type with a specific value), and but if there exists an enum, we want to "transfer" the enum choice to the value variable in the normalize function and return either {value: true} or {value:false}?

Comment 27

[Rob Wu [:robwu]]

(In reply to edward.i.wu from comment #26)
> Ah ok after rereading your comments, I think I understand now. Usually the
> value parameter in the BooleanType normalize function is undefined(because
> there is no way to define a boolean type with a specific value), and but if
> there exists an enum, we want to "transfer" the enum choice to the value
> variable in the normalize function and return either {value: true} or
> {value:false}?

"normalize" doesn't "transfer" the enum choice; "normalize" uses "enum" while processing the input value: When an enum is present, normalize should only return the input value (return {value}) if it is in the enumeration, and otherwise return an error.

Comment 28

[edward.i.wu]

I've wrote normalize and have base tests that pass in text_ext_schemas.js that verify for the working case, and checks for an exact error of: 

      /Type error for parameter arg \(Invalid enumeration value false\) for booleanEnum\.paramMustBeTrue\./

in the error case. 

I will look at test_ext_eventpage_warning.js tomorrow, and add a choices test case as well.

Comment 29

[edward.i.wu]

Added cases in test_ext_eventpage_warning.js with persistent = true, and checking console messages it generates against an empty string. Now I am setting up phabricator and arcanist to submit the patch.

Comment 30

[edward.i.wu]

After pulling the latest code and merging I can't run 

./mach test toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js

without it skipping all the tests. It says MOZ_NODE_PATH env variable is not set, but after I set it in windows system variables as variable=MOZ_NODE_PATH and value=C:\Program Files\nodejs i get this other error: 


$ ./mach test toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js
 0:43.12 ERROR node not found at MOZ_NODE_PATH C:\Program Files\nodejs
Error running mach:

    ['test', 'toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

IOError: node not found at MOZ_NODE_PATH C:\Program Files\nodejs

  File "c:\mozilla-source\mozilla-central\testing/mach_commands.py", line 169, in test
    argv=extra_args, test_objects=tests, **kwargs)
  File "c:\mozilla-source\mozilla-central\python/mach\mach\registrar.py", line 138, in dispatch
    return self._run_command_handler(handler, context=context, **kwargs)
  File "c:\mozilla-source\mozilla-central\python/mach\mach\registrar.py", line 95, in _run_command_handler
    result = fn(**kwargs)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 260, in run_xpcshell_test
    return xpcshell.run_test(**params)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 60, in run_test
    return self.run_suite(**kwargs)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 46, in run_suite
    return self._run_xpcshell_harness(**kwargs)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 136, in _run_xpcshell_harness
    result = xpcshell.runTests(filtered_args)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell\runxpcshelltests.py", line 1322, in runTests
    self.trySetupNode()
  File "c:\mozilla-source\mozilla-central\testing/xpcshell\runxpcshelltests.py", line 1109, in trySetupNode
    raise IOError(error)

Comment 31

[Rob Wu [:robwu]]

MOZ_NODE_PATH is expected to be the path to the binary (.exe file), not the directory.

Comment 32

[edward.i.wu]

Ah thank you, now it finds node. However, the tests are still being skipped in test_ext_eventpage_warning.js, even when I revert to the remote code base, without any of my local changes. There's no error, it just says:

 0:06.65 TEST_START: xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js
 0:06.65 TEST_START: xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js
 0:06.65 TEST_END: SKIP
 0:06.65 TEST_END: SKIP

At the bottom summary, it states: 

Ran 2 checks (2 tests)
Expected results: 0
Skipped: 2 tests


How should I get these tests to run? Or is it ok to submit the patch with only test_ext_schemas.js running?

Comment 33

[Rob Wu [:robwu]]

"mach test" currently behaves unexpectedly because of bug 1498636.

Use "mach xpcshell-test" instead, to launch the tests.

(FWIW, I think that you didn't even have to set MOZ_NODE_PATH because the WebExtensions tests are not using it)

Comment 34

[edward.i.wu]

Attachment: Bug 1370077 - Avoid deprecation message when background.persistent is true r=robwu

prevent error msgs for background.persistent = true by allowing BooleanType in Schema.jsm to accept enums and PersistentBackgroundProperty in
manifest.json to contain 2 separate choices with a single possible enum each.

Comment 35

[edward.i.wu]

Thanks Rob, that let me run and fix up the eventpage warning tests to submit the patch above.

Comment 36

[edward.i.wu]

Hi Rob, updated the patch, let me know what you think.

Comment 37

[edward.i.wu]

Keywords: che

Comment 38

[edward.i.wu]

The last patch is the same as the approved one before, but with an automated test detecting an error msg that I added changed to match the edits made to the error message during review.

Comment 39

[Rob Wu [:robwu]]

Please wait for aswan's approval (r+) before adding checkin-needed.

As a mentor I am able to help you with guiding through the bug and the review, but all patches need to have be approved by a module peer (https://www.mozilla.org/en-US/about/governance/policies/module-owners) prior to landing, which I am not (but aswan is).

Comment 40

[edward.i.wu]

Ok great, Thank you Rob for your patience and all the help. It was interesting learning the submission process and how to read and decipher a large code base like the extensions, good experience I hope.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e676737b9689
Avoid deprecation message when background.persistent is true r=robwu,aswan

Comment 42

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/e676737b9689

Comment 43

[Caitlin Neiman [:caitmuenster]]

Thanks so much for the patch, Edward! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Comment 44

[Vlad Jiman]

Verified using the attached test extension and I've managed to reproduce the issue on the reported version 53 and 55. On Nightly v66.0a1 (20190116093310) the console does not show the described error. Tests were run using windows 10 x64 bit, attaching screenshot

Comment 45

[Vlad Jiman]

Attachment: verified.png

Comment 46

[edward.i.wu]

(In reply to Caitlin Neiman [:caitmuenster] from comment #43)

Thanks so much for the patch, Edward! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Thanks Caitlin! That'd be pretty cool, I've just created a mozillian account here:

https://mozillians.org/en-US/u/edward.i.wu/

Comment 47

[Caitlin Neiman [:caitmuenster]]

Woohoo! You're vouched. \o/ I look forward to seeing you around the project!

Comment 48

[edward.i.wu]

(In reply to Caitlin Neiman [:caitmuenster] from comment #47)

Woohoo! You're vouched. \o/ I look forward to seeing you around the project!\

Thanks!