Brendan Dahl [:bdahl] Assignee
	Description • 6 years ago

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

	Brendan Dahl [:bdahl] Assignee
	Comment 3 • 6 years ago

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

	Daniel Holbert [:dholbert]
	Comment 4 • 6 years ago
mozreview-review

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.

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 5 • 6 years ago

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

	Brian Grinstead [:bgrins]
	Comment 6 • 6 years ago
mozreview-review

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=

	Brendan Dahl [:bdahl] Assignee
	Comment 7 • 6 years ago
mozreview-review

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.

	Brian Grinstead [:bgrins]
	Comment 8 • 6 years ago

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

	Daniel Holbert [:dholbert]
	Comment 9 • 6 years ago
mozreview-review

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

	Daniel Holbert [:dholbert]
	Comment 10 • 6 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 13 • 6 years ago
mozreview-review

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

	Brendan Dahl [:bdahl] Assignee
	Comment 14 • 6 years ago

(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

	Brendan Dahl [:bdahl] Assignee
	Comment 15 • 6 years ago

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

	Brendan Dahl [:bdahl] Assignee
	Comment 19 • 6 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 23 • 6 years ago

(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

	Brendan Dahl [:bdahl] Assignee
	Comment 24 • 6 years ago

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.

	Mats Palmgren (inactive)
	Comment 26 • 6 years ago
mozreview-review

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

	Mats Palmgren (inactive)
	Comment 27 • 6 years ago
mozreview-review

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.

	Brendan Dahl [:bdahl] Assignee
	Comment 28 • 6 years ago
mozreview-review-reply

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.

	Brendan Dahl [:bdahl] Assignee
	Comment 32 • 6 years ago
mozreview-review-reply

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.

	Mats Palmgren (inactive)
	Comment 33 • 6 years ago

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

	Mats Palmgren (inactive)
	Comment 34 • 6 years ago
mozreview-review

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

	Mats Palmgren (inactive)
	Comment 35 • 6 years ago

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

	Mats Palmgren (inactive)
	Comment 39 • 6 years ago
mozreview-review

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

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

	Mats Palmgren (inactive)
	Comment 40 • 6 years ago
mozreview-review

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

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

Thanks.

	Mozilla Autoland
	Comment 41 • 6 years ago

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)

	Pulsebot
	Comment 45 • 6 years ago

Pushed by bdahl@mozilla.com:
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

	Andrei Ciure[:aciure]
	Comment 46 • 6 years ago

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

	Pulsebot
	Comment 47 • 6 years ago

Backout by aciure@mozilla.com:
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

	Brendan Dahl [:bdahl] Assignee
	Comment 51 • 6 years ago

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.

	Mats Palmgren (inactive)
	Comment 52 • 6 years ago
mozreview-review

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

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

	Mozilla Autoland
	Comment 53 • 6 years ago

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)

	Pulsebot
	Comment 57 • 6 years ago

Pushed by bdahl@mozilla.com:
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

	Eliza Balazs [:ebalazs_]
	Comment 58 • 6 years ago
bugherder

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