Fix: Avoid redundant draggable region notifications [chromium/src : main]
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Commit-Queue | +2 |
1 comment
Zoraiz NaeemIt's not clear what problem this fixes or why the update was unexpected/redundant.
Isn't it possible that the caller isn't supposed to be calling this function multiple times in a row if the state hasn't changed?
Also, note that this area is not owned by anyone at Google anymore–and furthermore, changes have a high risk of randomly breaking things since a lot of the drag-and-drop paths are poorly understood. So at minimum, the CL description needs to be a lot clearer about what exactly is broken and what the fix is (so we can have some confidence that this isn't going to break something else that works fine today), and the CL itself needs to include some tests.
>> So at minimum, the CL description needs to be a lot clearer...
Done
>> Also, note that this area is not owned by anyone at Google anymore
Acked!
>> Isn't it possible that the caller isn't supposed to be calling this function multiple times in a row if the state hasn't changed?
Basically, `WebContents::SetSupportsDraggableRegions()` is called whenever a web-contents becomes primary. [1] It was done to persist the setting when primary page changed. (See [2] for motivation)
The problem that I am trying to solve arises from the change in [2]. Whenever, a page becomes primary, we call `WebContents::SetSupportsDraggableRegions()` to sync the setting.
In turn `WebViewImpl::SetSupportsDraggableRegions()` is called and set the draggable regions to zero and notify the ChromeClient. [3] This will eventually notify WebContentDelegate via `WebContentsDelegate::DraggableRegionsChanged()`. [4]
Now when we are tracking draggable regions, `WebContentsDelegate::DraggableRegionsChanged()` is only called when there is change in draggable regions. [5]
But when draggable regions are not being tracked, `WebContentsDelegate::DraggableRegionsChanged()` is always called multiple times since for each call to `SetSupportsDraggableRegions(false)`, we notify the ChromeClient [3],
This is unexpected as for webcontents that by default do not track draggable regions, we will `WebContentsDelegate::DraggableRegionsChanged()` for them multiple times which is incorrect/unexpected.
[2]
https://chromium-review.googlesource.com/c/chromium/src/+/7043264
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/exported/web_view_impl.cc
Insertions: 1, Deletions: 3.
@@ -4206,9 +4206,7 @@
if (supports_draggable_regions_) {
local_frame->View()->UpdateDocumentDraggableRegions();
} else {
- std::vector<WebDraggableRegion> web_regions =
- MainFrameImpl()->GetDocument().DraggableRegions();
- if (web_regions.empty()) {
+ if (!local_frame->GetDocument()->HasDraggableRegions()) {
return;
}
```
Change information
Fix: Avoid redundant draggable region notifications
When support for draggable regions is disabled, the code clears any
existing draggable regions and notifies ChromeClient.
This is turn notifies `WebContentsDelegate::DraggableRegionsChanged()`.
Now when draggable regions are being tracked,
`WebContentsDelegate::DraggableRegionsChanged()` is only called when the
draggable regions are changed. [1]
However, there is an edge case:
`WebContents::SetSupportsDraggableRegions()` is called every time a page
becomes primary in order to sync the support for draggable regions. [2]
Now when WebContents that do not support draggable regions (the default
for each WebContent), and every time primary page changes, ChromeClient
will still be notified that draggable regions have changed (which it
hasn't)
This behavior is different than what is described above (in case of
tracking draggable regions). And this CL fixes these
redundant/unexpected notifications,
[1]
[2]
- M third_party/blink/renderer/core/exported/web_view_impl.cc
Code-Review: +1 by Khushal Sagar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Zoraiz NaeemIt's not clear what problem this fixes or why the update was unexpected/redundant.
Isn't it possible that the caller isn't supposed to be calling this function multiple times in a row if the state hasn't changed?
Also, note that this area is not owned by anyone at Google anymore–and furthermore, changes have a high risk of randomly breaking things since a lot of the drag-and-drop paths are poorly understood. So at minimum, the CL description needs to be a lot clearer about what exactly is broken and what the fix is (so we can have some confidence that this isn't going to break something else that works fine today), and the CL itself needs to include some tests.
>> So at minimum, the CL description needs to be a lot clearer...
Done
>> Also, note that this area is not owned by anyone at Google anymore
Acked!
>> Isn't it possible that the caller isn't supposed to be calling this function multiple times in a row if the state hasn't changed?
Basically, `WebContents::SetSupportsDraggableRegions()` is called whenever a web-contents becomes primary. [1] It was done to persist the setting when primary page changed. (See [2] for motivation)
The problem that I am trying to solve arises from the change in [2]. Whenever, a page becomes primary, we call `WebContents::SetSupportsDraggableRegions()` to sync the setting.
In turn `WebViewImpl::SetSupportsDraggableRegions()` is called and set the draggable regions to zero and notify the ChromeClient. [3] This will eventually notify WebContentDelegate via `WebContentsDelegate::DraggableRegionsChanged()`. [4]
Now when we are tracking draggable regions, `WebContentsDelegate::DraggableRegionsChanged()` is only called when there is change in draggable regions. [5]
But when draggable regions are not being tracked, `WebContentsDelegate::DraggableRegionsChanged()` is always called multiple times since for each call to `SetSupportsDraggableRegions(false)`, we notify the ChromeClient [3],
This is unexpected as for webcontents that by default do not track draggable regions, we will `WebContentsDelegate::DraggableRegionsChanged()` for them multiple times which is incorrect/unexpected.
[2]
https://chromium-review.googlesource.com/c/chromium/src/+/7043264
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |