1246464 - Mark some stylesheet-related args as 'const'

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

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 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 2

[Cameron McCormack (: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.

Comment 3

[Daniel Holbert [:dholbert]]

(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!

Comment 4

[Daniel Holbert [:dholbert]]

(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.)

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8209287ef070

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8209287ef070