Closed Bug 590615 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(fennec2.0b2+)

VERIFIED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: philikon, Assigned: philikon)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 590633
Attached patch v1 (obsolete) — Splinter Review
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 ;)).
(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
(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).
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b1+
Assignee: nobody → philipp
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.
Attachment #469133 - Flags: review?(21)
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.
Attachment #469133 - Flags: review?(21)
tracking-fennec: 2.0b1+ → 2.0b2+
Comment on attachment 469133 [details] [diff] [review]
v1

All clear for us now
Attachment #469133 - Flags: review?(21)
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.
Attachment #469133 - Flags: review?(21) → review-
Attached patch v2 (obsolete) — Splinter Review
Address review comments and unbitrot. Still untested (haven't built Fennec yet).
Attachment #469133 - Attachment is obsolete: true
Attachment #475882 - Flags: review?(21)
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.
Attachment #475882 - Flags: review?(21) → review+
Attached patch v3Splinter Review
Address review comments.
Attachment #475882 - Attachment is obsolete: true
(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.
Vivien - feel free to r+ and use the [fennec-checkin-postb1] whiteboard flag :)
pushed:
http://hg.mozilla.org/mobile-browser/rev/2753b05a11d5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fennec-checkin-postb1]
Verified this on Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919 Firefox/9.0a1 Fennec/9.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.