642655 - Get rid of hashchange, switch to postMessage

Status: RESOLVED FIXED

(Cloud Services :: Share: Firefox Client, defect)

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: fixes to remove hashchange use

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: fixes to remove hashchange use

new patch addressing the possibility of adding multiple popupshown event handlers.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 523130 [details] [diff] [review]
fixes to remove hashchange use

>+  panelReady: function() {
>+    // called when the panel is opened for the very first time.  We need to
>+    // hook up a listener for popupshown so we can set the share state
>+    // in the future, and we need to set the initial share state now.
>+
>+    // We have to do this after the first time the panel is loaded and NOT
>+    // before so we know the postMessage hookups are all in place and the
>+    // content can react properly.
>+    let self = this;
>+    this.panel.addEventListener('popupshown', function(event) {
>+      self.getShareState();
>+    }, true);
>+
>+    // we replace panelReady to only do a getShareState call, and never add
>+    // another event listener again.  Content MAY call panelReady again without
>+    // a negative side effect.
>+    this.panelReady = function() {
>+      this.getShareState();
>+    }
>+
>+    // call the new function now to force the first-time call to getShareState
>+    this.panelReady();
>+  },

I don't see this function being called anywhere. Am I missing something?

>-    let url = this.ffshare.prefs.shareURL + '#options=' + encodeURIComponent(JSON.stringify(options));
>+    let url = this.ffshare.prefs.shareURL;
> 
>     // adjust the size
>     this.browser.style.width = this.lastWidth + 'px';
>     this.browser.style.height = this.lastHeight + 'px';
> 
>     // Make sure it can go all the way to zero.
>     this.browser.style.minHeight = 0;
> 
>     if (this.forceReload) {
>-      this.browser.loadURI(url);
>+      // first time load, just set the src, otherwise force a real reload
>+      if (this.browser.getAttribute('src') !== url) {
>+        this.browser.setAttribute('src', url);
>+      } else {
>+        this.browser.loadURI(url);
>+      }
>       this.forceReload = false;
>     } else {
>       this.browser.setAttribute('src', url);
>     }

Nit: please define 'url' close to where it's being used (IOW, move it just before the if).

>     // fx 4

Nit: please remove this comment :)

>-    let position = (this.window.getComputedStyle(this.window.gNavToolbox, "").direction === "rtl") ? 'bottomcenter topright' : 'bottomcenter topleft';
>-    this.panel.openPopup(anchor, position, 0, 0, false, false);
>+    if (this.panel.state === 'open') {
>+      this.getShareState();
>+    } else {
>+      let position = (this.window.getComputedStyle(this.window.gNavToolbox, "").direction === "rtl") ? 'bottomcenter topright' : 'bottomcenter topleft';

Nit: wrap overlong line.

r- because of the panelReady question.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: fixes to remove hashchange use

improve comment in panelReady
fix code for other comments from prior review

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 523154 [details] [diff] [review]
fixes to remove hashchange use

Sorry, I have more nits :/

>+  panelReady: function() {
>+    // called from web content via post message whenever it is loaded.

Thanks, this is much clearer now.

>+
>+    // when the panel is opened for the very first time, we have to hook up a
>+    // listener for popupshown so we can set the share state in the future, and
>+    // we need to set the initial share state now.  We replace the panelReady
>+    // function with a simpler version after the first call.
>+
>+    // We have to do this after the first time the panel is loaded and NOT
>+    // before so we know the postMessage hookups are all in place and the
>+    // content can react properly.
>+    let self = this;
>+    this.panel.addEventListener('popupshown', function(event) {
>+      self.getShareState();
>+    }, true);

Nit: use 'false' for the useCapture argument. There's also no need for the "self = this" hack + closures in Firefox 4+ if all you want is have a bound function called:

  this.panel.addEventListener("popupshown", this.getShareState.bind(this), false);

>+    // we replace panelReady to only do a getShareState call, and never add
>+    // another event listener again.  Content MAY call panelReady again without
>+    // a negative side effect.
>+    this.panelReady = function() {
>+      this.getShareState();
>+    }

Why not do

  this.panelReady = this.getShareState

?

>+    // call the new function now to force the first-time call to getShareState
>+    this.panelReady();
>+  },

I would vote for a) calling getShareState here to have less indirection (readability) and b) calling this at the top of the function.

>-    let position = (this.window.getComputedStyle(this.window.gNavToolbox, "").direction === "rtl") ? 'bottomcenter topright' : 'bottomcenter topleft';
>-    this.panel.openPopup(anchor, position, 0, 0, false, false);
>+    if (this.panel.state === 'open') {
>+      this.getShareState();
>+    } else {
>+      let position = (this.window.getComputedStyle(this.window.gNavToolbox,
>+                            "").direction === "rtl") ?
>+                            'bottomcenter topright' : 'bottomcenter topleft';

Apart from the fact that the wrapping isn't according to our style guide (I'm a pedantic little bastard, I know), I think it'd be much easier to read if we'd just break this out into an if/else clause.

r=me with the nits addressed

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: fixes to remove hashchange use

last comments corrected.

Comment 7

[Philipp von Weitershausen [:philikon]]

Comment on attachment 523167 [details] [diff] [review]
fixes to remove hashchange use

Looks great, will land.

Comment 8

[Philipp von Weitershausen [:philikon]]

Landed Shane's patch: https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/7a33d13d052a

Still needs tests, not resolving for now.

Comment 9

[Philipp von Weitershausen [:philikon]]

We also need to document the new API call (panelReady).

Comment 10

[Philipp von Weitershausen [:philikon]]

Landed a test for this: https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/043222e12cf6

Comment 11

[Shane Caraveo (:mixedpuppy)]

updated doc comment on https://github.com/mozilla/f1/wiki/Chrome-web-data-api:-v1