571409 - Add Account Manager support to Firefox

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Account Manager, defect)

Description

[Dan Mills [:thunder]]

Tracking bug for adding Account Manager to Firefox.

Comment 1

[Dan Mills [:thunder]]

Requesting blocking for Firefox 4.

Comment 2

[Paul [pwd]]

Final or are you hoping to get this into one of the betas Dan?

Comment 3

[d]

With such a large new feature like the Account Manager, it should probably go through at least one beta release before the RCs and final.

Comment 4

[Dan Mills [:thunder]]

We are currently targeting the end of August for landing the final bits, though it will be in multiple patches so some features (e.g. sign in/out) should land earlier.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

We can approve patches, but at this time I'm not comfortable blocking on the feature based on schedule. That should not be taken as a statement about not wanting the feature, just about keeping focus on issues that would actually block the release at this time.

We can renominate when we have a better understanding of how close we are on the specific bugs being used to track the Account Manager feature.

Comment 6

[Ed Lee :Mardak]

Attachment: super diff -U8 40686b8e0b04

Initial implementation that has UI/functionality to sign in for saved accounts and sign out.

Comment 7

[Ed Lee :Mardak]

Attachment: startup glue 8e574050bd89

Comment 8

[Ed Lee :Mardak]

Attachment: ui bits 8e574050bd89

Comment 9

[Ed Lee :Mardak]

Attachment: user/pass form profile 8e574050bd89

Comment 10

[Ed Lee :Mardak]

Attachment: helper classes 8e574050bd89

Comment 11

[Ed Lee :Mardak]

Attachment: core service 8e574050bd89

Comment 12

[Ed Lee :Mardak]

Attachment: header parser + generated 8e574050bd89

Comment 13

[Ed Lee :Mardak]

Attachment: mochitest 8e574050bd89

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Awesome to see this pulling together. Have you guys got a tryserver run that you can run through compare-talos to get us some early idea of performance impact?

Comment 15

[Ed Lee :Mardak]

Gavin: For the service files it may be useful to check in order:

1) helper classes
2) core service

Then the ui bits for firefox integration and profile-specific:

3) ui bits
4) user/pass form profile

Then various other patches can be somewhat individually looked at:

5) startup glue
6) header parser + generated
7) mochitest

Comment 16

[Ed Lee :Mardak]

Attachment: helper b2ce53f22083

Comment 17

[Ed Lee :Mardak]

Attachment: service b2ce53f22083

Comment 18

[Ed Lee :Mardak]

Attachment: ui b2ce53f22083

Comment 19

[Ed Lee :Mardak]

Attachment: profile b2ce53f22083

Comment 20

[Ed Lee :Mardak]

Attachment: startup b2ce53f22083

Comment 21

[Ed Lee :Mardak]

Attachment: parser b2ce53f22083

Comment 22

[Ed Lee :Mardak]

Attachment: test b2ce53f22083

Comment 23

[Ed Lee :Mardak]

I pushed several runs of Ts to maple with some baseline of the pre-merged changeset:

(platform: 2 base runs, 4 merged runs, %overhead)

linux:   b456 b455 472 464 472 483 3.7%
linux64: b429 b419 434 434 441 431 2.6%
os x:    b660 b663 660 672 672 658 0.6%
os x64:  b715 b787 793 806 801 799 1.6%
winxp:   b348 b341 359 361 360 359 4.4%
win7:    b454 b448 450 461 460 464 1.7%

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we know what's consuming so much time on startup on XP and Linux? Or do we at least have an idea?

Comment 25

[Dan Mills [:thunder]]

I don't have a specific answer, but we do have some juicy targets for Ts speedup.  There are a couple of things we do right now that we shouldn't, and they possibly affect Linux and XP more than the rest - I don't know the platform differences well enough to say.

Comment 26

[Ed Lee :Mardak]

Attachment: helper c8672a625590

Comment 27

[Ed Lee :Mardak]

Attachment: service c8672a625590

Comment 28

[Ed Lee :Mardak]

Attachment: ui c8672a625590

Comment 29

[Ed Lee :Mardak]

Attachment: profile c8672a625590

Comment 30

[Ed Lee :Mardak]

Attachment: makefile c8672a625590

Comment 31

[Ed Lee :Mardak]

Attachment: parser c8672a625590

Comment 32

[Ed Lee :Mardak]

Attachment: test c8672a625590

Comment 33

[Ed Lee :Mardak]

I reran the Ts tests (5 times) with the removal of the xpcom component and moved startup to the browser delayed startup:

linux: 2.1%
lin64: 2.4%
macos: 0.5%
mac64: 2.2%
winxp: 2.9%
win 7: 0.3%

Comment 34

[Ed Lee :Mardak]

Attachment: helper af9c96341d4c

Comment 35

[Ed Lee :Mardak]

Attachment: service af9c96341d4c

Comment 36

[Ed Lee :Mardak]

Attachment: ui af9c96341d4c

Comment 37

[Ed Lee :Mardak]

Attachment: profile af9c96341d4c

Comment 38

[Ed Lee :Mardak]

Attachment: makefile af9c96341d4c

Comment 39

[Ed Lee :Mardak]

Attachment: parser af9c96341d4c

Comment 40

[Ed Lee :Mardak]

Attachment: test af9c96341d4c

Comment 41

[Ed Lee :Mardak]

We simplified the ui a bit to get rid of the inline svg/clip-path and reran Ts:

From 2 base and 6 test runs (5 test runs for windows as it's still building..):

linux: 1.7%
lin64: 2.4%
macos: 0.2%
mac64: 2.0%
winxp: 1.3%
win 7: 0.5%

That's an average of 1.35% across all 6 platforms, but we're probably hitting noise on some of the platforms, so that might not be totally accurate.

Comment 42

[Ed Lee :Mardak]

Attachment: helper f47972d05473

Comment 43

[Ed Lee :Mardak]

Attachment: service f47972d05473

Comment 44

[Ed Lee :Mardak]

Attachment: ui f47972d05473

Comment 45

[Ed Lee :Mardak]

Attachment: profile f47972d05473

Comment 46

[Ed Lee :Mardak]

Attachment: makefile f47972d05473

Comment 47

[Ed Lee :Mardak]

Attachment: parser f47972d05473

Comment 48

[Ed Lee :Mardak]

Attachment: test f47972d05473

Comment 49

[:Margaret Leibovic]

Comment on attachment 471259 [details] [diff] [review]
ui f47972d05473

The popup notifications bit looks like it makes sense to me, although I'm not an expert :) However, for css rules using the popup notification id, you should be aware of bug 591026, which appends "-notification" to the id you supply when building the notification.

Also, this patch may eventually cause a conflict with bug 567814 and bug 588309, which are moving save password behavior to popup notifications.

Comment 50

[(no longer active)]

Comment on attachment 471263 [details] [diff] [review]
test f47972d05473

I took a look at the sjs files and the tests in general.  They all looked sane to me, but I don't know anything about how the account manager works, so this feedback should be taken with a grain of salt.

Comment 51

[Philipp von Weitershausen [:philikon]]

Comment on attachment 471261 [details] [diff] [review]
makefile f47972d05473

Nit: license header copied w/o updating the info ;)

>+DIRS = \
>+  browser
>+  $(NULL)

You're missing browser/Makefile.in. Looks good otherwise.

Comment 52

[:Margaret Leibovic]

Comment on attachment 471259 [details] [diff] [review]
ui f47972d05473

Oops, missed the feedback flag!

Comment 53

[Shawn Wilsher :sdwilsh]

I talked with thunder today, and this is going to be changing.  With that being said, whoever ends up actually implementing this should chat with me about approaches since I'm *very* likely to be on the hook for reviews.

Comment 54

[Dan Mills [:thunder]]

We're changing direction and putting the previous Account Manager protocol on hold for a while. Closing these bugs to reflect reality.