chore: Move draggable regions implementation from NativeBrowserView into InspectableWebContentsView#35007
Merged
jkleinsc merged 5 commits intoelectron:mainfrom Oct 17, 2022
Conversation
zcbenz
reviewed
Jul 22, 2022
Contributor
zcbenz
left a comment
There was a problem hiding this comment.
Can you fix the lint error?
$ node ./script/lint.js && npm run lint:docs
shell/browser/ui/inspectable_web_contents_view.h:76: (cpplint) Add #include <vector> for vector<> [build/include_what_you_use] [4]
shell/browser/ui/inspectable_web_contents_view_mac.h:36: (cpplint) Add #include <vector> for vector<> [build/include_what_you_use] [4]
590e8e8 to
6047cb7
Compare
Contributor
|
There is still one more lint warning, not sure why it was not showing in the first time. |
6047cb7 to
1a345e2
Compare
Contributor
Author
|
Sorry, I have troubles to launch lint.js locally on my machine |
zcbenz
approved these changes
Jul 26, 2022
codebytere
approved these changes
Jul 27, 2022
Member
|
@daniel-koc can you rebase your PR on the latest from main? Also, can you log out of CircleCI before pushing the commit? Those two things should resolve the CI failures. |
Contributor
Author
Yes, off course. What do you mean by "log out of CircleCI "? |
…to InspectableWebContentsView The draggable regions implementation is related to WebView, so InspectableWebContentsView is a more appropriate place to put it there. Also, this refactoring will allow the subsequent extension of the WebContentsView API, which will eventually replace BrowserView API.
1a345e2 to
61e679a
Compare
3 tasks
Member
|
Windows CI and Linux CI seem to fail on the goma/setup steps, but it seems like there's a legitimate error in the macOS build: cc @daniel-koc |
Contributor
Author
|
@erickzhao I'll push the fixup today. The implementation on main changed - so the refactor requires adjustment |
Contributor
|
Can we close this in favor of #35603? |
Contributor
Author
|
This PR was originally part of BrowserView refactoring - moving the implementation to WebContentsView. Do we plan to support draggable regions for all WebContentsView in the hierarchy of window's contentView? |
Contributor
Contributor
Author
Contributor
|
@daniel-koc I think the refactoring in this PR is good to have, if you are willing to rebase this PR after merging #35603, then I am good to merge this one. |
Contributor
Author
10 tasks
Contributor
Author
zcbenz
approved these changes
Oct 17, 2022
|
No Release Notes |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
Contributor
|
I don't think this was ready to land, it includes an incorrect implementation of UpdateDraggableRegions on Mac. It was not properly updated after #35603 landed. |
5 tasks
Contributor
Author
Contributor
|
@daniel-koc for one, this included code for manipulating the child hit testing NSViews on macOS, which is a technique that was obviated by #35603. |
Contributor
|
I've opened #36230 to address it. |
Contributor
Author
|
OK, it makes sense. |
khalwa
pushed a commit
to solarwindscloud/electron
that referenced
this pull request
Feb 22, 2023
…nto InspectableWebContentsView (electron#35007) * hore: Move draggable regions implementation from NativeBrowserView into InspectableWebContentsView The draggable regions implementation is related to WebView, so InspectableWebContentsView is a more appropriate place to put it there. Also, this refactoring will allow the subsequent extension of the WebContentsView API, which will eventually replace BrowserView API. * fix: Lint error * fix: Adjusted owner-window
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
The draggable regions implementation is related to WebView, so
InspectableWebContentsView is a more appropriate place to put it there.
Also, this refactoring will allow the subsequent extension of the
WebContentsView API, which will eventually replace BrowserView API.
Checklist
npm testpassesRelease Notes
Notes: none