#10810 pick the right target using given window handle

[asolntsev]

Description

Fix for issue #10810

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

[codecov-commenter]

Codecov Report

Merging #10811 (dbb9c72) into trunk (2fbfc62) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            trunk   #10811   +/-   ##
=======================================
  Coverage   50.46%   50.46%           
=======================================
  Files          84       84           
  Lines        5477     5477           
  Branches      278      278           
=======================================
  Hits         2764     2764           
  Misses       2435     2435           
  Partials      278      278           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fbfc62...dbb9c72. Read the comment docs.

[pujagani]

Thank you for your contribution! I appreciate that the changes are trying to create a session correctly in the current tab. However, the changes are a breaking change to two public APIs. One way to tackle this could be to find a way to not explicitly pass the windowHandle id. Maybe have a method in DevTools class that lets the user set the pageId. If that is available then that is used else the it continues to use the first page found. If you think there is a better approach to tackle this, then feel free to discuss it.

[asolntsev]

@pujagani There is a simpler way probably.
We can leave old methods createSession() and createSessionIfThereIsNotOne() - let them guess the right TargetID.
We can even mark them deprecated.

[pujagani]

Keeping both the methods and calling out the default behavior was the first idea. But, I am not sure if we want to remove the old methods createSession() and createSessionIfThereIsNotOne() at some point. Hence, I suggested allowing the user to set the pageId and use that if available. CC: @diemol When time permits, can you please have a look and suggest what might be the best way forward? Thank you!

[asolntsev]

I suggested allowing the user to set the pageId and use that if available.

I realized that user doesn't even need to set pageId.
Look, this is a typical code for using DevTools:

DevTools devTools = ((HasDevTools) webDriver).getDevTools();  // this method knows current window handle, and can set it to `DevTools` instance. 
devTools.createSession();

Nothing changes for end user.
But it implies architectural change: now instance of DevTools will know TargetId, meaning that an instance of ChromiumDriver cannot have the single field DevTools (as it has now), but will need to create a different instance of DevTools for every window. I don't know it this is consistent with the initial design of DevTools... @diemol ?

[pujagani]

That would be ideal from the user's perspective. The current Devtools design ties the lifecycle of a page session with the parent session. So to create a new page session, a fresh CDP session will be required. As you mentioned, this will require major changes including managing different page sessions and ensuring there are no hanging sessions if the parent/page session is closed. The long-term goal is to move to BiDi and only support that once all browsers support BiDi fully. I am not sure if it is worth investing time and effort to get this done accurately without a major ask from the users.

However, I agree that there has to be a way to allow users to provide the page they want the CDP session for. For that, one way is to provide overloaded methods createSession(), createSession(String windowHandle) and keep both of them.

[diemol]

I believe @asolntsev idea is very good because it would provide a better user experience. However, it requires considerable effort, and given that BiDi is slowly coming in, making major changes to the CDP integration is less than ideal.

I would keep both signatures, and when the window handle is passed by the user, use that one.

[asolntsev]

@diemol @pujagani Ok, I've returned the parameterless methods getSession() and getSessionIfNeeded(). They will work in most cases (when the webdriver has only one tab opened).

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

[diemol]

Thank you, @asolntsev!