Skip to content
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

[bidi][js] Add methods to add/remove handlers in Script module #14230

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jul 5, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Precursor to adding methods for Dom Mutations handling.

Motivation and Context

Adding a way to add callbacks and remove the callbacks for script module related events.

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)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement


Description

  • Added ScriptEvent constants to define event types (MESSAGE, REALM_CREATED, REALM_DESTROYED).
  • Introduced addCallback and removeCallback methods to manage event callbacks.
  • Implemented invokeCallbacks method to trigger all registered callbacks for a specific event.
  • Updated onMessage, onRealmCreated, and onRealmDestroyed methods to use ScriptEvent constants.
  • Modified subscribeAndHandleEvent to register callbacks and handle WebSocket messages.

Changes walkthrough 📝

Relevant files
Enhancement
scriptManager.js
Add callback management and event handling in ScriptManager

javascript/node/selenium-webdriver/bidi/scriptManager.js

  • Added ScriptEvent constants for event types.
  • Introduced methods to add and remove callbacks.
  • Implemented invokeCallbacks to trigger callbacks.
  • Updated event subscription methods to use ScriptEvent constants.
  • +62/-9   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling:
    The removeCallback method throws an error if the callback ID is not found. This could be handled more gracefully, perhaps by returning a boolean indicating success or failure, rather than throwing an error which might need to be caught by the caller.

    Memory Management:
    There is no apparent mechanism to prevent the #listener map from growing indefinitely as callbacks are added and not removed. This could potentially lead to memory leaks if not managed properly.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Improve the performance of the removeCallback method by storing a mapping of callback IDs to their respective event types

    In the removeCallback method, instead of iterating over all event types to find the
    callback ID, you can store a mapping of callback IDs to their respective event types. This
    will improve the performance of the removeCallback method.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [62-71]

    -let hasId = false
    -for (const [, callbacks] of this.#listener) {
    -  if (callbacks.has(id)) {
    -    callbacks.delete(id)
    -    hasId = true
    -  }
    -}
    -
    -if (!hasId) {
    +const eventType = this.#callbackIdToEventType.get(id)
    +if (!eventType) {
       throw Error(`Callback with id ${id} not found`)
     }
    +this.#listener.get(eventType).delete(id)
    +this.#callbackIdToEventType.delete(id)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a performance issue in the removeCallback method and provides a valid solution to optimize the callback management by mapping IDs to event types, which can significantly reduce the time complexity of the method.

    8
    Possible issue
    Add a check in the addCallback method to ensure the eventType is valid before adding the callback

    In the addCallback method, add a check to ensure that the eventType is valid before adding
    the callback. This will prevent potential issues if an invalid event type is passed.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [54-58]

    +if (!this.#listener.has(eventType)) {
    +  throw new Error(`Invalid event type: ${eventType}`)
    +}
     const id = ++this.#callbackId
     
     const eventCallbackMap = this.#listener.get(eventType)
     eventCallbackMap.set(id, callback)
     return id
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion correctly addresses a potential issue where an invalid eventType could be passed to the addCallback method. Adding a check enhances the robustness of the method.

    7
    Robustness
    Add error handling in the invokeCallbacks method to catch exceptions thrown by callback functions

    In the invokeCallbacks method, add error handling to catch any exceptions thrown by the
    callback functions to prevent the entire process from failing.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [78-79]

     for (const [, callback] of callbacks) {
    -  callback(data)
    +  try {
    +    callback(data)
    +  } catch (error) {
    +    console.error(`Error invoking callback: ${error}`)
    +  }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding error handling in the invokeCallbacks method is a good practice to ensure that exceptions in callbacks do not interrupt the overall execution flow, thus improving the robustness of the event handling system.

    7
    Best practice
    Remove the redundant await keyword before the return statement in the subscribeAndHandleEvent method

    In the subscribeAndHandleEvent method, remove the redundant await keyword before the
    return statement as it is not necessary.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [488]

    -return await this.subscribeAndHandleEvent(ScriptEvent.MESSAGE, callback)
    +return this.subscribeAndHandleEvent(ScriptEvent.MESSAGE, callback)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to remove the redundant await is correct as it simplifies the code without affecting functionality. However, it is a minor improvement in terms of code cleanliness and does not impact performance or functionality significantly.

    5

    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.

    1 participant