Bug 230384 – Add MIME type and URL to WebCore::Model to allow processing different model types

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Add MIME type and URL to WebCore::Model to allow processing different model types

[Sam Weinig]

Created attachment 438435 [details]
Patch

[Tim Horton]

Comment on attachment 438435 [details]
Patch

Don't know why it's not r?, but r+ because I have written exactly the same patch :)

[Sam Weinig]

Created attachment 438436 [details]
Patch

[Sam Weinig]

(In reply to Tim Horton from comment #2)
> Comment on attachment 438435 [details]
> Patch
> 
> Don't know why it's not r?, but r+ because I have written exactly the same
> patch :)

Was just waiting for the bots to run before marking r?. Marked now.

[Darin Adler]

Comment on attachment 438436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438436&action=review

> Source/WebCore/platform/graphics/Model.h:33
>  #include <wtf/RefCounted.h>
> +#include <wtf/URL.h>
> +#include <wtf/text/WTFString.h>

You can just include URL.h, don’t need the other two.

> Source/WebCore/platform/graphics/Model.h:43
> +    WEBCORE_EXPORT static Ref<Model> create(Ref<SharedBuffer>, String, URL);

Bold trailblazing to use String for a string that we are adopting, as opposed to the more obvious String&& and old school const String&. Does seem simpler than String&& with no obvious disadvantage except for being less explicit.

[EWS]

Committed r282667 (241808@main): <https://commits.webkit.org/241808@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438436 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83246034>

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> Comment on attachment 438436 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438436&action=review
> 
> > Source/WebCore/platform/graphics/Model.h:33
> >  #include <wtf/RefCounted.h>
> > +#include <wtf/URL.h>
> > +#include <wtf/text/WTFString.h>
> 
> You can just include URL.h, don’t need the other two.

Will fix.

> 
> > Source/WebCore/platform/graphics/Model.h:43
> > +    WEBCORE_EXPORT static Ref<Model> create(Ref<SharedBuffer>, String, URL);
> 
> Bold trailblazing to use String for a string that we are adopting, as
> opposed to the more obvious String&& and old school const String&. Does seem
> simpler than String&& with no obvious disadvantage except for being less
> explicit.

:). This is something I want to explore more deeply. I have a feeling we have a ton of unnecessary ref count churn, especially at the bindings layer, where using "const String&" is the cause.

I think there are three cases to consider for String parameters:

1. The function always takes ownership of the string.
2. The function may or may not take ownership of the string.
3. The function never takes ownership of the string.

I believe (though I would have to re-convince myself to be sure) "const String&" as a parameter should be reserved for case #2 (though in those cases, I would usually suggest overloading with one taking a "const String&" and one taking a "String", so the move case is still optimal). 

For #1, "String" should be used.
For #3, "const String&" is fine, but we should probably be using StringView in most cases.

[Darin Adler]

(In reply to Sam Weinig from comment #8)
> I think there are three cases to consider for String parameters:
> 
> 1. The function always takes ownership of the string.
> 2. The function may or may not take ownership of the string.
> 3. The function never takes ownership of the string.
> 
> I believe (though I would have to re-convince myself to be sure) "const
> String&" as a parameter should be reserved for case #2 (though in those
> cases, I would usually suggest overloading with one taking a "const String&"
> and one taking a "String", so the move case is still optimal). 
> 
> For #1, "String" should be used.
> For #3, "const String&" is fine, but we should probably be using StringView
> in most cases.

Three things I can think of:

- Why is String better than String&&? We haven’t been consistent about this in WebKit before now and with other objects. I prefer the aesthetics of String. Are there benefits to writing String&& instead?

- There’s a variation on #3 worth considering. The function never takes ownership of the string, but does some operation that in turn requires a String, and so if passed a StringView that comes from a String will be less efficient than if it took a String.

[Darin Adler]

(sorry forgot the third thing)

- The function's interface should not say whether it takes ownership of the string or not, because it's an abstraction and should not expose implementation details. Unclear what is the best thing to do in that case.

[Sam Weinig]

(In reply to Darin Adler from comment #9)
> (In reply to Sam Weinig from comment #8)
> > I think there are three cases to consider for String parameters:
> > 
> > 1. The function always takes ownership of the string.
> > 2. The function may or may not take ownership of the string.
> > 3. The function never takes ownership of the string.
> > 
> > I believe (though I would have to re-convince myself to be sure) "const
> > String&" as a parameter should be reserved for case #2 (though in those
> > cases, I would usually suggest overloading with one taking a "const String&"
> > and one taking a "String", so the move case is still optimal). 
> > 
> > For #1, "String" should be used.
> > For #3, "const String&" is fine, but we should probably be using StringView
> > in most cases.
> 
> Three things I can think of:
> 
> - Why is String better than String&&? We haven’t been consistent about this
> in WebKit before now and with other objects. I prefer the aesthetics of
> String. Are there benefits to writing String&& instead?

String is better than String&& because it will work for both lvalues and rvalues, where as the latter will only worth with rvalues.

As a concrete example. The function

    static void foo(String&& bar)
    {
        (void)bar;
    }


with a caller:

    String string = "hello";
    foo(string);

will fail to compile with the following error:

... note: candidate function not viable: expects an rvalue for 1st argument
static void foo(String&& bar)

You would have to call it like:

    String string = "hello";
    foo(WTFMove(string));

which might not be what you want to do.

> - There’s a variation on #3 worth considering. The function never takes
> ownership of the string, but does some operation that in turn requires a
> String, and so if passed a StringView that comes from a String will be less
> efficient than if it took a String.

Hm, I can't think of a case when this would happen and it wasn't because ownership of a String was needed in some form, but I could totally be wrong.

[Sam Weinig]

(In reply to Darin Adler from comment #10)
> (sorry forgot the third thing)
> 
> - The function's interface should not say whether it takes ownership of the
> string or not, because it's an abstraction and should not expose
> implementation details. Unclear what is the best thing to do in that case.

I mean, I agree, but if we take that to the extreme, we would want to use either "foo(String)" or foo(const String&)" everywhere. However, each has its downsides.

"foo(String)" will always cause a ref, unless you explicitly WTFMove() (or otherwise rvalue your way), in which case, you can avoid a ref.

"foo(const String&)" allows the implementation decide if it needs to ref, but doesn't allow the caller to "move" the object into it.

While sticking to a principle would be nice, I think the efficiency gained by being cognizant of lifetime when choosing parameter types is worth it.

[Darin Adler]

(In reply to Sam Weinig from comment #11)
> You would have to call it like:
> 
>     String string = "hello";
>     foo(WTFMove(string));
> 
> which might not be what you want to do.

Or like this:

    String string = "hello";
    foo(String { string });

I’m not saying it’s elegant, but you don’t *have* to move the value in.

> > - There’s a variation on #3 worth considering. The function never takes
> > ownership of the string, but does some operation that in turn requires a
> > String, and so if passed a StringView that comes from a String will be less
> > efficient than if it took a String.
> 
> Hm, I can't think of a case when this would happen and it wasn't because
> ownership of a String was needed in some form, but I could totally be wrong.

The case where this happens without ownership is for a function that looks something up in a hash table that uses a String for entire key or part of the key. We don’t currently support using a StringView for such hash table lookups. String caches the hash across any number of multiple calls to compute it, which StringView will not do. We could add convenient key translation and would lose that optimization.

Also worth noting that if the function wants to call any function that currently takes a const String& or String, but should take a StringView, we end up being kind of slow. We won't be converting the entire code base all at once. This means we'd have to do things bottom-up?

Any function that makes an AtomString or Identifier, I suppose means it "may or may not take ownership", which is a sort of irritating subtlety.

(In reply to Sam Weinig from comment #12)
> (In reply to Darin Adler from comment #10)
> > - The function's interface should not say whether it takes ownership of the
> > string or not, because it's an abstraction and should not expose
> > implementation details. Unclear what is the best thing to do in that case.
> 
> I mean, I agree, but if we take that to the extreme, we would want to use
> either "foo(String)" or foo(const String&)" everywhere. However, each has
> its downsides.

I just meant that we should consider when this is the primary concern, not that every function needs to be abstract.

[Sam Weinig]

(In reply to Darin Adler from comment #13)
> (In reply to Sam Weinig from comment #11)
> > You would have to call it like:
> > 
> >     String string = "hello";
> >     foo(WTFMove(string));
> > 
> > which might not be what you want to do.
> 
> Or like this:
> 
>     String string = "hello";
>     foo(String { string });
> 
> I’m not saying it’s elegant, but you don’t *have* to move the value in.
> 
> > > - There’s a variation on #3 worth considering. The function never takes
> > > ownership of the string, but does some operation that in turn requires a
> > > String, and so if passed a StringView that comes from a String will be less
> > > efficient than if it took a String.
> > 
> > Hm, I can't think of a case when this would happen and it wasn't because
> > ownership of a String was needed in some form, but I could totally be wrong.
> 
> The case where this happens without ownership is for a function that looks
> something up in a hash table that uses a String for entire key or part of
> the key. We don’t currently support using a StringView for such hash table
> lookups. String caches the hash across any number of multiple calls to
> compute it, which StringView will not do. We could add convenient key
> translation and would lose that optimization.
> 

I don't feel strongly about the StringView thing, but this makes me think we should consider adding a cache of the string hash to the StringView, and populate it when creating a StringView from a String (and lazily populate it when creating a StringView in another way.)

There may be a good reason we haven't done that.