Closed Bug 1193431 Opened 5 years ago Closed 4 years ago

Remove unused Contact API related code

Categories

(Firefox for Android :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1284283

People

(Reporter: jonalmeida, Unassigned, Mentored)

References

(Blocks 2 open bugs)

Details

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

Attachments

(1 file, 2 obsolete files)

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.
> 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)
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 :(
Bug 1193431 - Remove unused Contact API related code r?rnewman
Attachment #8647243 - Flags: review?(rnewman)
Assignee: nobody → jalmeida
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 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 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: → 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)
See Also: → 1041037
See Also: → 1199784
So yes, I just want to confirm that we need that for b2gdroid.
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)
Attachment #8647243 - Flags: review?(rnewman)
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.
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)
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.
Latest patch including changes from comments. Still need to fix the test failures (see comment 22).
Flags: needinfo?(jonalmeida942)
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
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.
Flags: needinfo?(margaret.leibovic)
Assignee: s.kaspari → nobody
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1284283
You need to log in before you can comment on or make changes to this bug.