Skip to content

DevTools: Improve browser extension iframe support #19827

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

Merged
merged 10 commits into from
Sep 14, 2020

Conversation

omarsy
Copy link
Contributor

@omarsy omarsy commented Sep 13, 2020

Summary

This pull requests is intended to improve iframe support for devtools browser extensions(#18945), without add a code to pass the hook through from the parent.

image

Test Plan

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 13, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 554ea7e:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Sep 13, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 554ea7e

@sizebot
Copy link

sizebot commented Sep 13, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 554ea7e

@bvaughn bvaughn self-assigned this Sep 14, 2020
@bvaughn bvaughn self-requested a review September 14, 2020 13:00
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good.

}

function isMainWindow(targetWindow: any): boolean {
return targetWindow.self === targetWindow.top;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why window.self rather than just window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the same but self can work with worker(it is more flexible)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok just wanted to confirm I wasn't missing anything. 😄

@bvaughn bvaughn merged commit ec39a5e into facebook:master Sep 14, 2020
@omarsy
Copy link
Contributor Author

omarsy commented Sep 14, 2020

Thank you @bvaughn !

@bvaughn
Copy link
Contributor

bvaughn commented Sep 17, 2020

Looks like this change broke the development test harness. To repro:

  1. Disable the React DevTools extension entirely.
  2. Run the local test harness.
  3. See nothing in the Components tab.

I think I'm going to have to revert it for now. Any interest in looking back into it @omarsy?

bvaughn pushed a commit to bvaughn/react that referenced this pull request Sep 17, 2020
@bvaughn bvaughn mentioned this pull request Sep 17, 2020
@omarsy
Copy link
Contributor Author

omarsy commented Sep 17, 2020

ok @bvaughn I'm looking it

bvaughn pushed a commit that referenced this pull request Sep 17, 2020
todortotev pushed a commit to todortotev/react that referenced this pull request Sep 18, 2020
Co-authored-by: Joel DSouza <joel.dsouza@kapturecrm.com>
Co-authored-by: Damien Maillard <damien.maillard@dailymotion.com>
Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>
todortotev pushed a commit to todortotev/react that referenced this pull request Sep 18, 2020
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
Co-authored-by: Joel DSouza <joel.dsouza@kapturecrm.com>
Co-authored-by: Damien Maillard <damien.maillard@dailymotion.com>
Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants