8000 chore: Move draggable regions implementation from NativeBrowserView into InspectableWebContentsView by daniel-koc · Pull Request #35007 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

chore: Move draggable regions implementation from NativeBrowserView into InspectableWebContentsView #35007

New issue < 8000 /summary>

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 17, 2022
Merged

chore: Move draggable regions implementation from NativeBrowserView into InspectableWebContentsView #35007

merged 5 commits into from
Oct 17, 2022

Conversation

daniel-koc
Copy link
Contributor
@daniel-koc daniel-koc commented Jul 20, 2022 •
edited by zcbenz
Loading

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

Release Notes

Notes: none

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jul 20, 2022
@codebytere codebytere self-requested a review July 20, 2022 17:55
@zcbenz zcbenz changed the title hore: Move draggable regions implementation from NativeBrowserView into InspectableWebContentsView chore: Move draggable regions implementation from NativeBrowserView into InspectableWebContentsView Jul 22, 2022
zcbenz
zcbenz reviewed Jul 22, 2022
View reviewed changes
Copy link
Contributor
@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

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]

@daniel-koc daniel-koc force-pushed the refactor_draggable_regions branch 2 times, most recently from 590e8e8 to 6047cb7 Compare July 24, 2022 23:52
@zcbenz
Copy link
Contributor
zcbenz commented Jul 25, 2022

There is still one more lint warning, not sure why it was not showing in the first time.

$ node ./script/lint.js && npm run lint:docs
shell/browser/ui/inspectable_web_contents_view_mac.mm:283:  (cpplint) Add #include <vector> for vector<>  [build/include_what_you_use] [4]
Total errors found: 1

@daniel-koc daniel-koc force-pushed the refactor_draggable_regions branch from 6047cb7 to 1a345e2 Compare July 25, 2022 16:31
@daniel-koc
Copy link
Contributor Author

Sorry, I have troubles to launch lint.js locally on my machine

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Jul 26, 2022
@jkleinsc
Copy link
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.

@daniel-koc
Copy link
Contributor Author

@daniel-koc can y 8000 ou 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.

Yes, off course. What do you mean by "log out of CircleCI "?

@daniel-koc @zcbenz
hore: Move draggable regions implementation from NativeBrowserView in…
45a3a42 
…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.
@daniel-koc @zcbenz
fix: Lint error
61e679a
@zcbenz zcbenz force-pushed the refactor_draggable_regions branch from 1a345e2 to 61e679a Compare August 30, 2022 10:52
@nornagon nornagon mentioned this pull request Sep 8, 2022
Merged
3 tasks
@erickzhao
Copy link
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:

FAILED: obj/electron/electron_lib/electron_api_browser_view.o 
/Users/distiller/project/build-tools/third_party/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/electron/electron_lib/electron_api_browser_view.o.d -DV8_DEPRECATION_WARNINGS -DMAS_BUILD -DALLOW_RUNTIME_CONFIGURABLE_KEY_STORAGE -DDCHECK_ALWAYS_ON=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DCR_XCODE_VERSION=1331 -DCR_CLANG_REVISION=\"llvmorg-16-init-3221-gce6989fd-1\" -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_NODISCARD -D_LIBCPP_DEBUG=0 -DCR_LIBCXX_REVISION=42e738f0a1928e8d70e27b96acb02cd23beefec8 -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORES=0 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DV8_USE_EXTERNAL_STARTUP_DATA -DNODE_WANT_INTERNALS=1 -DELECTRON_PRODUCT_NAME=\"Electron\" -DELECTRON_PROJECT_NAME=\"electron\" -DENABLE_IPC_FUZZER -DLIBYUV_DISABLE_NEON -DWEBP_EXTERN=extern -DUSE_EGL -DVK_USE_PLATFORM_METAL_EXT -D_WTL_NO_AUTOMATIC_NAMESPACE -DON_FOCUS_PING_ENABLED -DTOOLKIT_VIEWS=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DUSE_CHROMIUM_ICU=1 -DU_ENABLE_TRACING=1 -DU_ENABLE_RESOURCE_TRACING=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DGOOGLE_PROTOBUF_INTERNAL_DONATE_STEAL_INLINE=0 -DHAVE_PTHREAD -DSK_CODEC_DECODES_PNG -DSK_CODEC_DECODES_WEBP -DSK_ENCODE_PNG -DSK_ENCODE_WEBP -DSK_ENABLE_SKSL -DSK_UNTIL_CRBUG_1187654_IS_FIXED -DSK_USER_CONFIG_HEADER=\"../../skia/config/SkUserConfig.h\" -DSK_WIN_FONTMGR_NO_SIMULATIONS -DSK_GL -DSK_CODEC_DECODES_JPEG -DSK_ENCODE_JPEG -DSK_HAS_WUFFS_LIBRARY -DSK_VULKAN=1 -DSK_SUPPORT_GPU=1 -DSK_GPU_WORKAROUNDS_HEADER=\"gpu/config/gpu_driver_bug_workaround_autogen.h\" -DSK_BUILD_FOR_MAC -DSK_METAL -DWEBRTC_ENABLE_AVX2 -DWEBRTC_NON_STATIC_TRACE_EVENT_HANDLERS=0 -DWEBRTC_CHROMIUM_BUILD -DWEBRTC_POSIX -DWEBRTC_MAC -DABSL_ALLOCATOR_NOTHROW=1 -DWEBRTC_USE_BUILTIN_ISAC_FIX=0 -DWEBRTC_USE_BUILTIN_ISAC_FLOAT=1 -DLOGGING_INSIDE_WEBRTC -DUSE_V8_CONTEXT_SNAPSHOT -DV8_CONTEXT_SNAPSHOT_FILENAME=\"v8_context_snapshot.x86_64.bin\" -DLEVELDB_PLATFORM_CHROMIUM=1 -DV8_COMPRESS_POINTERS -DV8_COMPRESS_POINTERS_IN_SHARED_CAGE -DV8_31BIT_SMIS_ON_64BIT_ARCH -DV8_ENABLE_SANDBOX -DCPPGC_CAGED_HEAP -DCPPGC_YOUNG_GENERATION -DCPPGC_POINTER_COMPRESSION -DOPENSCREEN_TEST_DATA_DIR=\"third_party/openscreen/src/test/data/\" -DUSE_CUPS -DHAVE_INSPECTOR=1 -DHAVE_OPENSSL=1 -DOPENSSL_NO_SSL_TRACE=1 -DNODE_HAVE_I18N_SUPPORT=1 -DNODE_USE_V8_PLATFORM=0 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_DARWIN_USE_64_BIT_INODE=1 -DFLATBUFFERS_LOCALE_INDEPENDENT=0 -I../../electron -Igen/electron -I../../third_party/blink/renderer -I../.. -Igen -I../../buildtools/third_party/libc++ -I../../third_party/perfetto/include -Igen/third_party/perfetto/build_config -Igen/third_party/perfetto -I../../third_party/libyuv/include -I../../third_party/jsoncpp/source/include -I../../third_party/libwebp/src/src -I../../third_party/khronos -I../../gpu -I../../third_party/vulkan-deps/vulkan-headers/src/include -Igen/third_party/dawn/include -I../../third_party/dawn/include -Igen/third_party/private_membership/src -Igen/third_party/shell-encryption/src -Igen/components/policy/proto -I../../third_party/wtl/include -I../../third_party/abseil-cpp -I../../third_party/boringssl/src/include -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/ced/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../net/third_party/quiche/overrides -I../../net/third_party/quiche/src/quiche/common/platform/default -I../../net/third_party/quiche/src -Igen/net/third_party/quiche/src -I../../third_party/skia -I../../third_party/wuffs/src/release/c -I../../third_party/vulkan/include -I../../third_party/webrtc_overrides -I../../third_party/webrtc -Igen/third_party/webrtc -I../../third_party/libwebm/source -I../../third_party/mesa_headers -I../../third_party/leveldatabase -I../../third_party/leveldatabase/src -I../../third_party/leveldatabase/src/include -I../../third_party/libaom/source/libaom -I../../v8/include -Igen/v8/include -Igen/third_party/metrics_proto -I../../third_party/zlib -I../../third_party/re2/src -I../../third_party/openscreen/src -Igen/third_party/openscreen/src -I../../third_party/google_toolbox_for_mac -I../../third_party/google_toolbox_for_mac/src -I../../third_party/google_toolbox_for_mac/src/AppKit -I../../third_party/google_toolbox_for_mac/src/DebugUtils -I../../third_party/google_toolbox_for_mac/src/Foundation -I../../third_party/electron_node/src -I../../third_party/electron_node/deps/uv/include -I../../third_party/flatbuffers/src/include -Wall -Werror -Wextra -Wimplicit-fallthrough -Wunreachable-code-aggressive -Wthread-safety -Wunguarded-availability -Wno-missing-field-initializers -Wno-unused-parameter -Wloop-analysis -Wno-unneeded-internal-declaration -Wenum-compare-conditional -Wno-psabi -Wno-ignored-pragma-optimize -Wno-deprecated-builtins -Wno-bitfield-constant-conversion -Wshadow -fno-delete-null-pointer-checks -fno-ident -fno-strict-aliasing -fstack-protector -femit-dwarf-unwind=no-compact-unwind -fcolor-diagnostics -fmerge-all-constants -fcrash-diagnostics-dir=../../tools/clang/crashreports -mllvm -instcombine-lower-dbg-declare=0 -ffp-contract=off -fcomplete-member-pointers -arch x86_64 -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -ffile-compilation-dir=. -no-canonical-prefixes -ftrivial-auto-var-init=pattern -O2 -fno-omit-frame-pointer -gdwarf-4 -g1 -gdwarf-aranges -isysroot sdk/xcode_links/MacOSX12.3.sdk -mmacos-version-min=10.13 -fvisibility=hidden -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang raw-ref-template-as-trivial-member -Xclang -plugin-arg-find-bad-constructs -Xclang check-gmock-objects -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Wno-null-pointer-subtraction -DPROTOBUF_ALLOW_DEPRECATED=1 -Wno-shadow -Wno-microsoft-include -std=c++17 -Wno-trigraphs -fno-exceptions -fno-rtti -nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include -isystem../../buildtools/third_party/libc++abi/trunk/include -fvisibility-inlines-hidden -Wno-deprecated-declarations -c ../../electron/shell/browser/api/electron_api_browser_view.cc -o obj/electron/electron_lib/electron_api_browser_view.o
../../electron/shell/browser/api/electron_api_browser_view.cc:109:20: error: no member named 'remove_inspectable_view' in 'electron::api::BaseWindow'
    owner_window_->remove_inspectable_view(
    ~~~~~~~~~~~~~~~^
../../electron/shell/browser/api/electron_api_browser_view.cc:116:20: error: no member named 'add_inspectable_view' in 'electron::api::BaseWindow'
    owner_window_->add_inspectable_view(view_->GetInspectableWebContentsView());
    ~~~~~~~~~~~~~~~^
2 errors generated.

cc @daniel-koc

@daniel-koc
Copy link
Contributor Author

@erickzhao I'll push the fixup today. The implementation on main changed - so the refactor requires adjustment

@daniel-koc daniel-koc requested a review from zcbenz September 26, 2022 18:21
@nornagon
Copy link
Contributor

Can we close this in favor of #35603?

@daniel-koc
Copy link
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?

@zcbenz
Copy link
Contributor
zcbenz commented Oct 5, 2022

I think we still want to move the implementation, but #35603 would remove most current draggable regions implementation code, so it would be much less work merging #35603 first and then do the move.

@daniel-koc
Copy link
Contributor Author

@zcbenz I totally agree that #35603 should first; on the other hand this PR doesn't go in the conflict with #35603. Do you think that I should close this PR anyway?

@zcbenz
Copy link
Contributor
zcbenz commented Oct 12, 2022

@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.

@daniel-koc
Copy link
Contributor Author

@zcbenz Yes, definitely I'll rebase this PR after merge #35603

@nornagon nornagon mentioned this pull request Oct 12, 2022
Merged
10 tasks
@daniel-koc
Copy link
Contributor Author

@zcbenz Yes, definitely I'll rebase this PR after merge #35603
@zcbenz I just merged main into this PR, looks OK for me for review.

@jkleinsc jkleinsc merged commit 23d4a25 into electron:main Oct 17, 2022
@release-clerk
Copy link
release-clerk bot commented Oct 17, 2022

No Release Notes

@welcome
Copy link
welcome bot commented Oct 17, 2022

Congrats on merging your first pull request! 🎉🎉🎉

@nornagon
Copy link
Contributor
nornagon commented Nov 1, 2022 •
edited
Loading

I don't think this was re 6D40 ady to land, it includes an incorrect implementation of UpdateDraggableRegions on Mac. It was not properly updated after #35603 landed.

@nornagon nornagon mentioned this pull request Nov 1, 2022
Merged
5 tasks
@daniel-koc
Copy link
Contributor Author

@nornagon Why do you think that it is because it was not properly updated after #35603 ? Definitely I'll try to figure out in the following days.

@nornagon
Copy link
Contributor
nornagon commented Nov 4, 2022

@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.

@nornagon
Copy link
Contributor
nornagon commented Nov 4, 2022

I've opened #36230 to address it.

@daniel-koc
Copy link
Contributor Author

OK, it makes sense.

@commit-lint commit-lint bot mentioned this pull request Feb 13, 2023
Open
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@daniel-koc @khalwa
chore: Move draggable regions implementation from NativeBrowserView i…
28c1695 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
no-backport semver/none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@daniel-koc @zcbenz @jkleinsc @erickzhao @nornagon @codebytere
0