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

Issue 5666188336037888: Issue 581 - Fixed element hiding/collapsing in anonymous frames on Chrome (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 7 months ago by Sebastian Noack
Modified:
5 years, 7 months ago
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

Issue 581 - Fixed element hiding/collapsing in anonymous frames on Chrome

Patch Set 1 #

Total comments: 8

Patch Set 2 : Addressed comments #

Patch Set 3 : Fixed accidental global variable #

Patch Set 4 : Handle Chrome 37+ where this issue has been addressed #

Total comments: 8

Patch Set 5 : Rely on document's URL instead frame's src attribute #

Patch Set 6 : Fall back to parent frame's URL when processing requests from anonymous frames #

Patch Set 7 : Rely on frame's src attribute again #

Total comments: 4

Patch Set 8 : Also handle <object type="text/html"> #

Total comments: 4

Patch Set 9 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+165 lines, -85 lines) Patch
M background.js View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M include.preload.js View 1 2 3 4 5 6 7 8 4 chunks +146 lines, -82 lines 0 comments Download
M lib/basedomain.js View 1 2 3 4 5 1 chunk +16 lines, -0 lines 0 comments Download
M webrequest.js View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 14
Sebastian Noack
5 years, 7 months ago (2014-05-30 11:32:32 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/include.preload.js#newcode55 include.preload.js:55: function checkCollapse(element) Please move this function and the typeMap ...
5 years, 7 months ago (2014-05-30 11:45:17 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/include.preload.js#newcode55 include.preload.js:55: function checkCollapse(element) On 2014/05/30 11:45:17, Wladimir Palant wrote: > ...
5 years, 7 months ago (2014-05-30 12:31:11 UTC) #3
Sebastian Noack
5 years, 7 months ago (2014-06-02 10:23:31 UTC) #4
Wladimir Palant
http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/include.preload.js File include.preload.js (left): http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/include.preload.js#oldcode164 include.preload.js:164: // In Chrome 18 the document might not be ...
5 years, 7 months ago (2014-06-02 11:20:40 UTC) #5
Sebastian Noack
http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/include.preload.js#newcode66 include.preload.js:66: } On 2014/06/02 11:20:40, Wladimir Palant wrote: > Why ...
5 years, 7 months ago (2014-06-03 06:55:27 UTC) #6
Sebastian Noack
http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/include.preload.js#newcode194 include.preload.js:194: if (fixAnonymousFrames && /^(javascript:|$)/.test(event.target.src)) It seems that checking for ...
5 years, 7 months ago (2014-06-03 07:40:35 UTC) #7
Sebastian Noack
5 years, 7 months ago (2014-06-03 08:34:25 UTC) #8
Wladimir Palant
http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/include.preload.js#newcode197 include.preload.js:197: /^\s*(javascript:|about:|$)/i.test(element.getAttribute("src")))) What about data: URLs? Note that checking whether ...
5 years, 7 months ago (2014-06-04 14:12:48 UTC) #9
Sebastian Noack
http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/include.preload.js#newcode197 include.preload.js:197: /^\s*(javascript:|about:|$)/i.test(element.getAttribute("src")))) On 2014/06/04 14:12:49, Wladimir Palant wrote: > What ...
5 years, 7 months ago (2014-06-04 16:12:30 UTC) #10
Sebastian Noack
5 years, 7 months ago (2014-06-04 16:12:34 UTC) #11
Wladimir Palant
http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/include.preload.js#newcode71 include.preload.js:71: return value == null || /^\s*(javascript:|about:|$)/i.test(value); I'm surprised you ...
5 years, 7 months ago (2014-06-17 13:00:24 UTC) #12
Sebastian Noack
http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/include.preload.js#newcode71 include.preload.js:71: return value == null || /^\s*(javascript:|about:|$)/i.test(value); On 2014/06/17 13:00:24, ...
5 years, 7 months ago (2014-06-17 14:13:47 UTC) #13
Wladimir Palant
5 years, 7 months ago (2014-06-17 15:40:53 UTC) #14
LGTM
Sign in to reply to this message.

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