Sebastian Zartner [:sebo] Reporter
	Description • 8 years ago

The SVGGeometryElement interface from the SVG 2 spec. should be implemented for geometric SVG elements.

The SVGGeometryElement interface provides two functions, isPointInFill() and isPointInStroke().

Sebastian

	Sebastian Zartner [:sebo] Reporter
	Comment 1 • 8 years ago

Additionally to the above the pathLength attribute and the getTotalLength() and getPointAtLength() methods were now moved from the SVGPathElement interface to the new SVGGeometryElement interface.[1]

Sebastian

[1] https://svgwg.org/svg2-draft/changes.html#types

	Robert Longson [:longsonr] Assignee
	Comment 2 • 8 years ago

This patch creates the interface (although without isPointInFill or isPointInStroke) and changes path elements to use that interface.

Follow ups will be required for

a) supporting other elements than path
b) supporting isPointInFill and isPointInStroke

	Cameron McCormack (:heycam)
	Comment 3 • 8 years ago

Comment on attachment 8806320 [details] [diff] [review]
geometry.txt

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

Looks fine to me.  As always, don't forget a DOM peers review for the .webidl changes.

::: dom/svg/SVGGeometryElement.cpp
@@ +16,5 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::gfx;
>  
> +nsSVGElement::NumberInfo SVGGeometryElement::sNumberInfo = 

Nit: drop this trailing space while moving this line.

::: dom/svg/SVGPathElement.cpp
@@ +268,5 @@
>  bool
>  SVGPathElement::AttributeDefinesGeometry(const nsIAtom *aName)
>  {
>    return aName == nsGkAtoms::d ||
>           aName == nsGkAtoms::pathLength;

For consistency with the basic shape DOM element classes, should we just check for d here, and defer to SVGPathElementBase::AttributeDefinesGeometry() to check for pathLength?

::: layout/svg/SVGGeometryFrame.cpp
@@ +71,4 @@
>    }
>  #endif
>  
> +  NS_DISPLAY_DECL_NAME("nsDisplaySVGGeometry", TYPE_SVG_PATH_GEOMETRY)

Maybe rename TYPE_SVG_PATH_GEOMETRY to TYPE_SVG_GEOMETRY, in line with the other renames?

::: layout/svg/SVGGeometryFrame.h
@@ +31,5 @@
>  class nsSVGMarkerProperty;
>  
>  struct nsRect;
>  
> +class SVGGeometryFrame : public nsFrame

Please put this in |namespace mozilla|.

	Robert Longson [:longsonr] Assignee
	Comment 4 • 8 years ago

Needs webidl review from a DOM peer.

	Robert Longson [:longsonr] Assignee
	Comment 5 • 8 years ago

(In reply to Cameron McCormack (:heycam) from comment #3)
> >  
> > +nsSVGElement::NumberInfo SVGGeometryElement::sNumberInfo = 
> 
> Nit: drop this trailing space while moving this line.

Done.

> 
> ::: dom/svg/SVGPathElement.cpp
> @@ +268,5 @@
> >  bool
> >  SVGPathElement::AttributeDefinesGeometry(const nsIAtom *aName)
> >  {
> >    return aName == nsGkAtoms::d ||
> >           aName == nsGkAtoms::pathLength;
> 
> For consistency with the basic shape DOM element classes, should we just
> check for d here, and defer to
> SVGPathElementBase::AttributeDefinesGeometry() to check for pathLength?

I've skipped that, given that path elements don't have length attributes so the base class length elements check is not required.

> 
> ::: layout/svg/SVGGeometryFrame.cpp
> @@ +71,4 @@
> >    }
> >  #endif
> >  
> > +  NS_DISPLAY_DECL_NAME("nsDisplaySVGGeometry", TYPE_SVG_PATH_GEOMETRY)
> 
> Maybe rename TYPE_SVG_PATH_GEOMETRY to TYPE_SVG_GEOMETRY, in line with the
> other renames?

Done.

> 
> ::: layout/svg/SVGGeometryFrame.h
> @@ +31,5 @@
> >  class nsSVGMarkerProperty;
> >  
> >  struct nsRect;
> >  
> > +class SVGGeometryFrame : public nsFrame
> 
> Please put this in |namespace mozilla|.

Done.

	Peter Van der Beken [:peterv]
	Comment 6 • 8 years ago

Comment on attachment 8816931 [details] [diff] [review]
address review comments

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

::: dom/webidl/SVGGeometryElement.webidl
@@ +10,5 @@
> + * liability, trademark and document use rules apply.
> + */
> +
> +interface SVGGeometryElement : SVGGraphicsElement {
> +  [Constant]

Please make this use SameObject, like in the spec.

	Robert Longson [:longsonr] Assignee
	Comment 7 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8794924547893515d05e0bc4fb2fa092eb46211b

	Pulsebot
	Comment 8 • 8 years ago

Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b44e8715bf5
Implement SVGGeometryElement interface. r=cam r=peterv

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 9 • 8 years ago

Backed out for build bustage (SVGGeometryElement.webidl missing):

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd45619670c7fbf5c5b2ce09e33cc4f28a65db5c

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

02:01:56     INFO -  Traceback (most recent call last):
02:01:56     INFO -    File "/tools/python27/lib/python2.7/runpy.py", line 162, in _run_module_as_main
02:01:56     INFO -      "__main__", fname, loader, pkg_name)
02:01:56     INFO -    File "/tools/python27/lib/python2.7/runpy.py", line 72, in _run_code
02:01:56     INFO -      exec code in run_globals
02:01:56     INFO -    File "/builds/slave/m-in-l64-000000000000000000000/build/src/python/mozbuild/mozbuild/action/webidl.py", line 19, in <module>
02:01:56     INFO -      sys.exit(main(sys.argv[1:]))
02:01:56     INFO -    File "/builds/slave/m-in-l64-000000000000000000000/build/src/python/mozbuild/mozbuild/action/webidl.py", line 15, in main
02:01:56     INFO -      manager.generate_build_files()
02:01:56     INFO -    File "/builds/slave/m-in-l64-000000000000000000000/build/src/dom/bindings/mozwebidlcodegen/__init__.py", line 248, in generate_build_files
02:01:56     INFO -      self._parse_webidl()
02:01:56     INFO -    File "/builds/slave/m-in-l64-000000000000000000000/build/src/dom/bindings/mozwebidlcodegen/__init__.py", line 330, in _parse_webidl
02:01:56     INFO -      with open(path, 'rb') as fh:
02:01:56     INFO -  IOError: [Errno 2] No such file or directory: u'/builds/slave/m-in-l64-000000000000000000000/build/src/dom/webidl/SVGGeometryElement.webidl'
02:01:56     INFO -  gmake[5]: *** [codegen.pp] Error 1

	Pulsebot
	Comment 10 • 8 years ago

Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5d03fe0e90
Implement SVGGeometryElement interface. r=cam r=peterv

	Robert Longson [:longsonr] Assignee
	Comment 11 • 8 years ago

This time with the missing file.

	Phil Ringnalda (:philor)
	Comment 12 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4f5d03fe0e90

	Sebastian Zartner [:sebo] Reporter
	Comment 13 • 7 years ago

I've documented the interface and its property and methods:
https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement
https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/pathLength
https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/isPointInFill
https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/isPointInStroke
https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/getTotalLength
https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/getPointAtLength

Also, I've added a release note and updated the SVG 2 support:
https://developer.mozilla.org/en-US/Firefox/Releases/53#SVG
https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_2_support_in_Mozilla

Robert, can you please check the docs and whether the compatibility info is correct?

Sebastian

	Robert Longson [:longsonr] Assignee
	Comment 14 • 7 years ago

The compatibility info is misleading, All UAs (including Firefox prior to 53) support pathLength, getTotalLength and getPointAtLength on path elements as that's part of SVG 1.1. SVG 2 extends that to other elements such as circles by creating a superclass interface (SVGGeometryElement) and moving the properties and methods that were specific to path to that. It then extends that superclass interface with 2 extra methods: isPointInFill and isPointInStroke.

Also why is the last line here in red (https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_2_support_in_Mozilla#Basic_Data_Types_and_Interfaces) I'd update this page further but I don't know how to change the colours in the table.

	Sebastian Zartner [:sebo] Reporter
	Comment 15 • 7 years ago

Thank you for the fast reply!

(In reply to Robert Longson from comment #14)
> The compatibility info is misleading, All UAs (including Firefox prior to
> 53) support pathLength, getTotalLength and getPointAtLength on path elements
> as that's part of SVG 1.1. SVG 2 extends that to other elements such as
> circles by creating a superclass interface (SVGGeometryElement) and moving
> the properties and methods that were specific to path to that. It then
> extends that superclass interface with 2 extra methods: isPointInFill and
> isPointInStroke.

Oops, I totally missed that. :-/ I'll correct them accordingly and come back to you about that.

> Also why is the last line here in red
> (https://developer.mozilla.org/en-US/docs/Web/SVG/
> SVG_2_support_in_Mozilla#Basic_Data_Types_and_Interfaces) I'd update this
> page further but I don't know how to change the colours in the table.

Another thing I've missed. That's fixed now. Editing the background color requires to switch to the source code of the page.

Sebastian

	Sebastian Zartner [:sebo] Reporter
	Comment 16 • 7 years ago

(In reply to Sebastian Zartner [:sebo] from comment #15)
> Thank you for the fast reply!
> 
> (In reply to Robert Longson from comment #14)
> > The compatibility info is misleading, All UAs (including Firefox prior to
> > 53) support pathLength, getTotalLength and getPointAtLength on path elements
> > as that's part of SVG 1.1. SVG 2 extends that to other elements such as
> > circles by creating a superclass interface (SVGGeometryElement) and moving
> > the properties and methods that were specific to path to that. It then
> > extends that superclass interface with 2 extra methods: isPointInFill and
> > isPointInStroke.
> 
> Oops, I totally missed that. :-/ I'll correct them accordingly and come back
> to you about that.

I've added notes to the related pages, so it should be clear now that the property and methods initially come from SVGPathElement. Also, I've updated the description of SVGPathElement[1] to reflect this change. Please let me know if you think I should adjust the descriptions.

> > Also why is the last line here in red
> > (https://developer.mozilla.org/en-US/docs/Web/SVG/
> > SVG_2_support_in_Mozilla#Basic_Data_Types_and_Interfaces) I'd update this
> > page further but I don't know how to change the colours in the table.
> 
> Another thing I've missed. That's fixed now. Editing the background color
> requires to switch to the source code of the page.

Fixed that.

Sebastian

[1] https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement

	Robert Longson [:longsonr] Assignee
	Comment 17 • 7 years ago

Seems good, the diagram at the top of https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement is now wrong as it needs SVGGeometryElement inserted between SVGGraphicsElement and SVGPathElement.

	Sebastian Zartner [:sebo] Reporter
	Comment 18 • 7 years ago

(In reply to Robert Longson from comment #17)
> Seems good, the diagram at the top of
> https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement is now wrong
> as it needs SVGGeometryElement inserted between SVGGraphicsElement and
> SVGPathElement.

That data is maintained on GitHub. So, I've created a PR adding SVGGeometryElement (which still needs to be reviewed):

https://github.com/mozilla/kumascript/pull/74

Sebastian

	Sebastian Zartner [:sebo] Reporter
	Comment 19 • 7 years ago

(In reply to Sebastian Zartner [:sebo] from comment #18)
> (In reply to Robert Longson from comment #17)
> > Seems good, the diagram at the top of
> > https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement is now wrong
> > as it needs SVGGeometryElement inserted between SVGGraphicsElement and
> > SVGPathElement.
> 
> That data is maintained on GitHub. So, I've created a PR adding
> SVGGeometryElement (which still needs to be reviewed):
> 
> https://github.com/mozilla/kumascript/pull/74

The PR still needs a bit to be reviewed due to the Christmas vacations, though I already mark this as dev-doc-complete in the meantime.

Sebastian