691232 - Improve about:sync-tabs

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

The existing page synchronously calls sync() on the tab engine, amongst other malarky. Instead, it should bump the tab engine score and allow the SyncScheduler to take care of business, watching for tab sync to complete before redrawing.

Note that this change will cause a whole sync to occur, rather than just the clients then tabs engine... but tabs syncs first anyway, so who cares?

The only remaining issue is that we won't prompt for master password on display of the page, but that's an existing problem.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

This has received only cursory testing, so that's next on my list.

Comment 2

[Richard Newman [:rnewman]]

I have verified with two profiles on the same build: once the 'generator' profile has synced, either reloading about:sync-tabs, or initiating a sync, will cause the page to redraw the tab list with the current tabs from the other device.

Removing active tabs from the other device also works fine.

No console errors!* \o/

* Low expectations.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 564114 [details] [diff] [review]
Proposed patch. v1

Yay for really old review requests ... Sorry about forgetting this on. Do you still want me to look this over?

Comment 4

[Richard Newman [:rnewman]]

If you've got the time, yes please!

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 564114 [details] [diff] [review]
Proposed patch. v1

Review of attachment 564114 [details] [diff] [review]:
-----------------------------------------------------------------

Despite the comment about triggering a sync, I /think/ the behavior is the same, so r+.

Please test on the current tree before landing.

::: browser/base/content/aboutSyncTabs.js
@@ +238,3 @@
>      switch (topic) {
>        case "weave:service:login:finish":
> +        this.refetchTabs();

So, this effectively triggers a sync on login. Don't we do that already? I would think this wouldn't be needed. I'm just trying to think what use case this would handle.

::: browser/base/content/browser-places.js
@@ +673,5 @@
>      }
>  
>      // The tabs engine might never be inited (if services.sync.registerEngines
>      // is modified), so make sure we avoid undefined errors.
> +    let enabled = Weave.Engines.get("tabs") &&

This seems like an unrelated change.

Comment 6

[Morpheus3k]

Is there anything that keeps this from landing?

Comment 7

[Richard Newman [:rnewman]]

(In reply to Morpheus3k from comment #6)
> Is there anything that keeps this from landing?

Just an utter lack of time on my part. If you've got the free time to test an un-bitrotted patch against a current tree, that would help.

Comment 8

[Gregory Szorc [:gps]]

Morpheus3k: can you submit a new patch against the current tree? I promise to give it an adequate review this time around.

Comment 9

[Morpheus3k]

I'm sorry, but I haven't done any development on the Mozilla code yet. I've just come across this bug and noticed that it got the r+, but was not checked-in.

Comment 10

[Gregory Szorc [:gps]]

(In reply to Morpheus3k from comment #9)
> I'm sorry, but I haven't done any development on the Mozilla code yet. I've
> just come across this bug and noticed that it got the r+, but was not
> checked-in.

Oops. I didn't notice that rnewman was the patch author! Since he just left on vacation, I guess I'll take this...

Comment 11

[Richard Newman [:rnewman]]

Happy to rubber stamp for you, gps.

Comment 12

[Richard Newman [:rnewman]]

I won't have time to work on this.

Comment 13

[Chris Karlof [:ckarlof]]

about:sync-tabs has been deprecated. Better synced tabs UI has been implemented in a menu panel (bug 1201331) and sidebar (bug 1210586).