1608515 - Update IsContainingWindowElementOfType for Fission

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Nika Layzell [:nika] (ooo, ni? for response)]

This method inspects the window's frame element, which is not possible from within an out-of-process frame. https://searchfox.org/mozilla-central/rev/d4d6f81e0ab479cde192453bae83d5e3edfb39d6/layout/svg/nsSVGOuterSVGFrame.cpp#905-923

Comment 1

[Nika Layzell [:nika] (ooo, ni? for response)]

This should be relatively straightforward to do once the type of the embedder element is stored on the BrowsingContext due to the changes in bug 1616171.

Comment 2

[Keefer Rourke]

Attachment: Bug 1608515: Update the SVG IsContainingWindowElementOfType impl for Fission r=barret,dholbert

In nsSVGOuterSVGFrame::IsContainingWindowElementOfType, we now
inspect the embedder from a browsing context, instead of the window's
frame element.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f82782c37e0
Update the SVG IsContainingWindowElementOfType impl for Fission r=barret,dholbert

Comment 4

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/5f82782c37e0

Comment 5

[Daniel Holbert [:dholbert]]

Late-breaking question: Is there a reason we needed to put the IsAnyAtomEqual forward-declarations in the header file here? I'm asking because it looks like IsAnyAtomEqual is only called from this one .cpp file (nsSVGOuterSVGFrame.cpp), so it kind of feels like an implementation detail of that file.

Would it make sense to just declare (and maybe just define) it at the top of that .cpp file, or was there a reason we put it in the header?

(This change would hypothetically happen in a followup bug, but I figured I'd ask here before filing a followup, in case I'm missing something.)

Comment 6

[Daniel Holbert [:dholbert]]

(Also: it'd be worth waiting a few days before hypothetically spinning up a patch for this change, because the nsSVGOuterSVGFrame.* files are about to be renamed to drop "ns" in https://phabricator.services.mozilla.com/D82650 )

Comment 7

[Keefer Rourke]

(In reply to Daniel Holbert [:dholbert] from comment #5)

Late-breaking question: Is there a reason we needed to put the IsAnyAtomEqual forward-declarations in the header file here? I'm asking because it looks like IsAnyAtomEqual is only called from this one .cpp file (nsSVGOuterSVGFrame.cpp), so it kind of feels like an implementation detail of that file.

It was definitely an implementation detail added to replace the call to frameElement->IsAnyOfHTMLElements() which performs a similar function.

Would it make sense to just declare (and maybe just define) it at the top of that .cpp file, or was there a reason we put it in the header?

That makes sense to me. I don't think there was a particular reason that I put it in the header other than — that's how I was taught to write c++, and this was my first patch to M-C :) Didn't think anything of it!

(This change would hypothetically happen in a followup bug, but I figured I'd ask here before filing a followup, in case I'm missing something.)

👍

Comment 8

[Daniel Holbert [:dholbert]]

Sounds good, thanks for the confirmation! I'll spin off a followup.

(RE C++ best practices: yeah, if you call into templated code from multiple .cpp files, then all of the templated code (including function definitions) do have to go in a header file, so that each .cpp "client" file can see & instantiate the full template with whatever configuration of the template-args that the .cpp file wants to use. But if a template is only used in a single .cpp file, then that .cpp file is the only thing that needs to see the template definition, so it's fine for the template to just live inline in that file.)