859154 - Add an addTab method to browser that returns a pageshow promise

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Description

[Jim Mathies [:jimm]]

This would be useful in testing. In tests I've noticed is that our delayed pageshow waiting can sometimes fail:

let tab = Browser.addTab(aUrl, true);
yield waitForEvent(tab.browser, "pageshow");

pageshow can fire before we set the handler for it, so this will occasionally time out.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Creating 20 tabs in succession I get no timeout with this. I'll file a follow up on updating tests that use addTab.

The routine I'm using from bug 859155:

function timeTab(aUrl) {
  return Task.spawn(function() {
    try {
      PerfTest.startStopwatch();
      let promise = Browser.promiseAddTab(aUrl, true);
      let tab = yield promise;
      PerfTest.stopStopwatch();
      Browser.closeTab(tab)
      yield waitForMs(500);
      info("time: " + PerfTest.stopwatch());
    } catch (ex) { info("" + ex); }
    throw new Task.Result(PerfTest.stopwatch());
  });
}

side note, our add tab functionality takes about 140 msec on average.

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 734649 [details] [diff] [review]
patch

This broke something in addTab. Need to track it down.

Comment 3

[Jim Mathies [:jimm]]

Attachment: patch

Fixed up. Missing parameter in _loadUsingParams.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 734815 [details] [diff] [review]
patch

Review of attachment 734815 [details] [diff] [review]:
-----------------------------------------------------------------

Is the plan to phase out the use of the non-promise versions of addTab and create? Having these variations seems like it will cause bad things to happen down the road - we should pick a pattern and stick with it. I would expect addTab to return a promise for the tab. If that can't be so, maybe a promise isn't the right approach and we should go with the event instead?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Sam Foster [:sfoster] from comment #4)
> Comment on attachment 734815 [details] [diff] [review]
> patch
> 
> Review of attachment 734815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is the plan to phase out the use of the non-promise versions of addTab and
> create? Having these variations seems like it will cause bad things to
> happen down the road - we should pick a pattern and stick with it. I would
> expect addTab to return a promise for the tab. If that can't be so, maybe a
> promise isn't the right approach and we should go with the event instead?

I don't think I'm qualified to answer these questions. :) I doubt we want to change every call of addTab just so we can add a more reliable addTab for tests.

Maybe mbrubeck/fyan have an opinion?

Comment 6

[Matt Brubeck (:mbrubeck)]

I definitely don't like the code duplication; it seems like asking for trouble when we make a change and forget to change or test both code paths.

What if addTab always returned the tab *and* always added the promise as a public property on the tab?  Then most of our code could remain the same:

  let tab = Browser.addTab(url);
  tab.doSomething();

But code that *does* want to wait for pageshow can do:

  Task.spawn(function() {
    let tab = Browser.addTab(url);
    yield tab.pageShowPromise;
    tab.doSomething();
  });

Comment 7

[Sam Foster [:sfoster] (he/him)]

pageShowPromise works for me.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 9

[Jim Mathies [:jimm]]

Attachment: patch v.3

Comment 10

[Jim Mathies [:jimm]]

I noticed a random js error today with this applied - 

[JavaScript Error: "TypeError: self._eventDeferred is null" {file: "chrome://browser/content/browser.js" line: 1557}]

Comment 11

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 735749 [details] [diff] [review]
patch v.3

Review of attachment 735749 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I like the pageShowPromise getter. Couple comments inline

::: browser/metro/base/content/browser.js
@@ +1538,5 @@
>      let browser = this._createBrowser(aURI, null);
>  
> +    let self = this;
> +    function onPageShowEvent(aEvent) {
> +      self._eventDeferred.resolve(this);

I can't see or imagine how self._eventDeferred could be null here. I guess we could guard against it. Would be better to understand it eventually though and the guard would impede that, so I'm inclined to leave it as-is

'this' is the global here, do you mean self, or is the resolved value even used at all (maybe true would be better)

Comment 12

[Jim Mathies [:jimm]]

(In reply to Sam Foster [:sfoster] from comment #11)
> 'this' is the global here, do you mean self, or is the resolved value even
> used at all (maybe true would be better)

Yes, I think 'this' should be 'self'. I think 'this' would be the event, and I don't believe anyone would have use of that.

Comment 13

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #10)
> I noticed a random js error today with this applied - 
> 
> [JavaScript Error: "TypeError: self._eventDeferred is null" {file:
> "chrome://browser/content/browser.js" line: 1557}]

I figured this out - iframe in pages fire additional pageshows, so we need to remove the event handler in the page show callback vs. cleanup. In fact I think we can get rid of cleanup all together.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ecc1c0eef3d