Issue 2426 - Open block.html as a popup window

Issue 2426 - Open block.html as a popup window

[kzar, 2016-02-08 12:42:05]

Patch Set 1 : Renamed include.postload.js to blockElement.postload.js
Patch Set 2 : Open block.html as a popup window

(I started a new code review because there was a large rebase since last time,
also I wanted to provide to split the rename of blockElement.postload.js from
all my other changes in order to provide a useful diff.)

I remember you weren't keen on my message forwarding approach, but after
spending quite a while on the code I can't see how we can get rid of all message
forwarding. When blocking elements via the context menu the current frame might
not be the top-level one and so I think we need communication between the
frames. I'm open to suggestions though.

Also as I discussed with you in IRC I have noticed an edge case in Chrome where
if the context menu was activated in a child frame the element picking doesn't
work so well until the user clicks somewhere on the page. For posterity here's
the bug report https://code.google.com/p/chromium/issues/detail?id=584981

[Sebastian Noack, 2016-02-10 12:26:57]

https://codereview.adblockplus.org/29336084/diff/29336088/background.js
File background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:287: var url = ext.getURL("block.html") + "?targetPageId=" +
sender.page._id +
Alternatively, you could send this data to the popup from the callback below,
similar to how we do it with the options page when opened to add a specific
subscription. I'm not entirely sure what's better, but at least you wouldn't
need to manually serialize/deserialize the data when using messaging.

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:318: return true;
By returning true but never calling sendResponse(), the underlying port will be
kept open forever introducing memory leaks.

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:441: targetPage = ext._getPage(msg.targetPageId);
I guess it's time, to remove the prefix from ext._getPage() and Page._id.
Oterwise this code looks a tad hacky, which it in fact is if we still consider
these APIs private.

https://codereview.adblockplus.org/29336084/diff/29336088/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/chrome/ext/backgrou...
chrome/ext/background.js:142: ext.pages.onRemoved._dispatch(tabId);
I guess we should dispatch the event with a Page object for consistence.

https://codereview.adblockplus.org/29336084/diff/29336088/chrome/ext/backgrou...
chrome/ext/background.js:595: create: (createData, callback) =>
Nit: I'm not sure if using an arrow function is appropriate here, if it's just
for consistence with the rest of this file.

https://codereview.adblockplus.org/29336084/diff/29336088/chrome/ext/backgrou...
chrome/ext/background.js:597: if (callback)
The logic here is duplicated from ext.tabs.open. We should probably move it into
a common helper function.

https://codereview.adblockplus.org/29336084/diff/29336088/report-html-page.po...
File report-html-page.postload.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/report-html-page.po...
report-html-page.postload.js:16: */
The logic in here belongs to the "Block element" feature. The idea is to detect
whether the Block Element feature is available on the current page in order to
hide/show the entry point in the context menu and icon popup.

https://codereview.adblockplus.org/29336084/diff/29336088/safari/ext/backgrou...
File safari/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/safari/ext/backgrou...
safari/ext/background.js:767: if (createData && createData.url)
Just assume the URL to be given? If it's missing, not doing anything, isn't the
expected result and inconsistent with Chrome.

[kzar, 2016-02-10 14:20:17]

Patch Set 3 : Addressed some initial feedback

https://codereview.adblockplus.org/29336084/diff/29336088/background.js
File background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:287: var url = ext.getURL("block.html") + "?targetPageId=" +
sender.page._id +
On 2016/02/10 12:26:56, Sebastian Noack wrote:
> Alternatively, you could send this data to the popup from the callback below,
> similar to how we do it with the options page when opened to add a specific
> subscription. I'm not entirely sure what's better, but at least you wouldn't
> need to manually serialize/deserialize the data when using messaging.

I think this would work for Chrome but not Safari. In Safari the callback could
be performed before the new page has finished loading, so the message might be
sent too early. (I agree that sending a message would be cleaner.)

The other option I tried was to have the popup send a message to request the
filters, but that led to even more mess than just passing the filters through in
the URL. Worse I still had to pass the page ID in the URL anyway, so it just
meant we did things in both ways.

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:318: return true;
On 2016/02/10 12:26:56, Sebastian Noack wrote:
> By returning true but never calling sendResponse(), the underlying port will
be
> kept open forever introducing memory leaks.

Done.

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:441: targetPage = ext._getPage(msg.targetPageId);
On 2016/02/10 12:26:56, Sebastian Noack wrote:
> I guess it's time, to remove the prefix from ext._getPage() and Page._id.
> Oterwise this code looks a tad hacky, which it in fact is if we still consider
> these APIs private.

Done.

https://codereview.adblockplus.org/29336084/diff/29336088/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/chrome/ext/backgrou...
chrome/ext/background.js:142: ext.pages.onRemoved._dispatch(tabId);
On 2016/02/10 12:26:56, Sebastian Noack wrote:
> I guess we should dispatch the event with a Page object for consistence.

I wasn't sure how well it would work using a Page object for a tab that had been
removed. (I nearly did return a Page object here initially.)

https://codereview.adblockplus.org/29336084/diff/29336088/chrome/ext/backgrou...
chrome/ext/background.js:595: create: (createData, callback) =>
On 2016/02/10 12:26:56, Sebastian Noack wrote:
> Nit: I'm not sure if using an arrow function is appropriate here, if it's just
> for consistence with the rest of this file.

Done.

https://codereview.adblockplus.org/29336084/diff/29336088/chrome/ext/backgrou...
chrome/ext/background.js:597: if (callback)
On 2016/02/10 12:26:57, Sebastian Noack wrote:
> The logic here is duplicated from ext.tabs.open. We should probably move it
into
> a common helper function.

Done.

https://codereview.adblockplus.org/29336084/diff/29336088/report-html-page.po...
File report-html-page.postload.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/report-html-page.po...
report-html-page.postload.js:16: */
On 2016/02/10 12:26:57, Sebastian Noack wrote:
> The logic in here belongs to the "Block element" feature. The idea is to
detect
> whether the Block Element feature is available on the current page in order to
> hide/show the entry point in the context menu and icon popup.

Done.

https://codereview.adblockplus.org/29336084/diff/29336088/safari/ext/backgrou...
File safari/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/safari/ext/backgrou...
safari/ext/background.js:767: if (createData && createData.url)
On 2016/02/10 12:26:57, Sebastian Noack wrote:
> Just assume the URL to be given? If it's missing, not doing anything, isn't
the
> expected result and inconsistent with Chrome.

Done.

[Sebastian Noack, 2016-02-10 14:43:24]

https://codereview.adblockplus.org/29336084/diff/29336088/background.js
File background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:287: var url = ext.getURL("block.html") + "?targetPageId=" +
sender.page._id +
On 2016/02/10 14:20:16, kzar wrote:
> On 2016/02/10 12:26:56, Sebastian Noack wrote:
> > Alternatively, you could send this data to the popup from the callback
below,
> > similar to how we do it with the options page when opened to add a specific
> > subscription. I'm not entirely sure what's better, but at least you wouldn't
> > need to manually serialize/deserialize the data when using messaging.
> 
> I think this would work for Chrome but not Safari. In Safari the callback
could
> be performed before the new page has finished loading, so the message might be
> sent too early. (I agree that sending a message would be cleaner.)

As I said, we already do so for the options page. So apparently it works on
Safari.

https://codereview.adblockplus.org/29336084/diff/29336218/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336218/chrome/ext/backgrou...
chrome/ext/background.js:605: return createdWindow.tabs[0];
Why don't you simply pass the tab in instead of a callback?

[kzar, 2016-02-11 15:20:21]

Patch Set 4 : Pass tab directly instead of getTab callback

https://codereview.adblockplus.org/29336084/diff/29336218/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336218/chrome/ext/backgrou...
chrome/ext/background.js:605: return createdWindow.tabs[0];
On 2016/02/10 14:43:24, Sebastian Noack wrote:
> Why don't you simply pass the tab in instead of a callback?

Done.

[Sebastian Noack, 2016-02-12 17:00:01]

https://codereview.adblockplus.org/29336084/diff/29336088/background.js
File background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:287: var url = ext.getURL("block.html") + "?targetPageId=" +
sender.page._id +
On 2016/02/10 14:43:24, Sebastian Noack wrote:
> On 2016/02/10 14:20:16, kzar wrote:
> > On 2016/02/10 12:26:56, Sebastian Noack wrote:
> > > Alternatively, you could send this data to the popup from the callback
> below,
> > > similar to how we do it with the options page when opened to add a
specific
> > > subscription. I'm not entirely sure what's better, but at least you
wouldn't
> > > need to manually serialize/deserialize the data when using messaging.
> > 
> > I think this would work for Chrome but not Safari. In Safari the callback
> could
> > be performed before the new page has finished loading, so the message might
be
> > sent too early. (I agree that sending a message would be cleaner.)
> 
> As I said, we already do so for the options page. So apparently it works on
> Safari.

So what's about that comment?

https://codereview.adblockplus.org/29336084/diff/29336260/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336260/chrome/ext/backgrou...
chrome/ext/background.js:82: chrome.tabs.create({url: url},
afterTabLoaded(callback));
I'd rather check here for the callback:

  callback && afterTabLoaded(callback)

That way we avoid one more block in afterTabLoaded() and in the function below
we assume the callback to be given anyway.

[kzar, 2016-02-13 14:18:13]

Patch Set 5 : Check callback exists outside of afterTabLoaded

https://codereview.adblockplus.org/29336084/diff/29336088/background.js
File background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:287: var url = ext.getURL("block.html") + "?targetPageId=" +
sender.page._id +
On 2016/02/12 17:00:00, Sebastian Noack wrote:
> On 2016/02/10 14:43:24, Sebastian Noack wrote:
> > On 2016/02/10 14:20:16, kzar wrote:
> > > On 2016/02/10 12:26:56, Sebastian Noack wrote:
> > > > Alternatively, you could send this data to the popup from the callback
> > below,
> > > > similar to how we do it with the options page when opened to add a
> specific
> > > > subscription. I'm not entirely sure what's better, but at least you
> wouldn't
> > > > need to manually serialize/deserialize the data when using messaging.
> > > 
> > > I think this would work for Chrome but not Safari. In Safari the callback
> > could
> > > be performed before the new page has finished loading, so the message
might
> be
> > > sent too early. (I agree that sending a message would be cleaner.)
> > 
> > As I said, we already do so for the options page. So apparently it works on
> > Safari.
> 
> So what's about that comment?

Well I tried it and it didn't work for me.

Looking at the code to open the options page it looks like we do something else,
like ensuring the tab is active, before performing the callback that sends the
message. Maybe that helped prevent the race condition from troubling us?

I could wait 100ms before performing the callback if you like, seems like a hack
though.

https://codereview.adblockplus.org/29336084/diff/29336260/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336260/chrome/ext/backgrou...
chrome/ext/background.js:82: chrome.tabs.create({url: url},
afterTabLoaded(callback));
On 2016/02/12 17:00:00, Sebastian Noack wrote:
> I'd rather check here for the callback:
> 
>   callback && afterTabLoaded(callback)
> 
> That way we avoid one more block in afterTabLoaded() and in the function below
> we assume the callback to be given anyway.

Done.

[Sebastian Noack, 2016-02-15 14:50:17]

https://codereview.adblockplus.org/29336084/diff/29336088/background.js
File background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336088/background.js#newco...
background.js:287: var url = ext.getURL("block.html") + "?targetPageId=" +
sender.page._id +
On 2016/02/13 14:18:12, kzar wrote:
> On 2016/02/12 17:00:00, Sebastian Noack wrote:
> > On 2016/02/10 14:43:24, Sebastian Noack wrote:
> > > On 2016/02/10 14:20:16, kzar wrote:
> > > > On 2016/02/10 12:26:56, Sebastian Noack wrote:
> > > > > Alternatively, you could send this data to the popup from the callback
> > > below,
> > > > > similar to how we do it with the options page when opened to add a
> > specific
> > > > > subscription. I'm not entirely sure what's better, but at least you
> > wouldn't
> > > > > need to manually serialize/deserialize the data when using messaging.
> > > > 
> > > > I think this would work for Chrome but not Safari. In Safari the
callback
> > > could
> > > > be performed before the new page has finished loading, so the message
> might
> > be
> > > > sent too early. (I agree that sending a message would be cleaner.)
> > > 
> > > As I said, we already do so for the options page. So apparently it works
on
> > > Safari.
> > 
> > So what's about that comment?
> 
> Well I tried it and it didn't work for me.
> 
> Looking at the code to open the options page it looks like we do something
else,
> like ensuring the tab is active, before performing the callback that sends the
> message. Maybe that helped prevent the race condition from troubling us?
> 
> I could wait 100ms before performing the callback if you like, seems like a
hack
> though.

Well, when opening the options page, we look for an open option page an activate
it if there is one. But otherwise we simply delegate to ext.pages.open().

So if it doesn't work here, it only works coincidentally for the option page, if
it does. And whatever solution we come up with, we should consistently apply,
for the options page too.

I mildly lend towards just waiting until the page is complete on Safari as well,
before calling the callback passed to ext.pages.open.

[Sebastian Noack, 2016-02-15 14:53:45]

https://codereview.adblockplus.org/29336084/diff/29336334/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336334/chrome/ext/backgrou...
chrome/ext/background.js:80: chrome.tabs.create({url: url},
afterTabLoaded(callback));
Why didn't you go for |callback && afterTabLoaded(callback)| as I initially
suggested? Then if the callback isn't given, we pass in undefined which is the
same as omitting the argument, but we don't need to switch (and partly
duplicate) the logic here.

https://codereview.adblockplus.org/29336084/diff/29336334/chrome/ext/backgrou...
chrome/ext/background.js:600: if (callback)
It seems, creating a window without callback is a case that we don't have. So no
reason to add that additional boilerplate.

[kzar, 2016-02-15 15:46:07]

Patch Set 6 : Reduce callback checking boilerplate

https://codereview.adblockplus.org/29336084/diff/29336334/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336334/chrome/ext/backgrou...
chrome/ext/background.js:80: chrome.tabs.create({url: url},
afterTabLoaded(callback));
On 2016/02/15 14:53:44, Sebastian Noack wrote:
> Why didn't you go for |callback && afterTabLoaded(callback)| as I initially
> suggested? Then if the callback isn't given, we pass in undefined which is the
> same as omitting the argument, but we don't need to switch (and partly
> duplicate) the logic here.

Done.

https://codereview.adblockplus.org/29336084/diff/29336334/chrome/ext/backgrou...
chrome/ext/background.js:600: if (callback)
On 2016/02/15 14:53:44, Sebastian Noack wrote:
> It seems, creating a window without callback is a case that we don't have. So
no
> reason to add that additional boilerplate.

Done.

[kzar, 2016-02-17 20:07:50]

Patch Set 7 : Rebased, replaced search param with proper messaging

[Sebastian Noack, 2016-02-17 20:44:06]

https://codereview.adblockplus.org/29336084/diff/29336546/safari/ext/backgrou...
File safari/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336546/safari/ext/backgrou...
safari/ext/background.js:768: if (createData)
I'd simply assume that createData is given and has an URL. Note that the
behavior when createData is missing is inconsistent with Chrome anyway. On
Chrome this will open an empty window. But no reason, to cover a case that we
don't have.

Otherwise, LGTM. Feel free to push, after removing the check, here.

[kzar, 2016-02-17 20:52:22]

Patch Set 8 : Assume createData parameter has been given

https://codereview.adblockplus.org/29336084/diff/29336546/safari/ext/backgrou...
File safari/ext/background.js (right):

https://codereview.adblockplus.org/29336084/diff/29336546/safari/ext/backgrou...
safari/ext/background.js:768: if (createData)
On 2016/02/17 20:44:06, Sebastian Noack wrote:
> I'd simply assume that createData is given and has an URL. Note that the
> behavior when createData is missing is inconsistent with Chrome anyway. On
> Chrome this will open an empty window. But no reason, to cover a case that we
> don't have.
> 
> Otherwise, LGTM. Feel free to push, after removing the check, here.

Done.