Closed
Bug 600815
Opened 15 years ago
Closed 15 years ago
Warnings into browser.js and about:home
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: vingtetun, Assigned: mfinkle)
References
Details
Attachments
(2 files, 2 obsolete files)
|
4.03 KB,
patch
|
Details | Diff | Splinter Review | |
|
6.60 KB,
patch
|
vingtetun
:
review+
|
Details | Diff | Splinter Review |
No description provided.
|
Reporter | |
Comment 1•15 years ago
|
||
Oups, I've forgot the patch! Thanks fabrice :)
Assignee: nobody → 21
Attachment #479745 -
Flags: review?(mark.finkle)
|
|
Reporter | |
Comment 2•15 years ago
|
||
I've add some check for Weave into bindings.xml.
Attachment #479789 -
Flags: review?(mark.finkle)
|
|
Reporter | |
Updated•15 years ago
|
Attachment #479745 -
Flags: review?(mark.finkle)
|
Assignee | |
Updated•15 years ago
|
Attachment #479745 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 3•15 years ago
|
||
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?
|
|
Reporter | |
Comment 4•15 years ago
|
||
(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 | |
Comment 6•15 years ago
|
||
Sorry about taking so long on this patch. I just hate the need for these kind of tests everywhere
|
|
Assignee | |
Comment 7•15 years ago
|
||
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)
|
|
Reporter | |
Comment 8•15 years ago
|
||
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 :)
|
|
Assignee | |
Comment 9•15 years ago
|
||
This patch addresses Vivien's review comments.
Attachment #481152 -
Attachment is obsolete: true
Attachment #481220 -
Flags: review?(21)
Attachment #481152 -
Flags: review?(21)
|
|
Reporter | |
Updated•15 years ago
|
Attachment #479789 -
Flags: review?(mark.finkle)
|
|
Reporter | |
Comment 10•15 years ago
|
||
Comment on attachment 481220 [details] [diff] [review]
alt patch 2
thanks
Attachment #481220 -
Flags: review?(21) → review+
|
|
Assignee | |
Comment 11•15 years ago
|
||
Assignee: 21 → mark.finkle
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•