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

The attributes popupNode, popupRangeParent, popupRangeOffset, and tooltipNode all need to be moved from XULDocument.webidl to Document.webidl so that tooltips and popups work when we switch to a top level HTML window.

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

One thing I should note on the test changes:

Previously, popupNode and tooltipNode were available to chrome and content privilege XUL documents. Now that I've changed them to chrome only the attributes become "undefined" when loading a XUL content privilege page. I've updated the test files to fix this, but this really defeats the purpose of what those tests were originally testing. However, with remote XUL going away I don't think this is really a problem. I'm open to re-working the tests if needed though.

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

Comment on attachment 8996846 [details]
Bug 1480206 - Move XULDocument popup attributes to Document.

Found a bug, clearing r? until I fix it.

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

I converted crashtest 473284.xul to a chrome test since crashtests load files as content privileged XUL.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8996846 [details]
Bug 1480206 - Move XULDocument popup attributes to Document.

https://reviewboard.mozilla.org/r/260870/#review268170

::: dom/base/nsDocument.cpp:10168
(Diff revision 2)
>      readyPromise->MaybeResolve(this);
>    }
>  }
>  
> +static JSObject*
> +GetScopeObjectOfNode(nsINode* node)

So... I know you just moved this code, but I wonder whether we actually need it.  Certainly the comments should stop referencing nsNodeSH::PreCreate, which hasn't existed in a while.  And I would expect the  new setup to not ever throw; just to use the current global if one cannot be gotten from the node.

Please adjust the comment and file a followup to consider deleting this code, ok?

::: dom/base/nsDocument.cpp:10195
(Diff revision 2)
> +{
> +  if (!mDocumentContainer) {
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsPIDOMWindowOuter> piWin = mDocumentContainer->GetWindow();

It's worth adding comment here acknowledging that it's not clear why this code can't just GetWindow().

::: dom/base/nsDocument.cpp:10215
(Diff revision 2)
> +        if (pm) {
> +            node = pm->GetLastTriggerPopupNode(this);
> +        }
> +    }
> +
> +    if (node && nsContentUtils::CanCallerAccess(node)

The nsContentUtils::CanCallerAccess check is pointless now.  If the caller is JS, this function is [ChromeOnly] so it's system-principal and will pass the check.  If the caller is C++... well, there aren't any C++ callers of this method.

::: dom/base/nsDocument.cpp:10244
(Diff revision 2)
> +        aRv.Throw(NS_ERROR_FAILURE);
> +        return nullptr;
> +    }
> +
> +    nsINode* rangeParent = pm->GetMouseLocationParent();
> +    if (rangeParent && !nsContentUtils::CanCallerAccess(rangeParent)) {

Again, the CanCallerAccess check is pointless here.  So you can just do:

    return pm->GetMouseLocationParent();
    
and remove a bunch of code here.

::: dom/base/nsDocument.cpp:10253
(Diff revision 2)
> +
> +    return rangeParent;
> +}
> +
> +// Returns the rangeOffset element from the XUL Popup Manager. We check the
> +// rangeParent to determine if the caller has rights to access to the data.

Again, should just remove that stuff.

::: dom/base/nsDocument.cpp:10264
(Diff revision 2)
> +        aRv.Throw(NS_ERROR_FAILURE);
> +        return 0;
> +    }
> +
> +    nsINode* rangeParent = pm->GetMouseLocationParent();
> +    if (rangeParent && !nsContentUtils::CanCallerAccess(rangeParent)) {

Again, remove CanCallerAccess.  And then we don't need this rangeParent thing at all.

::: dom/base/nsDocument.cpp:10278
(Diff revision 2)
> +nsIDocument::GetTooltipNode()
> +{
> +  nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
> +  if (pm) {
> +    nsCOMPtr<nsINode> node = pm->GetLastTriggerTooltipNode(this);
> +    if (node && nsContentUtils::CanCallerAccess(node)) {

Again, remove CanCallerAccess.

::: dom/base/nsIDocument.h:1386
(Diff revision 2)
>    /**
>     * Clears any Servo element data stored on Elements in the document.
>     */
>    void ClearStaleServoData();
>  
> +  already_AddRefed<nsPIWindowRoot> GetWindowRoot();

Needs to be documented.

::: dom/webidl/Document.webidl:412
(Diff revision 2)
> +
> +  [ChromeOnly]
> +  attribute Node? popupNode;
> +
> +  /**
> +   * These attributes correspond to trustedGetPopupNode().rangeOffset and

There is no trustedGetPopupNode().

I guess say that these correspond to the rangeParent and rangeOffset of the popup node or something?  The original documentation here is not great.  :(

::: toolkit/content/tests/chrome/popup_trigger.js:456
(Diff revision 2)
>        for (var t = 0; t < 2; t++) {
>          var child = childframe.contentDocument;
>          var evt = child.createEvent("Event");
>          evt.initEvent("click", true, true);
>          child.documentElement.dispatchEvent(evt);
> -        is(child.documentElement.getAttribute("data"), "xnull",
> +        is(child.documentElement.getAttribute("data"), "xundefined",

Why this change?  Commit message should say.

::: toolkit/content/tests/chrome/window_tooltip.xul:116
(Diff revision 2)
>  
>      var child = $("childframe").contentDocument; 
>      var evt = child.createEvent("Event");
>      evt.initEvent("click", true, true);
>      child.documentElement.dispatchEvent(evt);
> -    is(child.documentElement.getAttribute("data"), "xnull",
> +    is(child.documentElement.getAttribute("data"), "xundefined",

And this one.

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

Comment on attachment 8996846 [details]
Bug 1480206 - Move XULDocument popup attributes to Document.

https://reviewboard.mozilla.org/r/260870/#review268170

> So... I know you just moved this code, but I wonder whether we actually need it.  Certainly the comments should stop referencing nsNodeSH::PreCreate, which hasn't existed in a while.  And I would expect the  new setup to not ever throw; just to use the current global if one cannot be gotten from the node.
> 
> Please adjust the comment and file a followup to consider deleting this code, ok?

https://bugzilla.mozilla.org/show_bug.cgi?id=1481279

> Why this change?  Commit message should say.

I left a comment in the bug about this, but I'll add it the commit message too.

	Pulsebot
	Comment 9 • 6 years ago

Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f054fef3e0b
Move XULDocument popup attributes to Document. r=bz

	ExE Boss
	Comment 10 • 6 years ago
mozreview-review

Comment on attachment 8996846 [details]
Bug 1480206 - Move XULDocument popup attributes to Document.

https://reviewboard.mozilla.org/r/260870/#review268614

::: dom/webidl/Document.webidl:410
(Diff revision 3)
>    // fired DOMContentLoaded and are ready to start layout.  This is used for the
>    // "document_idle" webextension script injection point.
>    [ChromeOnly, Throws]
>    readonly attribute Promise<Document> documentReadyForIdle;
> +
> +  [ChromeOnly]

Would using `Func="IsChromeOrXBL"` here help with solving the test issues mentioned in Comment 2?

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

(In reply to ExE Boss from comment #10)
> Would using `Func="IsChromeOrXBL"` here help with solving the test issues
> mentioned in Comment 2?

Nope, the pages are XUL with content privilege which means IsChromeOrXBL=false.

	Andrei Ciure[:aciure]
	Comment 12 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6f054fef3e0b