Support context menus from top-level HTML pages

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P5
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: bdahl, Assigned: bdahl)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

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

Updated

11 months ago
Priority: -- → P5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

11 months 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
Assignee: nobody → bdahl

Comment 4

11 months 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.
Attachment #8983963 - Flags: review?(dholbert) → review-
Blocks: 1459972
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

10 months 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=
Attachment #8983964 - Flags: review?(bgrinstead) → review+
See Also: → 1469339
Blocks: 1456852
Blocks: 1469341

Updated

10 months ago
Blocks: 1467572
(Assignee)

Comment 7

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

10 months 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.)
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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!)
Attachment #8983963 - Flags: review?(dbaron) → review+
(Assignee)

Comment 14

10 months 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
Flags: needinfo?(dbaron)
Blocks: 1460691
(Assignee)

Comment 15

10 months ago
I talked with dbaron a bit about the above and it still seems reasonable for the frames to be on the nsCanvasFrame.
Flags: needinfo?(dbaron)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

10 months 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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
(Assignee)

Comment 24

10 months 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.
Comment hidden (mozreview-request)

Comment 26

10 months 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...
Attachment #8983963 - Flags: review?(mats) → review-

Comment 27

10 months 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.
(Assignee)

Comment 28

10 months 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 32

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

10 months 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).
Attachment #8983963 - Flags: review?(mats) → review-
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

10 months 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
Attachment #8983963 - Flags: review?(mats) → review+

Comment 40

10 months ago
mozreview-review
Comment on attachment 8988020 [details]
Bug 1466897 - Rename nsIRootBox to nsIPopupContainer.

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

Thanks.
Attachment #8988020 - Flags: review?(mats) → review+

Comment 41

10 months 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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 45

10 months 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

Comment 47

10 months 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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 51

10 months 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.
Flags: needinfo?(bdahl)
Attachment #8983963 - Flags: review+ → review?(mats)

Comment 52

10 months 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
Attachment #8983963 - Flags: review?(mats) → review+

Comment 53

10 months 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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 57

10 months 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

Comment 58

10 months 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
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.