Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1225813003:
The src argument for HTMLAudioElement constructor should not have default value. (Closed)
DescriptionThe src argument for HTMLAudioElement constructor should not have default value.
As per spec link:
https://html.spec.whatwg.org/#the-audio-element,
src argument for HTMLAudioElement constructor should
not have default value.
Patch Set 1 #
Messages
Total messages: 10 (2 generated)
shiva.jm@samsung.com changed reviewers: + philipj@opera.com, tkent@chromium.org
pls have a look. custom-constructors.js is covering the test, so did not add new test.
philipj@opera.com changed reviewers: + jl@opera.com
I've started linux_blink_rel. I think this will change the behavior of `new
Audio(undefined)`, which per Web IDL should behave like `new Audio()` but I
think will now instead behave like `new Audio("undefined")`. If no test fails,
can you check if this has in fact changed?
The problem, I think, is that we're only treating undefined as a missing
argument when there's a default:
https://codereview.chromium.org/373043004
Jens, do you have any hunch about how much effort it would be to treat undefined
as a missing argument in this case too? I think there are quite a few
not-per-spec default arguments in our IDL where removing them would be
unobservable if not for this little quirk.
On 2015/07/08 08:23:55, philipj wrote:
> I've started linux_blink_rel. I think this will change the behavior of `new
> Audio(undefined)`, which per Web IDL should behave like `new Audio()` but I
> think will now instead behave like `new Audio("undefined")`. If no test fails,
> can you check if this has in fact changed?
>
> The problem, I think, is that we're only treating undefined as a missing
> argument when there's a default:
> https://codereview.chromium.org/373043004
>
> Jens, do you have any hunch about how much effort it would be to treat
undefined
> as a missing argument in this case too? I think there are quite a few
> not-per-spec default arguments in our IDL where removing them would be
> unobservable if not for this little quirk.
It wouldn't be difficult to treat trailing undefined argument values as missing
for optional arguments. The tricky bit is to treat non-trailing undefined
argument values as missing. (That's also a much more obscure case, of course.)
The worry here is really that we make our implementation inconsistent by
behaving differently depending on whether undefined argument values are trailing
or not. OTOH, we already behave differently in that case if the corresponding
attribute has a default value, so...
Background: the problem here is really on the C++ side, where missing trailing
arguments are implemented by calling the C++ implementation with fewer
arguments. This of course only works for trailing arguments; we can't very well
call a C++ function with only the first argument missing. So to really solve it,
we need a whole new mechanism for signalling "missing" on the C++ side. My patch
https://codereview.chromium.org/337343002/ is an old attempt to implement such a
new mechanism.
On 2015/07/08 08:45:44, Jens Widell wrote:
> On 2015/07/08 08:23:55, philipj wrote:
> > I've started linux_blink_rel. I think this will change the behavior of `new
> > Audio(undefined)`, which per Web IDL should behave like `new Audio()` but I
> > think will now instead behave like `new Audio("undefined")`. If no test
fails,
> > can you check if this has in fact changed?
> >
> > The problem, I think, is that we're only treating undefined as a missing
> > argument when there's a default:
> > https://codereview.chromium.org/373043004
> >
> > Jens, do you have any hunch about how much effort it would be to treat
> undefined
> > as a missing argument in this case too? I think there are quite a few
> > not-per-spec default arguments in our IDL where removing them would be
> > unobservable if not for this little quirk.
>
> It wouldn't be difficult to treat trailing undefined argument values as
missing
> for optional arguments. The tricky bit is to treat non-trailing undefined
> argument values as missing. (That's also a much more obscure case, of course.)
>
> The worry here is really that we make our implementation inconsistent by
> behaving differently depending on whether undefined argument values are
trailing
> or not. OTOH, we already behave differently in that case if the corresponding
> attribute has a default value, so...
>
> Background: the problem here is really on the C++ side, where missing trailing
> arguments are implemented by calling the C++ implementation with fewer
> arguments. This of course only works for trailing arguments; we can't very
well
> call a C++ function with only the first argument missing. So to really solve
it,
> we need a whole new mechanism for signalling "missing" on the C++ side. My
patch
> https://codereview.chromium.org/337343002/ is an old attempt to implement such
a
> new mechanism.
It would be pretty nice to have an optional<T> on the C++ side that could be
used for types that don't already have some other way of representing
null/missing (like String and pointers), or where the the type can be both null
or missing, like "optional DOMString? foo = null". Do you think that would solve
the problem?
In the meantime I think just handling this for trailing optional arguments would
resolve something like 80% of our the cases, would that amount to some small
tweak to Source/bindings/templates/methods.cpp?
On 2015/07/08 09:01:44, philipj wrote:
> On 2015/07/08 08:45:44, Jens Widell wrote:
> > On 2015/07/08 08:23:55, philipj wrote:
> > > I've started linux_blink_rel. I think this will change the behavior of
`new
> > > Audio(undefined)`, which per Web IDL should behave like `new Audio()` but
I
> > > think will now instead behave like `new Audio("undefined")`. If no test
> fails,
> > > can you check if this has in fact changed?
> > >
> > > The problem, I think, is that we're only treating undefined as a missing
> > > argument when there's a default:
> > > https://codereview.chromium.org/373043004
> > >
> > > Jens, do you have any hunch about how much effort it would be to treat
> > undefined
> > > as a missing argument in this case too? I think there are quite a few
> > > not-per-spec default arguments in our IDL where removing them would be
> > > unobservable if not for this little quirk.
> >
> > It wouldn't be difficult to treat trailing undefined argument values as
> missing
> > for optional arguments. The tricky bit is to treat non-trailing undefined
> > argument values as missing. (That's also a much more obscure case, of
course.)
> >
> > The worry here is really that we make our implementation inconsistent by
> > behaving differently depending on whether undefined argument values are
> trailing
> > or not. OTOH, we already behave differently in that case if the
corresponding
> > attribute has a default value, so...
> >
> > Background: the problem here is really on the C++ side, where missing
trailing
> > arguments are implemented by calling the C++ implementation with fewer
> > arguments. This of course only works for trailing arguments; we can't very
> well
> > call a C++ function with only the first argument missing. So to really solve
> it,
> > we need a whole new mechanism for signalling "missing" on the C++ side. My
> patch
> > https://codereview.chromium.org/337343002/ is an old attempt to implement
such
> a
> > new mechanism.
>
> It would be pretty nice to have an optional<T> on the C++ side that could be
> used for types that don't already have some other way of representing
> null/missing (like String and pointers), or where the the type can be both
null
> or missing, like "optional DOMString? foo = null". Do you think that would
solve
> the problem?
I think optional<T>, as added by the CL I linked to, would be a way to solve the
problem completely (i.e. make it possible to follow WebIDL completely in this
area.) I haven't come up with any better option, so the problem is that it
requires quite a lot of changes to solve what's mostly a corner case.
> In the meantime I think just handling this for trailing optional arguments
would
> resolve something like 80% of our the cases, would that amount to some small
> tweak to Source/bindings/templates/methods.cpp?
Yeah, supporting trailing missing arguments would just be about tweaking the
generated code to do something like
int length = std::min({{method.number_of_arguments}}, info.Length());
while (length > {{method.number_of_required_arguments}} && info[length -
1].IsUndefined())
--length;
On 2015/07/08 10:11:40, Jens Widell wrote:
> On 2015/07/08 09:01:44, philipj wrote:
> > On 2015/07/08 08:45:44, Jens Widell wrote:
> > > On 2015/07/08 08:23:55, philipj wrote:
> > > > I've started linux_blink_rel. I think this will change the behavior of
> `new
> > > > Audio(undefined)`, which per Web IDL should behave like `new Audio()`
but
> I
> > > > think will now instead behave like `new Audio("undefined")`. If no test
> > fails,
> > > > can you check if this has in fact changed?
> > > >
> > > > The problem, I think, is that we're only treating undefined as a missing
> > > > argument when there's a default:
> > > > https://codereview.chromium.org/373043004
> > > >
> > > > Jens, do you have any hunch about how much effort it would be to treat
> > > undefined
> > > > as a missing argument in this case too? I think there are quite a few
> > > > not-per-spec default arguments in our IDL where removing them would be
> > > > unobservable if not for this little quirk.
> > >
> > > It wouldn't be difficult to treat trailing undefined argument values as
> > missing
> > > for optional arguments. The tricky bit is to treat non-trailing undefined
> > > argument values as missing. (That's also a much more obscure case, of
> course.)
> > >
> > > The worry here is really that we make our implementation inconsistent by
> > > behaving differently depending on whether undefined argument values are
> > trailing
> > > or not. OTOH, we already behave differently in that case if the
> corresponding
> > > attribute has a default value, so...
> > >
> > > Background: the problem here is really on the C++ side, where missing
> trailing
> > > arguments are implemented by calling the C++ implementation with fewer
> > > arguments. This of course only works for trailing arguments; we can't very
> > well
> > > call a C++ function with only the first argument missing. So to really
solve
> > it,
> > > we need a whole new mechanism for signalling "missing" on the C++ side. My
> > patch
> > > https://codereview.chromium.org/337343002/ is an old attempt to implement
> such
> > a
> > > new mechanism.
> >
> > It would be pretty nice to have an optional<T> on the C++ side that could be
> > used for types that don't already have some other way of representing
> > null/missing (like String and pointers), or where the the type can be both
> null
> > or missing, like "optional DOMString? foo = null". Do you think that would
> solve
> > the problem?
>
> I think optional<T>, as added by the CL I linked to, would be a way to solve
the
> problem completely (i.e. make it possible to follow WebIDL completely in this
> area.) I haven't come up with any better option, so the problem is that it
> requires quite a lot of changes to solve what's mostly a corner case.
Ah, I didn't look very carefully, so many changed files :)
> > In the meantime I think just handling this for trailing optional arguments
> would
> > resolve something like 80% of our the cases, would that amount to some small
> > tweak to Source/bindings/templates/methods.cpp?
>
> Yeah, supporting trailing missing arguments would just be about tweaking the
> generated code to do something like
>
> int length = std::min({{method.number_of_arguments}}, info.Length());
> while (length > {{method.number_of_required_arguments}} && info[length -
> 1].IsUndefined())
> --length;
Shiva, if you want to work on getting rid of these default arguments, looking
into making that change might be worthwhile.
On 2015/07/08 10:17:08, philipj wrote:
> On 2015/07/08 10:11:40, Jens Widell wrote:
> > On 2015/07/08 09:01:44, philipj wrote:
> > > On 2015/07/08 08:45:44, Jens Widell wrote:
> > > > On 2015/07/08 08:23:55, philipj wrote:
> > > > > I've started linux_blink_rel. I think this will change the behavior of
> > `new
> > > > > Audio(undefined)`, which per Web IDL should behave like `new Audio()`
> but
> > I
> > > > > think will now instead behave like `new Audio("undefined")`. If no
test
> > > fails,
> > > > > can you check if this has in fact changed?
> > > > >
> > > > > The problem, I think, is that we're only treating undefined as a
missing
> > > > > argument when there's a default:
> > > > > https://codereview.chromium.org/373043004
> > > > >
> > > > > Jens, do you have any hunch about how much effort it would be to treat
> > > > undefined
> > > > > as a missing argument in this case too? I think there are quite a few
> > > > > not-per-spec default arguments in our IDL where removing them would be
> > > > > unobservable if not for this little quirk.
> > > >
> > > > It wouldn't be difficult to treat trailing undefined argument values as
> > > missing
> > > > for optional arguments. The tricky bit is to treat non-trailing
undefined
> > > > argument values as missing. (That's also a much more obscure case, of
> > course.)
> > > >
> > > > The worry here is really that we make our implementation inconsistent by
> > > > behaving differently depending on whether undefined argument values are
> > > trailing
> > > > or not. OTOH, we already behave differently in that case if the
> > corresponding
> > > > attribute has a default value, so...
> > > >
> > > > Background: the problem here is really on the C++ side, where missing
> > trailing
> > > > arguments are implemented by calling the C++ implementation with fewer
> > > > arguments. This of course only works for trailing arguments; we can't
very
> > > well
> > > > call a C++ function with only the first argument missing. So to really
> solve
> > > it,
> > > > we need a whole new mechanism for signalling "missing" on the C++ side.
My
> > > patch
> > > > https://codereview.chromium.org/337343002/ is an old attempt to
implement
> > such
> > > a
> > > > new mechanism.
> > >
> > > It would be pretty nice to have an optional<T> on the C++ side that could
be
> > > used for types that don't already have some other way of representing
> > > null/missing (like String and pointers), or where the the type can be both
> > null
> > > or missing, like "optional DOMString? foo = null". Do you think that would
> > solve
> > > the problem?
> >
> > I think optional<T>, as added by the CL I linked to, would be a way to solve
> the
> > problem completely (i.e. make it possible to follow WebIDL completely in
this
> > area.) I haven't come up with any better option, so the problem is that it
> > requires quite a lot of changes to solve what's mostly a corner case.
>
> Ah, I didn't look very carefully, so many changed files :)
>
> > > In the meantime I think just handling this for trailing optional arguments
> > would
> > > resolve something like 80% of our the cases, would that amount to some
small
> > > tweak to Source/bindings/templates/methods.cpp?
> >
> > Yeah, supporting trailing missing arguments would just be about tweaking the
> > generated code to do something like
> >
> > int length = std::min({{method.number_of_arguments}}, info.Length());
> > while (length > {{method.number_of_required_arguments}} && info[length -
> > 1].IsUndefined())
> > --length;
>
> Shiva, if you want to work on getting rid of these default arguments, looking
> into making that change might be worthwhile.
Ok, So to use optional<T>, we need "bindings/core/v8/Optional.h", so we need to
wait for https://codereview.chromium.org/337343002/ issue to commit first ?
On 2015/07/08 10:56:07, shiva.jm wrote:
> On 2015/07/08 10:17:08, philipj wrote:
> > On 2015/07/08 10:11:40, Jens Widell wrote:
> > > On 2015/07/08 09:01:44, philipj wrote:
> > > > On 2015/07/08 08:45:44, Jens Widell wrote:
> > > > > On 2015/07/08 08:23:55, philipj wrote:
> > > > > > I've started linux_blink_rel. I think this will change the behavior
of
> > > `new
> > > > > > Audio(undefined)`, which per Web IDL should behave like `new
Audio()`
> > but
> > > I
> > > > > > think will now instead behave like `new Audio("undefined")`. If no
> test
> > > > fails,
> > > > > > can you check if this has in fact changed?
> > > > > >
> > > > > > The problem, I think, is that we're only treating undefined as a
> missing
> > > > > > argument when there's a default:
> > > > > > https://codereview.chromium.org/373043004
> > > > > >
> > > > > > Jens, do you have any hunch about how much effort it would be to
treat
> > > > > undefined
> > > > > > as a missing argument in this case too? I think there are quite a
few
> > > > > > not-per-spec default arguments in our IDL where removing them would
be
> > > > > > unobservable if not for this little quirk.
> > > > >
> > > > > It wouldn't be difficult to treat trailing undefined argument values
as
> > > > missing
> > > > > for optional arguments. The tricky bit is to treat non-trailing
> undefined
> > > > > argument values as missing. (That's also a much more obscure case, of
> > > course.)
> > > > >
> > > > > The worry here is really that we make our implementation inconsistent
by
> > > > > behaving differently depending on whether undefined argument values
are
> > > > trailing
> > > > > or not. OTOH, we already behave differently in that case if the
> > > corresponding
> > > > > attribute has a default value, so...
> > > > >
> > > > > Background: the problem here is really on the C++ side, where missing
> > > trailing
> > > > > arguments are implemented by calling the C++ implementation with fewer
> > > > > arguments. This of course only works for trailing arguments; we can't
> very
> > > > well
> > > > > call a C++ function with only the first argument missing. So to really
> > solve
> > > > it,
> > > > > we need a whole new mechanism for signalling "missing" on the C++
side.
> My
> > > > patch
> > > > > https://codereview.chromium.org/337343002/ is an old attempt to
> implement
> > > such
> > > > a
> > > > > new mechanism.
> > > >
> > > > It would be pretty nice to have an optional<T> on the C++ side that
could
> be
> > > > used for types that don't already have some other way of representing
> > > > null/missing (like String and pointers), or where the the type can be
both
> > > null
> > > > or missing, like "optional DOMString? foo = null". Do you think that
would
> > > solve
> > > > the problem?
> > >
> > > I think optional<T>, as added by the CL I linked to, would be a way to
solve
> > the
> > > problem completely (i.e. make it possible to follow WebIDL completely in
> this
> > > area.) I haven't come up with any better option, so the problem is that it
> > > requires quite a lot of changes to solve what's mostly a corner case.
> >
> > Ah, I didn't look very carefully, so many changed files :)
> >
> > > > In the meantime I think just handling this for trailing optional
arguments
> > > would
> > > > resolve something like 80% of our the cases, would that amount to some
> small
> > > > tweak to Source/bindings/templates/methods.cpp?
> > >
> > > Yeah, supporting trailing missing arguments would just be about tweaking
the
> > > generated code to do something like
> > >
> > > int length = std::min({{method.number_of_arguments}}, info.Length());
> > > while (length > {{method.number_of_required_arguments}} && info[length -
> > > 1].IsUndefined())
> > > --length;
> >
> > Shiva, if you want to work on getting rid of these default arguments,
looking
> > into making that change might be worthwhile.
>
> Ok, So to use optional<T>, we need "bindings/core/v8/Optional.h", so we need
to
> wait for https://codereview.chromium.org/337343002/ issue to commit first ?
No, that CL isn't getting any immediate attention, I think. Rather, if you want
to try to fix this, try making the smaller change of treating undefined as a
missing argument only for trailing arguments. I don't know if it's tricky or
not, but something like what Jens suggests in
Source/bindings/templates/methods.cpp might do it.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
