Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 7 years ago

(Note that this is filed for annotating tests in the progress of bug 1407847. The test is not going to fail without patch there, because XUL is still using Gecko backend at the moment.)

There are testcases failing because that style attribute is not handled on XUL elements.

I suppose that style attribute should always be handled by nsStyledElement, but maybe that class only handles namespace-less style attributes, but attributes in XUL documents are generally in XUL namespace, so probably handled elsewhere.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 1 • 7 years ago

I suspect that the relevant code is https://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/dom/xul/nsXULElement.cpp#2331-2350 which apparently doesn't handle the Stylo case.

It is not clear to me whether it is enough to just have it parse with Servo, or there is other code needs to be added.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 2 • 7 years ago

OK, I think just add the parsing code is enough.

It probably doesn't make much sense to share the parsing code in nsAttrValue for now, since there are much more logic involves in the latter. If we do want to share code between the two places, that should be done in a separate patch.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 3 • 7 years ago

Hmmm, not as simple as I thought...

We may not have backend type when in nsXULPrototypeElement::SetAttrAt, and there is even no guarantee that we would have that when the declaration reaches nsXULElement::MakeHeavyweight, so basically we don't know whether we should build a Gecko declaration block or a Servo one...

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 4 • 7 years ago

Cannot figure out an elegant way to fix this... Probably we can land bug 1407847 first and then rely on the fact that only document with system principal doesn't get stylo...

Anyway, I thought it would take less effort to fix this bug than annotating all related failures, but maybe I was wrong, so I decided to move forward on bug 1407847 for now.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 5 • 7 years ago

Trying the approach mentioned in comment 4.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 7 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59722e72915d2954c7af1bfd2ae2716aa00a3ee3

	Cameron McCormack (:heycam)
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8920538 [details]
Bug 1408235 part 1 - Move stylo checking logic into a separate function.

https://reviewboard.mozilla.org/r/191542/#review196732

::: layout/base/nsLayoutUtils.h:2563
(Diff revision 1)
>    static void Initialize();
>    static void Shutdown();
>  
>  #ifdef MOZ_STYLO
>    /**
> +   * Return stylo should be used for a given document URI and principal.

"Return whether"

	Cameron McCormack (:heycam)
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8920539 [details]
Bug 1408235 part 2 - Parse XUL style attribute into stylo declaration block when the document should use stylo.

https://reviewboard.mozilla.org/r/191544/#review196734

	Cameron McCormack (:heycam)
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8920540 [details]
Bug 1408235 part 3 - Update test expectation for this bug.

https://reviewboard.mozilla.org/r/191546/#review196736

Nice!

	Mozilla Autoland
	Comment 17 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b9c95f1f2bf9 -d 067d0a4c0bed: rebasing 428730:b9c95f1f2bf9 "Bug 1408235 part 1 - Move stylo checking logic into a separate function. r=heycam"
merging layout/base/nsLayoutUtils.cpp
merging layout/base/nsLayoutUtils.h
rebasing 428731:8ebd8b9e35e5 "Bug 1408235 part 2 - Parse XUL style attribute into stylo declaration block when the document should use stylo. r=heycam"
rebasing 428732:77c5fb689dff "Bug 1408235 part 3 - Update test expectation for this bug. r=heycam" (tip)
merging layout/reftests/bidi/reftest.list
merging layout/reftests/bugs/reftest.list
merging layout/reftests/css-display/reftest.list
merging layout/reftests/xul/reftest.list
warning: conflicts while merging layout/reftests/bugs/reftest.list! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/reftests/css-display/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 18 • 7 years ago

Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61e3281b53ae
part 1 - Move stylo checking logic into a separate function. r=heycam
https://hg.mozilla.org/integration/autoland/rev/46d06b3ab9e2
part 2 - Parse XUL style attribute into stylo declaration block when the document should use stylo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4a0e5e6c2b73
part 3 - Update test expectation for this bug. r=heycam

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

https://hg.mozilla.org/mozilla-central/rev/61e3281b53ae
https://hg.mozilla.org/mozilla-central/rev/46d06b3ab9e2
https://hg.mozilla.org/mozilla-central/rev/4a0e5e6c2b73