Implement basic DOM bindings for Servo

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

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

Updated

3 years ago
Depends on: 1250379
Assignee

Comment 1

3 years ago
Attachment #8722804 - Flags: review?(cam)
Assignee

Updated

3 years ago
Attachment #8722806 - Attachment description: Part 3- Reorder Gecko EventState to match Servo's ElementState, and add an API to let Servo access it. v1 → Part 3 - Reorder Gecko EventState to match Servo's ElementState, and add an API to let Servo access it. v1
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.
Attachment #8722804 - Flags: review?(cam) → review+
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"
Attachment #8722805 - Flags: review?(cam) → review+
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.
Attachment #8722806 - Flags: review?(cam) → review+
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...
Attachment #8722806 - Flags: review?(bzbarsky) → review+
Comment on attachment 8722805 [details] [diff] [review]
Part 2 - Add support for hanging servo node data off Gecko nodes. v1

r=me
Attachment #8722805 - Flags: review?(bzbarsky) → review+
Assignee

Comment 9

3 years ago
(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.
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....
Flags: needinfo?(bobbyholley)
If that fixes it, please file a bug on moving AnnotateCrashReport to a less evil header?
Assignee

Updated

3 years ago
Flags: needinfo?(bobbyholley)
You need to log in before you can comment on or make changes to this bug.