Closed
Bug 1285789
Opened 8 years ago
Closed 8 years ago
Lazy-browser-tabs: add `TabBrowserCreated` event
Categories
(Firefox :: Tabbed Browser, enhancement, P2)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: u462496, Assigned: u462496)
References
Details
Attachments
(1 file, 2 obsolete files)
6.08 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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.
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 3•8 years ago
|
||
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)
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
Comment 7•8 years ago
|
||
(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)
Updated•8 years ago
|
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.
Assignee | ||
Comment 10•8 years ago
|
||
(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?
Comment 11•8 years ago
|
||
(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?
Comment 12•8 years ago
|
||
(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.
Assignee | ||
Comment 13•8 years ago
|
||
(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.
Assignee | ||
Comment 14•8 years ago
|
||
(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.
Comment 15•8 years ago
|
||
(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 16•8 years ago
|
||
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-
Comment 17•8 years ago
|
||
(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.
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8769506 -
Attachment is obsolete: true
Attachment #8771394 -
Flags: review?(dao+bmo)
Updated•8 years ago
|
Attachment #8771394 -
Flags: review?(dao+bmo) → review+
Comment 19•8 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/270f0c316a08 Implement TabBrowserCreated event. r=dao
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/270f0c316a08
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in
before you can comment on or make changes to this bug.
Description
•