-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Addition of missing devtools details #20403
base: main
Are you sure you want to change the base?
Conversation
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.
Question: why are some features of edge not a mirror of Chrome?
Because I missed updating them from the copied content. I will fix that. |
This pull request has merge conflicts that must be resolved before it can be merged. |
Hey @Rob--W, looks like this has been waiting on your review for a while! |
While I'm not against adding explicit entries for API support statuses in the BCD, the BCD also link to MDN documentation that haven't been merged yet. I am reluctant to merge these MDN articles, because they document Chrome-only behavior. It's my intent to revisit these PRs eventually, but it's a low priority for me. |
Should we try to get a Chrome reviewer for this one? |
That's not necessary. I am familiar with this API in both Firefox and Chrome. The difficulty here is presenting the content in a way such that it is obvious which parts are cross-browser and which ones are Chrome-only. The current documentation lists the least common denominator, so anyone who relies on that can build an extension that works across browsers. If we add the Chrome-specific bits here as well, then I'm concerned that an extension dev would try out the documentation and then become disappointed when they discovered that the documented features don't actually work in Firefox. |
I think that so long as the browser compatibility data is up to date, then we should be fine on that front. Web extensions don't have a standard specification that we can refer to, so since browsers are free to implement what they want to, I wouldn't expect much cross-browser support anyways. |
Also addresses #16859 |
This pull request has merge conflicts that must be resolved before it can be merged. |
This PR has been sitting around in need of a review for a while, and is the second oldest PR open. We should get this reviewed and landed, or close it! |
I think that it is okay for BCD notes to be added, even without mdn documentation. @rebloor could you update the PR and tag dotproto for review? If there is anything that requires my attention, please pm me. |
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.
LGTM
"mdn_url": "https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/API/devtools/inspectedWindow/resource", | ||
"support": { | ||
"chrome": { | ||
"version_added": true |
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.
I tried to figure out when this and the other compact entries were added in Chromium, but unfortunately I ran out of history to inspect. The most concrete thing I can say is that this is the earliest commit I see where onResourceAdded
and other such properties are exposed in third_party/closure_compiler/externs/chrome_extensions.js
. That commit landed in m68. Based on some testing of old Chromium versions, it seem to be older than that, but I can't determine how much older because I haven't been able to get a Chromium build older than 68 to run locally.
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.
Closure compiler is not the right reference for when it was introduced. It merely added typing.
Considering that Resource specifies the type of getResources, it is safe to use the same version as getResources.
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.
@dotproto @Rob--W It looks like it was added in chromium/chromium@e975d11, i.e. 22.0.1223.0, before Chrome 22 (22.0.1229). Or is this only when the docs landed? 🤔
"chrome": { | ||
"version_added": true | ||
}, | ||
"edge": "mirror", |
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.
@rebloor, does "mirror" automatically indicate that this only came in after Edge adopted Chromium?
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.
@dotproto That is my understanding
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.
I can confirm this.
"mdn_url": "https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/API/devtools/panels/SourcesPanel/onSelectionChanged", | ||
"support": { | ||
"chrome": { | ||
"version_added": true |
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.
This appears to be from before Chrome 21 as the reference to this file existed before the oldest historical entry on chrome/common/extensions/api/devtools/panels.json.
This pull request has merge conflicts that must be resolved before it can be merged. |
Co-authored-by: Simeon Vincent <[email protected]>
This pull request has merge conflicts that must be resolved before it can be merged. |
@rebloor Since this PR updates support data for Chromium, Firefox, and Safari, and we seem to be differently certain about each of the browsers, how about we split this up and extract Chromium/Safari changes into two separate PRs? I think it'll be easier to land it that way. PS: Meanwhile eef1292 has added some Chromium data for the devtools API. |
Summary
Add details, previously missing for devtools features not supported in Firefox
Related issues
Complete devtools.inspectedWindow content#28131 containing complementary updates to complete devtools.inspectedWindow documentation coverage
Fixes #16859