stylo: Enable stylo for SVG-as-an-image.

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
25 days ago
9 hours ago

People

(Reporter: kuoe0, Assigned: kuoe0)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

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

Attachments

(7 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
kuoe0
: review?
heycam
Details | Review
(Assignee)

Description

25 days ago
I found SVG-as-an-image is not parsed by stylo. In [1], we pass aContainer with nullptr to create the document. The document without a container will not be parsed by stylo [2].

So, I tend to set the backend type after the document created[3] and before the document loading[4].

[1]: https://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/image/SVGDocumentWrapper.cpp#344-349
[2]: http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/dom/base/nsDocument.cpp#13173
[3]: https://dxr.mozilla.org/mozilla-central/rev/9af23c413a1f8d337b19b4f8450e241e91b71136/layout/build/nsContentDLF.cpp#366
[4]: https://dxr.mozilla.org/mozilla-central/rev/9af23c413a1f8d337b19b4f8450e241e91b71136/layout/build/nsContentDLF.cpp#377
(Assignee)

Updated

25 days ago
Summary: stylo: Pass backend type to CreateDocument() to specitfy the backend type of documents. → stylo: Enable stylo for SVG-as-an-image.
(Assignee)

Comment 1

25 days ago
Due to the SVG used in chrome document should be parsed by gecko. So, we can't set the backend type to servo for all SVG documents. I think we should set the backend type according to where the `nsStyleImageRequest` object is created from.
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)
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 days ago
Attachment #8886051 - Attachment is obsolete: true
(Assignee)

Updated

11 days ago
Attachment #8886050 - Flags: review?(cam)
Attachment #8886046 - Flags: review?(cam)
Attachment #8886047 - Flags: review?(cam)
Attachment #8886048 - Flags: review?(cam)
Attachment #8886049 - Flags: review?(cam)
(Assignee)

Comment 19

11 days ago
Because `context-{fill|stroke}-opacity` are not implemented in stylo, it would cause the test cases I added in Bug 1373159 failed. Do you think I should add fail-if(stylo) for these test cases? Or, we can land this bug after Bug 1338764 landed?
Flags: needinfo?(cam)
(Assignee)

Updated

11 days ago
Blocks: 1352284

Updated

11 days ago
Priority: -- → P1

Comment 20

11 days ago
mozreview-review
Comment on attachment 8886049 [details]
Bug 1377158 - (Part 5) Decide style backend type according to the loading document.

https://reviewboard.mozilla.org/r/156844/#review162390

::: image/imgRequest.cpp:1134
(Diff revision 3)
>    // appropriate image.
>    if (newPartPending) {
>      NewPartResult result = PrepareForNewPart(aRequest, aInStr, aCount, mURI,
>                                               isMultipart, image,
> -                                             progressTracker, mInnerWindowId);
> +                                             progressTracker, mInnerWindowId,
> +                                             IsChrome() ? StyleBackendType::Gecko :

I'm not sure IsChrome() is the exact thing we want to check.  That returns true if the image URL is a chrome:// URL, but there are at least some other chrome-ish URLs like resource:// that are used to load SVG images from chrome documents.  And add-ons can have their own random protocols.

Ideally, we would know whether the requesting document is a content document or not (or perhaps whether it is a stylo-styled document or not) and use that to determine whether the SVG image it loads should use stylo or not.  I remember we discussed choosing based on which nsStyleImageRequest constructor was called, but I guess that only works for images loaded from CSS, and we probably want to control this for HTML <img> elements too.

CSS image loads and HTML <img> loads all end up going through nsContentUtils::LoadImage, I think.  And it looks like it passes through the document that caused the image load to imgLoader::LoadImage.  Can we make the decision at that point, in imgLoader::LoadImage, by taking aLoadingDocument's StyleBackendType and passing it in to imgRequest::Init?

Comment 21

11 days ago
mozreview-review
Comment on attachment 8886046 [details]
Bug 1377158 - (Part 2) Add style backend type member in SVGDocumentWrapper and create SVG document with specified style backend.

https://reviewboard.mozilla.org/r/156838/#review162388

::: image/SVGDocumentWrapper.cpp:351
(Diff revision 3)
> +  RefPtr<nsContentDLF> contentDLF =
> +    static_cast<nsContentDLF*>(docLoaderFactory.get());
> +  NS_ENSURE_TRUE(contentDLF, NS_ERROR_UNEXPECTED);

In theory the nsIDocumentLoaderFactory we get back might be some other object and not nsContentDLF.  (I don't think that's the case for Firefox, but there might be other XUL-based apps that could do this.)

So for now, while we still have to use this indirect XPCOM stuff, I think we should just add an argument to the nsIDocumentLoaderFactory createInstance function, which we can pass down to CreateDocument, so that we don't have to do this cast here.
I'll hold off on the rest of the reviews for comment 20.

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #19)
> Because `context-{fill|stroke}-opacity` are not implemented in stylo, it
> would cause the test cases I added in Bug 1373159 failed. Do you think I
> should add fail-if(stylo) for these test cases? Or, we can land this bug
> after Bug 1338764 landed?

I think it's fine to mark them as fails-if(stylo), if this bug lands first, and then we can remove the annotations in bug 1338764.
Flags: needinfo?(cam)
(Assignee)

Comment 23

11 days ago
mozreview-review
Comment on attachment 8886049 [details]
Bug 1377158 - (Part 5) Decide style backend type according to the loading document.

https://reviewboard.mozilla.org/r/156844/#review162426

::: image/imgRequest.cpp:1134
(Diff revision 3)
>    // appropriate image.
>    if (newPartPending) {
>      NewPartResult result = PrepareForNewPart(aRequest, aInStr, aCount, mURI,
>                                               isMultipart, image,
> -                                             progressTracker, mInnerWindowId);
> +                                             progressTracker, mInnerWindowId,
> +                                             IsChrome() ? StyleBackendType::Gecko :

Did you mean we should call GetStyleBackendType() from aLoadingDocument? Because the SVG document created from SVGDocumentWrapper doesn't have a container, the style backend type would be always Gecko here. [1]

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1256
(Assignee)

Comment 24

11 days ago
mozreview-review
Comment on attachment 8886046 [details]
Bug 1377158 - (Part 2) Add style backend type member in SVGDocumentWrapper and create SVG document with specified style backend.

https://reviewboard.mozilla.org/r/156838/#review162424

::: image/SVGDocumentWrapper.cpp:351
(Diff revision 3)
> +  RefPtr<nsContentDLF> contentDLF =
> +    static_cast<nsContentDLF*>(docLoaderFactory.get());
> +  NS_ENSURE_TRUE(contentDLF, NS_ERROR_UNEXPECTED);

Okay, I just think it's better to not change the interface. But if it's possible to be other object, I think we should change the interface.

Comment 25

10 days ago
mozreview-review-reply
Comment on attachment 8886049 [details]
Bug 1377158 - (Part 5) Decide style backend type according to the loading document.

https://reviewboard.mozilla.org/r/156844/#review162426

> Did you mean we should call GetStyleBackendType() from aLoadingDocument? Because the SVG document created from SVGDocumentWrapper doesn't have a container, the style backend type would be always Gecko here. [1]
> 
> [1]: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1256

I think aLoadingDocument should be the document that caused the image load, e.g. the HTML document that has the <img> element or which has elements styled with background-image, etc.  That document should already have decided its StyleBackendType, since we're rendering it.
(Assignee)

Comment 26

10 days ago
Oh, so aLoadingDocument is the document which contains the images would be loaded. Is it right?
I think so!  At the point that nsContentUtils::LoadImage is called, the referenced SVG document image hasn't been created yet.
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)

Updated

5 days ago
Attachment #8886046 - Flags: review?(cam)
Attachment #8886049 - Flags: review?(cam)
Attachment #8887813 - Flags: review?(cam)
(Assignee)

Comment 34

5 days ago
Hi Cameron, I haven't modified the issue you mentioned for part 2. Please review the other parts first.
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 41

5 days ago
It's ready to review!
Flags: needinfo?(cam)

Comment 42

5 days ago
mozreview-review
Comment on attachment 8886050 [details]
Bug 1377158 - (Part 1) Make nsIDocumentLoaderFactory::createInstance() accept style backend type.

https://reviewboard.mozilla.org/r/156846/#review162382

r=me with these comments addressed.

::: docshell/base/nsIDocumentLoaderFactory.idl:27
(Diff revision 5)
>   * The component is a service, so use GetService, not CreateInstance to get it.
>   */
>  
>  [scriptable, uuid(e795239e-9d3c-47c4-b063-9e600fb3b287)]
>  interface nsIDocumentLoaderFactory : nsISupports {
>      nsIContentViewer createInstance(in string aCommand,

I'm not sure if there really are other JS users of this API.  But I think to be complete, we should add some consts here for them to use.  It will make the API look a bit more complete.  So something like:

  const octet STYLE_BACKEND_TYPE_NONE = 0;
  const octet ...

And I know this is not documented at all, but can you add a comment here to say that aStyleBackendType can be used to choose the style backend type for the created document, or STYLE_BACKEND_TYPE_NONE can be used to mean the default will be used.

::: dom/base/nsDocument.cpp:1022
(Diff revision 5)
>  
>    nsCOMPtr<nsIContentViewer> viewer;
>    nsCOMPtr<nsIStreamListener> listener;
>    rv = docLoaderFactory->CreateInstance("external-resource", chan, newLoadGroup,
>                                          type, nullptr, nullptr,
> +                                        0 /* StyleBackendType::None */,

With the XPIDL constants added, you can write:

  nsIDocumentLoaderFactory::STYLE_BACKENDTYPE_NONE,

(Here and in the other functions that call CreateInstance.)

::: layout/build/nsContentDLF.h:13
(Diff revision 5)
>  
>  #include "nsIDocumentLoaderFactory.h"
>  #include "nsMimeTypes.h"
> +#include "mozilla/StyleBackendType.h"
> +
> +using mozilla::StyleBackendType;

Nit: generally we prefer not to add "using" statements to header files, since it can affect other files including this one.  So I would prefer "mozilla::StyleBackendType" to be written out full in the CreateDocument declaration.  But adding a "using namespace mozilla;" is OK in the .cpp file.

::: layout/build/nsContentDLF.cpp:136
(Diff revision 5)
>  
>  NS_IMETHODIMP
>  nsContentDLF::CreateInstance(const char* aCommand,

Just above here, can you add some static assertions that each of the three values of the XPIDL constants match the values of the C++ StyleBackendType enum?

::: layout/build/nsContentDLF.cpp:381
(Diff revision 5)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Set style backend type before document loading. Can't set None as the
> +  // backend type, it would hit an assertion.
> +  if (aStyleBackendType != StyleBackendType::None) {
> +    // To dynamic switch the backend type, check stylo is enabled or not

"dynamically"
Attachment #8886050 - Flags: review?(cam) → review+

Comment 43

5 days ago
mozreview-review
Comment on attachment 8886046 [details]
Bug 1377158 - (Part 2) Add style backend type member in SVGDocumentWrapper and create SVG document with specified style backend.

https://reviewboard.mozilla.org/r/156838/#review164498

::: image/SVGDocumentWrapper.h:43
(Diff revision 5)
>    SVGDocumentWrapper();
> +  explicit SVGDocumentWrapper(StyleBackendType aStyleBackendType);

Just make aStyleBackendType an optional argument of the existing constructor.
Attachment #8886046 - Flags: review?(cam) → review+

Comment 44

5 days ago
mozreview-review
Comment on attachment 8886047 [details]
Bug 1377158 - (Part 3) Add style backend type member in VectorImage to be used to create SVGDocumentWrapper with specified style backend.

https://reviewboard.mozilla.org/r/156840/#review164500
Attachment #8886047 - Flags: review?(cam) → review+

Comment 45

5 days ago
mozreview-review
Comment on attachment 8886048 [details]
Bug 1377158 - (Part 4) Add a new parameter to pass style backend type to create image.

https://reviewboard.mozilla.org/r/156842/#review164502
Attachment #8886048 - Flags: review?(cam) → review+

Comment 46

5 days ago
mozreview-review
Comment on attachment 8886046 [details]
Bug 1377158 - (Part 2) Add style backend type member in SVGDocumentWrapper and create SVG document with specified style backend.

https://reviewboard.mozilla.org/r/156838/#review164504

::: commit-message-6fec4:1
(Diff revision 5)
> +Bug 1377158 - (Part 2) Add style backend type member in SvgDocumentWrapper and create SVG document with specified style backend.

"SVGDocumentWrapper"

Comment 47

5 days ago
mozreview-review
Comment on attachment 8886049 [details]
Bug 1377158 - (Part 5) Decide style backend type according to the loading document.

https://reviewboard.mozilla.org/r/156844/#review164508

::: image/imgLoader.cpp:2240
(Diff revision 5)
>      nsCOMPtr<nsILoadGroup> channelLoadGroup;
>      newChannel->GetLoadGroup(getter_AddRefs(channelLoadGroup));
>      rv = request->Init(aURI, aURI, /* aHadInsecureRedirect = */ false,

Please add a comment in here saying that we look at the loading document's style backend type so that Chrome documents, where stylo is disabled, will use a Gecko backend for any SVG-as-image documents they load.
Attachment #8886049 - Flags: review?(cam) → review+

Comment 48

5 days ago
mozreview-review
Comment on attachment 8887813 [details]
Bug 1377158 - (Part 7) Disable failed test cases.

https://reviewboard.mozilla.org/r/158720/#review164510

Looks great, thanks for fixing this!
Attachment #8887813 - Flags: review?(cam) → review+
(Assignee)

Comment 49

3 days ago
We expected the test cases of context-*-opacity should be failed after we enabled SVG-as-an-image for stylo. But I noticed the test cases still passed in the mode "styloVsGecko". I think this is caused by the image cache. When an SVG image is loaded, it would be cached. And we'll use it when we reload the page. So, when we turn off the stylo pref and reload the page, it still uses the same image to compare.
Flags: needinfo?(cam)
(Assignee)

Comment 50

3 days ago
I'll add style backend type to image cache and make stylo and gecko can get different cache.
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 hours ago
Attachment #8889336 - Flags: review?(cam)
You need to log in before you can comment on or make changes to this bug.