Skip to content
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

Complete devtools.inspectedWindow #28131

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rebloor
Copy link
Contributor

@rebloor rebloor commented Jul 23, 2023

Description

Add details of the features of devtools.inspectedWindow not supported by Firefox.

Motivation

To ensure the MDN documentation is authoritative and pave the way for adding BCD and removing unsupported features list from the Extend the developer tools article.

Related issues and pull requests

@rebloor rebloor added the Content:WebExt WebExtensions docs label Jul 23, 2023
@rebloor rebloor requested a review from Rob--W July 23, 2023 10:23
@rebloor rebloor requested a review from a team as a code owner July 23, 2023 10:23
@rebloor rebloor self-assigned this Jul 23, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 23, 2023

Preview URLs (6 pages)
Flaws (4)

Note! 2 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/devtools/inspectedWindow/onResourceContentCommitted
Title: devtools.inspectedWindow.onResourceContentCommitted
Flaw count: 1

  • bad_bcd_queries:
    • No BCD data for query: webextensions.api.devtools.inspectedWindow.onResourceContentCommitted

URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/devtools/inspectedWindow/Resource
Title: devtools.inspectedWindow.Resource
Flaw count: 1

  • bad_bcd_queries:
    • No BCD data for query: webextensions.api.devtools.inspectedWindow.Resource

URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/devtools/inspectedWindow/getResources
Title: devtools.inspectedWindow.getResources()
Flaw count: 1

  • bad_bcd_queries:
    • No BCD data for query: webextensions.api.devtools.inspectedWindow.getResources

URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/devtools/inspectedWindow/onResourceAdded
Title: devtools.inspectedWindow.onResourceAdded
Flaw count: 1

  • bad_bcd_queries:
    • No BCD data for query: webextensions.api.devtools.inspectedWindow.onResourceAdded

(comment last updated: 2024-09-13 19:00:44)

dotproto
dotproto previously approved these changes Jul 1, 2024
Copy link
Collaborator

@dotproto dotproto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tentatively approving with the assumption that removing Firefox support notes form the text is intentional.

@@ -49,14 +49,16 @@ let evaluating = browser.devtools.inspectedWindow.eval(
- : `string`. The JavaScript expression to evaluate. The string must evaluate to an object that can be represented as JSON, or an exception will be thrown. For example, `expression` must not evaluate to a function.
- `options` {{optional_inline}}

- : `object`. Options for the function (Note that Firefox does not yet support this options), as follows:
- : `object`. Options for the function, as follows:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like these properties are still labeled as unsupported in Firefox: https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/devtools_inspected_window.json#107-123. I also tested in Firefox Nightly 129.0a1 (2024-06-28) (64-bit) and Firefox Stable 127.0 (64-bit) and observed that they threw an "unsupported" error.

Is the intent of this change to move notes about what is (not) supported to the compat tables?


### Parameters

This function takes no parameters.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Chrome's documentation, this function takes a callback parameter and does not return a promise. https://developer.chrome.com/docs/extensions/reference/api/devtools/inspectedWindow#method-getResources

I tested and verified this behavior in Chrome 126.0.6478.127 (Official Build) (x86_64).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On MDN we are using the conventional of omitting the callback in favor of specifying a Promise value. But because we don't support this in Firefox and Chrome doesn't support Promises in their devtools API either, I wouldn't mind being explicit with the callback here.

Probably makes sense to explicitly document that Chrome doesn't support Promise in devtools namespace, which is a deviation from all other MV3 APIs that now support Promises: https://issues.chromium.org/issues/41483013

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rob--W, yesterday I didn't realize that Safari supports this method and returns a Promise (when called without a callback argument). Since Chrome still doesn't support Promises on DevTools APIs, would it make more sense to explicitly document the callback, to stick with the current convention, or something else?

Copy link
Member

@Rob--W Rob--W Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's stick to the Promise convention.

Note that the APIs are documented with the browser namespace which Chrome doesn't support yet. So the webextension-polyfill might work here.


### Return value

A [`Promise`](/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) fulfilled with an array of {{WebExtAPIRef("devtools.inspectedWindow.Resource")}}. If the request fails, the promise is rejected with an error message.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chrome returns undefined.

@dotproto dotproto dismissed their stale review July 3, 2024 00:16

Compatibility issues discovered while testing Safari 17.5

@github-actions github-actions bot added the size/m [PR only] 51-500 LoC changed label Sep 13, 2024
- `encoding`
- : `string`. Empty if the content is not encoded, otherwise the encoding name.
- `setContent`
- : `function`. Sets the content of the resource. This function takes these parameters:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[mdn-linter] reported by reviewdog 🐶

Suggested change
- : `function`. Sets the content of the resource. This function takes these parameters:
- : `function`. Sets the content of the resource. This function takes these parameters:

- : `string`. Empty if the content is not encoded, otherwise the encoding name.
- `setContent`
- : `function`. Sets the content of the resource. This function takes these parameters:
- `content`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[mdn-linter] reported by reviewdog 🐶

Suggested change
- `content`
- `content`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebExt WebExtensions docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants