Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(80)

Issue 29362203: Issue 4599 - Update the page structure more eagerly (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 9 months ago by kzar
Modified:
2 years, 9 months ago
Visibility:
Public.

Description

Issue 4599 - Update the page structure more eagerly

Patch Set 1 #

Patch Set 2 : Improve explanation in comment #

Total comments: 1

Patch Set 3 : Only eagerly update structure for new tabs #

Patch Set 4 : Improved comment #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -7 lines) Patch
M chrome/ext/background.js View 1 2 3 2 chunks +16 lines, -7 lines 2 comments Download

Messages

Total messages: 9
kzar
Patch Set 1
2 years, 9 months ago (2016-11-10 15:18:57 UTC) #1
Sebastian Noack
Frankly, I don't completely understand why we have that inconsistent state here in the first ...
2 years, 9 months ago (2016-11-10 18:12:05 UTC) #2
kzar
Patch Set 2 : Improve explanation in comment On 2016/11/10 18:12:05, Sebastian Noack wrote: > ...
2 years, 9 months ago (2016-11-11 09:17:13 UTC) #3
kzar
On 2016/11/11 09:17:13, kzar wrote: > I don't completely understand why the sitekey workaround is ...
2 years, 9 months ago (2016-11-11 10:04:56 UTC) #4
Wladimir Palant
NOT LGTM https://codereview.adblockplus.org/29362203/diff/29362445/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29362203/diff/29362445/chrome/ext/background.js#newcode176 chrome/ext/background.js:176: details.url, false, details.parentFrameId); You are essentially reverting ...
2 years, 9 months ago (2016-11-14 08:18:44 UTC) #5
kzar
Patch Set 3 : Only eagerly update structure for new tabs Well how about this? ...
2 years, 9 months ago (2016-11-14 14:55:32 UTC) #6
kzar
Patch Set 4 : Improved comment
2 years, 9 months ago (2016-11-14 15:12:44 UTC) #7
Wladimir Palant
https://codereview.adblockplus.org/29362203/diff/29362520/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29362203/diff/29362520/chrome/ext/background.js#newcode176 chrome/ext/background.js:176: if (!(details.tabId in framesOfTabs)) While this is probably not ...
2 years, 9 months ago (2016-11-14 15:56:29 UTC) #8
kzar
2 years, 9 months ago (2016-11-15 12:19:29 UTC) #9
https://codereview.adblockplus.org/29362203/diff/29362520/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29362203/diff/29362520/chrome/ext/backgrou...
chrome/ext/background.js:176: if (!(details.tabId in framesOfTabs))
On 2016/11/14 15:56:29, Wladimir Palant wrote:
> While this is probably not wrong, I think that you are only solving a 
> small part of the issue with this approach...

Well so far the issue only described a problem with pre-rendered tabs, but I
have just tested and confirmed you're indeed right that the problem is not
limited to them. When clicking a link to http://www.apptap.com I saw requests
being made just before its onComitted event fired. Back to the drawing board...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5