stylo: Enable stylo for XBL documents

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
18 days ago
3 days ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Assignee)

Description

18 days ago
Enable stylo XBL should be able to make <marquee> elements on the MDN page working https://developer.mozilla.org/en-US/docs/Web/HTML/Element/marquee
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

18 days ago
Get some tests pass, but loading .xml files will crash. Need to investigate before requesting review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53206f2f7e9d01bd830f5b37a9036befa2e690db
(Assignee)

Comment 6

16 days ago
This bug is needed for stylo because bindings can have style attribute like <marquee> [1], but we cannot enable stylo for all xbl document because the binding could apply to document on documents with gecko style backend ...

[1] http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/layout/style/xbl-marquee/xbl-marquee.xml#679
Blocks: 1243581
(Assignee)

Updated

15 days ago
See Also: → bug 1341725
Assignee: nobody → tlin
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

8 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9774f474b10fa245042c6b3d875f516970d3b9ff

Comment 15

8 days ago
mozreview-review
Comment on attachment 8866197 [details]
Bug 1363640 Part 1 - Move IsContentDocument() and IsTopLevelContentDocument() from nsDocument to nsIDocument.

https://reviewboard.mozilla.org/r/137840/#review144840
Attachment #8866197 - Flags: review?(cam) → review+

Comment 16

8 days ago
mozreview-review
Comment on attachment 8866198 [details]
Bug 1363640 Part 2 - Move the logic in SupportsServoStyleBackend() to UpdateStyleBackendType().

https://reviewboard.mozilla.org/r/137842/#review144842
Attachment #8866198 - Flags: review?(cam) → review+

Comment 17

8 days ago
mozreview-review
Comment on attachment 8866199 [details]
Bug 1363640 Part 3 - Strip whitespaces for files under dom/xbl.

https://reviewboard.mozilla.org/r/137844/#review144844
Attachment #8866199 - Flags: review?(cam) → review+

Comment 18

8 days ago
mozreview-review
Comment on attachment 8866200 [details]
Bug 1363640 Part 4 - Rename TableForBackendType to StyleSheetTableFor.

https://reviewboard.mozilla.org/r/137846/#review144846
Attachment #8866200 - Flags: review?(cam) → review+

Comment 19

8 days ago
mozreview-review
Comment on attachment 8869663 [details]
Bug 1363640 Part 5 - Add a table in nsXULPrototypeCache to cache XBL documents with servo backend.

https://reviewboard.mozilla.org/r/141248/#review144852

This generally looks OK to me, but I might hand it over to bz.

::: dom/xbl/nsXBLService.cpp:954
(Diff revision 1)
> +      StyleBackendType boundDocumentStyleBackend
> +        = aBoundDocument ? aBoundDocument->GetStyleBackendType()
> +                         : StyleBackendType::None;

Nit: this is indented too much.

I don't know if we should be using StyleBackendType::None, here.  For one thing, it will result in us using mServoXBLDocTable.

But I'm not really sure what we should be doing here for a null binding document.  I guess this is for the platform bindings stuff?  
http://searchfox.org/mozilla-central/rev/02cae69058d41e2c6b635edccbec91a6ca2cb57f/dom/xbl/nsXBLWindowKeyHandler.cpp#108  Since they don't have bindings with style sheets in them, maybe it doesn't matter.  Though it does seem safer to stick with a Gecko backed in that case, for now.
Attachment #8869663 - Flags: review?(cam) → review+
Attachment #8869663 - Flags: review?(bzbarsky)

Comment 20

8 days ago
mozreview-review
Comment on attachment 8869664 [details]
Bug 1363640 Part 6 - Use bound document's style backend when creating an XBL document.

https://reviewboard.mozilla.org/r/141250/#review144854

::: dom/base/nsIDocument.h:1260
(Diff revision 1)
>      }
>      MOZ_ASSERT(mStyleBackendType != mozilla::StyleBackendType::None);
>      return mStyleBackendType;
>    }
>  
> +  void SetStyleBackendType(mozilla::StyleBackendType aStyleBackendType) {

Maybe add a comment above this function saying that documents generally decide their style backend type themselves, and this is only used for XBL documents.

Also, please assert that mStyleBackendType is None here.
Attachment #8869664 - Flags: review?(cam) → review+

Comment 21

8 days ago
mozreview-review
Comment on attachment 8869665 [details]
Bug 1363640 Part 7 - Update reftest expectations after enabling stylo on XBL documents.

https://reviewboard.mozilla.org/r/141252/#review144858
Attachment #8869665 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

8 days ago
mozreview-review-reply
Comment on attachment 8869663 [details]
Bug 1363640 Part 5 - Add a table in nsXULPrototypeCache to cache XBL documents with servo backend.

https://reviewboard.mozilla.org/r/141248/#review144852

> Nit: this is indented too much.
> 
> I don't know if we should be using StyleBackendType::None, here.  For one thing, it will result in us using mServoXBLDocTable.
> 
> But I'm not really sure what we should be doing here for a null binding document.  I guess this is for the platform bindings stuff?  
> http://searchfox.org/mozilla-central/rev/02cae69058d41e2c6b635edccbec91a6ca2cb57f/dom/xbl/nsXBLWindowKeyHandler.cpp#108  Since they don't have bindings with style sheets in them, maybe it doesn't matter.  Though it does seem safer to stick with a Gecko backed in that case, for now.

Yes, the platform binding (chrome://global/content/platformHTMLBindings.xml) is loaded without a bound document. I change the patch to explicit query and set by using Gecko backend in Part 5 and Part 6 for XBL documents without a bound document, and ban the usage of StyleBackend::None.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8869663 [details]
Bug 1363640 Part 5 - Add a table in nsXULPrototypeCache to cache XBL documents with servo backend.

https://reviewboard.mozilla.org/r/141248/#review145688

r=me.  Thank you for doing this, and sorry for the horrible review lag!

::: dom/xbl/nsXBLService.cpp:954
(Diff revision 3)
>    // if it's being used.
>    nsXULPrototypeCache* cache = nsXULPrototypeCache::GetInstance();
>    bool useXULCache = cache && cache->IsEnabled();
>  
>    if (!info && useXULCache) {
> +    // Assume Gecko style backend for the XBL document without a bound

This only happens for the bindings loaded via nsXBLSpecialDocInfo::LoadDocInfo and that loads chrome://global/content/platformHTMLBindings.xml and the "dom.userHTMLBindings.uri" preference.

In practice, I doubt anyone ever uses that preference.  We should file a followup to remove it.

For platformHTMLBindings.xml the style backend likely does not matter; they don't have styles.  We should document that here.

::: dom/xul/nsXULPrototypeCache.cpp:255
(Diff revision 3)
>  nsresult
>  nsXULPrototypeCache::PutXBLDocumentInfo(nsXBLDocumentInfo* aDocumentInfo)
>  {
> -    nsIURI* uri = aDocumentInfo->DocumentURI();
> +  nsIURI* uri = aDocumentInfo->DocumentURI();
> +  nsCOMPtr<nsIDocument> doc = aDocumentInfo->GetDocument();
> +  XBLDocTable& table = XBLDocTableFor(doc->GetStyleBackendType());

So I think we have two options here.

Option 1 is to completely partition by backend type, as you have done.  In that case, the caller of GetXBLDocumentInfo() over in LoadBindingDocumentInfo should probably assert that the document info it got back matches the backend type passedto GetXBLDocumentInfo.

Option 2 is to store document infos with no stylesheets in both hashtables, so we only end up having to parse such bindings once.  Lookup would still just check the hashtable that matches the given style backend.  This might not work as well for things like style attributes, in case that matters...

I guess option 1 is safer for now, but we should perhaps file a followup to investigate option 2 and if it's not viable document why not.

::: dom/xul/nsXULPrototypeCache.cpp:258
(Diff revision 3)
> -    nsIURI* uri = aDocumentInfo->DocumentURI();
> +  nsIURI* uri = aDocumentInfo->DocumentURI();
> +  nsCOMPtr<nsIDocument> doc = aDocumentInfo->GetDocument();
> +  XBLDocTable& table = XBLDocTableFor(doc->GetStyleBackendType());
>  
> -    RefPtr<nsXBLDocumentInfo> info;
> +  RefPtr<nsXBLDocumentInfo> info;
> -    mXBLDocTable.Get(uri, getter_AddRefs(info));
> +  table.Get(uri, getter_AddRefs(info));

Preexisting, but this could be a GetWeak() and not hold a strong ref.
Attachment #8869663 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

4 days ago
mozreview-review-reply
Comment on attachment 8869663 [details]
Bug 1363640 Part 5 - Add a table in nsXULPrototypeCache to cache XBL documents with servo backend.

https://reviewboard.mozilla.org/r/141248/#review145688

> This only happens for the bindings loaded via nsXBLSpecialDocInfo::LoadDocInfo and that loads chrome://global/content/platformHTMLBindings.xml and the "dom.userHTMLBindings.uri" preference.
> 
> In practice, I doubt anyone ever uses that preference.  We should file a followup to remove it.
> 
> For platformHTMLBindings.xml the style backend likely does not matter; they don't have styles.  We should document that here.

Filed bug 1367286 for removing "dom.userHTMLBindings.uri".

> So I think we have two options here.
> 
> Option 1 is to completely partition by backend type, as you have done.  In that case, the caller of GetXBLDocumentInfo() over in LoadBindingDocumentInfo should probably assert that the document info it got back matches the backend type passedto GetXBLDocumentInfo.
> 
> Option 2 is to store document infos with no stylesheets in both hashtables, so we only end up having to parse such bindings once.  Lookup would still just check the hashtable that matches the given style backend.  This might not work as well for things like style attributes, in case that matters...
> 
> I guess option 1 is safer for now, but we should perhaps file a followup to investigate option 2 and if it's not viable document why not.

Option 2 is not working because style attributes in XBL document do matter. The style attributes need to be processed by servo in  http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/dom/base/nsAttrValue.cpp#1755-1760
(Assignee)

Comment 38

4 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63d037a31b3878df3f72a78d55c49402efc406a9

Comment 39

4 days ago
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae938236308c
Part 1 - Move IsContentDocument() and IsTopLevelContentDocument() from nsDocument to nsIDocument. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2eb47be4c8c8
Part 2 - Move the logic in SupportsServoStyleBackend() to UpdateStyleBackendType(). r=heycam
https://hg.mozilla.org/integration/autoland/rev/4f1badc64320
Part 3 - Strip whitespaces for files under dom/xbl. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c2b029d53305
Part 4 - Rename TableForBackendType to StyleSheetTableFor. r=heycam
https://hg.mozilla.org/integration/autoland/rev/99a6bbeaabef
Part 5 - Add a table in nsXULPrototypeCache to cache XBL documents with servo backend. r=bz,heycam
https://hg.mozilla.org/integration/autoland/rev/7f72289a5cab
Part 6 - Use bound document's style backend when creating an XBL document. r=heycam
https://hg.mozilla.org/integration/autoland/rev/977525db7dbb
Part 7 - Update reftest expectations after enabling stylo on XBL documents. r=heycam

Comment 40

3 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae938236308c
https://hg.mozilla.org/mozilla-central/rev/2eb47be4c8c8
https://hg.mozilla.org/mozilla-central/rev/4f1badc64320
https://hg.mozilla.org/mozilla-central/rev/c2b029d53305
https://hg.mozilla.org/mozilla-central/rev/99a6bbeaabef
https://hg.mozilla.org/mozilla-central/rev/7f72289a5cab
https://hg.mozilla.org/mozilla-central/rev/977525db7dbb
Status: NEW → RESOLVED
Last Resolved: 3 days ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.