Remove devtools.toolbox.content-frame preference and related workaround
Categories
(DevTools :: General, task, P1)
Tracking
(Fission Milestone:M6, firefox74 fixed)
| Tracking | Status | |
|---|---|---|
| firefox74 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
(Blocks 1 open bug)
Details
(Whiteboard: dt-fission-m2-mvp)
Attachments
(5 files)
Now that https://bugzilla.mozilla.org/show_bug.cgi?id=1578979 landed, we are not using the pref devtools.toolbox.content-frame anymore. We can remove:
|
||
Updated•5 years ago
|
|
Assignee | |
Comment 1•5 years ago
|
||
|
|
Assignee | |
Comment 2•5 years ago
|
||
Depends on D47963
|
|
||
Updated•5 years ago
|
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/97efcd0fa370 Remove devtools preference devtools.toolbox.content-frame r=ochameau https://hg.mozilla.org/integration/autoland/rev/c211bfbf59b2 Remove devtools-specific workaround in BrowsingContext.cpp r=nika
|
||
Comment 4•5 years ago
|
||
Backed out changeset 97efcd0fa370 (Bug 1585747) for causing mochitest permafailures in /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BrowsingContextGroup CLOSED TREE
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269735389&repo=autoland&lineNumber=1937
Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1a654db74777 Backed out changeset c211bfbf59b2 for causing mochitest permafailures in /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BrowsingContextGroup CLOSED TREE https://hg.mozilla.org/integration/autoland/rev/ed16b88db27e Backed out changeset 97efcd0fa370 for causing mochitest permafailures in /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BrowsingContextGroup CLOSED TREE
|
|
Assignee | |
Comment 6•5 years ago
|
||
I am hitting a test failure on https://searchfox.org/mozilla-central/source/layout/base/tests/chrome/test_css_visibility_propagation.xul which was added in Bug 1541253. The test is using swapFrameLoaders with chrome type iframes.
We no longer support swapping frames which are in the same browsing context tree as the embedder.
I suppose we could rewrite the test in order to create the iframes in a content type embedder?
But I already tried doing that for https://searchfox.org/mozilla-central/source/dom/base/test/chrome/test_bug814638.xul and didn't get anywhere.
Nika: do you have any suggestions on how to modify this test properly? Do we need to switch this to a browser mochitest?
|
||
Comment 7•5 years ago
|
||
Moving ni? to :hiro, who wrote the test.
Using swapFrameLoaders outside of the context of toplevel content is pretty broken. We put in effort to remove the last remaining usage for devtools, but your test added in bug 1541253 introduced a new consumer. I'm curious why your test performs swapFrameLoaders on a chrome iframe (see comment 6). The only fully supported configuration for frameLoader swapping is between <xul:browser type="content"> elements.
|
||
Comment 8•5 years ago
|
||
The intention of the test case was to ensure the machinery introduced in bug 1541253 works when nsSubDocumentFrame::EndSwapDocShells is called because, at that time, there were possibilities that the function is called. I am totally fine to drop the test in this bug with filing the dropping the propagation stuff introduced in bug 1541253.
|
||
Comment 9•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
The intention of the test case was to ensure the machinery introduced in bug 1541253 works when nsSubDocumentFrame::EndSwapDocShells is called because, at that time, there were possibilities that the function is called. I am totally fine to drop the test in this bug with filing the dropping the propagation stuff introduced in bug 1541253.
Do we not need that code?
|
|
||
Comment 10•5 years ago
|
||
Hmm, we may still want the code if we swap such browser elements, are there such code somewhere?
|
||
Comment 11•5 years ago
|
||
There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jdescottes, could you have a look please?
For more information, please visit auto_nag documentation.
|
|
Assignee | |
Comment 12•5 years ago
|
||
This can't land until test_css_visibility_propagation.xul is either removed or updated.
|
|
||
Updated•5 years ago
|
|
||
Comment 14•4 years ago
|
||
Tracking Fission DevTools bugs for Fission Nightly (M6)
|
|
Assignee | |
Comment 15•4 years ago
|
||
Depends on D47964
|
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 16•4 years ago
|
||
Depends on D59868
|
|
Assignee | |
Comment 17•4 years ago
•
|
||
Hi Nika!
Sorry to bother you again with this but I have a new test failure on the latest central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=978c102bd686164ebe8c524ed7703734335c5e3a&selectedJob=284936975
The failing test is test_bug814638.xhtml , which uses swapFrameLoader with iframes. Basically the test loads the following structure twice, in 2 different windows
<iframe> <!-- toplevel iframe -->
#document
<keyset><key key="T"/></keyset>
<iframe src="about:mozilla">...</iframe> <!-- inner iframe -->
</iframe>
The test calls swapFrameLoaders on the two top-level iframes, then triggers an event in one of the "inner iframes" and checks that the <key> still executes the expected command.
In order to work without the workaround, those iframes should be switched to type=content (or even <browser type=content>). But I don't think <keyset> works in content frames, and overall I don't know if the feature tested here is still relevant. It was added in Bug 814638 , seems like it was mostly added for devtools. And when we switched to using frames with type=content in devtools, part of the change was actually to reattach chromeEventHandler events after a call to swapFrameLoaders.
So I wonder if we could just remove the test? But I also see that the code added in the bug is not directly attached to the workaround: nsFrameLoader.cpp#741-744 . Should we also delete some of this code?
Let me know what you think!
(note: I don't know why this test didn't fail the first time we tried to land this bug, at that time it was using XUL files instead of XHTML, but I'm not sure what this implies)
|
|
||
Comment 18•4 years ago
|
||
This seems to only be relevant for chrome subframes, such as old devtools.
I think we can remove the test now, as the thing it's testing (swapping around chrome iframes) is explicitly not supported anymore.
|
|
Assignee | |
Comment 19•4 years ago
|
||
Depends on D59879
|
||
Comment 20•4 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/015bfc52238a Remove devtools preference devtools.toolbox.content-frame r=ochameau https://hg.mozilla.org/integration/autoland/rev/1cfecd018732 Remove devtools-specific workaround in BrowsingContext.cpp r=nika https://hg.mozilla.org/integration/autoland/rev/335bf7caf23f Fix test test_css_visibility_propagation.xhtml r=hiro https://hg.mozilla.org/integration/autoland/rev/41789a363bed Use browser elements instead of iframe elements in test_css_visibility_propagation.xhtml r=hiro https://hg.mozilla.org/integration/autoland/rev/30e7f3facfa8 Remove test_bug814638.xhtml r=nika
|
||
Comment 21•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/015bfc52238a
https://hg.mozilla.org/mozilla-central/rev/1cfecd018732
https://hg.mozilla.org/mozilla-central/rev/335bf7caf23f
https://hg.mozilla.org/mozilla-central/rev/41789a363bed
https://hg.mozilla.org/mozilla-central/rev/30e7f3facfa8
Description
•