Closed Bug 1478462 Opened 6 years ago Closed 6 years ago

Make it easier to change the conditions checked for the cookie restrictions feature

Categories

(Firefox :: Protections UI, enhancement, P3)

enhancement

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(2 files)

      No description provided.
Assignee: nobody → ehsan
Depends on: 1478428
Blocks: 1478427
Comment on attachment 8994955 [details] [diff] [review]
Part 2: Switch the existing checks for the cookie restrictions feature to use the centralized helpers

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

::: netwerk/base/nsChannelClassifier.cpp
@@ +1016,5 @@
>           mChannelClassifier.get(), channel.get()));
>      channel->Cancel(aErrorCode);
>    } else {
>      MOZ_ASSERT(mChannelClassifier->ShouldEnableTrackingAnnotation() ||
> +               AntiTrackingCommon::ShouldActivate());

If we're going to annotate first-party channels anyways, why do we need a separate thing here? Can't we roll that into tracking annotations?

This is getting confusing with three different anti-tracking things...
Status: NEW → ASSIGNED
Priority: -- → P3
(In reply to François Marier [:francois] from comment #3)
> Comment on attachment 8994955 [details] [diff] [review]
> Part 2: Switch the existing checks for the cookie restrictions feature to
> use the centralized helpers
> 
> Review of attachment 8994955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsChannelClassifier.cpp
> @@ +1016,5 @@
> >           mChannelClassifier.get(), channel.get()));
> >      channel->Cancel(aErrorCode);
> >    } else {
> >      MOZ_ASSERT(mChannelClassifier->ShouldEnableTrackingAnnotation() ||
> > +               AntiTrackingCommon::ShouldActivate());
> 
> If we're going to annotate first-party channels anyways, why do we need a
> separate thing here? Can't we roll that into tracking annotations?
> 
> This is getting confusing with three different anti-tracking things...

Hmm, this was added in attachment 8992870 [details] [diff] [review], specifically so that we classify a) when TP is on, b) when the TP annotations are turned on, or c) when the anti-tracking feature is on, possibly including top-level channels in that case.  In the (b) scenario, we don't annotate top-level channels, only in (c).  As far as I understand, the change in the assertion is made to mirror the condition check that was modified in that patch.

Did you mean to suggest to change these three levels somehow?  If yes, I'd appreciate if you can file a follow-up please for doing that, my goal in this bug is to explicitly not change our existing behavior, only to refactor code...
Flags: needinfo?(francois)
Attachment #8994954 - Flags: review?(amarchesini) → review+
Attachment #8994955 - Flags: review?(amarchesini) → review+
(In reply to (PTO, back on Aug 8th) from comment #4)
> Did you mean to suggest to change these three levels somehow?  If yes, I'd
> appreciate if you can file a follow-up please for doing that, my goal in
> this bug is to explicitly not change our existing behavior, only to refactor
> code...

Yes, I think we can have only 2 levels: tracking protection and tracking annotations.

I filed bug 1480923 for this refactoring.
Flags: needinfo?(francois)
These patches are stale and no longer needed.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: