nsICommandParams should be builtin class

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(4 attachments)

Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED

Comment 7

9 months ago
mozreview-review
Comment on attachment 8991209 [details]
Bug 1450882 - part 1: Make nsICommandParams builtinclass

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

Great!
Attachment #8991209 - Flags: review?(ehsan) → review+

Comment 8

9 months ago
mozreview-review
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
Attachment #8991210 - Flags: review?(ehsan) → review+

Comment 9

9 months ago
mozreview-review
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!
Attachment #8991211 - Flags: review?(ehsan) → review+

Comment 10

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

https://reviewboard.mozilla.org/r/255980/#review263506
Attachment #8991212 - Flags: review?(ehsan) → review+
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

9 months ago
Pushed by masayuki@d-toybox.com:
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
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.