1250767 - Implement basic DOM bindings for Servo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

We've had these scattered across a bunch of patches, and it's time to get them in the tree.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Basic Servo Bindings. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add support for hanging servo node data off Gecko nodes. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Reorder Gecko EventState to match Servo's ElementState, and add an API to let Servo access it. v1

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8722804 [details] [diff] [review]
Part 1 - Basic Servo Bindings. v1

Review of attachment 8722804 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoBindings.cpp
@@ +100,5 @@
> +
> +int
> +Gecko_IsLink(RawGeckoElement* aElement)
> +{
> +  return aElement->State().HasAtLeastOneOfStates(NS_EVENT_STATE_VISITED | NS_EVENT_STATE_UNVISITED);

Use nsCSSRuleProcessor::IsLink() to save repeating the HasAtLeastOneOfStates(...), and to use StyleState() so that selector matching takes notice of any locked state set from devtools.

@@ +111,5 @@
> +
> +int
> +Gecko_IsVisitedLink(RawGeckoElement* aElement)
> +{
> +  return aElement->State().HasState(NS_EVENT_STATE_VISITED);

Use StyleState().

@@ +117,5 @@
> +
> +int
> +Gecko_IsUnvisitedLink(RawGeckoElement* aElement)
> +{
> +  return aElement->State().HasState(NS_EVENT_STATE_UNVISITED);

And here.

::: layout/style/ServoBindings.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

FWIW this differs from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_servo_ExternalAPI_h
> +#define mozilla_servo_ExternalAPI_h

s/ExternalAPI/ServoBindings/

@@ +66,5 @@
> +#ifdef __cplusplus
> +} // extern "C"
> +#endif
> +
> +#endif // mozilla_servo_ExternalAPI_h

And here.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8722805 [details] [diff] [review]
Part 2 - Add support for hanging servo node data off Gecko nodes. v1

Review of attachment 8722805 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

::: dom/base/nsINode.cpp
@@ +25,5 @@
>  #include "mozilla/css/StyleRule.h"
>  #include "mozilla/dom/Element.h"
>  #include "mozilla/dom/Event.h"
>  #include "mozilla/dom/ShadowRoot.h"
> +#include "mozilla/servo/ExternalAPI.h"

"mozilla/ServoBindings.h"

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8722806 [details] [diff] [review]
Part 3 - Reorder Gecko EventState to match Servo's ElementState, and add an API to let Servo access it. v1

Review of attachment 8722806 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/EventStates.h
@@ +179,5 @@
> + * long as Servo never supports a state that Gecko doesn't).
> + *
> + * This is unfortunately rather fragile for now, but we should soon have
> + * the infrastructure to statically-assert that these match up. If you
> + * need to change these, please notify somebody involved with Stylo.

Can you add a link in the comment here to Servo's ElementState definition?  (And also add a corresponding comment to element_state.rs?)

::: layout/style/ServoBindings.cpp
@@ +94,5 @@
>  
> +uint8_t
> +Gecko_ElementState(RawGeckoElement* aElement)
> +{
> +  return aElement->State().GetInternalValue() & ((1 << (NS_EVENT_STATE_HIGHEST_SERVO_BIT + 1)) - 1);

StyleState()

And please wrap at 80 columns.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8722806 [details] [diff] [review]
Part 3 - Reorder Gecko EventState to match Servo's ElementState, and add an API to let Servo access it. v1

So any time Servo adds a state we will need to do more reordering here, right?

That's fairly annoying, and _definitely_ fragile.  Can we get Servo to use states with values that match the Gecko ones, possibly?

r=me, I guess, but I'm not super-happy with this...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8722805 [details] [diff] [review]
Part 2 - Add support for hanging servo node data off Gecko nodes. v1

r=me

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8722806 [details] [diff] [review]
> Part 3 - Reorder Gecko EventState to match Servo's ElementState, and add an
> API to let Servo access it. v1
> 
> So any time Servo adds a state we will need to do more reordering here,
> right?
> 
> That's fairly annoying, and _definitely_ fragile.  Can we get Servo to use
> states with values that match the Gecko ones, possibly?
> 
> r=me, I guess, but I'm not super-happy with this...

It's possible that, with a more advanced binding generator and various other things, we might be able to use the gecko values directly from servo. It would be tricky though, because we'd somehow need to associate NS_EVENT_STATE_HOVER with :hover in a stylesheet, which is tough to do automatically.

Anyway, I'm not super happy with it either, but I don't think there's much point in worrying about it for now. I'll add it to the list of things to sort out if we go production with this.

Comment 10

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d1889ab2d81

Comment 11

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e0d3ed9f499

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24309d9b5c7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/59ee7ae2519d
https://hg.mozilla.org/integration/mozilla-inbound/rev/796b681561a7

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Windows bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2dd26535e73c

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd26535e73c
https://hg.mozilla.org/integration/mozilla-inbound/rev/198c8498c62e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b3ff4c722a6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=796b681561a7
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=22312413&repo=mozilla-inbound

13:22:37     INFO -  c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\dom\html\nsGenericHTMLElement.h(348) : error C3668: 'nsGenericHTMLElement::GetClassNameW' : method with override specifier 'override' did not override any base class methods
13:22:37     INFO -  c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\dom\html\nsGenericHTMLElement.h(348) : error C2039: 'GetClassNameW' : is not a member of 'mozilla::dom::Element'
13:22:37     INFO -          c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\obj-firefox\dist\include\mozilla/dom/Element.h(141) : see declaration of 'mozilla::dom::Element'
13:22:37     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/layout/style/nsStyleStruct.cpp(3760) : error C2872: 'Side' : ambiguous symbol
13:22:37     INFO -          could be 'c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\layout\style\nsStyleConsts.h(19) : mozilla::Side mozilla::css::Side'
13:22:37     INFO -          or       'c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\obj-firefox\dist\include\mozilla\gfx\Types.h(376) : mozilla::Side'
13:22:37     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/config/rules.mk:964: recipe for target 'Unified_cpp_layout_style3.obj' failed
13:22:37     INFO -  mozmake.EXE[5]: *** [Unified_cpp_layout_style3.obj] Error 2
13:22:37     INFO -  mozmake.EXE[5]: Leaving directory 'c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/obj-firefox/layout/style'
13:22:37     INFO -  c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/src/config/recurse.mk:71: recipe for target 'layout/style/target' failed
13:22:37     INFO -  mozmake.EXE[4]: *** [layout/style/target] Error 2
13:22:37     INFO -  mozmake.EXE[4]: *** Waiting for unfinished jobs....

Comment 14

[Cameron McCormack (:heycam)]

I think I found the culprit.  Try run with the fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d14ad785657

Comment 15

[Boris Zbarsky [:bzbarsky]]

If that fixes it, please file a bug on moving AnnotateCrashReport to a less evil header?

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee63c83bf272
https://hg.mozilla.org/integration/mozilla-inbound/rev/7661eb78c9ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/a746bcde4833
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5859c104f63

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee63c83bf272
https://hg.mozilla.org/mozilla-central/rev/7661eb78c9ac
https://hg.mozilla.org/mozilla-central/rev/a746bcde4833
https://hg.mozilla.org/mozilla-central/rev/b5859c104f63