feat(ObjectHandles): Implement Object Handles #943

aslushnikov · 2017-10-03T01:38:00Z

This patch:

introduces ExecutionContext class that incapsulates javascript
execution context. Examples of execution contexts are workers and
frames
introduces ObjectHandle that holds a references to the javascript
object in ExecutionContext
inherits ElementHandle from ObjectHandle

Fixes #382.

a1ph · 2017-10-05T20:58:07Z

docs/api.md

nit: () => Promise.resolve(window)

pavelfeldman · 2017-10-04T23:20:48Z

docs/api.md

executionContext

pavelfeldman · 2017-10-04T23:21:16Z

docs/api.md

evaluateObject

pavelfeldman · 2017-10-05T20:42:16Z

docs/api.md

pavelfeldman · 2017-10-05T20:43:26Z

docs/api.md

pavelfeldman · 2017-10-05T20:43:45Z

docs/api.md

getProperties(own) ? Returns Map.

pavelfeldman · 2017-10-05T20:49:05Z

docs/api.md

Return null for everything not array. Don't assign non-indexed property.

pavelfeldman · 2017-10-05T20:53:10Z

docs/api.md

Use getProperties instead.

pavelfeldman · 2017-10-05T21:05:34Z

docs/api.md

As discussed offline, renamed into JSHandle

pavelfeldman · 2017-10-05T21:06:08Z

docs/api.md

getProperty

pavelfeldman · 2017-10-05T21:07:59Z

docs/api.md

Remove type, I would add evaluate.

Killed type; as per offline discussion, holding off from adding .evaluate in this patch until its necessary

a1ph · 2017-10-05T20:59:54Z

docs/api.md

a1ph · 2017-10-05T21:00:09Z

docs/api.md

a1ph · 2017-10-05T21:02:50Z

docs/api.md

I'd expect this method to return a string

Renamed into objectHandle.jsonValue()

a1ph · 2017-10-05T21:11:40Z

utils/doclint/check_public_api/MDBuilder.js

spaceIndex === -1 ? spaceIndex.length looks sketchy.

Thanks, done.

dgozman · 2017-10-05T21:29:23Z

ObjectHandle -> JSHandle
object -> evaluateHandle
get -> getProperty
asArray, asObject -> getProperties
asJSON -> jsonValue
handle.context().evaluate(h => h.arr[2].prop, handle) as a best-practice in docs?

aslushnikov

addressed all comments

aslushnikov · 2017-10-05T21:49:47Z

docs/api.md

aslushnikov · 2017-10-05T21:53:38Z

docs/api.md

aslushnikov · 2017-10-05T21:53:40Z

docs/api.md

aslushnikov · 2017-10-05T21:55:34Z

docs/api.md

As discussed offline, renamed into JSHandle

aslushnikov · 2017-10-05T22:01:28Z

docs/api.md

aslushnikov · 2017-10-05T22:22:53Z

utils/doclint/check_public_api/MDBuilder.js

Thanks, done.

aslushnikov · 2017-10-05T22:23:17Z

docs/api.md

Renamed into objectHandle.jsonValue()

aslushnikov · 2017-10-05T23:14:40Z

docs/api.md

aslushnikov · 2017-10-05T23:35:26Z

docs/api.md

aslushnikov · 2017-10-05T23:36:07Z

docs/api.md

pavelfeldman

lgtm

This patch: - introduces ExecutionContext class that incapsulates javascript execution context. An examples of execution contexts are workers and frames - introduces ObjectHandle that holds a references to the javascript object in ExecutionContext - inherits ElementHandle from ObjectHandle Fixes puppeteer#382.

JoelEinbinder · 2017-10-06T19:21:25Z

docs/api.md

+A string can also be passed in instead of a function.
+
+```js
+const aHandle = await page.evaluateHandle('document'); // Handle on the 'document'.


Handle of the 'document'

Went on with "for".

JoelEinbinder · 2017-10-06T19:24:25Z

docs/api.md


+### class: ExecutionContext
+
+The class represents a context for javascript execution. Examples of javascript contexts are:


nit: JavaScript

JoelEinbinder · 2017-10-06T19:24:49Z

docs/api.md

+### class: ExecutionContext
+
+The class represents a context for javascript execution. Examples of javascript contexts are:
+- each frame has a separate execution context


Link to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe

JoelEinbinder · 2017-10-06T19:25:18Z

docs/api.md

+
+The class represents a context for javascript execution. Examples of javascript contexts are:
+- each frame has a separate execution context
+- all kind of workers have their own contexts


Link to https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API

JoelEinbinder · 2017-10-06T19:27:48Z

docs/api.md

+console.log(await executionContext.evaluate('1 + 2')); // prints "3"
+```
+
+[JSHandle] instances could be passed as arguments to the `frame.evaluate`:


~~could~~ can

Let's try to use a consistent tense.

Replaced here for now. Probably will replace everywhere later.

JoelEinbinder · 2017-10-06T19:47:15Z

lib/ExecutionContext.js

+  async getProperties() {
+    const response = await this._client.send('Runtime.getProperties', {
+      objectId: this._remoteObject.objectId,
+      ownProperties: true


Why do we want only enumerable and "own" properties? Feels arbitrary.

Non-enumerable might clash with names.
ownProperties is a good call, I removed this flag and added a test.

JoelEinbinder · 2017-10-06T19:50:15Z

lib/ExecutionContext.js

+    await helper.releaseObject(this._client, this._remoteObject);
+  }
+
+  /**


JoelEinbinder · 2017-10-06T19:51:23Z

lib/ElementHandle.js

+    const center = await this.executionContext().evaluate(element => {
      if (!element.ownerDocument.contains(element))
        return null;
+      if (element.nodeType !== HTMLElement.ELEMENT_NODE)


Why are we making element handles for nodes?

There's no good way to differentiate for now; as discussed offline, I'll make them throw for elementHandle methods for now.

JoelEinbinder · 2017-10-06T19:52:38Z

lib/FrameManager.js

+    const context = this._contextIdToContext.get(contextId);
+    console.assert(context, 'INTERNAL ERROR: missing context with id = ' + contextId);
+    if (remoteObject.subtype === 'node')
+      return new ElementHandle(context, this._client, remoteObject, this._mouse, this._touchscreen);


Can we check if it is an element here, instead of just a node?

We can't: it requires either an async hop to the page, or breaking change to the protocol. We'd be very unhappy with both.

JoelEinbinder · 2017-10-06T19:53:43Z

utils/doclint/check_public_api/MDBuilder.js

          let spaceIndex = actualText.indexOf(' ');
-          angleIndex = angleIndex === -1 ? angleText.length : angleIndex;
-          spaceIndex = spaceIndex === -1 ? spaceIndex.length : spaceIndex + 1;
+          angleIndex = angleIndex === -1 ? actualText.length : angleIndex;


What does this fix?

There's no such thing as 'angleText'. This fixes a typo.

JoelEinbinder · 2017-10-06T19:54:19Z

test/test.js

+    it('should work for TextNodes', SX(async function() {
+      await page.goto(PREFIX + '/input/button.html');
+      const buttonTextNode = await page.evaluateHandle(() => document.querySelector('button').firstChild);
+      await buttonTextNode.click('button');


element.click doesn't take a string.

This lies about clicking the text node.

good call about a string. Regarding clicking text node: i switched to throwing in case of text nodes as we discussed offline.

JoelEinbinder · 2017-10-06T19:56:17Z

test/test.js

+      const [frame1, frame2] = page.frames();
+      expect(frame1.executionContext()).toBeTruthy();
+      expect(frame2.executionContext()).toBeTruthy();
+      expect(frame1.executionContext() !== frame2.executionContext()).toBeTruthy();


Can we evaluate something just so this test makes me feel safer?

Sure thing; done.

aslushnikov requested a review from pavelfeldman October 3, 2017 01:46

a1ph reviewed Oct 5, 2017

View reviewed changes

pavelfeldman reviewed Oct 5, 2017

View reviewed changes

a1ph reviewed Oct 5, 2017

View reviewed changes

aslushnikov force-pushed the object-handles branch from 974a350 to 64fcfba Compare October 5, 2017 23:54

aslushnikov commented Oct 5, 2017

View reviewed changes

pavelfeldman reviewed Oct 6, 2017

View reviewed changes

aslushnikov added 11 commits October 6, 2017 11:27

s/page.object/page.evaluateHandle/g

b95db6b

s/frame.context()/frame.executionContext()/g

b51b601

s/ObjectHandle/JSHandle/g

e65c761

s/asJSON/jsonValue/g

816d19e

s/objectHandle.context/objectHandle.executionContext/g

9e63e65

Introduce JSHandle.getProperty(-ies)

44c6fe7

kill JSHandle.type()

9207cab

address doclint comments

e5df97b

Update docs/api.md

7ed28d2

Do not change ConsoleMessage in this patch

a557255

aslushnikov force-pushed the object-handles branch from 64fcfba to a557255 Compare October 6, 2017 18:48

add JSHandle.toString() test

65d1a7c

JoelEinbinder reviewed Oct 6, 2017

View reviewed changes

aslushnikov added 2 commits October 6, 2017 14:21

address comments on documentation

f506946

Address Joel's comments

ddda822

JoelEinbinder approved these changes Oct 6, 2017

View reviewed changes

aslushnikov merged commit 0d0f9b7 into puppeteer:master Oct 6, 2017

JoelEinbinder mentioned this pull request Oct 6, 2017

fix(input): clicking an element should take into account frame position #971

Merged

aslushnikov deleted the object-handles branch October 10, 2017 00:18


		### class: ExecutionContext

		The class represents a context for javascript execution. Examples of javascript contexts are:

feat(ObjectHandles): Implement Object Handles #943

feat(ObjectHandles): Implement Object Handles #943

Uh oh!

Conversation

aslushnikov commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgozman commented Oct 5, 2017

Uh oh!

aslushnikov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aslushnikov commented Oct 3, 2017 •

edited

Loading