1466897 - Support context menus from top-level HTML pages

Status: RESOLVED FIXED

(Core :: XUL, enhancement, P5)

Description

[Brendan Dahl [:bdahl]]

The current plan is to support popupgroup/popupset for top level chrome html pages.

Comment 1

[Brendan Dahl [:bdahl]]

Attachment: Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

Allow a popupgroup element  to be created from top level chrome HTML
document. Implement nsIRootBox for nsCanvasFrame to store
the popup set frame. Adjust nsIRootBox::GetRootBox to be able to find the
popupset frame in a non-xul frame layout.

Review commit: https://reviewboard.mozilla.org/r/249818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249818/

Comment 2

[Brendan Dahl [:bdahl]]

Attachment: Bug 1466897 - Support devtools context menus in top level HTML pages.

Create a popugroup element to setup the context menu behind the scenes.
Create all the menu related elements with the XUL namespace.

Review commit: https://reviewboard.mozilla.org/r/249820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249820/

Comment 3

[Brendan Dahl [:bdahl]]

This differs a bit from how context menus work with XUL. In XUL, the nsDocElementBoxFrame is responsible for creating the popupgroup element and adding it as anonymous content[1], whereas in this patch the document will be responsible for creating the popupgroup. I did this so the document explicitly has to enable context menus.

One thing I'm not really sure on is where the nsPopupSetFrame should live in the non-xul frame hierarchy. Currently, it's on the nsCanvasFrame. Alternatively, it could probably go on the nsHTMLScrollFrame. I initially did this, but at the time I was creating the popupgroup element during the creation of the nsHTMLScrollFrame and I was running into issues with the order in which we create the scroll frame and link it up to it's children/parent frames. Now that I'm requiring the document to create the popupgroup, I could probably move the nsPopupSetFrame back to the nsHTMLScrollFrame, but I'll defer to the reviewer for suggestions.


[1]https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/layout/xul/nsDocElementBoxFrame.cpp#93

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

https://reviewboard.mozilla.org/r/249818/#review256670

I'd prefer to have dbaron sign off on this, rather than me... I'm not sure I fully understand the implications at this point.

I've got a few minor nits, but would you mind tagging dbaron for the next round of review?

::: layout/base/nsCSSFrameConstructor.cpp:4332
(Diff revision 1)
>  const nsCSSFrameConstructor::FrameConstructionData*
>  nsCSSFrameConstructor::FindPopupGroupData(Element* aElement,
>                                            ComputedStyle* /* unused */)
>  {
> -  if (!aElement->IsRootOfNativeAnonymousSubtree()) {
> +  if (!aElement->IsRootOfNativeAnonymousSubtree() &&
> +      (!aElement->IsInChromeDocument() || !aElement->IsInHTMLDocument())) {
>      return nullptr;

Can you add a brief comment to explain the intent of this (now-a-bit-more-complex) chain of logic?

::: layout/generic/nsCanvasFrame.h:44
(Diff revision 1)
>  public:
>    explicit nsCanvasFrame(ComputedStyle* aStyle)
>      : nsContainerFrame(aStyle, kClassID)
>      , mDoPaintFocus(false)
>      , mAddedScrollPositionListener(false)
> +    , mPopupSetFrame(nullptr)

Could you add an assertion at the very end of nsCanvasFrame::DestroyFrom(), to assert that this member-var is null?  That'll help validate that its lifetime is managed correctly.

(It looks like nsPopupSetFrame::DestroyFrom should null out this pointer[1], whenever the nsPopupSetFrame is destroyed.  But if it fails to do so for some reason, we'd be left with this member-var being a dangling pointer to a deleted frame, which is a situation we don't want to be in.)

[1] https://dxr.mozilla.org/mozilla-central/rev/199a085199815cc99daa658956a7c9436e1d436b/layout/xul/nsPopupSetFrame.cpp#113

::: layout/generic/nsCanvasFrame.h:50
(Diff revision 1)
> +  virtual nsPopupSetFrame* GetPopupSetFrame() override;
> +  virtual void SetPopupSetFrame(nsPopupSetFrame* aPopupSet) override;
> +  virtual Element* GetDefaultTooltip() override;
> +  virtual void SetDefaultTooltip(Element* aTooltip) override;

These should just be declared as "override", not "virtual...override", per 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

("Method declarations must use, at most, one of the following keywords: virtual, override, or final.")

(We have lots of old code that hasn't been upgraded to conform to this yet, but new code should be written to follow this.)

::: layout/generic/nsCanvasFrame.cpp:276
(Diff revision 1)
> +}
> +
> +void
> +nsCanvasFrame::SetPopupSetFrame(nsPopupSetFrame* aPopupSet)
> +{
> +  MOZ_ASSERT(!aPopupSet || !mPopupSetFrame, "Popup set is already defined! Only 1 allowed.");

Nit: this line is too long (93 chars > maximum of 80 chars).

Please wrap after the comma.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Sending some energy here. We would love this for 63 so we can use native context menu in RDM, and support the new RDM designs. 

Relevant bugs are:
Bug 1467572 - Implement the new design for RDM
Bug 1459972 - Use native menus in RDM

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

https://reviewboard.mozilla.org/r/249820/#review257728

Looks good to me, thanks

::: devtools/client/framework/menu.js:89
(Diff revision 1)
>   */
>  Menu.prototype.popup = function(screenX, screenY, toolbox) {
>    const doc = toolbox.doc;
> -  const popupset = doc.querySelector("popupset");
> +
> +  if (doc.documentElement.namespaceURI !== XUL_NS) {
> +    let popupgroup = doc.querySelector("popupgroup");

Would it make sense to automatically create these in chrome HTML documents without requiring JS? AFAICT we only explictly make them in XUL docs in a few tests so presumably it's implicitly there for XUL  https://searchfox.org/mozilla-central/search?q=%3Cpopupgroup&path=

Comment 7

[Brendan Dahl [:bdahl]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

https://reviewboard.mozilla.org/r/249820/#review257740

::: devtools/client/framework/menu.js:89
(Diff revision 1)
>   */
>  Menu.prototype.popup = function(screenX, screenY, toolbox) {
>    const doc = toolbox.doc;
> -  const popupset = doc.querySelector("popupset");
> +
> +  if (doc.documentElement.namespaceURI !== XUL_NS) {
> +    let popupgroup = doc.querySelector("popupgroup");

In one version of my patch I had it behave like XUL and automatically create the element during the creation of a canvas frame, but I wasn't too happy with it. It required us to add a check during the creation of every HTML page to see if it was chrome priv and top level. It also required adding some more code to the canvas frame to reflow the popupgroup. I also kind of prefer having to explicitly create it so it's easier to see where this magic element comes from.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Brendan Dahl [:bdahl] from comment #7)
> Comment on attachment 8983964 [details]
> Bug 1466897 - Support devtools context menus in top level HTML pages.
> 
> https://reviewboard.mozilla.org/r/249820/#review257740
> 
> ::: devtools/client/framework/menu.js:89
> (Diff revision 1)
> >   */
> >  Menu.prototype.popup = function(screenX, screenY, toolbox) {
> >    const doc = toolbox.doc;
> > -  const popupset = doc.querySelector("popupset");
> > +
> > +  if (doc.documentElement.namespaceURI !== XUL_NS) {
> > +    let popupgroup = doc.querySelector("popupgroup");
> 
> In one version of my patch I had it behave like XUL and automatically create
> the element during the creation of a canvas frame, but I wasn't too happy
> with it. It required us to add a check during the creation of every HTML
> page to see if it was chrome priv and top level. It also required adding
> some more code to the canvas frame to reflow the popupgroup. I also kind of
> prefer having to explicitly create it so it's easier to see where this magic
> element comes from.

OK - seems reasonable especially since all menus in HTML docs will have to run through a JS API anyway.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

https://reviewboard.mozilla.org/r/249818/#review257794

::: layout/generic/nsCanvasFrame.h:44
(Diff revision 1)
>  public:
>    explicit nsCanvasFrame(ComputedStyle* aStyle)
>      : nsContainerFrame(aStyle, kClassID)
>      , mDoPaintFocus(false)
>      , mAddedScrollPositionListener(false)
> +    , mPopupSetFrame(nullptr)

As you noted in IRC, this hypothetical assertion is fail-happy.

I think really the correct assertion (for this file and also for layout/xul/nsRootBoxFrame.cpp) would be:

    MOZ_ASSERT(!mPopupSetFrame ||
               nsLayoutUtils::IsProperAncestorFrame(this, mPopupSetFrame),
               "Someone forgot to clear popup set frame");

...an it should probably go just before we invoke the superclass ::DestroyFrom method.

(i.e. asserting that either mPopupSetFrame was already nulled out [if it was previously destroyed], OR that it's a descendant of ours and hence it's still alive and is currently being destroyed along with the rest of our subtree.)

Comment 10

[Daniel Holbert [:dholbert]]

(Comment 9 was posted in response to myself on MozReview -- just to make it clear on Bugzilla, it's in reference to my earlier request in comment 4 here about "Could you add an assertion at the very end of nsCanvasFrame::DestroyFrom(), to assert that this member-var is null?")

Comment 11

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249818/diff/1-2/

Comment 12

[Brendan Dahl [:bdahl]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249820/diff/1-2/

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

https://reviewboard.mozilla.org/r/249818/#review258914

This seems ok to me, although:
 - I think nsIRootBox should probably be renamed at this point
 - I think you should also request review from :mats, since I think he's looked into popup management a bit more than I have.  (I'm particularly interested that he check that there won't be dangling pointers.)
 - I'm curious if you considered possibilities other than nsCanvasFrame, and if so, why you rejected them

::: layout/base/nsCSSFrameConstructor.cpp:4337
(Diff revision 2)
> +  // create popupset frames.
> +  if (!aElement->IsRootOfNativeAnonymousSubtree() &&
> +      (!aElement->IsInChromeDocument() || !aElement->IsInHTMLDocument())) {
>      return nullptr;

The IsInHTMLDocument test here doesn't feel right.  This shouldn't distinguish between HTML/XHTML/SVG/etc. here.  I think a distinction that may makes sense is **not in a XUL document**.  My inclination is that you should either:
 - ditch the || !aElement->IsInHTMLDocument(), or
 - replace it with a is-a-XUL-document check

I also think it would be easier to read if the logic was written as:

if (!(conditions where we support popups)) {
  return nullptr;
}

e.g., !(aElement->IsRootOfNativeAnonymousSubtree() || aElement->IsInChromeDocument())

::: layout/generic/nsCanvasFrame.cpp:279
(Diff revision 2)
> +  MOZ_ASSERT(!aPopupSet || !mPopupSetFrame,
> +             "Popup set is already defined! Only 1 allowed.");

Could you change the nsRootBoxFrame version of this to a MOZ_ASSERT as well, if you're going to do this?

::: layout/xul/nsRootBoxFrame.cpp:45
(Diff revision 2)
>    nsIRootBox* rootBox = do_QueryFrame(rootFrame);
> +
> +  // For top level chrome privileged HTML pages the root box is the
> +  // nsCanvasFrame.
> +  if (rootFrame && !rootBox) {
> +    rootFrame = rootFrame->GetContentInsertionFrame();

why is this needed?  (Probably should be answered in a comment!)

Comment 14

[Brendan Dahl [:bdahl]]

(In reply to David Baron :dbaron: ⌚UTC-7 from comment #13)
>  - I'm curious if you considered possibilities other than nsCanvasFrame, and
> if so, why you rejected them

I went into this a bit in comment #3. I initially tried to create the popupset frame automatically during nsHTMLScrollFrame::CreateAnonymousContent (similar to what nsDocElementBoxFrame does), however that didn't work because we can't find the nsIRootBox frame from the viewport[1] at that point. It's not found because during nsHTMLScrollFrame::CreateAnonymousContent the ViewportFrame is not yet linked to the nsHTMLScrollframe. I'm open to suggestions, but maybe putting the popupset frame on the ViewportFrame itself makes more sense?

I'll address everything else above.

[1] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/layout/xul/nsPopupSetFrame.cpp#36

Comment 15

[Brendan Dahl [:bdahl]]

I talked with dbaron a bit about the above and it still seems reasonable for the frames to be on the nsCanvasFrame.

Comment 16

[Brendan Dahl [:bdahl]]

Attachment: Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

Review commit: https://reviewboard.mozilla.org/r/253274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253274/

Comment 17

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249818/diff/2-3/

Comment 18

[Brendan Dahl [:bdahl]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249820/diff/2-3/

Comment 19

[Brendan Dahl [:bdahl]]

Mats,
This has already been reviewed twice, but from dbaron:
>  - I think you should also request review from :mats, since I think he's looked into popup management a bit more than I have.  (I'm particularly interested that he check that there won't be dangling pointers.)

BTW, in the patch where I rename nsIRoot to PopupContainer it looks like cinnabar/git didn't pick up nsIRootBox.h move. I'll use mercurial when I commit and `hg move` it.

Comment 20

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249818/diff/3-4/

Comment 21

[Brendan Dahl [:bdahl]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249820/diff/3-4/

Comment 22

[Brendan Dahl [:bdahl]]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253274/diff/1-2/

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brendan Dahl [:bdahl] from comment #19)
> BTW, in the patch where I rename nsIRoot to PopupContainer it looks like
> cinnabar/git didn't pick up nsIRootBox.h move. I'll use mercurial when I
> commit and `hg move` it.

If you prefer, you can also tweak cinnabar[1] to still detect moves / copies even when they aren't a 100% match.

[1]: https://github.com/jryans/git-cinnabar/commit/2446aa25f7983c8622326b82db0c9157be6f218f

Comment 24

[Brendan Dahl [:bdahl]]

I made a few slight changes. I found that after switching nsRootBoxFrame::SetPopupSetFrame to use a MOZ_ASSERT combined with my original changes to nsCSSFrameConstructor::FindPopupGroupData, it was possible for a chrome document to create two popupgroup elements and crash (test_bug398982-2.xul). Instead I now create the popupgroup during the creation of a top level chrome privileged nsCanvasFrame. This more closely matches what XUL does (nsDocElementBoxFrame::CreateAnonymousContent), it is what I was planning to do in future tooltip work anyways, and was okay with dbaron when I talked to him.

Comment 25

[Brendan Dahl [:bdahl]]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253274/diff/2-3/

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

https://reviewboard.mozilla.org/r/249818/#review260124

Is this patch supposed to work standalone or does it depend on other work that hasn't landed yet?
When I applied it locally to a m-i debug build, it fatally asserts on startup here:
layout/base/nsCSSFrameConstructor.cpp:10151
10151       MOZ_ASSERT(!content->IsElement() || content->AsElement()->HasServoData());

(the latter term is false)

::: layout/generic/nsCanvasFrame.h:45
(Diff revision 4)
> +    , mPopupgroupContent(nullptr)
>    {}

Initializing mPopupgroupContent is redundant and common practice is to not explicitly initialize smart pointer members.

::: layout/generic/nsCanvasFrame.cpp:135
(Diff revision 4)
>    }
>  
> +  // Create a popupgroup element for chrome privileged top level canvas pages
> +  // to support context menus.
> +  if (PresContext()->IsChrome() && PresContext()->IsRoot()) {
> +    nsNodeInfoManager *nodeInfoManager = doc->NodeInfoManager();

nit: move the * next to the type name please

::: layout/generic/nsCanvasFrame.cpp:136
(Diff revision 4)
> +    RefPtr<NodeInfo> nodeInfo;
> +    nodeInfo = nodeInfoManager->GetNodeInfo(nsGkAtoms::popupgroup,

nit: might as well write this as:
RefPtr<NodeInfo> nodeInfo =
  nodeInfoManager->GetNodeInfo(...

::: layout/generic/nsCanvasFrame.cpp:309
(Diff revision 4)
> +}
> +
> +Element*
> +nsCanvasFrame::GetDefaultTooltip()
> +{
> +  NS_WARNING("GetDefaultTooltip not implemented");

A warning is OK if we intend to implement these in the near future.
If not, please change to MOZ_ASSERT_UNREACHABLE.

::: layout/generic/nsCanvasFrame.cpp:834
(Diff revision 4)
> +  if (mPopupSetFrame) {
> +    mPopupSetFrame->Reflow(aPresContext, aDesiredSize, aReflowInput, aStatus);
> +  }

This seems a little weird... I'm expecting that mPopupSetFrame is a child frame of 'this' on the kPopupList so we should create a child reflow state for it.  Anyway, I'd like to see what the frame tree looks like before reviewing this...

Comment 27

[Mats Palmgren (inactive)]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

https://reviewboard.mozilla.org/r/253274/#review260198

I think the new name loses some nuance that the old name has.
The "I" in nsIRootBox signals that it's an abstract interface, and other
such interfaces also starts with "nsI".
The "Box" hints that it's something you might find on a (XUL) frame instance.
The "ns" helps in making it unlikely to clash with other (3rd party) names.

The new name PopupContainer has none of those properties.  It's rather
bland and non-descriptive.  It also sounds like something a Windows
header might define. :-)

How about renaming the type to nsIPopupContainer for now?
(popupContainer as a variable name, and GetPopupContainer as the method
name seems fine though)

It would be fine to drop the "ns" part if we moved it inside the mozilla
namespace, but that seems like something we should do all at once for
all frame-related types.

Comment 28

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

https://reviewboard.mozilla.org/r/249818/#review260124

Uhg...missed a chunk. I'll repush.

> A warning is OK if we intend to implement these in the near future.
> If not, please change to MOZ_ASSERT_UNREACHABLE.

I have this implemented locally and will be doing the work over in bug 1460691.

> This seems a little weird... I'm expecting that mPopupSetFrame is a child frame of 'this' on the kPopupList so we should create a child reflow state for it.  Anyway, I'd like to see what the frame tree looks like before reviewing this...

Without this XULLayout() is never called on nsPopupSetFrame. I'm not very familiar with layout code, so advice is welcome on better ways to handle this.

Comment 29

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249818/diff/4-5/

Comment 30

[Brendan Dahl [:bdahl]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249820/diff/4-5/

Comment 31

[Brendan Dahl [:bdahl]]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253274/diff/3-4/

Comment 32

[Brendan Dahl [:bdahl]]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

https://reviewboard.mozilla.org/r/253274/#review260198

David proposed dropping the "nsI" part, though I do agree "nsI" follows more of the current convention for these abstract implementations. I'm happy to change, I just don't want to keep changing it around.

Comment 33

[Mats Palmgren (inactive)]

For the record, this is what the frame tree looks like:
Canvas(window)(-1) [cs=7f53ab4fb408:-moz-scrolled-canvas]<
  Block(window) <
    line 7f53ab22abd8: count=1 <
      FrameOuter(browser src=)
    >
  >
  PopupSet(popupgroup)(-1) ... [state=0009000080c00008] ...<>
>

Comment 34

[Mats Palmgren (inactive)]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

https://reviewboard.mozilla.org/r/249818/#review260856

::: layout/generic/nsCanvasFrame.cpp:741
(Diff revision 5)
>    if (mFrames.IsEmpty()) {
>      // We have no child frame, so return an empty size
>      aDesiredSize.Width() = aDesiredSize.Height() = 0;
>    } else {
>      nsIFrame* kidFrame = mFrames.FirstChild();

This piece of existing code seems to imply that mFrames can be empty.
Does that mean that we can end up with a frame tree like this now:
Canvas(window)(-1) \[cs=7f53ab4fb408:-moz-scrolled-canvas\]<
  PopupSet(popupgroup)(-1)<>
\>
and thus "kidFrame == mPopupSetFrame" here?

If so, you probably want to change this "else" to "else if (mFrames.FirstChild() != mPopupSetFrame)"

::: layout/generic/nsCanvasFrame.cpp:835
(Diff revision 5)
>                                      aStatus);
>    }
>  
>    FinishReflowWithAbsoluteFrames(aPresContext, aDesiredSize, aReflowInput, aStatus);
>  
> +  if (mPopupSetFrame) {

nit: please move this block just before FinishReflowWithAbsoluteFrames.

::: layout/generic/nsCanvasFrame.cpp:843
(Diff revision 5)
> +    mPopupSetFrame->Reflow(aPresContext, popupDesiredSize, popupReflowInput,
> +                           aStatus);

This doesn't follow the reflow protocol (missing DidReflow call).
Please call ReflowChild then FinishReflowChild instead of directly
calling Reflow.  Also, popupStatus is currently unused.  I'm guessing
you want to use that instead of aStatus to ignore this child's status -
assuming you don't want to fragment it.

Also, this code assumes mPopupSetFrame is normal flow - otherwise it
would already have been reflowed by FinishReflowWithAbsoluteFrames if
it's posiotion:absolute, or by the ViewportFrame if it's position:fixed.
I don't know if this is something you need to handle - it depends on how
it's used.  If you only plan to support normal flow then I think you can
assert that mFrames.Contains(mPopupSetFrame).

Comment 35

[Mats Palmgren (inactive)]

(In reply to Brendan Dahl [:bdahl] from comment #32)
> David proposed dropping the "nsI" part, though I do agree "nsI" follows more
> of the current convention for these abstract implementations. I'm happy to
> change, I just don't want to keep changing it around.

Perhaps dbaron assumed you'd put it in some namespace?
I tend to think we should keep "nsI" on these names until we do.
Also, I think the "I" for Interface makes the code easier to understand.

Comment 36

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249818/diff/5-6/

Comment 37

[Brendan Dahl [:bdahl]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249820/diff/5-6/

Comment 38

[Brendan Dahl [:bdahl]]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253274/diff/4-5/

Comment 39

[Mats Palmgren (inactive)]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

https://reviewboard.mozilla.org/r/249818/#review261148

Comment 40

[Mats Palmgren (inactive)]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

https://reviewboard.mozilla.org/r/253274/#review261150

Thanks.

Comment 41

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 3cb54548a043f87bc804723df5b92649126ceee7 -d d783a8ddd72d: rebasing 470893:3cb54548a043 "Bug 1466897 - Support context menus in top level chrome privileged HTML pages. r=dbaron,mats"
merging layout/xul/nsRootBoxFrame.cpp
warning: conflicts while merging layout/xul/nsRootBoxFrame.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 42

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249818/diff/6-7/

Comment 43

[Brendan Dahl [:bdahl]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249820/diff/6-7/

Comment 44

[Brendan Dahl [:bdahl]]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253274/diff/5-6/

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/067ee6342782
Support context menus in top level chrome privileged HTML pages. r=dbaron,mats
https://hg.mozilla.org/integration/autoland/rev/4443f1f6e8f3
Support devtools context menus in top level HTML pages. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/87bb8b686a7e
Rename nsIRootBox to nsIPopupContainer. r=mats

Comment 46

[Andrei Ciure[:aciure]]

Backed out 3 changesets (bug 1466897) for xpcshell failures test_ext_schemas_interactive.js and test_ext_contentscript_create_iframe.js

push that caused the backouts: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=87bb8b686a7e913611ef9c02928cadf5708d98b1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=08ad3e81d9afce3f437807660d0a958a7a2b2dbc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-searchStr=xpcshell

logs: https://treeherder.mozilla.org/logviewer.html#?job_id=186103534&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=186103558&repo=autoland

Comment 47

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20e5aadac8ce
Backed out 3 changesets for xpcshell failures test_ext_schemas_interactive.js and test_ext_contentscript_create_iframe.js CLOSED TREE

Comment 48

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249818/diff/7-8/

Comment 49

[Brendan Dahl [:bdahl]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249820/diff/7-8/

Comment 50

[Brendan Dahl [:bdahl]]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253274/diff/6-7/

Comment 51

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

When we create the dummy windowless browsers for background extensions we end up with XUL document that has the frame structure of a non-XUL document (it has a nsCanvasFrame). I'm not sure why this test doesn't also fail on desktop, as it appears to be the same there too. Maybe there's some XUL handling differences on android?

Anyway, this fixes the android failures.

Comment 52

[Mats Palmgren (inactive)]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

https://reviewboard.mozilla.org/r/249818/#review261480

Comment 53

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 159658bb8451df41eca2872586b080735ec44068 -d cb9a1ff4a249: rebasing 471394:159658bb8451 "Bug 1466897 - Support context menus in top level chrome privileged HTML pages. r=dbaron,mats"
merging layout/generic/nsCanvasFrame.cpp
merging layout/generic/nsCanvasFrame.h
merging layout/xul/nsRootBoxFrame.cpp
rebasing 471395:5e04565e4b4d "Bug 1466897 - Support devtools context menus in top level HTML pages. r=bgrins"
merging devtools/client/framework/menu.js
warning: conflicts while merging devtools/client/framework/menu.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 54

[Brendan Dahl [:bdahl]]

Comment on attachment 8983963 [details]
Bug 1466897 - Support context menus in top level chrome privileged HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249818/diff/8-9/

Comment 55

[Brendan Dahl [:bdahl]]

Comment on attachment 8983964 [details]
Bug 1466897 - Support devtools context menus in top level HTML pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249820/diff/8-9/

Comment 56

[Brendan Dahl [:bdahl]]

Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253274/diff/7-8/

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5dd2fdeda52
Support context menus in top level chrome privileged HTML pages. r=dbaron,mats
https://hg.mozilla.org/integration/autoland/rev/e9bb8a5898b9
Support devtools context menus in top level HTML pages. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/fe1a76e6f538
Rename nsIRootBox to nsIPopupContainer. r=mats

Comment 58

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c5dd2fdeda52
https://hg.mozilla.org/mozilla-central/rev/e9bb8a5898b9
https://hg.mozilla.org/mozilla-central/rev/fe1a76e6f538