1324836 - make OriginScope use mozilla::Variant

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Nathan Froyd [:froydnj]]

This data structure is the textbook case for when to use Variant.  Perhaps you'd want to use Maybe<Variant<...>> so you can represent the eNull case.

Comment 1

[Jan Varga [:janv]]

Yeah, but doesn't nsVariant take much more space in memory ?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Jan Varga [:janv] from comment #1)
> Yeah, but doesn't nsVariant take much more space in memory ?

Did you mean nsVariant or mozilla::Variant?  nsVariant would be unwieldy here, I think.  But mozilla::Variant could handle this with no memory overhead if you wanted to store pointers, or it could just store actual objects, which would make things a little simpler.

Comment 3

[Jan Varga [:janv]]

Oh, I see. mozilla::Variant would be no worse.

Comment 4

[Tom Tung [:tt, :ttung]]

I would like to take this bug!

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: [WIP] Bug-1324836-v0- Make QriginScope use mozilla::Variant.

Comment 6

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1324836-v1- Make QriginScope use mozilla::Variant.

- Using Maybe<Variant<...>> to replace union{...} for class OriginScope.
- Remove assertions in all the getter functions since these assertions have already been done in Variant::as().
- Remove function Destroy() since the only thing we need to do is call reset().

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ecc331edfb756e428ca92b7a3f4929c394d221c

Comment 7

[Tom Tung [:tt, :ttung]]

Hi Bevis,

I rewrite the OriginScope as mention in comment #6. Before I ask for the review to :jan, I would like to ask  your feedback about my patch, could you give me some feedback about this? Thanks!

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8824900 [details] [diff] [review]
Bug-1324836-v1- Make QriginScope use mozilla::Variant.

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

Variant looks quite useful here, but there is still something we could improve. :)
Please see my comment inlined.

Thanks!

::: dom/quota/OriginScope.h
@@ +44,5 @@
>        MOZ_COUNT_DTOR(OriginAndAttributes);
>      }
>    };
>  
> +  using OriginOrPatternOrPrefix =

using (Type|VariantType) shall be good enough.
Or we'll have to keep appending OrXxx if a new type is introduced.

@@ +52,2 @@
>  
> +  mozilla::Maybe<OriginOrPatternOrPrefix> mOriginScope;

ditto

@@ +211,4 @@
>      } else if (IsPrefix()) {
> +      match = StringBeginsWith(
> +                aOther.mOriginScope->as<OriginAndAttributes>().mOrigin,
> +                GetPrefix());

We could probably simply this as:
      match = StringBeginsWith(aOther.GetOrigin(), GetPrefix());

@@ +225,5 @@
>  
>      bool match;
>  
>      if (IsOrigin()) {
> +      match = GetPattern().Matches(GetOriginAttributes());

Boom! We die here because this->IsOrigin() is false. 
We should use aOther.GetPattern() for matching:
      match = aOther.GetPattern().Matches(GetOriginAttributes());

@@ +249,5 @@
>  
>      if (IsOrigin()) {
> +      match = StringBeginsWith(
> +                mOriginScope->as<OriginAndAttributes>().mOrigin,
> +                aOther.GetPrefix());

We could simply this as:
      match = StringBeginsWith(GetOrigin(), aOther.GetPrefix());

@@ +312,5 @@
> +  {
> +    mOriginScope.emplace(mozilla::OriginAttributesPattern());
> +    mozilla::OriginAttributesPattern pattern =
> +      mOriginScope->as<mozilla::OriginAttributesPattern>();
> +    MOZ_ALWAYS_TRUE(pattern.Init(aJSONPattern));

I think we can just call SetFromJSONPattern(aJSONPatten) here.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8824900 [details] [diff] [review]
Bug-1324836-v1- Make QriginScope use mozilla::Variant.

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

This is a great start, thank you.

::: dom/quota/OriginScope.h
@@ +247,4 @@
>  
>      bool match;
>  
>      if (IsOrigin()) {

It would be slightly better if this and other Matches* functions were written with Variant's match() support:

http://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#399

Adding new cases would be caught at compile-time, rather than run-time.

Comment 10

[Tom Tung [:tt, :ttung]]

Thanks for the feedback! 

It helps me to find out some problems in my patch! I will address them into my patch.

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> @@ +225,5 @@
> >  
> >      bool match;
> >  
> >      if (IsOrigin()) {
> > +      match = GetPattern().Matches(GetOriginAttributes());
> 
> Boom! We die here because this->IsOrigin() is false. 
> We should use aOther.GetPattern() for matching:
>       match = aOther.GetPattern().Matches(GetOriginAttributes());

Oh, I should find it out before sending request :(

> @@ +312,5 @@
> > +  {
> > +    mOriginScope.emplace(mozilla::OriginAttributesPattern());
> > +    mozilla::OriginAttributesPattern pattern =
> > +      mOriginScope->as<mozilla::OriginAttributesPattern>();
> > +    MOZ_ALWAYS_TRUE(pattern.Init(aJSONPattern));
> 
> I think we can just call SetFromJSONPattern(aJSONPatten) here.

I will apply it to the other ctors if they can as well.

Comment 11

[Tom Tung [:tt, :ttung]]

Thanks for the feedback, too!

(In reply to Nathan Froyd [:froydnj] from comment #9)
> ::: dom/quota/OriginScope.h
> @@ +247,4 @@
> >  
> >      bool match;
> >  
> >      if (IsOrigin()) {
> 
> It would be slightly better if this and other Matches* functions were
> written with Variant's match() support:
> 
> http://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#399
> 
> Adding new cases would be caught at compile-time, rather than run-time.

I was thinking that it might be a problem (makes all the member functions become static), but it turn out it's not a problem. 
So, I will try to implement that!

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1324836-v2- Make QriginScope use mozilla::Variant.

- Address the comment!
try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=63d803050379cb7f5b2dd0f20f3eea3fef3c9360

Comment 13

[Tom Tung [:tt, :ttung]]

Comment on attachment 8825630 [details] [diff] [review]
Bug-1324836-v2- Make QriginScope use mozilla::Variant.

Hi Bevis,

I address comment 8 & 9, could you give me some feedback again? Thanks!

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Keep MatchesXxx non-static

Here is my WIP for your reference to Keep MatchXxx non-static for your reference.
In addition, please also double check if there original design of the |matches| function promise that A.matchesXxx(B) == B.matchesXxx(A).

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8825630 [details] [diff] [review]
Bug-1324836-v2- Make QriginScope use mozilla::Variant.

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

It looks better, but I still suggest to keep the |Matches| methods non-static.
Please check the modified version of your patch attached in comment 13.

In addition, please also double check if there original design of the |matches| function promise that A.matchesXxx(B) == B.matchesXxx(A), if we modify the code in ActorsParent.cpp.

Comment 16

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1324836-v3- Make QriginScope use mozilla::Variant.

Hi Bevis,

Thanks for the feedback! Your suggestion for using non-static member function is great!

Basically, I address your comment in this patch, but I would like to keep using  |MatchesXXX(const XXXType& aXXX)| rather than aOther.  

For the case |A.matchesXxx(B) == B.matchesXxx(A)|, I believe the logic here is same as original code. Because, in each match, we return the same functions as results and pass with the same arguments to each of function.

Could you give me feedback again? Thanks!!

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8826095 [details] [diff] [review]
Bug-1324836-v3- Make QriginScope use mozilla::Variant.

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

Looks great!
If you think aXxx (The Xxx represents the type) is important to you, then
I think aOtherXxx is still better for readability to prevent improper comparison accidentally.

Comment 18

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #17)
Thanks for the feedback! I learn a lot from them!
 
I find out that aOtherXXX is too long, so I address your previous comment which is using aOther/mOther to indicate OtherType.

Comment 19

[Tom Tung [:tt, :ttung]]

Comment on attachment 8826491 [details] [diff] [review]
Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.

Hi Jan,

I use Maybe<..> and Variant<..> to replace the original OriginScope. Besides, I move MatchesOrigin()/MatchesPattern()/MatchesPrefix() from public into private since the same logic come be done by just calling Match(..). 
Could you help me to review this patch? Thanks!

Hi Nathan,

Since you are the reporter and you've already give me some initial feedback, would you mind helping me to review this patch as well? Maybe review the part to using Maybe<> and Variant<> especially. Thanks!

Comment 20

[Jan Varga [:janv]]

Comment on attachment 8826491 [details] [diff] [review]
Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.

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

I haven't checked the "matchers" yet, just some obvious stuff.

::: dom/quota/OriginScope.h
@@ +52,2 @@
>  
> +  mozilla::Maybe<VariantType> mOriginScope;

It's a bit strange for class OriginScope to have a member mOriginScope.

@@ -203,5 @@
>  
>    const nsACString&
>    GetOrigin() const
>    {
> -    MOZ_ASSERT(IsOrigin());

So we don't need this assertion anymore, here and in other similar getters/setters below ?

Comment 21

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #20)
> Comment on attachment 8826491 [details] [diff] [review]
> Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.
> 
> Review of attachment 8826491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't checked the "matchers" yet, just some obvious stuff.
> 
> ::: dom/quota/OriginScope.h
> @@ +52,2 @@
> >  
> > +  mozilla::Maybe<VariantType> mOriginScope;
> 
> It's a bit strange for class OriginScope to have a member mOriginScope.

Hmm, I will try to find out better one!

> @@ -203,5 @@
> >  
> >    const nsACString&
> >    GetOrigin() const
> >    {
> > -    MOZ_ASSERT(IsOrigin());
> 
> So we don't need this assertion anymore, here and in other similar
> getters/setters below ?

Right, since it will hit the asserts in Variant<..>::as(type) if the type is wrong [1] [2]. Sorry for forgetting mention that!

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#578
[2] https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#587

Comment 22

[Jan Varga [:janv]]

Does it hurt to catch the problem a bit earlier, especially when it's a debug-only code ?
Anyway, SetPrefix() still have the assertion ...
If you decide to remove these assertions, you should at least add a comment that Variant::as will catch it, but I think that MOZ_ASSERT(IsOrigin()) is just easier, because it behaves like a comment too.

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #22)
> Does it hurt to catch the problem a bit earlier, especially when it's a
> debug-only code ?
Oh, the MOZ_RELEASE_ASSERT is used in Variant::as() instead.
> Anyway, SetPrefix() still have the assertion ...
Sorry for not capturing this. :\
> If you decide to remove these assertions, you should at least add a comment
> that Variant::as will catch it, but I think that MOZ_ASSERT(IsOrigin()) is
> just easier, because it behaves like a comment too.
IMHO, it's Variant's design intention to move all this assertion into Variant::as(), so the Variant user don't have to worry about this anymore.
Otherwise, it's a punishment to |true| condition that same ASSERTION has to be check twice in debug build. :-(

Comment 24

[Jan Varga [:janv]]

Extra paranoia in debug builds never hurts, we do it all over IDB implementation, but if you think that this will badly affect debug builds... ok :)

Comment 25

[Nathan Froyd [:froydnj]]

Comment on attachment 8826491 [details] [diff] [review]
Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.

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

Looks reasonable to me, but please let Jan have the final say in everything.  Thanks for doing this!

::: dom/quota/OriginScope.h
@@ +125,2 @@
>  
> +    mOriginScope.emplace(OriginAndAttributes(aOrigin));

I don't think you need the extra constructor here and in other emplace() calls; does the code not work correctly if you remove the extra constructor call?

@@ -203,5 @@
>  
>    const nsACString&
>    GetOrigin() const
>    {
> -    MOZ_ASSERT(IsOrigin());

I think we should keep these assertions, even with Variant::as supposedly doing release-mode assertion checks.  The compiler can probably remove the redundant checks when doing debug + optimize builds, and in a completely unoptimized debug build, doing multiple checks here is not going to be your performance problem. ;)

@@ +309,5 @@
> +      bool match(const nsACString& aPrefix) {
> +        // The match will be always true here because any origin attributes
> +        // pattern overlaps any origin prefix (an origin prefix targets all
> +        // origin attributes).
> +        return true; }

Nit: closing brace on its own line, please.

Comment 26

[Tom Tung [:tt, :ttung]]

(In reply to Nathan Froyd [:froydnj] from comment #25)
Thanks for the review and feedback!

> Comment on attachment 8826491 [details] [diff] [review]
> Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.
> 
> Review of attachment 8826491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable to me, but please let Jan have the final say in everything.
> Thanks for doing this!
> 
> ::: dom/quota/OriginScope.h
> @@ +125,2 @@
> >  
> > +    mOriginScope.emplace(OriginAndAttributes(aOrigin));
> 
> I don't think you need the extra constructor here and in other emplace()
> calls; does the code not work correctly if you remove the extra constructor
> call?

Do you mean the OriginAndAttributes(aOrigin)? If the answer is yes, we want to distinguish the prefix(nsCString) and origin(OriginAndAttributes->nsCString).

> @@ -203,5 @@
> >  
> >    const nsACString&
> >    GetOrigin() const
> >    {
> > -    MOZ_ASSERT(IsOrigin());
> 
> I think we should keep these assertions, even with Variant::as supposedly
> doing release-mode assertion checks.  The compiler can probably remove the
> redundant checks when doing debug + optimize builds, and in a completely
> unoptimized debug build, doing multiple checks here is not going to be your
> performance problem. ;)

Ok, I will add them back since both :jan and you suggest that.

Comment 27

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1324836-v5- Make QriginScope use mozilla::Variant. f=bevis.

Update the patch and address the comment so far.
- Add the assertions in getter funcitons and setter funcitons back.
- Rename mOriginScope to mData.
- Fix the nits.

Comment 28

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #26)
> (In reply to Nathan Froyd [:froydnj] from comment #25)
> Thanks for the review and feedback!
> 
> > Comment on attachment 8826491 [details] [diff] [review]
> > Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.
> > 
> > Review of attachment 8826491 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks reasonable to me, but please let Jan have the final say in everything.
> > Thanks for doing this!
> > 
> > ::: dom/quota/OriginScope.h
> > @@ +125,2 @@
> > >  
> > > +    mOriginScope.emplace(OriginAndAttributes(aOrigin));
> > 
> > I don't think you need the extra constructor here and in other emplace()
> > calls; does the code not work correctly if you remove the extra constructor
> > call?
> 
> Do you mean the OriginAndAttributes(aOrigin)? If the answer is yes, we want
> to distinguish the prefix(nsCString) and
> origin(OriginAndAttributes->nsCString).

I find out I cant remove one for SetFromPattern() since they both are OriginAttributesPattern.

Comment 29

[Tom Tung [:tt, :ttung]]

There is a typo in the previous comment :P

(In reply to Tom Tung [:tt] from comment #26)
> > I don't think you need the extra constructor here and in other emplace()
> > calls; does the code not work correctly if you remove the extra constructor
> > call?
> 
> Do you mean the OriginAndAttributes(aOrigin)? If the answer is yes, we want
> to distinguish the prefix(nsCString) and
> origin(OriginAndAttributes->nsCString).

I find out I can remove one for SetFromPattern() since they both are OriginAttributesPattern.

Comment 30

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1324836-v5.1- Make QriginScope use mozilla::Variant. f=bevis. r=froydnj.

Update patch per comment#29

Comment 31

[Jan Varga [:janv]]

btw, here's |sizeof(OriginScope)| comparison:
16 w/o changes
136 patch applied
128 Maybe<...> removed

So, even null OriginScope will take 128 bytes.
Anyway, this comparison is not fair since current implementation allocates additional structures dynamically and it needs to keep pointers to them.

Anyway, Maybe<...> can be removed I believe, I have a patch for that, needs more testing ...

Comment 32

[Jan Varga [:janv]]

I think it would be great to add tests for OriginScope. It has to be written in C++ I guess.
We are doing quite big changes here and I don't feel comfortable with giving final r+ w/o having some tests.
For example, the test should check (pseudo code):
OriginScope::FromOrigin("http+++www.mozilla.org^appId=1007") matches OriginScope::FromPattern("appId=1007");

Tom, could you write such a test ?
Thanks.

Comment 33

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #32)
> I think it would be great to add tests for OriginScope. It has to be written
> in C++ I guess.
> We are doing quite big changes here and I don't feel comfortable with giving
> final r+ w/o having some tests.
> For example, the test should check (pseudo code):
> OriginScope::FromOrigin("http+++www.mozilla.org^appId=1007") matches
> OriginScope::FromPattern("appId=1007");
> 
> Tom, could you write such a test ?
> Thanks.

Sure, but I've never written c++ test to test class functionality in gecko before. Do you mean Gtest? 
I'll work on it and provide a patch for tests!

Comment 34

[Jan Varga [:janv]]

Yes, a GTest.

Comment 35

[Jan Varga [:janv]]

Attachment: patch

Comment 36

[Jan Varga [:janv]]

Attachment: interdiff

Comment 37

[Jan Varga [:janv]]

Attachment: OriginScope.h

Comment 38

[Jan Varga [:janv]]

OriginAttributes and OriginAttributesPattern are quite big structs so I decided to allocate them dynamically. I also replaced Maybe with a Null variant. OriginScope now takes 32 bytes (up 16 bytes from current m-c, but down 104 bytes from v5.1 patch). It's not exact comparison, but the pattern type is not used much and it takes over 100 bytes, so I think it makes sense to allocate it dynamically. There are also some other fixes and improvements.

I'm going to ask :asuth for review.

Tom, you can work on the test in the meantime.

Thanks!

Comment 39

[Jan Varga [:janv]]

Comment on attachment 8833742 [details] [diff] [review]
patch

Andrew, there's also an interdiff patch and whole OriginScope.h attached. I recommend looking at the whole file.

Comment 40

[Tom Tung [:tt, :ttung]]

Attachment: [WIP] Bug-1324836-P2- gtest for OriginScope.

Init patch for gtest to test OriginScope::Matches()

Comment 41

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8833742 [details] [diff] [review]
patch

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

::: dom/quota/OriginScope.h
@@ +51,2 @@
>  
> +    Origin(const Origin& aOther)

This copy-constructor will suppress the implicit move constructor, per http://en.cppreference.com/w/cpp/language/move_constructor and it's likely the lack of move-support is obscured by the fact that implicitly converts T&& to T&.

Since Move() is explicitly used throughout, and UniquePtr does not have a copy constructor (meaning you can't use the default/implicit copy constructor), I think the right thing to do is:
  Origin(Origin&& aOther) == default

@@ +90,5 @@
> +    {
> +      MOZ_ALWAYS_TRUE(mPattern->Init(aJSONPattern));
> +    }
> +
> +    Pattern(const Pattern& aOther)

Same deal with suppressed implicit move, suggest adding:
  Pattern(Pattern&& AOther) = default;

Comment 42

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8833742 [details] [diff] [review]
patch

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

::: dom/quota/OriginScope.h
@@ +226,5 @@
> +    Origin& origin = mData.as<Origin>();
> +
> +    origin.mOrigin = aOrigin;
> +
> +    origin.InitAttributes();

Acknowledging that I see the call to InitAttributes() was added and that it's more correct than what was there before, but it's not a big deal because it does not change QM behavior.  The affected code does an Origin-matches-Origin check which only compares the origins, not the attributes.  It was bad they were stale and this is better, but it didn't matter.

== Details, no need to read:

The existing use of SetOrigin in QM is:
  nsCString originSanitized(originScope.GetOrigin());
  SanitizeOriginString(originSanitized);
  originScope.SetOrigin(originSanitized);
Where SanitizeOriginString replaces kReplaceChars with +'s:
  QuotaManager::kReplaceChars[] = CONTROL_CHARACTERS "/:*?\"<>|\\";
With the mainly observable effect of "http://example.com" becoming "http+++example.com".

Because OriginAttributes cares about '^', '=', and '&' and these are left untouched, the only thing that matters is string attributes which could have contents that could get mangled.  OriginAttributesDictionary has string members mAddonId and mFirstPartyDomain, all the rest are bool or uint32_t.  It sorta looks like mAddonId should be a FS-safe UUID or a "[e-mail]" value, and so should never have been sanitized.  But at least a few tests indicate firstPartyDomain includes the "https://" bit, so they would likely have been affected if matching against a pattern.  (But no one did that.)

Comment 43

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1324836-P2 - v0 - Gtest for OriginScope.

Comment 44

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1324836-P2 - v0.1 - Gtest for OriginScope.

Hi Jan,

I wrote a gtest to test OriginScope. Could you help me to review it? However, there are two things that I'm not pretty sure about that. I listed them below inline.

The first thing is that I notice there is a warning [1](file not found) when calling OriginAttributesPatternDictionary::Init(). This warning doesn't effect on the result of test so I'm not sure whether it is matter.  

Besides, could you take more care on prefix part? I'm not sure about the test case for prefix is correct since I'm not sure exactly about the definition of origin prefix. Does it represent to the scheme of an origin? Or just "prefix" of origin? (like: http://moz, http://mozilla ?)

[1] http://searchfox.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#2652

Comment 45

[Jan Varga [:janv]]

The prefix was added for clearing entire sites. For example, if we have:
http://www.mozilla.org
http://www.mozilla.org^userContextId=1
http://www.mozilla.org^userContextId=2
then we create OriginScope with "http://www.mozilla.org" prefix to clear all these origin directories

We don't care about variations like http://moz, http://mozilla, etc. right now.

Comment 46

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #45)
> The prefix was added for clearing entire sites. For example, if we have:
> http://www.mozilla.org
> http://www.mozilla.org^userContextId=1
> http://www.mozilla.org^userContextId=2
> then we create OriginScope with "http://www.mozilla.org" prefix to clear all
> these origin directories
> 
> We don't care about variations like http://moz, http://mozilla, etc. right
> now.

Oh, I see! Thanks for the explanation! I will update test later!

Comment 47

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1324836-P2 - v1 - Gtest for OriginScope.

Update patch for changing test case about prefix and tests for testing matches() function.

Comment 48

[Eric Rahm [:erahm]]

Is this work still active or should we unassign and open it up for other contributors?

Comment 49

[Jan Varga [:janv]]

(In reply to Eric Rahm [:erahm] from comment #48)
> Is this work still active or should we unassign and open it up for other
> contributors?

This is a low priority bug. The primary fix is finished/reviewed. It's the C++ test that needs to be verified by me.
I don't think we need other contributors here.

Comment 50

[Eric Rahm [:erahm]]

(In reply to Jan Varga [:janv] from comment #49)
> (In reply to Eric Rahm [:erahm] from comment #48)
> > Is this work still active or should we unassign and open it up for other
> > contributors?
> 
> This is a low priority bug. The primary fix is finished/reviewed. It's the
> C++ test that needs to be verified by me.
> I don't think we need other contributors here.

Okay lets clear the good first bug flag since this is basically done, but isn't assigned.

Comment 51

[Jan Varga [:janv]]

I'm going to resurrect this.

Comment 52

[Jan Varga [:janv]]

Comment on attachment 8834343 [details] [diff] [review]
Bug-1324836-P2 - v1 - Gtest for OriginScope.

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

This looks too complicated. We need something more straightforward.

Comment 53

[Jan Varga [:janv]]

Attachment: rebased main patch

Comment 54

[Jan Varga [:janv]]

Attachment: gtest

Comment 55

[Jan Varga [:janv]]

Comment on attachment 9007151 [details] [diff] [review]
gtest

Andrew, you r+'ed the main patch long time ago, but I somehow couldn't find time to review/update a new gtest for it.
I had to do something similar for new local storage recently, so I decided to finish this bug too.
Thanks.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35c9f4bc32ed
Part 1: Make OriginScope use mozilla::Variant; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/898a443cbe3d
Part 2: Add a gtest for OriginScope testing; r=asuth

Comment 57

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/35c9f4bc32ed
https://hg.mozilla.org/mozilla-central/rev/898a443cbe3d