Closed Bug 1608515 Opened 11 months ago Closed 6 months ago

Update IsContainingWindowElementOfType for Fission

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla79
Fission Milestone M6
Tracking Status
firefox79 --- fixed

People

(Reporter: nika, Assigned: mail)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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

Blocks: 1608530

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.

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

Assignee: nobody → krourke
Attachment #9152826 - Attachment description: Bug 1608515: Update IsContainingWindowElementOfType for Fission r=barret,nika → Bug 1608515: Update IsContainingWindowElementOfType for Fission r=barret,dholbert
Attachment #9152826 - Attachment description: Bug 1608515: Update IsContainingWindowElementOfType for Fission r=barret,dholbert → Bug 1608515: Update the SVG IsContainingWindowElementOfType impl for Fission r=barret,dholbert
Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f82782c37e0
Update the SVG IsContainingWindowElementOfType impl for Fission r=barret,dholbert
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79

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.)

Flags: needinfo?(krourke)

(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 )

(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.)

👍

Flags: needinfo?(krourke)

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.)

You need to log in before you can comment on or make changes to this bug.