Closed Bug 1246464 Opened 4 years ago Closed 4 years ago

Mark some stylesheet-related args as 'const'

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

While reviewing bug 1244068 & bug 1244074, I've noticed 2 instances where we have a function with a passed-by-reference/pointer arg, which is not marked as 'const', but which probably should be.

So far:
 * RemoveStyleSheetsFromStyleSets(): the arg aSheets (a reference to a nsTArray) should be const.
 * GetIndexOfStyleSheet() the arg aSheet should be const. (Currently this is a CSSStyleSheet*, and it's changing to a handle in bug 1244074.)

Planning on fixing these after bug 1244068 & bug 1244074 land (to avoid bitrotting heycam).
Comment on attachment 8723701 [details] [diff] [review]
fix v1

Review of attachment 8723701 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.h
@@ +805,5 @@
>     */
>    virtual int32_t GetNumberOfStyleSheets() const override;
>    virtual mozilla::StyleSheetHandle GetStyleSheetAt(int32_t aIndex) const override;
> +  virtual int32_t GetIndexOfStyleSheet(
> +      const mozilla::StyleSheetHandle aSheet) const override;

I think you'll need to change this in nsIDocument, too.
Attachment #8723701 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) (away Feb 27 – Mar 13) from comment #2)
> > +  virtual int32_t GetIndexOfStyleSheet(
> > +      const mozilla::StyleSheetHandle aSheet) const override;
> 
> I think you'll need to change this in nsIDocument, too.

To my surprise, I do not actually *need* to change this, in order to compile.  We actually compile just fine, with the patch as-is, without any changes to nsIDocument here.  Though I agree that I *should* change nsIDocument.

I suppose the reason is that the "const" qualifier here is an implementation detail -- the arg (of type StyleSheetHandle) is being passed by copy (rather than by reference).

Anyway, I'll fix this before landing. Thanks!
(In reply to Daniel Holbert [:dholbert] from comment #3)
> I suppose the reason is that the "const" qualifier here is an implementation
> detail -- the arg (of type StyleSheetHandle) is being passed by copy (rather
> than by reference).

(Finishing this thought: so from the caller / interface's perspective, the "const" qualifier doesn't matter - it only promises that the function will not modify the copy of the argument that it created for itself, which doesn't much matter to the caller in general.

It does matter *to us* in this case, though, because we happen to know that StyleSheetHandle has a pointer to something which does not get copied [the underlying CSSStyleSheet object], and its const methods promise not to mess with that thing.)
https://hg.mozilla.org/mozilla-central/rev/8209287ef070
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.