938025 - hit a little more stuff with constexpr to fix some static constructors

Status: RESOLVED FIXED

(Core :: General, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: bug 938025 - some static constructors

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 831373 [details] [diff] [review]
bug 938025 - some static constructors

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

::: gfx/layers/FrameMetrics.h
@@ +36,5 @@
>    // We use IDs to identify frames across processes.
>    typedef uint64_t ViewID;
>    static const ViewID NULL_SCROLL_ID;   // This container layer does not scroll.
>    static const ViewID ROOT_SCROLL_ID;   // This is the root scroll frame.
> +  static const ViewID START_SCROLL_ID = 2;  // This is the ID that scrolling subframes

This seems unrelated to the rest of this patch, r-.

::: gfx/layers/Layers.cpp
@@ -37,5 @@
>  
>  typedef FrameMetrics::ViewID ViewID;
>  const ViewID FrameMetrics::NULL_SCROLL_ID = 0;
>  const ViewID FrameMetrics::ROOT_SCROLL_ID = 1;
> -const ViewID FrameMetrics::START_SCROLL_ID = 2;

This seems unrelated to the rest of this patch, r-.

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 831373 [details] [diff] [review]
> bug 938025 - some static constructors
> 
> Review of attachment 831373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/FrameMetrics.h
> @@ +36,5 @@
> >    // We use IDs to identify frames across processes.
> >    typedef uint64_t ViewID;
> >    static const ViewID NULL_SCROLL_ID;   // This container layer does not scroll.
> >    static const ViewID ROOT_SCROLL_ID;   // This is the root scroll frame.
> > +  static const ViewID START_SCROLL_ID = 2;  // This is the ID that scrolling subframes
> 
> This seems unrelated to the rest of this patch, r-.

err, forgot about that bit, but actually it was done for the same reason to get rid of a static initializer somewhere.  What happen is this, somefile did
static const int something = FrameMetrics::VIEW_ID;
So the compiler created a static initializer to copy the value of VIEW_ID to something because it couldn't tell at compile time what that was.  Which is oh so sad making *sigh*

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: bug 938025 - some static constructors

the RootingAPI.h changes are necessary because of js/src/vm/Id.cpp which calls FromMarkedLocation on a global.
The FrameMetrics change is to allow another constant based on it to be computable at compile time.

waldo for the js stuff
bjacob for the gfx/ stuff
bz for the other odds and ends

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8335478 [details] [diff] [review]
bug 938025 - some static constructors

Why do you need to expand out NS_DECL_ISUPPORTS?

Comment 6

[Boris Zbarsky [:bzbarsky]]

And if the key is that you want the MOZ_OVERRIDE, then we should just fix the codegenerator for xpidl to do that automatically, I think.

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8335478 [details] [diff] [review]
> bug 938025 - some static constructors
> 
> Why do you need to expand out NS_DECL_ISUPPORTS?

I don't want the nsAutoRefCnt since AddRef / Release just return a constant and the nsAutoRefcnt constructor isn't constexpr.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah.  So you're looking for NS_DECL_ISUPPORTS_INHERITED, then.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Ah.  So you're looking for NS_DECL_ISUPPORTS_INHERITED, then.

uh, I guess that actually works, I'd forgotten we always over ride AddRef / Release and didn't think about it because this isn't really inheriting from anything.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8335478 [details] [diff] [review]
bug 938025 - some static constructors

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

::: gfx/layers/FrameMetrics.h
@@ +36,5 @@
>    // We use IDs to identify frames across processes.
>    typedef uint64_t ViewID;
>    static const ViewID NULL_SCROLL_ID;   // This container layer does not scroll.
>    static const ViewID ROOT_SCROLL_ID;   // This is the root scroll frame.
> +  static const ViewID START_SCROLL_ID = 2;  // This is the ID that scrolling subframes

This looks like an unintentional change in this patch!

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Benoit Jacob [:bjacob] from comment #10)
> Comment on attachment 8335478 [details] [diff] [review]
> bug 938025 - some static constructors
> 
> Review of attachment 8335478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/FrameMetrics.h
> @@ +36,5 @@
> >    // We use IDs to identify frames across processes.
> >    typedef uint64_t ViewID;
> >    static const ViewID NULL_SCROLL_ID;   // This container layer does not scroll.
> >    static const ViewID ROOT_SCROLL_ID;   // This is the root scroll frame.
> > +  static const ViewID START_SCROLL_ID = 2;  // This is the ID that scrolling subframes
> 
> This looks like an unintentional change in this patch!

no, read comment 3 and comment 4

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8335478 [details] [diff] [review]
bug 938025 - some static constructors

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

Ah, OK, sorry.

Comment 13

[Trevor Saunders (:tbsaunde)]

Attachment: bug 938025 - some static constructors

use the NS_DECL_ISUPPORTS_INHERITED hack

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8335641 [details] [diff] [review]
bug 938025 - some static constructors

r=me on my bits

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 8335641 [details] [diff] [review]
bug 938025 - some static constructors

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

r+ with a tweak or two, and with the small but rather important changes noted to RootingAPI.h.

::: content/html/content/src/HTMLTrackElement.cpp
@@ +61,5 @@
>  namespace mozilla {
>  namespace dom {
>  
>  // The default value for kKindTable is "subtitles"
> +static MOZ_CONSTEXPR const char* kKindTableDefaultString = kKindTable->tag;

I think we added MOZ_CONSTEXPR_VAR for const variables.  That lets you kill the const here, and it also prevents warnings with some compilers for constexpr *and* const both on the same variable.  (Which was not legal at one time but became so later in C++11, or was legal but warned, or something.  Point is, MOZ_CONSTEXPR const is bad.  :-) )

::: content/html/content/src/HTMLTrackElement.h
@@ +21,5 @@
>  namespace mozilla {
>  namespace dom {
>  
>  // Map html attribute string values to TextTrackKind enums.
> +static MOZ_CONSTEXPR nsAttrValue::EnumTable kKindTable[] = {

MOZ_CONSTEXPR_VAR

::: js/public/RootingAPI.h
@@ +479,5 @@
>      void repoint(const Handle &rhs) { ptr = rhs.address(); }
>  
>    private:
>      Handle() {}
> +    MOZ_CONSTEXPR Handle(const T *p) : ptr(p) {}

So, adding a constructor like this -- even privately -- is kind of asking for trouble.  It's pretty similar to the other Handle constructors in that it can take one argument, and it'd be really pretty easy to accidentally call this when you meant to call another one.  The results of doing so would be very unsafe!  (And private doesn't save you here, because that still lets the whole of Handle use it, and also MutableHandle would see it, too, because friend.  I don't really want to have to trust both classes to carefully never call this except when it's okay -- unsafe stuff should be big and blaring as fromMarkedLocation is.)  Basically exactly the same concerns would apply to this constructor, as apply to fromMarkedLocation.

But we should be able to address these concerns without too much trouble -- we just need to gussy up the overload so that calling it means you *really* meant to get this exact constructor, and no other.  So let's do this:

  static MOZ_CONSTEXPR Handle fromMarkedLocation(const T *p) {
    return Handle(p, DeliberatelyChoosingThisOverload, ImUsingThisOnlyInFromFromMarkedLocation);
  }

  enum Disambiguator { DeliberatelyChoosingThisOverload = 42 };
  enum CallerIdentity { ImUsingThisOnlyInFromFromMarkedLocation = 17 };
  MOZ_CONSTEXPR Handle(const T *p, Disambiguator, CallerIdentity) : ptr(p) {}

If you're calling the method in this case, it's only ever going to be because someone really really meant to.  :-)

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 8335478 [details] [diff] [review]
bug 938025 - some static constructors

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

This...looks almost like a carbon copy of the previous patch?  Reflag if you really want me looking at this patch, please, beyond my saying "do what I said re the other patch".

::: content/html/content/src/HTMLTrackElement.cpp
@@ +61,5 @@
>  namespace mozilla {
>  namespace dom {
>  
>  // The default value for kKindTable is "subtitles"
> +static MOZ_CONSTEXPR const char* kKindTableDefaultString = kKindTable->tag;

MOZ_CONSTEXPR_VAR

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/93370e831032

Comment 18

[Trevor Saunders (:tbsaunde)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> Comment on attachment 8335478 [details] [diff] [review]
> bug 938025 - some static constructors
> 
> Review of attachment 8335478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This...looks almost like a carbon copy of the previous patch?  Reflag if you
> really want me looking at this patch, please, beyond my saying "do what I
> said re the other patch".

indeed, apparently the r? on the old one didn't get canceled :/ sorry!

Comment 19

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca137ceb2877

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca137ceb2877