Closed Bug 1257133 Opened 7 years ago Closed 4 years ago

Remove useless tabId support of client.getTab/TabActor.getTab

Categories

(DevTools :: Framework, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: [btpp-fix-later])

Attachments

(1 file, 1 obsolete file)

Bug 1233463 is about to make getTab's tabId parameter useless.
We should now be able to always refer to a given tab with its outerWindowID, even for out of process tabs.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (at work week until Mar. 18) from comment #40)
> Comment on attachment 8730693 [details] [diff] [review]
> Remove support of tabId in getTab request.
> 
> Review of attachment 8730693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Need more context, up to you if you want to move to a follow up.
> 
> I think it makes sense, but want to understand what was using this before.
> 
> ::: devtools/shared/client/main.js
> @@ -1613,5 @@
> >  
> >      if (aFilter) {
> >        if (typeof(aFilter.outerWindowID) == "number") {
> >          packet.outerWindowID = aFilter.outerWindowID;
> > -      } else if (typeof(aFilter.tabId) == "number") {
> 
> Where is the code that used this feature of passing tabIds?

I don't think it was used anywhere. I'm not sure we have any usage of getTab with outerWindowID/tabId parameters.
What was used is the code a few line after, when passing a "tab" parameter.
This code was sending a tabId to the actor when the tab was OOP.
(And outerWindowId for tabs in parent process).

So tabId codepath was used in the actor, but not the aFilter.tabId block.
Assignee: nobody → poirot.alex
Attached patch patch v1 (obsolete) — Splinter Review
Moved to a followup as I have a mixed feeling about html:iframe.
outerWindowID is available on xul:browser thanks to additional work being done
in remote-browser XBL:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#221

So if we apply this patch, I'm expecting things to break for html...
Attached patch patch v2Splinter Review
Fixed typo.
Attachment #8731186 - Attachment is obsolete: true
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Product: Firefox → DevTools
Moving this inactive P2 to the backlog (P3)
Priority: P2 → P3
The code I wanted to removed might still be useful for debugging html frame, which we might have if we get rid of XUL.
Given that it is still covered by tests, we may as well keep it until we have a stronger case proving it is really useless.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.