697259 - Follow-up: e10s-friendly handling of sync link visibility on sync setup or unlink

Status: RESOLVED INVALID

(Firefox :: Sync, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Part 2, v2b: async message passing for sync link visibility

WIP patch for better handling of sync link visibility change. Follow-up to patch 2 of bug 675821.

Successfully changes "Set Up Sync" link visibility in UI, listeners not firing in testing.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: part 1, v1: async messages + tests (listener fails to fire)

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 569817 [details] [diff] [review]
part 1, v1: async messages + tests (listener fails to fire)

This part looks good. I'll attach a patch for test tweaks in a second.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: test fixes

For some reason the content.js redeclaration error seems to only affect tests, I'm not sure why.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: part 2, v1: changed elt.setAttr(hidden) to elt.hidden

apply "2b, v1" and then part 2 on top of that. Test is browser_aboutHome.js, and fails on un-hiding setupSyncLink after unpairing (although the converse, hiding setupSyncLink after setup, *does* work and passes testing).

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 570803 [details] [diff] [review]
part 2, v1: changed elt.setAttr(hidden) to elt.hidden

>diff --git a/browser/base/content/content.js b/browser/base/content/content.js

>-    if (hidden)
>-      item.setAttribute("hidden", "true");
>-    else
>-      item.removeAttribute("hidden");
>+    if (hidden) {
>+      item.hidden = true;
>+    } else {
>+      item.hidden = false;
>+    }

Why this change? Setting the attribute is generally more reliable (not all elements have the hidden attribute reflected as a DOM property). I think you should instead change the test to also check the attribute (rather than the property).

Comment 6

[Richard Newman [:rnewman]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #5)

> Why this change?

I suggested this to liuche on IRC, based on this:

  "Using setAttribute() to modify certain attributes, most notably value in
   XUL, works inconsistently, as the attribute specifies the default value. To
   access or modify the current values, you should use the properties. For
   example, use elt.value instead of elt.setAttribute('value', val)."

   -- https://developer.mozilla.org/en/DOM/element.setAttribute

This was a stab-in-the-dark suggestion, 'cos I'm still waiting for a build to finish so that I can give better advice :)

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Ah, it turns out I was wrong - my advice holds for XUL, where the hidden attribute may not be reflected consistently, but in HTML it appears that .hidden is specced to apply to all elements, and we seem to implement that, so sticking to ".hidden" everywhere makes sense.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 570803 [details] [diff] [review]
part 2, v1: changed elt.setAttr(hidden) to elt.hidden

you could simplify to item.hidden = hidden;, though

Comment 9

[Richard Newman [:rnewman]]

Anyone feel strongly about landing this now?

Comment 10

[Chenxia Liu [:liuche]]

No longer relevant with the current about:home.