590615 - Sync UI: Avoid accessing Weave.Service (= importing service.js)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Philipp von Weitershausen [:philikon]]

Bug 587027 moves some things around so that it's possible to not having to
import all of Sync if it isn't being used at all. The Sync UI in Fennec should
take advantage of this.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

This should do the trick, though it's untested (mostly due to the fact that bug 587027 hasn't landed in m-c yet, but also because I don't have a Fennec build set up ;)).

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #0)
> Bug 587027 moves some things around so that it's possible to not having to
> import all of Sync if it isn't being used at all. The Sync UI in Fennec should
> take advantage of this.

There is still some Weave.Service call into the file you have patched

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #2)
> There is still some Weave.Service call into the file you have patched

Yes, but hopefully only when necessary (e.g. after or during set up).

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Vivien - try this out and make sure we don't get any errors and Sync still connects ok.

If all is OK, let's land it.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Oh, we are waiting for bug 590633. Guess we can't really test it yet, let alone land it.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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

All clear for us now

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

>diff --git a/chrome/content/aboutHome.xhtml b/chrome/content/aboutHome.xhtml
>     function updateWeaveButton() {
>-      let isDisabled = !getChromeWin().Weave.Service.isLoggedIn;
>+      let win = getChromeWin();
>+      let isDisabled =
>+        (win.Weave.Status.checkSetup() == Weave.CLIENT_NOT_CONFIGURED)
>+        || !win.Weave.Service.isLoggedIn;
>       document.getElementById("remoteTabs").setAttribute("disabled", isDisabled);


We could potentially do a "let weave = getchromeWin().Weave;" and use that, what do you think?
Also it seems that the expression is wrong because Weave.CLIENT_NOT_CONFIGURED will probably not work since Weave will be undefined if i am correct.

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Address review comments and unbitrot. Still untested (haven't built Fennec yet).

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

>diff --git a/chrome/content/aboutHome.xhtml b/chrome/content/aboutHome.xhtml
>+      let Weave = getChromeWin().Weave;
>+      if (!Weave)
>+        return;

Nit: add a line break here

>+      let isDisabled =
>+        (Weave.Status.checkSetup() == Weave.CLIENT_NOT_CONFIGURED)
>+        || !Weave.Service.isLoggedIn;
>+      document.getElementById("remoteTabs").setAttribute("disabled", isDisabled);
>     }

It works on my build, but I want the Weave variable to be renamed to weave before landing.

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: v3

Address review comments.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #10)
> Created attachment 476030 [details] [diff] [review]
> v3
> 
> Address review comments.

Thanks Philipp, I'm waiting for the beta 1 code freeze and branching to land this one.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Vivien - feel free to r+ and use the [fennec-checkin-postb1] whiteboard flag :)

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/2753b05a11d5

Comment 14

[Catalin Suciu [:csuciu]]

Verified this on Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919 Firefox/9.0a1 Fennec/9.0a1