1319571 - Change the output format nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to array

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Robin Steuber (she/her) [:bytesized]]

Currently, the output of nsUrlClassifierDBService::ClassifyLocalWithTables is converted from an array to a comma separated list. This is difficult to consume without converting it back to an array.

Comment 1

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1319571 - Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings

Review commit: https://reviewboard.mozilla.org/r/94830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94830/

Comment 2

[Robin Steuber (she/her) [:bytesized]]

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

Comment 3

[François Marier [:francois]]

Comment on attachment 8813438 [details]
Bug 1319571 - Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings

https://reviewboard.mozilla.org/r/94830/#review95126

Comment 4

[Robin Steuber (she/her) [:bytesized]]

It looks like there is another consumer of this function now. I need to change the patch to fix that consumer as well.

Comment 5

[Robin Steuber (she/her) [:bytesized]]

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

Comment 6

[Robin Steuber (she/her) [:bytesized]]

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

Comment 7

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8813438 [details]
Bug 1319571 - Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94830/diff/1-2/

Comment 8

[Robin Steuber (she/her) [:bytesized]]

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

Comment 9

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8813438 [details]
Bug 1319571 - Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94830/diff/2-3/

Comment 10

[François Marier [:francois]]

Comment on attachment 8813438 [details]
Bug 1319571 - Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings

https://reviewboard.mozilla.org/r/94830/#review96588

::: netwerk/base/nsIURIClassifier.idl:72
(Diff revision 3)
>     * containing the given tables. This does not make network requests.
> -   * The result is a comma-separated string of tables that match.
> +   * The result is an array of table names that match.
> +   */
> +  [noscript] StringArrayRef classifyLocalWithTables(in nsIURI aURI, in ACString aTables);
> +  /**
> +   * Same as above, but returns a comma separated list of table names.

Maybe we should add a comment saying that it's just for tests:

    * This is an internal interface used only for testing purposes.

Comment 11

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8813438 [details]
Bug 1319571 - Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94830/diff/3-4/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/673b0b18d599
Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings r=francois

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/673b0b18d599

Comment 14

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8813438 [details]
Bug 1319571 - Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings

Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Can't run the study as intended
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not for this feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected
[List of other uplifts needed for the feature/fix]: Bug 1318768, Bug 1323220, Bug 1325255, Bug 1322204, Bug 1325651, Bug 1319571, Bug 1321377, Bug 1307604, Bug 1323064, Bug 1335549, Bug 1333303, Bug 1333483, Bug 1336714, Bug 1338287
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: Changes an function definition with very few consumers.
[String changes made/needed]: none

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8813438 [details]
Bug 1319571 - Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings

It's been in FF53 already. Aurora53-.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8813438 [details]
Bug 1319571 - Change the output format of nsUrlClassifierDBService::ClassifyLocalWithTables from comma separated string to an array of strings

this was deemed too risky for beta