Closed Bug 1587142 Opened 2 months ago Closed Last month

Remove XBL related tests

Categories

(Core :: XBL, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: surkov, Assigned: bgrins)

References

(Blocks 3 open bugs)

Details

Attachments

(4 files)

bug 1510785 puts all XBL related stuff including tests under MOZ_XBL directive, and has a patch D45616 that disables all XBL related tests. Bz pointed out that numbers of tests might be relevant for shadow DOM and thus need to be rewritten to use shadow DOM.

It might be a good idea to track this work separately from bug 1510785: the work volume might be large to mix with other stuff and it seems there's no point to keep XBL tests running after we have no XBL binding left in the tree (textbox is the last one bug 1513325), so we can remove them safely without any interference with bug 1510785.

Unblocking Bug 1397874 since I'd like to track this type of work in Bug 1566221 specifically.

No longer blocks: war-on-xbl

Hello, i was browsing the code repo and noticed that there are a lot of tests that declare bindings https://dxr.mozilla.org/mozilla-central/search?q=%3Cbindings+-path%3A*.rs+-path%3A*.cpp&redirect=false

(In reply to Vlad Stanimir from comment #2)

Hello, i was browsing the code repo and noticed that there are a lot of tests that declare bindings https://dxr.mozilla.org/mozilla-central/search?q=%3Cbindings+-path%3A*.rs+-path%3A*.cpp&redirect=false

These are being ported to use shadow dom when applicable in Bug 1510785 / https://phabricator.services.mozilla.com/D45616

(In reply to Brian Grinstead [:bgrins] from comment #3)

(In reply to Vlad Stanimir from comment #2)

Hello, i was browsing the code repo and noticed that there are a lot of tests that declare bindings https://dxr.mozilla.org/mozilla-central/search?q=%3Cbindings+-path%3A*.rs+-path%3A*.cpp&redirect=false

These are being ported to use shadow dom when applicable in Bug 1510785 / https://phabricator.services.mozilla.com/D45616

for some reason the phab didn't show me the whole history of the revision, so if Brendan is about to finish all the conversion there, then let's keep that bug to remove tests that live under MOZ_XBL flag.

No longer blocks: 1510785
Depends on: 1510785, 1583314

Converting is happening in Bug 1583314. We could use this bug to actually remove the tests once we start to rip out XBL code.

Summary: remove or convert XBL related tests → Remove XBL related tests

At this point we can remove any test that is marked with skip-if = !xbl. This should make further code removal easier.

Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

Depends on D50650

Depends on D50651

Attachment #9104301 - Attachment description: Bug 1587142 - Bug 1587142 - Remove miscellaneous other XBL tests → Bug 1587142 - Bug 1587142 - Remove miscellaneous XBL tests

Depends on D50652

Attachment #9104301 - Attachment description: Bug 1587142 - Bug 1587142 - Remove miscellaneous XBL tests → Bug 1587142 - Remove miscellaneous XBL tests
Keywords: leave-open
Blocks: 1591588
Attachment #9104299 - Attachment description: Bug 1587142 - Remove XBL tests in dom/ → Bug 1587142 - Remove most XBL tests in dom/
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f9d771a4e4d
Remove miscellaneous XBL tests r=bzbarsky
Depends on: 1587627

It could be interesting to look at how code coverage data will change after this, to see if there is opportunity for even more dead code removal.

(In reply to Marco Castelluccio [:marco] from comment #13)

It could be interesting to look at how code coverage data will change after this, to see if there is opportunity for even more dead code removal.

I like that idea! I'm planning to track any code removal bugs that we can do in Bug 1566221. Could you help track down the changes in ccov? I was planning to land this in pieces but I could hold off and only push dom/ changes until once layout/ is ready if that makes it easier.

Flags: needinfo?(mcastelluccio)

I thought about it some more, and I think we've already landed the changeset that would have caused the ccov changes in Bug 1583314. At that point we disabled MOZ_XBL so all of these tests became effectively skip-if=true. So it'd be interesting to see if we detect the GetWholeText removal (in Bug 1591588) in ccov after those changesets landed.

Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ccbd65a92619
Remove most XBL tests in dom/ r=bzbarsky
See Also: → 1593058

(In reply to Brian Grinstead [:bgrins] from comment #15)

I thought about it some more, and I think we've already landed the changeset that would have caused the ccov changes in Bug 1583314. At that point we disabled MOZ_XBL so all of these tests became effectively skip-if=true. So it'd be interesting to see if we detect the GetWholeText removal (in Bug 1591588) in ccov after those changesets landed.

The if branch was taken before 1 and not taken after 2.

From a quick look at the results of a quick and dirty script, I also see a few differences in dom/base/FragmentOrElement.cpp and dom/base/nsGlobalWindowInner.cpp (e.g. SetXBLInsertionPoint 3 is covered before and not after). Analyzing the differences is a bit of a manual process though, as clearly there are a few differences that are simply due to intermittency and a few other which are due to other changes which landed at the same time.

Flags: needinfo?(mcastelluccio)
See Also: → 1593710
Blocks: 1579952
Blocks: 1594122
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f86d070215fd
Remove XBL tests in layout/ r=bzbarsky
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.