Closure is currently correct - omitting this (or having null) will result in validation failure.  Changing the API is completely doable: we'd just need to change this line [1] to be `optional DOMString pageId` instead of `DOMString pageId`, and it should work.

That said, I have no insight into whether that's best for this API, so +Steven and Michael for any thoughts (I certainly don't have any objections).

Settings doesn't use this argument (it just passes an empty string). Are there any long-term plans to add a page ID here for metrics purposes?

I have no problem making the argument optional since no one's really using it anyway.

@michaelpg: I am not aware of any plans to pass a page ID from settings to that function.

Not sure how you would make optional a parameter that is not the last one. I think that would lead to pretty complicated signatures and runtime checking (is this a function or a string? to determine if it is the optional param or the callback).

FWIW, I think optional params should be passed in a wrapper Options object, instead of being positional parameters, just a suggestion. Example call below.

chrome.settingsPrivate.setPref
    required1, required2, required3, {
      optional1: 'foo',
      optional2: 'baz'
      optional3: 'bar',
});

Triage!

katie@, can you own this?

dpapad@: Extensions APIs do indeed support optional inner parameters, as long as the signature are non-ambiguous (and yep, it does make our signature matching code pretty complicated - but we've supported it for years, and likely will need to continue to).  I'd be supportive of changing to an options argument as well, but that likely is a very large cleanup.

If we are going to spend any significant effort on revamping the API signatures, I think it would be more well spent on migrating from callback based APIs to Promise-based APIs.

This is already tracked by issue 328932.  A good start would be to require Promise-based signatures for any newly added APIs, but that's a different discussion I guess.

Just my $0.02, but I don't think making pageId optional needs to be blocked on revamping all signatures?  (Since making it optional is a one-line change to the IDL + an externs update)

> I don't think making pageId optional needs to be blocked on revamping all signatures

Agreed.

>  I'd be supportive of changing to an options argument as well, but that likely is a very large cleanup.

I was only suggesting to invest in Promise based callback instead of converting optional params to a single options argument across all signatures.

Yes, I can own making pageId optional.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9169bf2a6a1f40af967b2ef07520f740e543917e

commit 9169bf2a6a1f40af967b2ef07520f740e543917e
Author: Katie D <katie@chromium.org>
Date: Mon Jul 30 21:54:40 2018

Makes pageId optional in settings_private setPref function.

Has to make callbacks optional too in order to continue to pass closure
compilation.

Bug:  866161 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Ie911461b0e575771708897bca30483fa9cc9f038
Reviewed-on: https://chromium-review.googlesource.com/1155643
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Katie Dektar <katie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579184}
[modify] https://crrev.com/9169bf2a6a1f40af967b2ef07520f740e543917e/chrome/common/extensions/api/settings_private.idl
[modify] https://crrev.com/9169bf2a6a1f40af967b2ef07520f740e543917e/third_party/closure_compiler/externs/settings_private.js