1193431 - Remove unused Contact API related code

Status: RESOLVED DUPLICATE of bug 1284283

(Firefox for Android Graveyard :: General, defect)

Description

[Jonathan Almeida [:jonalmeida]]

We have Contact API code that isn't being used for anything that can be removed.

Comment 1

[Reuben Morais [:reuben]]

We'll definitely use it for B2GDroid. Unless you mean Fennec front-end specific Contact API code.

Comment 2

[Jonathan Almeida [:jonalmeida]]

> We'll definitely use it for B2GDroid. Unless you mean Fennec front-end specific Contact API code.

Yes, fennec frontend.

I realized I'm not sure what this entirely encompasses. rnewman, can you recommend a good place to start with this?

Comment 3

[Richard Newman [:rnewman]]

IIRC, start with ContactService.java and see what else you can delete from there. 

If b2gdroid uses CS.java, it should move out of m/a/b, be excluded from the Fennec build, and someone should fix the concurrency and other bugs in the implementation. I seem to remember it being riddled with holes to the point of me wondering how it got r+ to land, but maybe I'm misremembering.

Comment 4

[Jonathan Almeida [:jonalmeida]]

So far I've only come across ContactSearch and some of picasso's Contact stuff (which I'm wondering if it already gets stripped by proguard or not).

Will start there then..

Comment 5

[Reuben Morais [:reuben]]

Yes, we use ContactService, and yes, I seem to remember the same :(

Comment 6

[Jonathan Almeida [:jonalmeida]]

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

Comment 7

[Jonathan Almeida [:jonalmeida]]

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

Comment 8

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1193431 - Remove unused Contact API related code r?rnewman

Bug 1193431 - Remove unused Contact API related code r?rnewman

Comment 9

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647243 [details]
MozReview Request: Bug 1193431 - Remove unused Contact API related code r?rnewman

Bug 1193431 - Remove unused Contact API related code r?rnewman

Comment 10

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647243 [details]
MozReview Request: Bug 1193431 - Remove unused Contact API related code r?rnewman

Bug 1193431 - Remove unused Contact API related code r?rnewman

Comment 11

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647243 [details]
MozReview Request: Bug 1193431 - Remove unused Contact API related code r?rnewman

Bug 1193431 - Remove unused Contact API related code r?rnewman

Comment 12

[Reuben Morais [:reuben]]

Don't forget to disable the API on the DOM side.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8647243 [details]
MozReview Request: Bug 1193431 - Remove unused Contact API related code r?rnewman

https://reviewboard.mozilla.org/r/15961/#review15125

::: mobile/android/installer/package-manifest.in:5
(Diff revision 1)
> -; Package file for the Fennec build. 
> +; Package file for the Fennec build.

I think you also need to remove ContactManager entries from the manifest:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in#299

Then verify that the DOM contacts API isn't present.

You might also need to change this:

https://dxr.mozilla.org/mozilla-central/source/dom/moz.build?offset=2700#56

to be a conditional include for != MOZ_WIDGET_ANDROID.

This looks fine to me once you verify that the DOM API fails correctly.

Comment 14

[Jonathan Almeida [:jonalmeida]]

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

Comment 15

[Jonathan Almeida [:jonalmeida]]

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

Comment 16

[[:fabrice] Fabrice Desré]

So yes, I just want to confirm that we need that for b2gdroid.

Comment 17

[Jonathan Almeida [:jonalmeida]]

NI from rnewman or nalexander on what should be done with this bug in that case.

Comment 18

[Richard Newman [:rnewman]]

See Comment 3.

We still want this gone from Fennec; it's dodgy code that we never want to ship.

Probably the sanest step forward is to go ahead with this patch (plus comments) and leave Bug 1196521 (thanks for filing, Reuben!) to implement a better version for b2gdroid. Whoever does that can use this old code for inspiration, but it shouldn't stick around in m/a/b in the mean time. Hooray for version control!

I assume that Fabrice and Nick have a good handle on how to add Java+JS code to b2gdroid that doesn't ship with Fennec, so this shouldn't be a big obstacle.

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #18)
> See Comment 3.
> 
> We still want this gone from Fennec; it's dodgy code that we never want to
> ship.
> 
> Probably the sanest step forward is to go ahead with this patch (plus
> comments) and leave Bug 1196521 (thanks for filing, Reuben!) to implement a
> better version for b2gdroid. Whoever does that can use this old code for
> inspiration, but it shouldn't stick around in m/a/b in the mean time. Hooray
> for version control!
> 
> I assume that Fabrice and Nick have a good handle on how to add Java+JS code
> to b2gdroid that doesn't ship with Fennec, so this shouldn't be a big
> obstacle.

For information radiation: there's no issue shipping new Java code in b2gdroid -- it's just like adding to m/a/b/moz.build.  We have no example of handling Gecko messages directly in b2gdroid.  I expect that b2gdroid can get a handle to a GeckoEventDispatcher in some way and listen directly.  (It's possible such a mechanism already exists, with a GeckoView wrapper around it.)  For JavaScript not shared with Fennec, b2gdroid ships a custom-built omni.ja with b2gdroid specific things.

Comment 20

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647243 [details]
MozReview Request: Bug 1193431 - Remove unused Contact API related code r?rnewman

Bug 1193431 - Remove unused Contact API related code r?rnewman

Removed all uses of the Contacts API in fennec, along with removing the
dom contacts API from packaging. This change correctly fails tests that
are contact related.

Will have to look into what needs to be done to stop the tests for failing.

Comment 21

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/15961/#review15125

> I think you also need to remove ContactManager entries from the manifest:
> 
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in#299
> 
> Then verify that the DOM contacts API isn't present.
> 
> You might also need to change this:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/moz.build?offset=2700#56
> 
> to be a conditional include for != MOZ_WIDGET_ANDROID.

Fixed all the comments and veried the tests fail now. I guess I need to figure out how to stop them from running those tests for fennec now..

Comment 22

[Richard Newman [:rnewman]]

Something like

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_bindings/XPConnect/xpcshell/Test_manifest_expressions

in the right test manifest file.

Comment 23

[Richard Newman [:rnewman]]

Jon, are you still going to be finishing this up?

Comment 24

[Jonathan Almeida [:jonalmeida]]

Still working on this, but I've been working slowly on bug 1095719. If someone wants to work on this immediately, I'll upload my last patch that can be used to continue.

Keeping NI for a self-reminder to update this bug.

Comment 25

[Jonathan Almeida [:jonalmeida]]

Attachment: contact_api_removal_1193431.patch

Latest patch including changes from comments. Still need to fix the test failures (see comment 22).

Comment 26

[Jonathan Almeida [:jonalmeida]]

Unassigning myself since I'm working on other things and I'm not actively working on this right now. Will pick it up later when I'm done elsewhere.

If anyone wants to continue this, definitely continue with the attached patch; it's almost done.

Comment 27

[Jonathan Almeida [:jonalmeida]]

There are strings in the browser.properties that can be removed as well: https://dxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties?case=true&from=%2Fmobile%2Fandroid%2Flocales%2Fen-US%2Fchrome%2Fbrowser.properties#133

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

We are working on supporting runtime permissions (Android 6) in bug 1212830 (meta). As this is the only code in need of the contacts permission (Nightly) there's a big interest in getting rid of this code in Fennec now.

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1193431 - Remove unused Contact API related code. r?

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

Comment 31

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/30197/#review26913

I see that browser.js still sends "Contact:Add":

https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#829

I found this by trying to understand if removing `dom/contacts` was enough to stop the contacts Web API from being offered to web content.  We should figure out if this does that, or what else needs to do that.

::: dom/moz.build:158
(Diff revision 1)
> +if not CONFIG['MOZ_WIDGET_ANDROID']:

Be sure to ni? fabrice or another b2gdroid person, since this will bite them.

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nick Alexander :nalexander from comment #31)
> https://reviewboard.mozilla.org/r/30197/#review26913
> 
> I see that browser.js still sends "Contact:Add":
> 
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#829

Thanks for the hint!

> ::: dom/moz.build:158
> (Diff revision 1)
> > +if not CONFIG['MOZ_WIDGET_ANDROID']:
> 
> Be sure to ni? fabrice or another b2gdroid person, since this will bite them.

Yeah, I want to at least fix the B2G build and then ping people in bug 1196521.

Comment 33

[Reuben Morais [:reuben]]

(In reply to Nick Alexander :nalexander from comment #31)
> https://reviewboard.mozilla.org/r/30197/#review26913
> 
> I see that browser.js still sends "Contact:Add":
> 
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#829

FWIW, that is unrelated to the DOM Contacts API.

Comment 34

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 35

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8705786 [details]
MozReview Request: Bug 1193431 - Remove unused Contact API related code. r?

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

Comment 36

[Sebastian Kaspari (:sebastian; :pocmo)]

What I still don't understand (and I can't get any meaningful stack traces): Why did test_bug707564.html break. It doesn't use the contacts API so I guess something broke that should not break.

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8705786 [details]
MozReview Request: Bug 1193431 - Remove unused Contact API related code. r?

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

Comment 38

[Sebastian Kaspari (:sebastian; :pocmo)]

I'm stuck here and can't find the cause why test_bug707564.html is failing here:
> var props = Object.getOwnPropertyNames(frames[0].navigator);

Error:
> NS_ERROR_FACTORY_NOT_REGISTERED

Looking at the navigator object with WebIDE it seems like it's broken and is missing properties: Not only mozContacts is missing but several others as well.

@margaret, nalexander: Do you have an idea what's happening here?

Comment 39

[Nick Alexander :nalexander [he/him]]

(In reply to Sebastian Kaspari (:sebastian) from comment #38)
> I'm stuck here and can't find the cause why test_bug707564.html is failing
> here:
> > var props = Object.getOwnPropertyNames(frames[0].navigator);
> 
> Error:
> > NS_ERROR_FACTORY_NOT_REGISTERED
> 
> Looking at the navigator object with WebIDE it seems like it's broken and is
> missing properties: Not only mozContacts is missing but several others as
> well.
> 
> @margaret, nalexander: Do you have an idea what's happening here?

My guess is that we need to remove make the Contacts.webidl conditional around https://dxr.mozilla.org/mozilla-central/source/dom/webidl/moz.build#896.  Unfortunately, modifying webidl in this manner doesn't work with an artifact build -- at least, I believe it does not.  I will investigate a little further.

Comment 40

[Sebastian Kaspari (:sebastian; :pocmo)]

Mh, I've been trying this today (full build) and this lead to compile errors and then I removed those and then I got new errors somewhere else... moving more and more away from the actual contacts code.

Comment 41

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #39)
> (In reply to Sebastian Kaspari (:sebastian) from comment #38)
> > I'm stuck here and can't find the cause why test_bug707564.html is failing
> > here:
> > > var props = Object.getOwnPropertyNames(frames[0].navigator);
> > 
> > Error:
> > > NS_ERROR_FACTORY_NOT_REGISTERED
> > 
> > Looking at the navigator object with WebIDE it seems like it's broken and is
> > missing properties: Not only mozContacts is missing but several others as
> > well.
> > 
> > @margaret, nalexander: Do you have an idea what's happening here?
> 
> My guess is that we need to remove make the Contacts.webidl conditional
> around
> https://dxr.mozilla.org/mozilla-central/source/dom/webidl/moz.build#896. 
> Unfortunately, modifying webidl in this manner doesn't work with an artifact
> build -- at least, I believe it does not.  I will investigate a little
> further.

This leads into a mess of moz-prefixed APIs that no Desktop or Fennec consumer should ever be exposed to: mozContacts, ICC, *STK.  fabrice in #mobile suggests these should all be guarded by MOZ_RIL or MOZ_B2G or similar, but of course they're not, so they're both bloating the platform part of the Fennec APK that we ship and preventing us getting rid of bad front-end code.

I've filed Bug 1240598 to try to make progress on the dom/ cruft.