Changes between Version 1 and Version 2 of Ticket #4599, comment 14


Ignore:
Timestamp:
11/12/2016 08:38:14 AM (3 years ago)
Author:
kzar
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #4599, comment 14

    v1 v2  
    1 Unless I'm mistaken it's basically the same problem as #4483. 
     1Edit: I was mistaken, since as [https://codereview.adblockplus.org/29362203/#msg2 Sebastian pointed out in the code review] Chrome gives pre-rendered pages a new tab id. As for the $sitekey stuff, I've opened #4632 for that. 
    22 
    3 If a page makes a web request before its `onCommitted` event has fired the request will be blocked / whitelisted based upon the old page structure. So if the URL for the previous page was of a different domain the request will be considered third party, or if the new page has a sitekey we won't know to take it into account. 
     3~~Unless I'm mistaken it's basically the same problem as #4483. 
    44 
    5 Thing is the old behaviour was no better, arguably worse. The `onBeforeNavigate` event we used wasn't fired after redirections, so the page structure was not updated at all for those. Also it was fired for pre-rendered pages which meant that we effectively assumed that pre-rendered pages would always be navigated to. So we can't simply revert #4386 and I think it's a little unfair to consider this issue a regression caused by that change. 
     5~~If a page makes a web request before its `onCommitted` event has fired the request will be blocked / whitelisted based upon the old page structure. So if the URL for the previous page was of a different domain the request will be considered third party, or if the new page has a sitekey we won't know to take it into account. 
    66 
    7 Ultimately I think we need to rethink the page structure logic so that pre-rendered pages can be accounted for properly. While we're at it I think adding unit tests for how the page structure is maintained based upon navigation events would be a pretty good idea. I don't think this is the right time to do that however since in the near future we plan to remove some of the Page abstractions (e.g. #4580) and other ext code now that Safari support has been dropped. I think it would make more sense to wait until after that work has been done. 
     7~~Thing is the old behaviour was no better, arguably worse. The `onBeforeNavigate` event we used wasn't fired after redirections, so the page structure was not updated at all for those. Also it was fired for pre-rendered pages which meant that we effectively assumed that pre-rendered pages would always be navigated to. So we can't simply revert #4386 and I think it's a little unfair to consider this issue a regression caused by that change. 
    88 
    9 ''So what can we do for now?'' Well how about we update the page structure both on `onBeforeNavigate` and `onCommitted`. We could remove the workaround for #4483 while at it and look at maintaining the page structure properly after the ext code has been removed. This would ''mostly'' be correct and not require major changes. 
     9~~Ultimately I think we need to rethink the page structure logic so that pre-rendered pages can be accounted for properly. While we're at it I think adding unit tests for how the page structure is maintained based upon navigation events would be a pretty good idea. I don't think this is the right time to do that however since in the near future we plan to remove some of the Page abstractions (e.g. #4580) and other ext code now that Safari support has been dropped. I think it would make more sense to wait until after that work has been done. 
    1010 
    11 (Updating the page structure for pre-rendered pages is incorrect but perhaps that doesn't matter too much. One pre-rendered page will replace another until the user eventually has finished typing the URL. The latest guess should always be handled correctly. The only problem would be if the user stopped typing the URL and then continued to use the existing page without navigating at all. Note this was the case before #4483 landed so apparently not too much of a problem.) 
     11~~''So what can we do for now?'' Well how about we update the page structure both on `onBeforeNavigate` and `onCommitted`. We could remove the workaround for #4483 while at it and look at maintaining the page structure properly after the ext code has been removed. This would ''mostly'' be correct and not require major changes. 
     12 
     13~~(Updating the page structure for pre-rendered pages is incorrect but perhaps that doesn't matter too much. One pre-rendered page will replace another until the user eventually has finished typing the URL. The latest guess should always be handled correctly. The only problem would be if the user stopped typing the URL and then continued to use the existing page without navigating at all. Note this was the case before #4483 landed so apparently not too much of a problem.)