Closed Bug 1285789 Opened 8 years ago Closed 8 years ago

Lazy-browser-tabs: add `TabBrowserCreated` event

Categories

(Firefox :: Tabbed Browser, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: u462496, Assigned: u462496)

References

Details

Attachments

(1 file, 2 obsolete files)

This is a support bug for bug 906076, lazy-browser-tabs.

Currently, `TabOpen` event gets fired in addTab.  In some cases, listeners act on the tab's linkedBrowser.  When lazy-browser-tabs are implemented, the browser may not be present until some time after `TabOpen` event is fired.  Thus we need a `TabLinkBrowser` event which will fire when the browser is linked to the tab.
Blocks: lazytabs
Attached patch Patch (obsolete) — Splinter Review
Adds `TabLinkBrowser` event dispatcher to `_linkBrowserToTab`.  Also adds `TabLinkBrowser` handlers to handle operations done on `tab.linkedBrowser` which were previously handled by `TabOpen` handlers.
Attachment #8769463 - Flags: review?(dao+bmo)
Comment on attachment 8769463 [details] [diff] [review]
Patch

Please rename the event to TabBrowserCreated.

What's the point of the services/sync/modules/engines/tabs.js and services/cloudsync/CloudSyncTabs.jsm changes?
Attachment #8769463 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [:dao] from comment #3)
> What's the point of the services/sync/modules/engines/tabs.js and
> services/cloudsync/CloudSyncTabs.jsm changes?

Both test/use tab.linkedBrowser in handler:

services/cloudsync/CloudSyncTabs.jsm

  let update = function (event) {
    if (event.originalTarget.linkedBrowser) {
      if (PrivateBrowsingUtils.isBrowserPrivate(event.originalTarget.linkedBrowser) &&
          !PrivateBrowsingUtils.permanentPrivateBrowsing) {
        return;
      }
    }

    eventSource.emit("change");
  };

https://dxr.mozilla.org/mozilla-central/source/services/cloudsync/CloudSyncTabs.jsm#149

services/sync/modules/engines/tabs.js:

  onTab: function (event) {
    if (event.originalTarget.linkedBrowser) {
      let browser = event.originalTarget.linkedBrowser;
      if (PrivateBrowsingUtils.isBrowserPrivate(browser) &&
          !PrivateBrowsingUtils.permanentPrivateBrowsing) {
        this._log.trace("Ignoring tab event from private browsing.");
        return;
      }
    }

https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/tabs.js#363
Flags: needinfo?(dao+bmo)
Attached patch Patch V2 (obsolete) — Splinter Review
Attachment #8769463 - Attachment is obsolete: true
Attachment #8769506 - Flags: review?(dao+bmo)
(In reply to Allasso Travesser from comment #5)
> Created attachment 8769506 [details] [diff] [review]
> Patch V2

Changed TabLinkBrowser to TabBrowserCreated
Summary: Lazy-browser-tabs: add `TabLinkBrowser` event → Lazy-browser-tabs: add `TabBrowserCreated` event
(In reply to Allasso Travesser from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > What's the point of the services/sync/modules/engines/tabs.js and
> > services/cloudsync/CloudSyncTabs.jsm changes?
> 
> Both test/use tab.linkedBrowser in handler:

But both listen to the TabOpen event too. Won't that cause the browser to be created there already?
Flags: needinfo?(dao+bmo)
Assignee: nobody → allassopraise
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to Allasso Travesser from comment #4)
> > (In reply to Dão Gottwald [:dao] from comment #3)
> > > What's the point of the services/sync/modules/engines/tabs.js and
> > > services/cloudsync/CloudSyncTabs.jsm changes?
> > 
> > Both test/use tab.linkedBrowser in handler:
> 
> But both listen to the TabOpen event too. Won't that cause the browser to be
> created there already?

No, not at this point.  They will only get `false` for event.originalTarget.linkedBrowser and pass.

IAE, we want to train Firefox to deal with tab and browser separately.  In future incarnations we will be linking the browser if linkedBrowser properties are accessed via a proxy, but this is only intended as a fallback for addons just to make sure things don't break.  We don't want Firefox to unnecessarily link the browser before its time.  See https://bugzilla.mozilla.org/show_bug.cgi?id=906076#c35.  Eventually we will be hunting down all call sites that access linkedBrowser properties and modifying code wherever possible so they can function without linking the browser if possible.
In future code we will implement a flag on the tab (called `hasLinkedBrowser` in the original patch) to indicate the tab's lazy state because testing for tab.linkedBrowser will return the proxy and resolve `true`.  Thus we'll need to make that change in services/cloudsync/CloudSyncTabs.jsm and services/sync/modules/engines/tabs.js as well.
(In reply to Allasso Travesser from comment #9)
> In future code we will implement a flag on the tab (called
> `hasLinkedBrowser` in the original patch) to indicate the tab's lazy state
> because testing for tab.linkedBrowser will return the proxy and resolve
> `true`.  Thus we'll need to make that change in
> services/cloudsync/CloudSyncTabs.jsm and
> services/sync/modules/engines/tabs.js as well.

Perhaps we should go ahead and introduce the flag in this bug, and test for it now in those sites?
(In reply to Allasso Travesser from comment #8)
> > But both listen to the TabOpen event too. Won't that cause the browser to be
> > created there already?
> 
> No, not at this point.  They will only get `false` for
> event.originalTarget.linkedBrowser and pass.

I thought the plan was to automatically create the browser whenever linkedBrowser is touched. Why would we not do this in this case?

Also, why listen to the TabOpen event at all when all that code cares about is the browser? I.e. shouldn't it use the TabBrowserCreated event *instead of* the TabOpen event?
(In reply to Allasso Travesser from comment #10)
> (In reply to Allasso Travesser from comment #9)
> > In future code we will implement a flag on the tab (called
> > `hasLinkedBrowser` in the original patch) to indicate the tab's lazy state
> > because testing for tab.linkedBrowser will return the proxy and resolve
> > `true`.  Thus we'll need to make that change in
> > services/cloudsync/CloudSyncTabs.jsm and
> > services/sync/modules/engines/tabs.js as well.
> 
> Perhaps we should go ahead and introduce the flag in this bug, and test for
> it now in those sites?

No, let's do that when it's clear that we'll actually need that. I don't think we're at that point yet.
(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to Allasso Travesser from comment #8)
> > > But both listen to the TabOpen event too. Won't that cause the browser to be
> > > created there already?
> > 
> > No, not at this point.  They will only get `false` for
> > event.originalTarget.linkedBrowser and pass.
> 
> I thought the plan was to automatically create the browser whenever
> linkedBrowser is touched. Why would we not do this in this case?

Well, that is what I was trying to explain in comment 8.  Automatically creating the browser should only be a fallback so things don't break.  But the whole point is to keep the browser lazy until it is absolutely needed.  We *don't want* to link the browser if it is not needed, we want to train Firefox to deal with the lazy browser implementation.  Otherwise our purpose for lazy browser tabs is void.

> Also, why listen to the TabOpen event at all when all that code cares about
> is the browser? I.e. shouldn't it use the TabBrowserCreated event *instead
> of* the TabOpen event?

Well, it appeared from the code that in both cases they may have still been interested in the opening of a new tab; in one case logging the occurance of a generic tab event, in the other calling `eventSource.emit("change")`.

But if you think that the `TabOpen` listener is not necessary, then yes, lets remove those.
(In reply to Dão Gottwald [:dao] from comment #11)
> I thought the plan was to automatically create the browser whenever
> linkedBrowser is touched. Why would we not do this in this case?

We don't want to create the browser in those cases.  For now we can test for linkedBrowser as it would return undefined if the browser is not created, but in the future when linkedBrowser is a proxy, we will want to use a flag, so we *don't* touch linkedBrowser.
(In reply to Allasso Travesser from comment #13)
> > Also, why listen to the TabOpen event at all when all that code cares about
> > is the browser? I.e. shouldn't it use the TabBrowserCreated event *instead
> > of* the TabOpen event?
> 
> Well, it appeared from the code that in both cases they may have still been
> interested in the opening of a new tab; in one case logging the occurance of
> a generic tab event, in the other calling `eventSource.emit("change")`.
> 
> But if you think that the `TabOpen` listener is not necessary, then yes,
> lets remove those.

Have you checked what the code listening to the change event does?

(In reply to Allasso Travesser from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #11)
> > I thought the plan was to automatically create the browser whenever
> > linkedBrowser is touched. Why would we not do this in this case?
> 
> We don't want to create the browser in those cases.  For now we can test for
> linkedBrowser as it would return undefined if the browser is not created,
> but in the future when linkedBrowser is a proxy, we will want to use a flag,
> so we *don't* touch linkedBrowser.

For now linkedBrowser exists anyway, so the null-checks are kind of pointless as far as the TabOpen event is concerned. If they weren't, that would actually be a problem, because then that code would fail to identify private-browsing tabs.
Comment on attachment 8769506 [details] [diff] [review]
Patch V2

The sync changes seem wrong. We need to decide which event we want here rather than just listening for both.
Attachment #8769506 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 8769506 [details] [diff] [review]
> Patch V2
> 
> The sync changes seem wrong. We need to decide which event we want here
> rather than just listening for both.

FWIW, feel free to file a Sync bug on this. It's independent from the rest of your patch, so no need to have this bug blocked by that.
Attached patch Patch V3Splinter Review
Attachment #8769506 - Attachment is obsolete: true
Attachment #8771394 - Flags: review?(dao+bmo)
Attachment #8771394 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/270f0c316a08
Implement TabBrowserCreated event. r=dao
https://hg.mozilla.org/mozilla-central/rev/270f0c316a08
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: