Closed Bug 807559 Opened 7 years ago Closed 7 years ago

Remove mobile's SafeBrowsing.jsm and use the toolkit version

Categories

(Firefox for Android :: General, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 826992

People

(Reporter: Dolske, Unassigned)

References

Details

Attachments

(1 file)

Bug 778608 is fixed now, mobile should use that code. This should be fairly trivial; just some build goo and the small detail of the 1 line difference between these two JSMs.

I think this works, but is entirely untested. :)

Wasn't sure what to do with mobile/android/installer/package-manifest.in -- is that line still needed? Do I need to worry about removing the current JSM on device?
Attached patch Patch v.1Splinter Review
Attachment #677264 - Flags: review?(mark.finkle)
(In reply to Justin Dolske [:Dolske] from comment #0)

> Wasn't sure what to do with mobile/android/installer/package-manifest.in --
> is that line still needed? Do I need to worry about removing the current JSM
> on device?

We still need to add SafeBrowsing.jsm via package-manifest.in, but you don't really need to worry about removing the old one. We don't do partial updates.
Comment on attachment 677264 [details] [diff] [review]
Patch v.1

Adding GCP since he was knee-deep in this code recently.
Attachment #677264 - Flags: review?(gpascutto)
Comment on attachment 677264 [details] [diff] [review]
Patch v.1

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

Seems to work after fixing, but note the remarks. Be sure to test the final result.

::: mobile/android/chrome/content/browser.js
@@ +37,5 @@
>  
>  #ifdef MOZ_SAFE_BROWSING
>  XPCOMUtils.defineLazyGetter(this, "SafeBrowsing", function() {
>    let tmp = {};
> + Cu.import("resource:///modules/SafeBrowsing.jsm", tmp);

Doesn't bug 809001 imply this is wrong? After testing, it indeed has to be Cu.import("resource://gre/modules/SafeBrowsing.jsm", tmp);

(Watch out for that triple / you got there when editing, tripped me up because I didn't notice)

::: mobile/android/installer/package-manifest.in
@@ +524,5 @@
>  @BINPATH@/components/PromptService.js
>  @BINPATH@/components/SessionStore.js
>  @BINPATH@/components/Sidebar.js
>  #ifdef MOZ_SAFE_BROWSING
> +# ???

??? !!!
Attachment #677264 - Flags: review?(gpascutto) → review+
Comment on attachment 677264 [details] [diff] [review]
Patch v.1


>diff --git a/mobile/android/installer/package-manifest.in b/mobile/android/installer/package-manifest.in

> #ifdef MOZ_SAFE_BROWSING
>+# ???
> @BINPATH@/components/SafeBrowsing.jsm
> #endif

No changes needed here

r+ (with GCP's comments too)
Attachment #677264 - Flags: review?(mark.finkle) → review+
> - Cu.import("resource://gre/modules/SafeBrowsing.jsm", tmp);
> + Cu.import("resource:///modules/SafeBrowsing.jsm", tmp);
This change is wrong.
This ended up being done in bug 826992.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 826992
You need to log in before you can comment on or make changes to this bug.