Bug 160165 – ClientRect properties should be on the prototype

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

ClientRect properties should be on the prototype. They are currently on the instance in WebKit. They are on the prototype in Firefox and Chrome.

[Chris Dumez]

Created attachment 284499 [details]
Patch

[Geoffrey Garen]

Comment on attachment 284499 [details]
Patch

r=me

[Joseph Pecoraro]

Comment on attachment 284499 [details]
Patch

The inspector tests look like regressions to me. What was valid numbers about the highlight have been lost.

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 284499 [details]
> Patch
> 
> The inspector tests look like regressions to me. What was valid numbers
> about the highlight have been lost.

Joe, As I mention in the ChangeLog, those tests are dumping JSON.stringify(clientRect).
This returns "{}" in Firefox and Chrome as well.

[Chris Dumez]

Oh, it looks like there are a few more tests failing on the bots:
Regressions: Unexpected text-only failures (3)
  media/controls/elementOrder.html [ Failure ]
  media/controls/fullscreen-button-inline-layout.html [ Failure ]
  media/controls/statusDisplayBad.html [ Failure ]

I'll investigate

[Build Bot]

Comment on attachment 284499 [details]
Patch

Attachment 284499 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1752151

New failing tests:
media/controls/fullscreen-button-inline-layout.html
media/controls/statusDisplayBad.html
media/controls/elementOrder.html

[Build Bot]

Created attachment 284507 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284499 [details]
Patch

Attachment 284499 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1752148

New failing tests:
media/controls/fullscreen-button-inline-layout.html
media/controls/statusDisplayBad.html
media/controls/elementOrder.html

[Build Bot]

Created attachment 284508 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284499 [details]
Patch

Attachment 284499 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1752174

New failing tests:
media/controls/fullscreen-button-inline-layout.html
media/controls/statusDisplayBad.html
media/controls/elementOrder.html

[Build Bot]

Created attachment 284510 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 284499 [details]
> > Patch
> > 
> > The inspector tests look like regressions to me. What was valid numbers
> > about the highlight have been lost.
> 
> Joe, As I mention in the ChangeLog, those tests are dumping
> JSON.stringify(clientRect).
> This returns "{}" in Firefox and Chrome as well.

Then it sounds like the tests should be changed to dump new values instead of what appears to be now be useless data.

[Chris Dumez]

Comment on attachment 284499 [details]
Patch

Improved patch coming. The specification says that ClientRect should have a JSON serializer:
https://drafts.fxtf.org/geometry/Overview.html#domrectreadonly

[Chris Dumez]

Created attachment 284524 [details]
Patch

[Chris Dumez]

Created attachment 284525 [details]
Patch

[Chris Dumez]

Created attachment 284526 [details]
Patch

[Chris Dumez]

Created attachment 284533 [details]
Patch

[Chris Dumez]

Requesting review again because I made additional (non-minor) changes.

[Geoffrey Garen]

Comment on attachment 284533 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284533&action=review

r=me

> Source/WebCore/bindings/js/JSClientRectCustom.cpp:47
> +    CodeBlock* codeBlock = state.codeBlock();
> +    PutPropertySlot slot(object, codeBlock ? codeBlock->isStrictMode() : false);

Can toJSON ever be called directly from a user JS function?

> Source/WebCore/bindings/js/JSClientRectCustom.cpp:53
> +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("top"), strlen("top")), jsNumber(rect.top()), slot);
> +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("right"), strlen("right")), jsNumber(rect.right()), slot);
> +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("bottom"), strlen("bottom")), jsNumber(rect.bottom()), slot);
> +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("left"), strlen("left")), jsNumber(rect.left()), slot);
> +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("width"), strlen("width")), jsNumber(rect.width()), slot);
> +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("height"), strlen("height")), jsNumber(rect.height()), slot);

I think Andreas would say you should use ASCIILIteral here.

[Chris Dumez]

(In reply to comment #19)
> Comment on attachment 284533 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284533&action=review
> 
> r=me
> 
> > Source/WebCore/bindings/js/JSClientRectCustom.cpp:47
> > +    CodeBlock* codeBlock = state.codeBlock();
> > +    PutPropertySlot slot(object, codeBlock ? codeBlock->isStrictMode() : false);
> 
> Can toJSON ever be called directly from a user JS function?

Yes, as per WebIDL. Why?

> 
> > Source/WebCore/bindings/js/JSClientRectCustom.cpp:53
> > +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("top"), strlen("top")), jsNumber(rect.top()), slot);
> > +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("right"), strlen("right")), jsNumber(rect.right()), slot);
> > +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("bottom"), strlen("bottom")), jsNumber(rect.bottom()), slot);
> > +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("left"), strlen("left")), jsNumber(rect.left()), slot);
> > +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("width"), strlen("width")), jsNumber(rect.width()), slot);
> > +    object.put(&state, Identifier::fromString(&vm, reinterpret_cast<const LChar*>("height"), strlen("height")), jsNumber(rect.height()), slot);
> 
> I think Andreas would say you should use ASCIILIteral here.

Ah Ok. Someone should probably update the bindings generator then because I copied the pattern from there.

[Chris Dumez]

Comment on attachment 284533 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284533&action=review

>>> Source/WebCore/bindings/js/JSClientRectCustom.cpp:47
>>> +    PutPropertySlot slot(object, codeBlock ? codeBlock->isStrictMode() : false);
>> 
>> Can toJSON ever be called directly from a user JS function?
> 
> Yes, as per WebIDL. Why?

c.f. http://heycam.github.io/webidl/#es-serializer which says there should be a "toJSON" property on the object. I don't see any reason why client JS would not be able to call it directly.

[Chris Dumez]

Created attachment 284547 [details]
Patch

[Chris Dumez]

Created attachment 284549 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284549 [details]
Patch

Clearing flags on attachment: 284549

Committed r203702: <http://trac.webkit.org/changeset/203702>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.