920752 - [Contacts] Fix gmail import in Firefox Nightly

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Kevin Grandon :kgrandon]

While doing some perf investigation, I found it hard to debug parts of the contacts application in firefox nightly.

It should be fairly trivial to support oauth windows in the browser with some simple origin changes.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Will ask for reviews once bug 920742 lands.

Comment 2

[Francisco Jordano [:arcturus] [:francisco]]

Nice :)

This will help a lot

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 810128 [details]
Github pull request

Hi Francisco, now that dependent bugs have landed I am able to import gmail contacts with this patch in Firefox Nightly. If you could please take a look it would be appreciated. Thanks!

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: 2013-10-28-10-38-39.png

Hi Kevin,

sorry for the late review, I've been pretty busy past weeks.

I've tried your patch in both the browser (working perfectly), and the phone. In the case of the phone, despite of being the same code I got this output after importing.

Are you familiar with that?

Thanks!

Comment 5

[Kevin Grandon :kgrandon]

(In reply to Francisco Jordano [:arcturus] from comment #4)
> Created attachment 823356 [details]
> 2013-10-28-10-38-39.png
> 
> Hi Kevin,
> 
> sorry for the late review, I've been pretty busy past weeks.
> 
> I've tried your patch in both the browser (working perfectly), and the
> phone. In the case of the phone, despite of being the same code I got this
> output after importing.
> 
> Are you familiar with that?

I haven't seen this before, but I haven't tested this in a while. Perhaps it is some rebasing/idl change? I will test and let you know!

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 810128 [details]
Github pull request

Clearing review flag until I test this on a device again.

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Thanks a lot!

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 810128 [details]
Github pull request

Hi Francisco - 

I think this may have been happening due to all of the weird WebIDL changes. I've just rebased this branch on top of a new gecko and everything seems well. Could you give it another shot? Thanks!

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 810128 [details]
Github pull request

Thanks a lot for the work, and sorry for being late reviewing it.

Tried on the phone and the browser and working nice!

Thanks again Kevin!

Comment 10

[Kevin Grandon :kgrandon]

Thanks! Landed in master: https://github.com/mozilla-b2g/gaia/commit/43ceb6c0b0d1a500d0de956f60e9eb32879018fd

Comment 11

[Jose Manuel Cantera]

Reverted, the changes break fb synchronization which it is done from a Web Worker.

Particularly, fb_contact_utils.js. Please be careful with these kind of 'find-replace' changes, as it can break things easily.

https://github.com/mozilla-b2g/gaia/commit/87b955d576b7422befadb85208f9a612153c4d08

Comment 12

[Kevin Grandon :kgrandon]

(In reply to Jose M. Cantera from comment #11)
> Reverted, the changes break fb synchronization which it is done from a Web
> Worker.

Ok, thanks for looking out for this. I'm going to put this on hold until we have more facebook/gmail integration tests as we should catch these things in travis/tbpl I hope.

Comment 13

[Kevin Grandon :kgrandon]

Oops - I'll leave this open for now and block on a testing bug.

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

Hi Jose,

thanks for catching this, tried gmail and hotmail but not facebook.

Hopefully the integration tests will be a better fix.

Thanks guys.

Comment 16

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Updating with recent pull request. Worker fixes have not been addressed yet.

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8358102 [details] [review]
Github pull request

Hi Francisco, Jose - 

Opening this one up for review again. I haven't made any changes, as I went through the code and it seems like everything should be ok from a worker. I've verified that both Facebook and Gmail contacts work on a device. (Gmail contacts will work in the browser here, but there are still some things to do for Facebook). I would like to handle Facebook in a follow-up patch though.

Please let me know if you find anything else. Francisco - this is the same patch you R+'d before, but I'm opening it up for review just in case you notice anything else.

Jose - Please let me know if you still see the issue with fb_contact_utils.js, and any steps to reproduce it. Thanks!

Comment 18

[Jose Manuel Cantera]

Comment on attachment 8358102 [details] [review]
Github pull request

location.origin is returning undefined in Worker, thus r- . 

Note: Apart from that blocker I don't like these changes. They are introducing risk for free and not end user benefit, taking into account that with App Manager nowadays we are having good debug tools based on real devices.

Comment 19

[Kevin Grandon :kgrandon]

(In reply to Jose M. Cantera from comment #18)
> Comment on attachment 8358102 [details] [review]
> Github pull request
> 
> location.origin is returning undefined in Worker, thus r- . 

I re-opened this because developers are asking for this, and it helps us debug issues. Jose - can you give me STR where location.origin returns undefined? Location attributes should be readable from a worker, but not writable. This is documented on MDN, and works from me on a device. I tested this by flashing the app to a device, and syncing my facebook friends. Is there anything I am missing here?

Anyway, if you can give me steps to reproduce the issue you are seeing - I will more than happily debug, fix them, and add a test.

Comment 20

[Kevin Grandon :kgrandon]

(In reply to Jose M. Cantera from comment #18)
> Comment on attachment 8358102 [details] [review]
> Github pull request
>
> Note: Apart from that blocker I don't like these changes. They are
> introducing risk for free and not end user benefit, taking into account that
> with App Manager nowadays we are having good debug tools based on real
> devices.

Also note that not all third party developers have devices, and they want to contribute to the project. We should knock down these barriers so that they can contribute to Mozilla.

Comment 21

[Jose Manuel Cantera]

it is returning undefined. In order to test it you need to exercise the worker code and that's done when you import some friends and later click another time on 'update facebook friends'. Then find a log trace 'Worker up and running' to verify that the worker is in execution. You can log another trace in sync_worker.js to see the value of the location, but please test it on a device, not in the browser :) .

Comment 22

[Jose Manuel Cantera]

(In reply to Kevin Grandon :kgrandon from comment #20)
> (In reply to Jose M. Cantera from comment #18)
> > Comment on attachment 8358102 [details] [review]
> > Github pull request
> >
> > Note: Apart from that blocker I don't like these changes. They are
> > introducing risk for free and not end user benefit, taking into account that
> > with App Manager nowadays we are having good debug tools based on real
> > devices.
> 
> Also note that not all third party developers have devices, and they want to
> contribute to the project. We should knock down these barriers so that they
> can contribute to Mozilla.

Yes but not at the cost of introducing regression risk. Developers have other options like b2g desktop ...

Comment 23

[Kevin Grandon :kgrandon]

Comment on attachment 8358102 [details] [review]
Github pull request

(In reply to Jose M. Cantera from comment #22)
> (In reply to Kevin Grandon :kgrandon from comment #20)
> Yes but not at the cost of introducing regression risk. Developers have
> other options like b2g desktop ...

B2G desktop is being phased out for a more browser-like environment, which will be a build of Firefox Nightly. Our code should be flexible enough to handle either environment.

Thank you for the reproduction details, I will take a look and update the PR if necessary.

Comment 24

[Francisco Jordano [:arcturus] [:francisco]]

Thanks a lot Kevin for taking care of this!

Comment 25

[Kevin Grandon :kgrandon]

Just updating to say that I'm still working on this when I have time. It turns out that we have the location object, and every property is there, except for the origin. I think adding the origin property here is our best bet in making this flexible.

I've filed bug 964148 to track this.

Comment 26

[Kevin Grandon :kgrandon]

Comment on attachment 8358102 [details] [review]
Github pull request

Dependent gecko patch has landed, and no additional changes here have happened. Carrying R+ from arcturus.

Comment 27

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/c997ce172978b080cc375e7881fa51f3e9e1ad48

Comment 28

[Kevin Grandon :kgrandon]

Note: location.origin in workers is a new feature. You will need a gecko build within the last week or so to utilize this.