Remove unused Contact API related code

RESOLVED DUPLICATE of bug 1284283

Status

()

RESOLVED DUPLICATE of bug 1284283
3 years ago
2 years ago

People

(Reporter: jonalmeida, Unassigned, Mentored)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=java][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
We have Contact API code that isn't being used for anything that can be removed.
We'll definitely use it for B2GDroid. Unless you mean Fennec front-end specific Contact API code.
(Reporter)

Comment 2

3 years ago
> 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?
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
(Reporter)

Comment 4

3 years ago
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..
Yes, we use ContactService, and yes, I seem to remember the same :(
(Reporter)

Comment 8

3 years ago
Created 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
Attachment #8647243 - Flags: review?(rnewman)
(Reporter)

Updated

3 years ago
Assignee: nobody → jalmeida
(Reporter)

Comment 9

3 years ago
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
(Reporter)

Comment 10

3 years ago
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
(Reporter)

Comment 11

3 years ago
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
Don't forget to disable the API on the DOM side.
See Also: → bug 1196521
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.
Attachment #8647243 - Flags: review?(rnewman)
(Reporter)

Updated

3 years ago
See Also: → bug 1041037
(Reporter)

Updated

3 years ago
See Also: → bug 1199784
So yes, I just want to confirm that we need that for b2gdroid.
(Reporter)

Comment 17

3 years ago
NI from rnewman or nalexander on what should be done with this bug in that case.
Flags: needinfo?(rnewman)
Flags: needinfo?(nalexander)
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.
Flags: needinfo?(rnewman)
(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.
Flags: needinfo?(nalexander)
(Reporter)

Updated

3 years ago
Attachment #8647243 - Flags: review?(rnewman)
(Reporter)

Comment 20

3 years ago
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.
(Reporter)

Comment 21

3 years ago
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..
Jon, are you still going to be finishing this up?
Flags: needinfo?(jonalmeida942)
(Reporter)

Comment 24

3 years ago
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.
(Reporter)

Comment 25

3 years ago
Created attachment 8660207 [details] [diff] [review]
contact_api_removal_1193431.patch

Latest patch including changes from comments. Still need to fix the test failures (see comment 22).
Flags: needinfo?(jonalmeida942)
(Reporter)

Comment 26

3 years ago
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.
Assignee: jonalmeida942 → nobody
Status: ASSIGNED → NEW
Mentor: rnewman
Whiteboard: [lang=java][lang=js]
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.
Assignee: nobody → s.kaspari
Attachment #8647243 - Attachment is obsolete: true
Attachment #8647243 - Flags: review?(rnewman)
Attachment #8660207 - Attachment is obsolete: true
Created attachment 8705786 [details]
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/
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.
(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.
(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 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/
Attachment #8705786 - Attachment description: MozReview Request: (WIP) Bug 1193431 - Remove unused Contact API related code. r? → MozReview Request: Bug 1193431 - Remove unused Contact API related code. r?
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 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/
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?
Flags: needinfo?(nalexander)
Flags: needinfo?(margaret.leibovic)
(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.
Flags: needinfo?(nalexander)
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.
(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.

Updated

3 years ago
Flags: needinfo?(margaret.leibovic)
Assignee: s.kaspari → nobody
Blocks: 1239284
Depends on: 1284283
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1284283
You need to log in before you can comment on or make changes to this bug.