Streamline the format of "handlers.json", align the implementation, and reorganize tests

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
This bug streamlines the JSON file format used as the new back-end of nsIHandlerService, aligns the behavior of the implementation with the RDF back-end, and improves testing.
(Assignee)

Comment 1

2 years ago
This will also fix the issue mentioned in bug 1287658 comment 24.
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
Still have to run tests on Linux and Android. This is a tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bed37d1c0d22c5e5c5c9783fbe5ff9614461afa

Comment 4

2 years ago
mozreview-review
Comment on attachment 8857156 [details]
Bug 1355585 - Streamline the format of "handlers.json", align the implementation, and reorganize tests.

https://reviewboard.mozilla.org/r/129100/#review132494

Looks like you have some failures on Try.

I quickly skimmed through the test, it looks good, but would require too much time to go through it line by line. If you need deeper inspection please let me know.

::: uriloader/exthandler/nsHandlerService-json.js:174
(Diff revision 1)
>      return handlers.enumerate();
>    },
>  
>    // nsIHandlerService
>    store(handlerInfo) {
> -    let handlerObj = {
> +    let type = handlerInfo.type;

where is this used?

::: uriloader/exthandler/nsHandlerService-json.js:191
(Diff revision 1)
> +      storedHandlerInfo.action = handlerInfo.preferredAction;
> +    } else {
> +      storedHandlerInfo.action = Ci.nsIHandlerInfo.useHelperApp;
> +    }
> +
> +    if (handlerInfo.alwaysAskBeforeHandling) {

please add some additional inline comments in store().
While what's up can be extracted from the code, it's a complex task to figure out the whole thing. Maybe even just a brief sum up of what's expected and how we proceed would help

::: uriloader/exthandler/nsHandlerService-json.js:255
(Diff revision 1)
> -    }
>  
> -    let preferHandler = null;
> -    if (storedHandlerInfo.preferredHandler) {
> -      preferHandler = this.handlerAppFromSerializable(storedHandlerInfo.preferredHandler);
> +    if (storedHandlerInfo.handlers) {
> +      let preferredHandlerIsNext = true;
> +      for (let handler of storedHandlerInfo.handlers) {
> +        let handlerApp = handler && this.handlerAppFromSerializable(handler);

let handlerApp = this.handlerAppFromSerializable(handler || {});

::: uriloader/exthandler/nsHandlerService-json.js:258
(Diff revision 1)
> -    if (storedHandlerInfo.preferredHandler) {
> -      preferHandler = this.handlerAppFromSerializable(storedHandlerInfo.preferredHandler);
> +      let preferredHandlerIsNext = true;
> +      for (let handler of storedHandlerInfo.handlers) {
> +        let handlerApp = handler && this.handlerAppFromSerializable(handler);
> +        if (preferredHandlerIsNext) {
> +          preferredHandlerIsNext = false;
> +          handlerInfo.preferredApplicationHandler = handlerApp;

Alternatively, instead of using a bool, you could just extract the first element:
handlerInfo.preferredApplicationHandler =
  this.handlerAppFromSerializable(storedHandlerInfo.handlers.shift() || {});

::: uriloader/exthandler/tests/HandlerServiceTestUtils.jsm:100
(Diff revision 1)
>        // that may have been imported from the default nsIHandlerService instance
>        // and is not overwritten by fillHandlerInfo later.
>        let handlerInfo = gMIMEService.getFromTypeAndExtension(type, null);
> +      while (handlerInfo.possibleApplicationHandlers.length) {
> +        handlerInfo.possibleApplicationHandlers.removeElementAt(0);
> +      }

.clear() ?

Comment 5

2 years ago
mozreview-review
Comment on attachment 8857156 [details]
Bug 1355585 - Streamline the format of "handlers.json", align the implementation, and reorganize tests.

https://reviewboard.mozilla.org/r/129100/#review132926

Just for the failures and because clearing reviews in mozreview sometimes stuck the review request into cleared state.
Attachment #8857156 - Flags: review?(mak77) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Depends on: 986975
(Assignee)

Comment 7

2 years ago
I figured out the failures on all platforms. On Android the tests revealed bug 986975 for which we crashed when comparing handler applications of different types, and there was also a bug in the RDF implementation for which we didn't delete the default handler. Here is an updated try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2eb0503e5aa7c56b3c301ab91a14b010ba62514

Comment 8

a year ago
mozreview-review
Comment on attachment 8857156 [details]
Bug 1355585 - Streamline the format of "handlers.json", align the implementation, and reorganize tests.

https://reviewboard.mozilla.org/r/129100/#review134936
Attachment #8857156 - Flags: review?(mak77) → review+

Comment 9

a year ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf5a28b1873
Streamline the format of "handlers.json", align the implementation, and reorganize tests. r=mak
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=93684714&repo=mozilla-inbound
Flags: needinfo?(paolo.mozmail)
(Assignee)

Updated

a year ago
Flags: needinfo?(paolo.mozmail)

Comment 12

a year ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84ef33cfa71a
Streamline the format of "handlers.json", align the implementation, and reorganize tests. r=mak

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84ef33cfa71a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.