Skip to content
This repository was archived by the owner on Jan 30, 2025. It is now read-only.

Commit 48f591c

Browse files
inancgumusinancgumus
committed
Apply review comments (#129)
1 parent 77949eb commit 48f591c

File tree

5 files changed

+66
-33
lines changed

5 files changed

+66
-33
lines changed

‎common/browser.go

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -85,21 +85,16 @@ type Browser struct {
8585
// NewBrowser creates a new browser
8686
func NewBrowser(ctx context.Context, cancelFn context.CancelFunc, browserProc *BrowserProcess, launchOpts *LaunchOptions, logger *Logger) (*Browser, error) {
8787
b := Browser{
88-
BaseEventEmitter: NewBaseEventEmitter(ctx),
89-
ctx: ctx,
90-
cancelFn: cancelFn,
91-
state: int64(BrowserStateOpen),
92-
browserProc: browserProc,
93-
conn: nil,
94-
connected: false,
95-
launchOpts: launchOpts,
96-
contexts: make(map[cdp.BrowserContextID]*BrowserContext),
97-
defaultContext: nil,
98-
pagesMu: sync.RWMutex{},
99-
pages: make(map[target.ID]*Page),
100-
sessionIDtoTargetIDMu: sync.RWMutex{},
101-
sessionIDtoTargetID: make(map[target.SessionID]target.ID),
102-
logger: logger,
88+
BaseEventEmitter: NewBaseEventEmitter(ctx),
89+
ctx: ctx,
90+
cancelFn: cancelFn,
91+
state: int64(BrowserStateOpen),
92+
browserProc: browserProc,
93+
launchOpts: launchOpts,
94+
contexts: make(map[cdp.BrowserContextID]*BrowserContext),
95+
pages: make(map[target.ID]*Page),
96+
sessionIDtoTargetID: make(map[target.SessionID]target.ID),
97+
logger: logger,
10398
}
10499
if err := b.connect(); err != nil {
105100
return nil, err
@@ -116,21 +111,27 @@ func (b *Browser) connect() error {
116111
}
117112

118113
b.connMu.Lock()
119-
defer b.connMu.Unlock()
120114
b.connected = true
115+
b.connMu.Unlock()
116+
117+
// We don't need to lock this because `connect()` is called only in NewBrowser
121118
b.defaultContext = NewBrowserContext(b.ctx, b.conn, b, "", NewBrowserContextOptions(), b.logger)
119+
122120
return b.initEvents()
123121
}
124122

125123
func (b *Browser) disposeContext(id cdp.BrowserContextID) error {
126124
b.logger.Debugf("Browser:disposeContext", "bctxid:%v", id)
125+
127126
action := target.DisposeBrowserContext(id)
128127
if err := action.Do(cdp.WithExecutor(b.ctx, b.conn)); err != nil {
129128
return fmt.Errorf("unable to dispose browser context %T: %w", action, err)
130129
}
130+
131131
b.contextsMu.Lock()
132132
defer b.contextsMu.Unlock()
133133
delete(b.contexts, id)
134+
134135
return nil
135136
}
136137

@@ -198,12 +199,12 @@ func (b *Browser) initEvents() error {
198199

199200
func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) {
200201
b.contextsMu.RLock()
201-
var browserCtx *BrowserContext = b.defaultContext
202+
browserCtx := b.defaultContext
202203
bctx, ok := b.contexts[ev.TargetInfo.BrowserContextID]
203204
if ok {
204205
browserCtx = bctx
205206
}
206-
b.logger.Debugf("Browser:onAttachedToTarget", "sid:%v tid:%v bctxid:%v bctx nil=%t", ev.SessionID, ev.TargetInfo.TargetID, ev.TargetInfo.BrowserContextID, bctx == nil)
207+
b.logger.Debugf("Browser:onAttachedToTarget", "sid:%v tid:%v bctxid:%v bctx nil:%t", ev.SessionID, ev.TargetInfo.TargetID, ev.TargetInfo.BrowserContextID, bctx == nil)
207208
b.contextsMu.RUnlock()
208209

209210
// We're not interested in the top-level browser target, other targets or DevTools targets right now.
@@ -213,7 +214,8 @@ func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) {
213214
return
214215
}
215216

216-
if ev.TargetInfo.Type == "background_page" {
217+
switch ev.TargetInfo.Type {
218+
case "background_page":
217219
p, err := NewPage(b.ctx, b.conn.getSession(ev.SessionID), browserCtx, ev.TargetInfo.TargetID, nil, false, b.logger)
218220
if err != nil {
219221
isRunning := atomic.LoadInt64(&b.state) == BrowserStateOpen && b.IsConnected() //b.conn.isConnected()
@@ -225,15 +227,17 @@ func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) {
225227
}
226228
k6Throw(b.ctx, "cannot create NewPage for background_page event: %w", err)
227229
}
230+
228231
b.pagesMu.Lock()
229232
b.logger.Debugf("Browser:onAttachedToTarget:background_page:addTid", "sid:%v tid:%v", ev.SessionID, ev.TargetInfo.TargetID)
230233
b.pages[ev.TargetInfo.TargetID] = p
231234
b.pagesMu.Unlock()
235+
232236
b.sessionIDtoTargetIDMu.Lock()
233237
b.logger.Debugf("Browser:onAttachedToTarget:background_page:addSid", "sid:%v tid:%v", ev.SessionID, ev.TargetInfo.TargetID)
234238
b.sessionIDtoTargetID[ev.SessionID] = ev.TargetInfo.TargetID
235239
b.sessionIDtoTargetIDMu.Unlock()
236-
} else if ev.TargetInfo.Type == "page" {
240+
case "page":
237241
var opener *Page = nil
238242
b.pagesMu.RLock()
239243
if t, ok := b.pages[ev.TargetInfo.OpenerID]; ok {
@@ -260,6 +264,10 @@ func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) {
260264
b.sessionIDtoTargetID[ev.SessionID] = ev.TargetInfo.TargetID
261265
b.sessionIDtoTargetIDMu.Unlock()
262266
browserCtx.emit(EventBrowserContextPage, p)
267+
default:
268+
b.logger.Warnf(
269+
"Browser:onAttachedToTarget", "sid:%v tid:%v bctxid:%v bctx nil:%t, warning: unknownTargetType",
270+
ev.SessionID, ev.TargetInfo.TargetID, ev.TargetInfo.BrowserContextID, bctx == nil)
263271
}
264272
}
265273

@@ -379,6 +387,7 @@ func (b *Browser) Contexts() []api.BrowserContext {
379387
func (b *Browser) IsConnected() bool {
380388
b.connMu.RLock()
381389
defer b.connMu.RUnlock()
390+
382391
return b.connected
383392
}
384393

‎common/execution_context.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,10 @@ func (e *ExecutionContext) adoptElementHandle(eh *ElementHandle) (*ElementHandle
164164

165165
// evaluate will evaluate provided callable within this execution context
166166
// and return by value or handle
167-
func (e *ExecutionContext) evaluate(apiCtx context.Context, opts evaluateOptions, pageFunc goja.Value, args ...goja.Value) (res interface{}, err error) {
167+
func (e *ExecutionContext) evaluate(
168+
apiCtx context.Context,
169+
opts evaluateOptions, pageFunc goja.Value, args ...goja.Value,
170+
) (res interface{}, err error) {
168171
e.logger.Debugf(
169172
"ExecutionContext:evaluate",
170173
"sid:%s stid:%s fid:%s ectxid:%d furl:%q %s",

‎common/frame.go

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ func (f *Frame) recalculateLifecycle() {
197197
f.log.Debugf("Frame:recalculateLifecycle", "fid:%s furl:%q", f.ID(), f.URL())
198198

199199
// Start with triggered events.
200-
var events map[LifecycleEvent]bool = make(map[LifecycleEvent]bool)
200+
events := make(map[LifecycleEvent]bool)
201201
f.lifecycleEventsMu.RLock()
202202
{
203203
for k, v := range f.lifecycleEvents {
@@ -325,7 +325,7 @@ func (f *Frame) document() (*ElementHandle, error) {
325325
forceCallable: false,
326326
returnByValue: false,
327327
}
328-
result, err := f.evaluate(mainWorld, f.ctx, opts, rt.ToValue("document"))
328+
result, err := f.evaluate(f.ctx, mainWorld, opts, rt.ToValue("document"))
329329
if err != nil {
330330
return nil, fmt.Errorf("frame document: cannot evaluate in main execution context: %w", err)
331331
}
@@ -493,7 +493,12 @@ func (f *Frame) waitForExecutionContext(world executionWorld) {
493493
}
494494
}
495495

496-
func (f *Frame) waitForFunction(apiCtx context.Context, world executionWorld, predicateFn goja.Value, polling PollingType, interval int64, timeout time.Duration, args ...goja.Value) (interface{}, error) {
496+
func (f *Frame) waitForFunction(
497+
apiCtx context.Context,
498+
world executionWorld, predicateFn goja.Value,
499+
polling PollingType, interval int64, timeout time.Duration,
500+
args ...goja.Value,
501+
) (interface{}, error) {
497502
f.log.Debugf(
498503
"Frame:waitForFunction",
499504
"fid:%s furl:%q world:%s pt:%s timeout:%s",
@@ -505,6 +510,9 @@ func (f *Frame) waitForFunction(apiCtx context.Context, world executionWorld, pr
505510
defer f.executionContextMu.RUnlock()
506511

507512
execCtx := f.executionContexts[world]
513+
if execCtx == nil {
514+
return nil, fmt.Errorf("cannot find execution context: %q", world)
515+
}
508516
injected, err := execCtx.getInjectedScript(apiCtx)
509517
if err != nil {
510518
return nil, err
@@ -700,7 +708,7 @@ func (f *Frame) DispatchEvent(selector string, typ string, eventInit goja.Value,
700708
}
701709

702710
// Evaluate will evaluate provided page function within an execution context
703-
func (f *Frame) Evaluate(pageFunc goja.Value, args ...goja.Value) (result interface{}) {
711+
func (f *Frame) Evaluate(pageFunc goja.Value, args ...goja.Value) interface{} {
704712
f.log.Debugf("Frame:Evaluate", "fid:%s furl:%q", f.ID(), f.URL())
705713

706714
rt := k6common.GetRuntime(f.ctx)
@@ -711,7 +719,7 @@ func (f *Frame) Evaluate(pageFunc goja.Value, args ...goja.Value) (result interf
711719
forceCallable: true,
712720
returnByValue: true,
713721
}
714-
result, err := f.evaluate(mainWorld, f.ctx, opts, pageFunc, args...)
722+
result, err := f.evaluate(f.ctx, mainWorld, opts, pageFunc, args...)
715723
if err != nil {
716724
k6common.Throw(rt, err)
717725
}
@@ -1232,7 +1240,7 @@ func (f *Frame) SetContent(html string, opts goja.Value) {
12321240
forceCallable: true,
12331241
returnByValue: true,
12341242
}
1235-
if _, err := f.evaluate(utilityWorld, f.ctx, eopts, rt.ToValue(js), rt.ToValue(html)); err != nil {
1243+
if _, err := f.evaluate(f.ctx, utilityWorld, eopts, rt.ToValue(js), rt.ToValue(html)); err != nil {
12361244
k6common.Throw(rt, err)
12371245
}
12381246

@@ -1438,18 +1446,32 @@ func (f *Frame) WaitForTimeout(timeout int64) {
14381446
}
14391447

14401448
func (f *Frame) adoptBackendNodeID(world executionWorld, id cdp.BackendNodeID) (*ElementHandle, error) {
1449+
f.log.Debugf("Frame:adoptBackendNodeID", "fid:%s furl:%q world:%s id:%d", f.ID(), f.URL(), world, id)
1450+
14411451
f.executionContextMu.RLock()
14421452
defer f.executionContextMu.RUnlock()
14431453

14441454
ec := f.executionContexts[world]
1455+
if ec == nil {
1456+
return nil, fmt.Errorf("cannot find execution context: %q for %d", world, id)
1457+
}
14451458
return ec.adoptBackendNodeID(id)
14461459
}
14471460

1448-
func (f *Frame) evaluate(world executionWorld, apiCtx context.Context, opts evaluateOptions, pageFunc goja.Value, args ...goja.Value) (res interface{}, err error) {
1461+
func (f *Frame) evaluate(
1462+
apiCtx context.Context,
1463+
world executionWorld,
1464+
opts evaluateOptions, pageFunc goja.Value, args ...goja.Value,
1465+
) (interface{}, error) {
1466+
f.log.Debugf("Frame:evaluate", "fid:%s furl:%q world:%s opts:%s", f.ID(), f.URL(), world, opts)
1467+
14491468
f.executionContextMu.RLock()
14501469
defer f.executionContextMu.RUnlock()
14511470

14521471
ec := f.executionContexts[world]
1472+
if ec == nil {
1473+
return nil, fmt.Errorf("cannot find execution context: %q", world)
1474+
}
14531475
return ec.evaluate(apiCtx, opts, pageFunc, args...)
14541476
}
14551477

‎common/frame_session.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -848,7 +848,7 @@ func (fs *FrameSession) onAttachedToTarget(event *target.EventAttachedToTarget)
848848
// ignore errors if we're no longer connected to browser
849849
// this happens when there is no browser but we still want to
850850
// attach a worker to it.
851-
if !fs.page.browserCtx.browser.connected {
851+
if !fs.page.browserCtx.browser.IsConnected() {
852852
return
853853
}
854854
select {
@@ -860,9 +860,8 @@ func (fs *FrameSession) onAttachedToTarget(event *target.EventAttachedToTarget)
860860
event.TargetInfo.Type)
861861
return
862862
default:
863-
k6Throw(fs.ctx, "cannot create frame session (worker): %w", err)
863+
k6Throw(fs.ctx, "cannot create new worker: %w", err)
864864
}
865-
k6Throw(fs.ctx, "cannot create new worker: %w", err)
866865
}
867866
fs.page.workers[session.id] = w
868867
}

‎common/screenshotter.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func (s *screenshotter) fullPageSize(p *Page) (*Size, error) {
5151
forceCallable: true,
5252
returnByValue: true,
5353
}
54-
result, err := p.frameManager.mainFrame.evaluate(mainWorld, s.ctx, opts, rt.ToValue(`
54+
result, err := p.frameManager.mainFrame.evaluate(s.ctx, mainWorld, opts, rt.ToValue(`
5555
() => {
5656
if (!document.body || !document.documentElement) {
5757
return null;
@@ -95,7 +95,7 @@ func (s *screenshotter) originalViewportSize(p *Page) (*Size, *Size, error) {
9595
forceCallable: true,
9696
returnByValue: true,
9797
}
98-
result, err := p.frameManager.mainFrame.evaluate(mainWorld, s.ctx, opts, rt.ToValue(`
98+
result, err := p.frameManager.mainFrame.evaluate(s.ctx, mainWorld, opts, rt.ToValue(`
9999
() => (
100100
{ width: window.innerWidth, height: window.innerHeight }
101101
)`))

0 commit comments

Comments
 (0)