916320 - Codegen bug: Optional<T> arg with specified default value in webidl is not handled properly

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Andrew Quartey [:drexler]]

From bug 850364, Ms2ger filed this: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23193. 

With the old spec:   void SetRangeText(DOMString replacement,
                                       unsigned long start, unsigned long end,
                                       optional SelectionMode selectionMode);
Codegen produces this:
--------------------------------------------------
 Optional<SelectionMode > arg3;
      if (3 < args.length()) {
        arg3.Construct();
        {
          bool ok;
          int index = FindEnumStringIndex<true>(cx, args[3],  SelectionModeValues::strings, "SelectionMode", "Argument 4 of HTMLInputElement.setRangeText", &ok);
          if (!ok) {
            return false;
          }
          MOZ_ASSERT(index >= 0);
          arg3.Value() = static_cast<SelectionMode>(index);
        }
      }
      ErrorResult rv;
      self->SetRangeText(NonNullHelper(Constify(arg0)), arg1, arg2, Constify(arg3), rv);
-----------------------------------------------------------------------------

NB: For easier code-reading see: https://pastebin.mozilla.org/3045483



With the new spec: void SetRangeText(DOMString replacement,
                          unsigned long start, unsigned long end,
                          optional SelectionMode selectionMode = "preserve");

Codegen produces this: 
---------------------------
SelectionMode arg3;
      if (3 < args.length()) {
        {
          bool ok;
          int index = FindEnumStringIndex<true>(cx, args[3], SelectionModeValues::strings, "SelectionMode", "Argument 4 of HTMLInputElement.setRangeText", &ok);
          if (!ok) {
            return false;
          }
          MOZ_ASSERT(index >= 0);
          arg3 = static_cast<SelectionMode>(index);
        }
      } else {
        arg3 = SelectionMode::Preserve;
      }
      ErrorResult rv;
      self->SetRangeText(NonNullHelper(Constify(arg0)), arg1, arg2, arg3, rv);
------------------------------------------------------------------------------

NB: For easier code-reading see: https://pastebin.mozilla.org/3047261


Above, we can see that |arg3| is not of type |Optional<SelectionMode>| as before. This breaks compilation.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> This breaks compilation.

Compilation of what?

Optional<> is used for arguments that can have a "not passed" state.  If it has a default value, it has no such state, so it doesn't need Optional.

Now of course this means that the actual HTMLInputElement code that implements SetRangeText needs to be modified: made simpler, in fact, since it no longer has to worry about a "not passed" case.  Which is the whole point of having default values in IDL.

This is all documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Optional and https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#typemapping (the paragraph starting "In all cases").

Amd I just missing something?

Comment 2

[Andrew Quartey [:drexler]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> 
> Optional<> is used for arguments that can have a "not passed" state.  If it
> has a default value, it has no such state, so it doesn't need Optional.
> 

Thanks...conflated the two in my head. I should have *re-read* the docs before filing this as a bug!

Comment 3

[Boris Zbarsky [:bzbarsky]]

All good.  Going to mark invalid, but please let me know if there's still an issue here!