-
Notifications
You must be signed in to change notification settings - Fork 22.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add documentation of component.utils exportFunction and cloneInto #30877
Conversation
Preview URLs
Flaws (9)URL:
URL:
URL:
URL:
External URLs (3)URL:
URL:
(comment last updated: 2024-09-20 16:58:46) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Components.utils API is an internal implementation detail of Firefox. Many years ago legacy Firefox add-ons had access to it, but WebExtensions don't. Their content scripts only have access to cloneInto
/exportFunction
, which exposes the same functionality.
There may be old links on the web pointing to these methods; if that is the concern I'd rather want redirects to the WebExtension-specific documentation.
If you need a place to host the method descriptions, I recommend to put them under Content_scripts/.
|
||
{{Compat}} | ||
|
||
> **Note:** This API is based on Chromium's [`chrome.devtools`](https://developer.chrome.com/docs/extensions/mv2/devtools/) API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a mistake? This API is Firefox-only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rob--W Indeed - I'll remove it.
@Rob--W I'm a little unclear about your feedback.
Just background?
The concern was, as per the issue: there is no documentation for exportFunction() and cloneInto() on MDN, however they are mentioned on Share objects with page scripts but only with examples but not syntax documentation or links to that documentation for the functions.
Do you mean:
Or could we simplify to :
If we aren't following the usual API pattern and as neither the component nor utils page provides much information. Or under the Share objects with page scripts? |
No, not just background information. I am asserting that it does not make sense to add the articles for
No.
Yes. That is what I meant. |
Thanks @Rob--W I've rearranged the content and added cross-references. Do you think we need to add the to functions to the navigation e.g.:
(I'm not even sure it's possible, we don't do this with any of the other narrative content.) If we're not considering these as part of the supported web extension API set, do you think we need browser compatibility data? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is partial feedback, please apply the general issues broadly to the whole PR and then request another review.
files/en-us/mozilla/add-ons/webextensions/content_scripts/cloneinto/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/content_scripts/cloneinto/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/content_scripts/cloneinto/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/content_scripts/cloneinto/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/content_scripts/cloneinto/index.md
Outdated
Show resolved
Hide resolved
|
||
## Examples | ||
|
||
This add-on script creates an object, clones it into the content window and makes it a property of the content window global: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace all references of "add-on script" with "content script"? The original Components.utils.cloneInto method and its family of functions was documented in a general way because privileged code in add-ons, in general, could use this API anywhere. But in WebExtensions, only content scripts (and userScripts) can use it.
files/en-us/mozilla/add-ons/webextensions/content_scripts/cloneinto/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/content_scripts/cloneinto/index.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Rob Wu <[email protected]>
This pull request has merge conflicts that must be resolved before it can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks technically accurate. Could you address the nits and merge it?
An Xray for an object refers to the original. Any changes to the argument made in the exported function affect the original object passed in. For example: | ||
|
||
```js | ||
// ad// privileged scope: for example, a content script |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is ad//
doing here and in other places below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea, some search/replace error, but a bit of head-scratcher.
files/en-us/mozilla/add-ons/webextensions/content_scripts/exportfunction/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/content_scripts/cloneinto/index.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Rob Wu <[email protected]>
…0877) * Add documentation of component.utils exportFunction and cloneInto * Resolved broken links, various edits * various edits * Various edits * Remove incorrect "based on" note * move exportFunction and cloneInto under Content_scripts * Cross-reference and slug corrections * Apply suggestions from review Co-authored-by: Rob Wu <[email protected]> * Further updates from feedback * casing fix * Apply suggestions from review Co-authored-by: Rob Wu <[email protected]> * Remove spurious ad// --------- Co-authored-by: Rob Wu <[email protected]>
Description
WIP to add component.utils exportFunction and cloneInto.
Todo:
Motivation
These functions are references in Sharing objects with page scripts but not documented on MDN.
Related issues and pull requests
Fixes #29771
BCD mdn/browser-compat-data#22767