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

Warnings #89

Closed
jamarat opened this issue Jun 23, 2020 · 3 comments · Fixed by #100
Closed

Warnings #89

jamarat opened this issue Jun 23, 2020 · 3 comments · Fixed by #100

Comments

@jamarat
Copy link

jamarat commented Jun 23, 2020

There are a two warnings when running rake test for app build with ferrum:

/Library/Ruby/Gems/2.6.0/gems/ferrum-0.8/lib/ferrum/context.rb:45: warning: assigned but unused variable - target_id
/Library/Ruby/Gems/2.6.0/gems/ferrum-0.8/lib/ferrum/browser/process.rb:96: warning: instance variable @user_data_dir not initialized

Probably nothing dangerous but a bit unclean.

@Mifrill
Copy link
Collaborator

Mifrill commented Jul 26, 2020

@jamarat thanks for the raising the flag!

The first case looks weird:

https://github.com/Mifrill/ferrum/blob/master/lib/ferrum/context.rb#L44-L52

    def create_target
      target_id = @browser.command("Target.createTarget",
                                   browserContextId: @id,
                                   url: "about:blank")["targetId"]
      target = @pendings.take(@browser.timeout)
      raise NoSuchTargetError unless target.is_a?(Target)
      @targets[target.id] = target
      target
    end

we probably should remove the declaration of target_id at all here:

	  target = @pendings.take(@browser.timeout)
      raise NoSuchTargetError unless target.is_a?(Target)
      @targets[target.id] = target
      target

or we should to apply something like this:

target = @targets[target_id] || @pendings.take(@browser.timeout)

because we may have nothing in @pendings Concurrent::MVar on @pendings.take fire, according to logic in #add_target:

	def add_target(params)
      target = Target.new(@browser, params)
      if target.window?
        @targets[target.id] = target
      else
        @pendings.put(target, @browser.timeout)
      end
    end

@route
Copy link
Member

route commented Jul 27, 2020

@Mifrill I think we can remove it, agree

@Mifrill
Copy link
Collaborator

Mifrill commented Jul 27, 2020

@route we can't just remove this declaration, because we should trigger the subscriber event here:
https://github.com/Mifrill/ferrum/blob/ecab9ec5a0aa8454e7021bc5fcfc8c5f1c3fb6f7/lib/ferrum/contexts.rb#L48
by browser.client.on("Target.targetCreated") to Creates a new page.
https://chromedevtools.github.io/devtools-protocol/tot/Target/#method-createTarget

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants