1049812 - Refactor History.cpp to make 'visitCount' unsigned and 'bookmarked' a bool, and keep track of whether they're initialized

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Daniel Holbert [:dholbert]]

Spinning this off of bug 922727. That bug is about visitCount being currently a signed int, which is silly because it's a count and counts can't be negative.

Mak says in bug 922727 comment 1:
> I think the original scope was to assert that they (both visitCount and
> bookmarked) were always initialized before being read, though those asserts
> don't exist as of now.
> I think the best thing would be to make visitCount a uint (init to 0),
> bookmarked a bool (init to false) and add an initialized field that we can
> assert in debug mode.
> Well, probably the cleanest thing would be to make both properties private,
> expose a setter that sets both and initialized, and 2 getters, one for each,
> that will assert if not initialized.

Spinning this bug off to cover that ^.

Comment 1

[Arnaud Sourioux [:Six]]

Hi,

Which solution is the best between case #1 and case #2 in this pastebin?
http://pastebin.com/SGg38WEU
Maybe there is already a templated class in mozilla such as IsSetProperty that i used in Case #2

Thanks,

Comment 2

[Daniel Holbert [:dholbert]]

We have a Maybe<T> template, which is effectively just a "T" and a bool that keeps track if the T is initialized or not.  I think that's similar to "IsSetProperty<int> _property2;" in your pastebin.

We don't want to use Maybe<> here, though.  Here, the "initialized" flags are only supposed to be a sanity-check, and we only want them to exist in *debug* builds.  And Maybe<> doesn't have any debug-specific stuff.

So, I think we really do just need some bools that are #ifdef DEBUG.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> So, I think we really do just need some bools that are #ifdef DEBUG.

Sorry, just re-read comment 0 -- sounds like Mak just wanted *one* "initialized" bool, with a single setter which sets both member-vars and the bool.

Comment 4

[Arnaud Sourioux [:Six]]

Attachment: refactor_PlaceHashKey_to_avoid_static_cast.patch

Hi,

This should almost be what you asked for.

I just have a question about thoses lines (begins at line 1808):
>       int32_t visitCount, bookmarked;
>       rv = stmt->GetInt32(5, &visitCount);
>       NS_ENSURE_SUCCESS(rv, rv);
>       rv = stmt->GetInt32(6, &bookmarked);

visitCount type is supposed to be changed to a uint32_t and bookmarked to a bool, so which methods should i call to keep this behaviour?

Thanks,

PS: i will also upload a separate indent patch (after this one is completed) for class PlaceHashKey as the indentation in the class definition doesn't seems correct in m-c

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8482950 [details] [diff] [review]
refactor_PlaceHashKey_to_avoid_static_cast.patch

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

::: toolkit/components/places/History.cpp
@@ +211,5 @@
>      {
>        MOZ_ASSERT(false, "Do not call me!");
>      }
>  
> +  void setProperties(uint visitCountValue, bool bookmarkedValue)

Use "uint32_t", not "uint".  (This applies to getVisitCount(), too.)

Also, the function name should be capitalized, per
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods
(This applies to getVisitCount and getBookmarked, too.)

The arguments should probably be named "aVisitCount" and "aIsBookmarked", too, per
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes

@@ +225,5 @@
> +  {
> +#ifdef DEBUG
> +    MOZ_ASSERT(isSetProperty, "PlaceHashKey::visitCount not set");
> +#endif
> +    return (visitCount);

Drop the unnecessary parenthesis around (visitCount) here, in getVisitCount().  Same for getBookmarked's return statement.

@@ +228,5 @@
> +#endif
> +    return (visitCount);
> +  }
> +
> +  bool getBookmarked() const

Perhaps this should be named "isBookmarked"? Either way is probably fine, though.

@@ +245,5 @@
> +  // Whether this place is bookmarked.
> +  bool bookmarked;
> +  // Whether previous attributes are set.
> +#ifdef DEBUG
> +  bool isSetProperty;

I don't understand what "isSetProperty" means. (the word "property" seems out of place)

Maybe call this "isInitialized"?  (or "mIsInitialized", except that the other member-vars don't use the "m" prefix, so maybe it'd be out of place until we convert them.)

@@ +1844,1 @@
>        rv = stmt->GetInt32(5, &visitCount);

Per your comment here, this does need to be changed. Probably best to match what we do above it for "transition" -- use an int32_t local var, and then static_case when we call SetProperties().

Comment 6

[Daniel Holbert [:dholbert]]

Sorry, that last part was missing the context. The context was:
>-      int32_t visitCount, bookmarked;
>+      uint32_t visitCount;
>+      bool bookmarked;
>+      //This needs to be changed
>       rv = stmt->GetInt32(5, &visitCount);

Comment 7

[Arnaud Sourioux [:Six]]

Attachment: refactor_PlaceHashKey_to_avoid_static_cast.patch Part1/3

Comment 8

[Arnaud Sourioux [:Six]]

Attachment: fix_indentation_toolkit_History.patch Part2/3

Comment 9

[Arnaud Sourioux [:Six]]

Attachment: rename_PlaceHashKey_members_with_m_prefix.patch Part 3/3

Comment 10

[Arnaud Sourioux [:Six]]

Attachment: fix_indentation_toolkit_History.patch

previous patch was marked Part1 instead of Part2

Comment 11

[Arnaud Sourioux [:Six]]

Attachment: fix_indentation_toolkit_History.patch Part2/3

Comment 12

[Arnaud Sourioux [:Six]]

Attachment: refactor_PlaceHashKey_to_avoid_static_cast.patch Part1/3

i forgot to remove a useless comment in the previous patch (thanks dholbert :) )

Comment 13

[Arnaud Sourioux [:Six]]

Tbpl is green:
https://tbpl.mozilla.org/?tree=Try&rev=c780ddb9557d

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8483008 [details] [diff] [review]
refactor_PlaceHashKey_to_avoid_static_cast.patch Part1/3

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

::: toolkit/components/places/History.cpp
@@ +220,5 @@
> +    isInitialized = true;
> +#endif
> +  }
> +
> +  uint32_t GetVisitCount() const

just VisitCount() will be fine, or you should also have GetIsBookmarked. let's keep the less verbose versions, so VisitCount() and IsBookmarked().

@@ +244,5 @@
> +  uint32_t visitCount;
> +  // Whether this place is bookmarked.
> +  bool bookmarked;
> +  // Whether previous attributes are set.
> +#ifdef DEBUG

please move the comment inside the ifdef

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8483001 [details] [diff] [review]
fix_indentation_toolkit_History.patch Part2/3

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

thank you for this cleanup

Comment 16

[Arnaud Sourioux [:Six]]

Attachment: refactor_PlaceHashKey_to_avoid_static_cast.patch Part1/3

Fixing previous version with Mak's comments (keeping the review+)

Comment 17

[Carsten Book [:Tomcat]]

Hi,

part 2 and part3 failed to apply like:

renamed 1049812 -> rename_PlaceHashKey_members_with_m_prefix.patch
applying rename_PlaceHashKey_members_with_m_prefix.patch
patching file toolkit/components/places/History.cpp
Hunk #1 FAILED at 182
Hunk #2 FAILED at 1599
Hunk #3 FAILED at 1690
3 out of 4 hunks FAILED -- saving rejects to file toolkit/components/places/History.cpp.rej
patch failed, unable to continue (try -v)


and renamed 1049812 -> fix_indentation_toolkit_History.patch
applying fix_indentation_toolkit_History.patch
patching file toolkit/components/places/History.cpp
Hunk #2 FAILED at 231
1 out of 2 hunks FAILED -- saving rejects to file toolkit/components/places/History.cpp.rej

could you take a look and maybe rebase on a current tree, thanks!

Comment 18

[Arnaud Sourioux [:Six]]

Hi,

You have to import fix_indentation_toolkit_History.patch before rename_PlaceHashKey_members_with_m_prefix.patch

this is why they are named Part 2/3 and Part 3/3 :)

Comment 19

[Daniel Holbert [:dholbert]]

I get conflicts when applying part 1/3 on a clean trunk, too. (probably from patch conflicts with bug 1060974, which landed in the last few days and touched History.cpp)

Comment 20

[Arnaud Sourioux [:Six]]

I discussed with Tomcat this morning and there is a patch in inbound that is preventing from applying correctly.
I will wait until it lands and fix them all.
Thanks for the feedback

Comment 21

[Arnaud Sourioux [:Six]]

Attachment: refactor_PlaceHashKey_to_avoid_static_cast.patch Part1/3

Comment 22

[Arnaud Sourioux [:Six]]

Attachment: fix_indentation_toolkit_History.patch Part2/3

Comment 23

[Arnaud Sourioux [:Six]]

Attachment: rename_PlaceHashKey_members_with_m_prefix.patch Part 3/3

Comment 24

[Arnaud Sourioux [:Six]]

Thoses are the same patches just with the last changes that where applied in bug 1060974 as Dholbert said.
I kept the review+ as nothing changed in my patches.
tbpl is still green: https://tbpl.mozilla.org/?tree=Try&rev=15096625607c

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8484495 [details] [diff] [review]
refactor_PlaceHashKey_to_avoid_static_cast.patch Part1/3

>diff --git a/toolkit/components/places/History.cpp b/toolkit/components/places/History.cpp
>-    , bookmarked(-1)
>+    , visitCount(0)
>+    , bookmarked(false) visits;
>+private:
>     // Visit count for this place.
>     int32_t visitCount;
>     // Whether this place is bookmarked.
>     int32_t bookmarked;


Looks like you're no longer doing the int32_t --> uint32_t type conversion (for visitCount) and int32_t --> bool (for 'bookmarked') conversions here. Those conversions were in the previous version of this patch (attachment 8483693 [details] [diff] [review]), but are missing in this version.

Comment 26

[Arnaud Sourioux [:Six]]

Yup unfortunatly you're totally right...
i didn't watch this correctly when modifying my patches...
Thanks for your eagle eyes that prevented thoses wrong patches to get landed. :)

Comment 27

[Arnaud Sourioux [:Six]]

Attachment: refactor_PlaceHashKey_to_avoid_static_cast.patch Part 1/3

Comment 28

[Arnaud Sourioux [:Six]]

Attachment: fix_indentation_toolkit_History.patch Part 2/3

Comment 29

[Arnaud Sourioux [:Six]]

Attachment: rename_PlaceHashKey_members_with_m_prefix.patch Part 3/3

Comment 30

[Arnaud Sourioux [:Six]]

Daniel can you take a last look at it to confirm that this time it's okay?

Comment 31

[Daniel Holbert [:dholbert]]

Yup, looks good now. (I just compared it side-by-side with the original (reviewed but bitrotted) patches, switching tabs back and forth to look for differences.)

Adding 'checkin-needed' back.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/27ea646472f9
https://hg.mozilla.org/integration/fx-team/rev/d38ec8e9cfd6
https://hg.mozilla.org/integration/fx-team/rev/750dac4dd853

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27ea646472f9
https://hg.mozilla.org/mozilla-central/rev/d38ec8e9cfd6
https://hg.mozilla.org/mozilla-central/rev/750dac4dd853