Closed Bug 1363640 Opened 6 years ago Closed 6 years ago

stylo: Enable stylo for XBL documents

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(7 files)

Bug 1363640 Part 1 - Move IsContentDocument() and IsTopLevelContentDocument() from nsDocument to nsIDocument.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363640 Part 2 - Move the logic in SupportsServoStyleBackend() to UpdateStyleBackendType().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363640 Part 3 - Strip whitespaces for files under dom/xbl.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363640 Part 4 - Rename TableForBackendType to StyleSheetTableFor.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363640 Part 5 - Add a table in nsXULPrototypeCache to cache XBL documents with servo backend.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1363640 Part 6 - Use bound document's style backend when creating an XBL document.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363640 Part 7 - Update reftest expectations after enabling stylo on XBL documents.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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
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
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: stylo
See Also: → 1341725
Assignee: nobody → tlin
Priority: -- → P1
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 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 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 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 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 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 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 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 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 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
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
See Also: 1341725 →
You need to log in before you can comment on or make changes to this bug.