Closed Bug 571898 Opened 10 years ago Closed 10 years ago

port 1.4 UI for Fennec to mobile-browser

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mconnor, Assigned: mfinkle)

References

()

Details

Attachments

(1 file, 1 obsolete file)

We've split out the Fennec-specific pieces now (see the URL).  There's one pending change (expose device name) to address feedback/support the iPhone release, but we're close to being ready to start this work.
Assignee: nobody → mark.finkle
We'll need to add testcases to litmus in the fennec test run
Flags: in-litmus?
Flags: in-testsuite?
Attached patch patch (obsolete) — Splinter Review
This patch adds the bits and pieces needed to get the Sync UI working in Fennec:
* Adds MOZ_SERVICES_SYNC=1 to confvars.sh
* Adds required preferences
* Adds about:sync-tabs component
* Adds support files for about:sync-tabs (aboutTabs.xhtml, .dtd and .css)
* Adds preferences XUL
* Adds the "Remote Tabs" button, which launches the about:sync-tabs page
* Adds the WeaveGlue support code (sync.js)
Attachment #454598 - Flags: review?(21)
Attachment #454598 - Flags: feedback?(edilee)
Moving to Fennec
Assignee: mark.finkle → nobody
Component: Fennec UI → General
Product: Weave → Fennec
QA Contact: fennec → general
Version: unspecified → Trunk
Comment on attachment 454598 [details] [diff] [review]
patch

>+++ b/chrome/content/sync.js
>+  _addListeners: function _addListeners() {
>+    // Wait for the options to load for Weave
>+    addEventListener("AddonOptionsLoad", function(event) {
>+      if (event.originalTarget.getAttribute("addonID") == Weave.WEAVE_ID)
>+        WeaveGlue._updateOptions();
>+    }, false);
So now the sync preferences are shown as part of the main browser prefs, so is this piece necessary? This code was originally to fill in values if they're available as well as display the correct set of buttons/fields.
Comment on attachment 454598 [details] [diff] [review]
patch

I'll also chime in with a removal suggestion:

>+let WeaveGlue = {
>+  init: function init() {
>+    Components.utils.import("resource://services-sync/service.js");
>+    Weave.Svc.Obs.add("weave:service:ready", function(subject, data) {
>+      Weave.Svc.Obs.remove("weave:service:ready", arguments.callee);
>+
>+      let wait = 5;
>+      Weave.Service.delayedAutoConnect(wait);
>+    });

You could get rid of the "weave:service:ready" observer and instead define the 'services.sync.autoconnectDelay' preference, giving it a default value of 5.
* I'll remove the "AddonOptionsLoad" code and call it when the window loads
* I'll remove the "weave:service:ready" observer and add "services.sync.autoconnectDelay" = 5
Attached patch patch 2Splinter Review
Updated patch based on feedback.

Switched to Matt for review because of timezones
Assignee: nobody → mark.finkle
Attachment #454598 - Attachment is obsolete: true
Attachment #454636 - Flags: review?(mbrubeck)
Attachment #454598 - Flags: review?(21)
Attachment #454598 - Flags: feedback?(edilee)
Comment on attachment 454636 [details] [diff] [review]
patch 2

For a followup bug: We need to update the Weave link on about:firstrun.

>diff --git a/chrome/content/aboutTabs.xhtml b/chrome/content/aboutTabs.xhtml

No boilerplate.

>+        if (holder.hasChildNodes()) {
>+          while (holder.childNodes.length >= 1)
>+            holder.removeChild(holder.firstChild);
>+        }

This could be written as:

  while (holder.firstChild)
    holder.removeChild(holder.firstChild);

I'm worried that the DOM code in this file will be slow on mobile for users with dozens of tabs.  Could maybe use documentFragments and/or cloneNode to speed it up, if it turns out to be a problem.

>+  <script type="application/javascript" src="chrome://browser/content/sync.js"/>

Should this be a JSM?

>+                <setting id="sync-device" type="string" title="&sync.deviceName;" onchange="WeaveGlue.changeName(this)"/>

This is unreadable in portrait (but we can wait for final UI design to fix this).

>+     addEventListener("unload", function() addRem(false), false);

I'm curious, what is the purpose of this?

>+    secret.value = Weave.Service.passphrase || "";
>+    device.value = Weave.Clients.localName;

Can localName not be null/undefined?  (Even if it can't now, let's guard it like the rest - less confusing to me, and more resistant to future change.)

>+      }
>+      else {
>+        connect.firstChild.disabled = false;

curly brace style

>+// Wait a little bit before loading a bunch of Weave files from service
>+addEventListener("UIReady", function ready() setTimeout(function() {
>+  removeEventListener("UIReady", ready, false);
>+  setTimeout(function() WeaveGlue.init(), 5000);
>+}, 0), false);

If you open up the preference panel immediately after opening Fennec then the Weave prefs appear blank for five seconds.  You can edit them, but any changes you make are then overwritten.

We should cancel this timeout and initialize immediately in PreferencesView._delayedInit.  Failing that, the Weave options should be disabled until they are initiliazed.  (Can be a followup bug if you want to wait for the actual UI design.)


r+, I don't think any of the above comments should block landing of the patch, but some can be fixed before checkin and/or in followups.
Attachment #454636 - Flags: review?(mbrubeck) → review+
(In reply to comment #8)
> (From update of attachment 454636 [details] [diff] [review])
> For a followup bug: We need to update the Weave link on about:firstrun.

Filing a bug

> >diff --git a/chrome/content/aboutTabs.xhtml b/chrome/content/aboutTabs.xhtml
> 
> No boilerplate.

True, I was going to add it, but this file is going away soon.

> >+        if (holder.hasChildNodes()) {
> >+          while (holder.childNodes.length >= 1)
> >+            holder.removeChild(holder.firstChild);
> >+        }
> 
> This could be written as:
> 
>   while (holder.firstChild)
>     holder.removeChild(holder.firstChild);
> 
> I'm worried that the DOM code in this file will be slow on mobile for users
> with dozens of tabs.  Could maybe use documentFragments and/or cloneNode to
> speed it up, if it turns out to be a problem.

All true. File going away, so I left it alone for the most part.

> >+  <script type="application/javascript" src="chrome://browser/content/sync.js"/>
> 
> Should this be a JSM?

It seems like it could. I'll file a bug

> >+     addEventListener("unload", function() addRem(false), false);
> 
> I'm curious, what is the purpose of this?

removes the observers for the many sync notifications when the window closes.

> >+// Wait a little bit before loading a bunch of Weave files from service
> >+addEventListener("UIReady", function ready() setTimeout(function() {
> >+  removeEventListener("UIReady", ready, false);
> >+  setTimeout(function() WeaveGlue.init(), 5000);
> >+}, 0), false);
> 
> If you open up the preference panel immediately after opening Fennec then the
> Weave prefs appear blank for five seconds.  You can edit them, but any changes
> you make are then overwritten.
> 
> We should cancel this timeout and initialize immediately in
> PreferencesView._delayedInit.  Failing that, the Weave options should be
> disabled until they are initiliazed.  (Can be a followup bug if you want to
> wait for the actual UI design.)

I moved this to the BrowserUI.init section where we delay-init the other code
pushed:
http://hg.mozilla.org/mobile-browser/rev/d260a02c3bce
http://hg.mozilla.org/mobile-browser/rev/da94a93a2903
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
verified FIXED on build:

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b2pre) Gecko/2010630 Namoroka/4.0b2pre Fennec/2.0a1pre

We'll need a whole slew of automated and manual tests here. Tracy, how would you want to deal with this on litmus?
Status: RESOLVED → VERIFIED
For Sync, as inefficient as it sounds, I plan on having a Sync subgroup within the Fx4 testgroup as well as maintaining the separate Firefox Sync extension test group.  

You'd have to determine what will work best for mobile.
I think that approach is fine.
Assignee: mark.finkle → tchung
Assigning to self to create a litmus testcase.
(In reply to comment #14)
> Assigning to self to create a litmus testcase.

The assignee field is only for the developer who wrote the patch.
Assignee: tchung → mark.finkle
Flags: in-litmus? → in-litmus?(tchung)
Fennec 2.0 Sync bft and smoketests added.
Flags: in-litmus?(tchung) → in-litmus+
You need to log in before you can comment on or make changes to this bug.