Closed Bug 778608 Opened 12 years ago Closed 12 years ago

Move SafeBrowsing.jsm to toolkit


(Toolkit :: Safe Browsing, defect)

Not set



Firefox 19


(Reporter: Dolske, Assigned: Dolske)


(Blocks 2 open bugs)



(1 file, 4 obsolete files)

Attached 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?(
Note to self, after this we should probably move Bugzilla's Firefox::PhishingProtection component over to Toolkit.
Blocks: 778609
Blocks: 778611
Attached patch Patch v.2 (obsolete) — Splinter Review
Oops, forgot to fix a path, but it seems to be working fine now.
Attachment #647043 - Attachment is obsolete: true
Attachment #647043 - Flags: review?(
Attachment #647046 - Flags: review?(
Attached patch Patch v.3 (obsolete) — Splinter Review
Oops again, tiny fix to
Attachment #647046 - Attachment is obsolete: true
Attachment #647046 - Flags: review?(
Attachment #647066 - Flags: review?(
Attached patch Patch v.4 (obsolete) — Splinter Review
Fixed few other small mistakes.

Mostly-green in
Fixed the M(oth) problem:
Attachment #647066 - Attachment is obsolete: true
Attachment #647066 - Flags: review?(
Attachment #648582 - Flags: review?(
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?( → 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.
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. :(
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/
@@ +20,5 @@
> +# Normally the "client ID" sent in updates is, but for
> +# official Firefox releases from Mozilla we use a special identifier.
> +ifdef MOZ_PHOENIX

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 there's also issues with this define for Fennec.
Blocks: 477718
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
Blocks: 806757

Filed bug 806757 to investigate moving the tests.
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Blocks: 807559
Depends on: 809001
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.