Closed Bug 1319571 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- affected
firefox53 --- fixed

People

(Reporter: bytesized, Assigned: bytesized)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → ksteuber
Attachment #8813438 - Flags: review?(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
Attachment #8813438 - Flags: review?(francois) → review+
It looks like there is another consumer of this function now. I need to change the patch to fix that consumer as well.
Keywords: checkin-needed
Attachment #8813438 - Flags: review+ → review?(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.
Attachment #8813438 - Flags: review?(francois) → review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/673b0b18d599
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Attachment #8813438 - Flags: approval-mozilla-beta?
Attachment #8813438 - Flags: approval-mozilla-aurora?
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-.
Attachment #8813438 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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
Attachment #8813438 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.