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

Issue 29696583: Issue 6385 - Don't attempt to inject styles for frames we can't find (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by kzar
Modified:
3 months ago
Reviewers:
Manish Jethani
CC:
Sebastian Noack
Visibility:
Public.

Description

Issue 6385 - Don't attempt to inject styles for frames we can't find

Patch Set 1 #

Total comments: 5

Patch Set 2 : Return `false` explicitly instead of `undefined` implicitly #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -0 lines) Patch
M lib/cssInjection.js View 1 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 5
kzar
Patch Set 1
3 months ago (2018-02-13 15:00:38 UTC) #1
Manish Jethani
https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js#newcode110 lib/cssInjection.js:110: return; OK, I guess this is the right thing ...
3 months ago (2018-02-14 13:41:17 UTC) #2
Manish Jethani
https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js#newcode110 lib/cssInjection.js:110: return; On 2018/02/14 13:41:16, Manish Jethani wrote: > OK, ...
3 months ago (2018-02-14 14:03:41 UTC) #3
kzar
Patch Set 2 : Return `false` explicitly instead of `undefined` implicitly https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js File lib/cssInjection.js (right): ...
3 months ago (2018-02-14 14:52:58 UTC) #4
Manish Jethani
3 months ago (2018-02-15 14:06:22 UTC) #5
https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js
File lib/cssInjection.js (right):

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js...
lib/cssInjection.js:110: return;
On 2018/02/14 14:52:58, kzar wrote:
> On 2018/02/14 14:03:41, Manish Jethani wrote:
> > On 2018/02/14 13:41:16, Manish Jethani wrote:
> > > OK, I guess this is the right thing to do. But we should return false
> > > explicitly.
> > 
> > If this is reproduceable, we should check why it is happening. Clearly the
> > content script is still around when it gets a response, then we should find
> out
> > why ext.getFrame doesn't find the frame.
> 
> This is reproducible, the issue description is up to date with steps of how to
> reproduce and also my findings (tl;dr `framesOfTabs` really doesn't have the
> frame in question - I'm not sure why).

See my comment here:

https://issues.adblockplus.org/ticket/6385#comment:3

So I think this is OK as a short-term fix, but I think this is going to happen
very often on Firefox so we should try to do something better.

LGTM
Sign in to reply to this message.

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