684160 - Port some recent SyncUI changes to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Sync UI, defect)

Description

[Justin Wood (:Callek)]

Attachment: v1

SeaMonkey should port the changes from Firefox to our Sync code.

This does that.

Bug 625320 touched Firefox Sync code, but should not be ported until we also port Panorama, the implications of not porting that are purely cosmetic.

I am porting many changes and csets, see dep bugs here.

Patch not yet tested, but by code inspection this looks good. Was a 1:1 port.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 557765 [details] [diff] [review]
v1

I've been preparing this at home, too, but this way is OK, too, since it allows me to review it myself instead of having to ask Neil.

Before I go into the details (which I cannot do from here since I need my version of the patch for it), some first feedback just from looking at the patch:

* while we're changing the _obs list, we should finally sort it, i.e. put weave:notification:added at the start

* why the Weave.Status.ready/this.initUI change? I see FF does it similarly, but we're a bit different in this respect and must ensure not to regress bug 639970. If you want to keep this change, I'll have to think about it further and maybe consult Neil.

* fine: the _isLoggedIn method is unused now; the removal is also in my patch

* alltabsPopupShowing: in addition to copying the comment, I also found an if (!popup) return; check that we can safely copy (i.e. please do!)

* the rest I have to compare with my patch, but I guess we'll have the same there. Final review will depend on that and of course testing it.

Comment 2

[Philip Chee]

Drive-by comment.

> * alltabsPopupShowing: in addition to copying the comment, I also found an if
> (!popup) return; check that we can safely copy (i.e. please do!)

> @@ -93,15 +94,12 @@ let gSyncUI = {
>      // Find the alltabs-popup
>      let popup = document.getElementById("alltabs-popup");

Ah, our alltabs popup is in anonymous content. We don't yet have tabs in a customizable toolbar....

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 557765 [details] [diff] [review]
v1

Having checked against my patch, indeed most stuff is equal. I don't feel safe giving this r=me since I haven't checked the equal parts, so I'll do f=me with the following nits fixed instead and requesting code review from Neil. In any case, please do the following:

1. move "weave:notification:added" to the beginning of the _obs array

2. either leave the Weave.Status.ready stuff as-is or convince Neil that it has no side-effects nor regresses bug 639970. It would be interesting to know whether we suffer from bug 633681 at all, i.e. whether the change is needed given our somewhat different setup (I for one think we just solved it differently, and unlike FF, we still have "weave:notification:added"). Alternatively, move this investigation to its own bug.

3. add the popup check I was talking about, cf. <http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser-syncui.js#161>

4. remove the "title" parameter from the this.clearError call in onStartOver ("title" is undefined in that context. FF has it right. Copy & paste mistake; I made the same!)

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

BTW if anyone wonders what's about the Weave.Notifications.removeAll cases left, see bug 683254 comment 8. Personally I'd like to not do more than FF did and maybe add regressions before they do, but I'll leave that to Neil for the code review.

Comment 5

[Justin Wood (:Callek)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #1)
> Comment on attachment 557765 [details] [diff] [review]
> v1
> 
> I've been preparing this at home, too, but this way is OK, too, since it
> allows me to review it myself instead of having to ask Neil.
> 
> Before I go into the details (which I cannot do from here since I need my
> version of the patch for it), some first feedback just from looking at the
> patch:
> 
> * while we're changing the _obs list, we should finally sort it, i.e. put
> weave:notification:added at the start

done

> * why the Weave.Status.ready/this.initUI change? I see FF does it similarly,
> but we're a bit different in this respect and must ensure not to regress bug
> 639970. If you want to keep this change, I'll have to think about it further
> and maybe consult Neil.

Bug 633681 (part 2) is what did this on Firefox. We won't regress 639970 because if "weave:service:ready" was already fired Weave.status.ready will be true, and we init. Else we add the observer (this is not racy code here, afaict). I would feel more comfortable matching bug for bug in this part. This way also saves us an extra observer/function to be held in memory.

(I'm also partial to moving the tools-menu stuff to initUI() rather than init() but we shouldn't do that in this bug).

> * fine: the _isLoggedIn method is unused now; the removal is also in my patch

So did I. (initially) :-)

> * alltabsPopupShowing: in addition to copying the comment, I also found an
> if (!popup) return; check that we can safely copy (i.e. please do!)

done in Firefox in Bug 595725, done for us though.

(In reply to Philip Chee from comment #2)
> Drive-by comment.
> 
> > * alltabsPopupShowing: in addition to copying the comment, I also found an if
> > (!popup) return; check that we can safely copy (i.e. please do!)
>
> Ah, our alltabs popup is in anonymous content. We don't yet have tabs in a
> customizable toolbar....

Which means this *always* returns null for us, and means that we should file a followup on it. (but with my if (!popup) we at least fake some sanity here)

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #3)
> Comment on attachment 557765 [details] [diff] [review]
> v1
> 
> Having checked against my patch, indeed most stuff is equal. I don't feel
> safe giving this r=me since I haven't checked the equal parts, so I'll do
> f=me with the following nits fixed instead and requesting code review from
> Neil.

Re-Requesting review shortly, I'll also attach a -w (whitespace removed) patch to help sanity in reviewing. If you still feel you need to punt to review-overloaded Neil then we can do that again.

> In any case, please do the following:
> 
> 1. move "weave:notification:added" to the beginning of the _obs array

Done

> 2. either leave the Weave.Status.ready stuff as-is or convince Neil that it
> has no side-effects nor regresses bug 639970. It would be interesting to
> know whether we suffer from bug 633681 at all, i.e. whether the change is
> needed given our somewhat different setup (I for one think we just solved it
> differently, and unlike FF, we still have "weave:notification:added").
> Alternatively, move this investigation to its own bug.

I explained it above, but we can move "this investigation" out to another bug if you insist.

> 3. add the popup check I was talking about, cf.
> <http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/
> browser-syncui.js#161>

Done.

> 4. remove the "title" parameter from the this.clearError call in onStartOver
> ("title" is undefined in that context. FF has it right. Copy & paste
> mistake; I made the same!)

Good catch, done.

Comment 6

[Justin Wood (:Callek)]

Attachment: v2

Comment 7

[Justin Wood (:Callek)]

Attachment: v2 (-w)

Comment 8

[Justin Wood (:Callek)]

Attachment: v2

(somehow the v2 I attached earlier was from a different bug, but had this bug # in my MQ, sounds quite odd)

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 558033 [details] [diff] [review]
v2

r=me as-is. I ran with the patch (on Windows; somehow I cannot get Linux to compile currently, even with gcc 4.5) and there seem to be no regressions (especially tested the bug 639970 conditions).

All I got was the below at some point, but it's in back-end code, so not to worry for us right now:

this._log is undefined
resource://services-sync/policies.js
line 543 (function: syncAndReportErrors)

Once this lands, please remove this bug and all its dependencies that are fully contained in it from bug 615950, which is solely for bugs fixed in FF 4 that are left to be ported. Any new bugs should go to a new tracker if needed. The reason is that the list over there is checked while anything after FF 4 is not and probably needs more investigation.

Thanks for bringing us back in line!

Comment 10

[Justin Wood (:Callek)]

http://hg.mozilla.org/comm-central/rev/2e6c207c7857