1450882 - nsICommandParams should be builtin class

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsICommandParams isn't implemented with JS even in comm-central nor in bluegriffon:
https://searchfox.org/mozilla-central/search?q=nsICommandParams&case=false&regexp=false&path=
https://searchfox.org/comm-central/search?q=nsICommandParams&case=false&regexp=false&path=
https://github.com/therealglazou/bluegriffon/search?utf8=%E2%9C%93&q=nsICommandParams&type=

So, we can make it builtin class and can make it accessible without virtual calls.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff06c83f99aaf41e8433f791a70cf7ff78242bb8

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1450882 - part 1: Make nsICommandParams builtinclass

nsICommandParams is not implemented with JS.  Therefore, we can make it
builtinclass.

Review commit: https://reviewboard.mozilla.org/r/255974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255974/

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1450882 - part 2: Make nsICommandParams::GetCStringValue() and nsICommandParams::SetCStringValue() treat nsACString instead of char

nsICommandParams::GetCStringValue() and nsICommandParams::SetCStringValue()
treat char.  However, this makes their callers complicated.  So, they should
be rewritten as treating nsACString.

Review commit: https://reviewboard.mozilla.org/r/255976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255976/

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1450882 - part 3: Create non-virtual methods to nsCommandParams and expose its header

nsCommandParams is used internally but accessed via nsICommandParams.  For
such internal users, nsCommandParams should have non-virtual methods to
get/set values.

The new method names are similar to Preferences API for consistency with it.

Review commit: https://reviewboard.mozilla.org/r/255978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255978/

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1450882 - part 4: Make C++ users of nsICommandParams use nsCommandParams directly

nsICommandParams is implemented only by nsCommandParams.  So, all C++ users
can treat all instances of nsICommandParams as nsCommandParams.  Therefore,
this patch makes all set/get value calls use non-virtual methods and all
constructors directly create nsCommandParams instance.

Review commit: https://reviewboard.mozilla.org/r/255980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255980/

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't know who is the best person to review the patches since any contributors around nsCommandParams are not currently working around here. So, I guess, DOM peer must be a good person to review. Ehsan is a DOM peer and has reviewed a change around here. Therefore, I request review to Ehsan. If there are some better reviewers, feel free to redirect to them.

Comment 7

[(no longer active)]

Comment on attachment 8991209 [details]
Bug 1450882 - part 1: Make nsICommandParams builtinclass

https://reviewboard.mozilla.org/r/255974/#review263494

Great!

Comment 8

[(no longer active)]

Comment on attachment 8991210 [details]
Bug 1450882 - part 2: Make nsICommandParams::GetCStringValue() and nsICommandParams::SetCStringValue() treat nsACString instead of char

https://reviewboard.mozilla.org/r/255976/#review263496

Comment 9

[(no longer active)]

Comment on attachment 8991211 [details]
Bug 1450882 - part 3: Create non-virtual methods to nsCommandParams and expose its header

https://reviewboard.mozilla.org/r/255978/#review263504

::: dom/commandhandler/nsCommandParams.cpp:299
(Diff revision 1)
>  
>  nsCommandParams::HashEntry*
> -nsCommandParams::GetNamedEntry(const char* aName)
> +nsCommandParams::GetNamedEntry(const char* aName) const
>  {
> -  return static_cast<HashEntry*>(mValuesHash.Search((void*)aName));
> +  return static_cast<HashEntry*>(
> +           const_cast<PLDHashTable&>(mValuesHash).Search((void*)aName));

Ugh, this const_cast<> is really unfortunate.  :-(

Do you mind submitting a follow-up patch to make PLDHashtable::Search() const and remove this?  I just checked and it should be relatively easy (it would only take making a couple of more members const for it to compile).  Thanks!

Comment 10

[(no longer active)]

Comment on attachment 8991212 [details]
Bug 1450882 - part 4: Make C++ users of nsICommandParams use nsCommandParams directly

https://reviewboard.mozilla.org/r/255980/#review263506

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8991211 [details]
Bug 1450882 - part 3: Create non-virtual methods to nsCommandParams and expose its header

https://reviewboard.mozilla.org/r/255978/#review263504

> Ugh, this const_cast<> is really unfortunate.  :-(
> 
> Do you mind submitting a follow-up patch to make PLDHashtable::Search() const and remove this?  I just checked and it should be relatively easy (it would only take making a couple of more members const for it to compile).  Thanks!

Sure. But I'll do it in a follow up bug. (I'd like to land these patches first due to my local queue is too long...)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a9c68683290
part 1: Make nsICommandParams builtinclass r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/cc00b4d8d557
part 2: Make nsICommandParams::GetCStringValue() and nsICommandParams::SetCStringValue() treat nsACString instead of char r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/da27408845b3
part 3: Create non-virtual methods to nsCommandParams and expose its header r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/da50b3158af3
part 4: Make C++ users of nsICommandParams use nsCommandParams directly r=Ehsan

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/1a9c68683290
https://hg.mozilla.org/mozilla-central/rev/cc00b4d8d557
https://hg.mozilla.org/mozilla-central/rev/da27408845b3
https://hg.mozilla.org/mozilla-central/rev/da50b3158af3