Warnings into browser.js and about:home

RESOLVED FIXED

Status

Firefox for Android Graveyard
General
--
trivial
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: vingtetun, Assigned: mfinkle)

Tracking

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 479745 [details] [diff] [review]
Patch

Oups, I've forgot the patch! Thanks fabrice :)
Assignee: nobody → 21
Attachment #479745 - Flags: review?(mark.finkle)
Created attachment 479789 [details] [diff] [review]
Patch v0.2

I've add some check for Weave into bindings.xml.
Attachment #479789 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Attachment #479745 - Attachment is obsolete: true
Comment on attachment 479789 [details] [diff] [review]
Patch v0.2

This | "Weave" in window | pattern is going to be very easy to break when we add new code. I am tempted to just rollback the original patch and load Service JSM. I didn't notice any change in Ts due to the patch anyway.

Vivien - any thoughts on rolling back before I review this?
(In reply to comment #3)
> Comment on attachment 479789 [details] [diff] [review]
> Patch v0.2
> 
> This | "Weave" in window | pattern is going to be very easy to break when we
> add new code. I am tempted to just rollback the original patch and load Service
> JSM. I didn't notice any change in Ts due to the patch anyway.
> 
> Vivien - any thoughts on rolling back before I review this?

No, I agree with you and I was thinking that myself while diffing into our code but mostly because I was bored to have to add this check before using the Weave var.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 600954
Sorry about taking so long on this patch. I just hate the need for these kind of tests everywhere
Created attachment 481152 [details] [diff] [review]
alt patch 1

This patch is similar to Vivien's patch but it uses the <command> to control code that uses Weave. WeaveGlue controls the <command> and all other code just observes the <command>.

We should even speed up loading the home page as well as avoid any freezes.
Attachment #481152 - Flags: review?(21)
Comment on attachment 481152 [details] [diff] [review]
alt patch 1

>diff --git a/chrome/content/aboutHome.xhtml b/chrome/content/aboutHome.xhtml
>     function initWeave() {
>       let chromeWin = getChromeWin();
>-      if ("WeaveGlue" in chromeWin) {
>-        chromeWin.Services.obs.addObserver(updateWeaveButton, "weave:service:login:finish", false);
>-        chromeWin.Services.obs.addObserver(updateWeaveButton, "weave:service:logout:finish", false);
>-
>-        updateWeaveButton();
>-      }
>-      else {
>-        let loading = document.getElementById("remoteTabs");
>-        loading.parentNode.removeChild(loading);
>-      }
>+      chromeWin.Services.obs.addObserver(updateWeaveButton, "weave:service:login:finish", false);
>+      chromeWin.Services.obs.addObserver(updateWeaveButton, "weave:service:logout:finish", false);
>     }

You need to be careful to not regress bug 591701 if you remove the initial call to updateWeaveButton
http://hg.mozilla.org/mobile-browser/rev/af07a287e16d

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml
>+
>+#ifdef MOZ_SERVICES_SYNC
>+              // once we have check badges, check if the tab is in the remote list

Nit: You can probably tweak the comment since we're not check bagdes yet


>       <method name="_getRemoteTabs">
>         <body><![CDATA[
>+          // Don't do anything if the Weave isn't ready
Nit: the comment sounds strange to me but it can be because of my english

>+          if (document.getElementById("cmd_remoteTabs").getAttribute("disabled") == "true")
>+            return [];
>+

Maybe we can move the command attribute from the AwesomePanel directly to the xul panel element and use something like:
document.getElementById(this.getAttribute("command")) ?
But we can do that later.


I'm just worried about the possible regression and you should also probably add the check for "cachedWidth" (see in my patch) which is not related to Weave :)
Created attachment 481220 [details] [diff] [review]
alt patch 2

This patch addresses Vivien's review comments.
Attachment #481152 - Attachment is obsolete: true
Attachment #481220 - Flags: review?(21)
Attachment #481152 - Flags: review?(21)
Comment on attachment 481220 [details] [diff] [review]
alt patch 2

thanks
Attachment #481220 - Flags: review?(21) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/6aec315eafcd
Assignee: 21 → mark.finkle
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.