571898 - port 1.4 UI for Fennec to mobile-browser

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Connor [:mconnor]]

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.

Comment 1

[Aakash Desai [:aakashd]]

We'll need to add testcases to litmus in the fennec test run

Comment 2

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

Attachment: patch

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)

Comment 3

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

Moving to Fennec

Comment 4

[Ed Lee :Mardak]

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 5

[Philipp von Weitershausen [:philikon]]

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.

Comment 6

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

* 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

Comment 7

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

Attachment: patch 2

Updated patch based on feedback.

Switched to Matt for review because of timezones

Comment 8

[Matt Brubeck (:mbrubeck)]

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.

Comment 9

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

(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

Comment 10

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/d260a02c3bce
http://hg.mozilla.org/mobile-browser/rev/da94a93a2903

Comment 11

[Aakash Desai [:aakashd]]

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?

Comment 12

[Tracy Walker [:tracy]]

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.

Comment 13

[Aakash Desai [:aakashd]]

I think that approach is fine.

Comment 14

[Tony Chung [:tchung]]

Assigning to self to create a litmus testcase.

Comment 15

[Reed Loden [:reed]]

(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.

Comment 16

[Tony Chung [:tchung]]

Fennec 2.0 Sync bft and smoketests added.