Move SafeBrowsing.jsm to toolkit

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

7 years ago
Posted patch Patch v.1 (obsolete) — Splinter Review
Other apps (mobile, b2g, webrt) will likely want to be using the code in SafeBrowsing.jsm. So let's move it to Toolkit. This leaves very little stuff in in browser/components/safebrowsing, so I've folded it into browser/base/content.

This is on top of the patch in bug 778606, and is not yet tested. ;-)
Attachment #647043 - Flags: review?(gavin.sharp)
Assignee

Comment 1

7 years ago
Note to self, after this we should probably move Bugzilla's Firefox::PhishingProtection component over to Toolkit.
Assignee

Updated

7 years ago
Blocks: 778609
Assignee

Updated

7 years ago
Blocks: 778611
Assignee

Comment 2

7 years ago
Posted patch Patch v.2 (obsolete) — Splinter Review
Oops, forgot to fix a jar.mn path, but it seems to be working fine now.
Attachment #647043 - Attachment is obsolete: true
Attachment #647043 - Flags: review?(gavin.sharp)
Attachment #647046 - Flags: review?(gavin.sharp)
Assignee

Comment 3

7 years ago
Posted patch Patch v.3 (obsolete) — Splinter Review
Oops again, tiny fix to makefiles.sh.
Attachment #647046 - Attachment is obsolete: true
Attachment #647046 - Flags: review?(gavin.sharp)
Attachment #647066 - Flags: review?(gavin.sharp)
Assignee

Comment 4

7 years ago
Posted patch Patch v.4 (obsolete) — Splinter Review
Fixed few other small mistakes.

Mostly-green in https://tbpl.mozilla.org/?tree=Try&rev=dbacbd4d62f4
Fixed the M(oth) problem: https://tbpl.mozilla.org/?tree=Try&rev=028530f8507a
Attachment #647066 - Attachment is obsolete: true
Attachment #647066 - Flags: review?(gavin.sharp)
Attachment #648582 - Flags: review?(gavin.sharp)
Assignee

Updated

7 years ago
Attachment #648582 - Attachment is patch: true
Comment on attachment 648582 [details] [diff] [review]
Patch v.4

It really pains me to add more tests to browser/base, particularly well-grouped tests that could live somewhere else - a browser chrome run of browser-base takes way too long already. I guess I don't have a better suggestion at the moment, though :/
Attachment #648582 - Flags: review?(gavin.sharp) → review+
This patch caused "Timed out while waiting for: test #6 succesful finish" failures in browser_webconsole_bug_595934_message_categories.js on win debug; exactly like in your second try push.
Assignee

Comment 9

7 years ago
Exciting. I can't reproduce on a local win-debug build, nor is it at all obvious how that test could possibly be affected by this patch!

Time to debug via Try, I guess. :(
Assignee

Comment 10

7 years ago
The test (referenced in comment 7) is loading an invalid XML page, and watching for "no element found" to be logged in the error console via a console observer. So far it appears that the test is successfully loading the page, but the parsing goop simply isn't logging this message. So, still baffled as to what's happening.
Comment on attachment 648582 [details] [diff] [review]
Patch v.4

Review of attachment 648582 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/Makefile.in
@@ +20,5 @@
> +# Normally the "client ID" sent in updates is appinfo.name, but for
> +# official Firefox releases from Mozilla we use a special identifier.
> +ifdef MOZILLA_OFFICIAL
> +ifdef MOZ_PHOENIX
> +DEFINES += -DUSE_HISTORIC_SAFEBROWSING_ID=1

Could we avoid the MOZ_PHOENIX, or maybe move it to a branding prefs file? (probably as a separate bug) ... it tends to be a pain when we have app specific ifdefs in core (e.g. for xulrunner style builds etc). It'd also avoid the need for preprocessing the file.
As pointed out here https://bugzilla.mozilla.org/show_bug.cgi?id=741808#c4 there's also issues with this define for Fennec.

Updated

7 years ago
Blocks: 477718
Assignee

Comment 13

7 years ago
A while back the thought occurred to me that the bizarre test failures this triggered before were likely just test timing issues. I trimmed down the patch to leave the tests in-place, and that's passed a few try runs at different times.

I keep forgetting to land it, though, so I'll do so now. Will file followup to move tests and finish debugging (if it's still reproducible).
Attachment #648582 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Blocks: 806757
Assignee

Comment 14

7 years ago
Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/70d9ba6b84f8

Filed bug 806757 to investigate moving the tests.
https://hg.mozilla.org/mozilla-central/rev/70d9ba6b84f8
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Assignee

Updated

7 years ago
Blocks: 807559

Updated

7 years ago
Depends on: 809001
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.