Add an addTab method to browser that returns a pageshow promise

RESOLVED FIXED in Firefox 23

Status

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 23
x86_64
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 734649 [details] [diff] [review]
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.
Assignee: nobody → jmathies
Attachment #734649 - Flags: review?(sfoster)
(Assignee)

Comment 2

6 years ago
Comment on attachment 734649 [details] [diff] [review]
patch

This broke something in addTab. Need to track it down.
Attachment #734649 - Flags: review?(sfoster)
(Assignee)

Comment 3

6 years ago
Created attachment 734815 [details] [diff] [review]
patch

Fixed up. Missing parameter in _loadUsingParams.
Attachment #734649 - Attachment is obsolete: true
Attachment #734815 - Flags: review?(sfoster)
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?
Attachment #734815 - Flags: review?(sfoster) → feedback+
(Assignee)

Comment 5

6 years ago
(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?
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();
  });
pageShowPromise works for me.
(Assignee)

Comment 8

6 years ago
Created attachment 735744 [details] [diff] [review]
patch v.2
(Assignee)

Comment 9

6 years ago
Created attachment 735749 [details] [diff] [review]
patch v.3
Attachment #734815 - Attachment is obsolete: true
Attachment #735744 - Attachment is obsolete: true
Attachment #735749 - Flags: review?(sfoster)
(Assignee)

Comment 10

6 years ago
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 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)
Attachment #735749 - Flags: review?(sfoster) → review+
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/2ecc1c0eef3d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23

Updated

5 years ago
Depends on: 895162

Updated

5 years ago
No longer depends on: 895162
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.