Opened 5 years ago

Closed 5 years ago

#1585 closed change (rejected)

Inconsistency between ReferrerMapping::BuildReferrerChain and comment for FilterEngine:: Matches

Reported by: sergz Assignee: sergz
Priority: Unknown Milestone:
Module: Libadblockplus Keywords:
Cc: fhd, trev Blocked By:
Blocking: #1564 Platform: Unknown
Ready: no Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/4840423822458880/

Description

Background

For A->B->C->D (A loads B, B loads C, C loads D) we build referrer chain as [A, B, C] https://hg.adblockplus.org/libadblockplus/file/38cb9fd4a841/src/ReferrerMapping.cpp#l56 . On another hand, according to the comment https://hg.adblockplus.org/libadblockplus/file/38cb9fd4a841/include/AdblockPlus/FilterEngine.h#l258 it should be [C, B, A].
As well as tests show that it's [A, B, C] https://hg.adblockplus.org/libadblockplus/file/38cb9fd4a841/test/ReferrerMapping.cpp#l41

What to change

Fix the impl of ReferrerMapping::BuildReferrerChain and fix the tests.

Change History (2)

comment:1 Changed 5 years ago by sergz

  • Status changed from new to reviewing

comment:2 Changed 5 years ago by fhd

  • Resolution set to rejected
  • Status changed from reviewing to closed

It's just the comment that's wrong, everything else is fine. I've created #1586 for fixing it.

If we were to actually reverse the order in the code right now, we'd have to change it in two more places:

  1. FilterEngine::Matches would actually have to work the wrong way around.
  2. ReferrerMapping.cpp is currently duplicated in ABP for Android as ReferrerMapping.java.

Anyway, IMO the current order (starting with the top-level frame) actually makes more sense when considering that this should represent the frame structure.

Note: See TracTickets for help on using tickets.