Issue 154 - Added devtools panel showing blocked and blockable items

[Sebastian Noack, 2014-12-28 16:47:11]

Note that the strings here aren't translated on purpose, since the devtools in
Chrome are English-only. Hence it would be pointless to translate our panel
integrated there.

[kzar, 2015-01-05 17:30:58]

Really cool feature, I gave it a try and I really like it!

Have you checked the new interface stuff with Sven? Some usability things I
noticed:

 - The "Block request" button is a little confusing as it's really creating a
filter to block future requests.
 - When you add a filter the "Reload page to see effect of filter changes"
notice is very easy to miss. I think it should be at the top instead of bottom,
centred, have a yellow background like an alert and bold/slightly larger text.
Maybe a little refresh icon there too?
 - I think for unblocked requests there should be green text saying "None" and
either more text afterwards that's a link to add a filter or a button that is
always displayed instead of only on mouse over. (To start with I did not notice
the mouse over buttons.)
 - The capitalisation of the options in the filter drop-down boxes is
inconsistent. For a sensible reason actually but it looked ugly to me. Perhaps
we should make it consistent somehow?

I guess full code review should wait until things like that are agreed upon and
implemented but that said I think you should review this too Wladimir as the
changes are non-trivial.

http://codereview.adblockplus.org/6393086494113792/diff/5665117697998848/chro...
File chrome/devtools/devtools-panel.js (right):

http://codereview.adblockplus.org/6393086494113792/diff/5665117697998848/chro...
chrome/devtools/devtools-panel.js:32: {
This function seems quite ugly, is there not a way we can create a row from a
template?

[Sebastian Noack, 2015-01-05 19:33:58]

On 2015/01/05 17:30:58, kzar wrote:
> Have you checked the new interface stuff with Sven?

He has been on vacation for the past weeks and still is. I will check back with
him once he is back. For now I stuck to the design used by the built-in devtool
panels. And frankly, at least as long as this is only for Chrome I'd prefer to
keep it like that.

> Some usability things I noticed:
> 
>  - The "Block request" button is a little confusing as it's really creating a
> filter to block future requests.

This is consistent with the "Block element" functionality. And "Add filter to
block request" / "Add filter to whitelist request" would be rather long. And the
footer saying "Reload page to see effect of filter changes" makes it obvious
that you have to reload, to see the effect of the request being
blocked/whitelisted.

>  - When you add a filter the "Reload page to see effect of filter changes"
> notice is very easy to miss. I think it should be at the top instead of
bottom,
> centred, have a yellow background like an alert and bold/slightly larger text.
> Maybe a little refresh icon there too?

By adding it at the top, we would move the filter options and table header down,
which IMO is a bad user experience. I don't think it's a big deal if users
sometimes don't realize the footer appearing. It's not an urgent notifications.
Frankly, I only added it that users don't claim its a bug that
blocking/whitelisting requests don't show immediate effect. 

>  - I think for unblocked requests there should be green text saying "None"

Are you talking about whitelisted request? If so, are you suggesting to replace
the URL or whitelisting filter with the text "None"? This would remove important
information.

In case you are talking about requests without any (neither blocking nor
whistelisting) filter matches, using green text would be confusing, since this
indicates whitelisting. Also I don't get how showing no value in the "Filter"
column isn't obvious enough to indicate that there is no matching filter.

> either more text afterwards that's a link to add a filter or a button that is
> always displayed instead of only on mouse over.

This would be inconsistent, and IMO makes it even less intuitive that there are
available actions for other items too.

> (To start with I did not notice the mouse over buttons.)

I decided to show the buttons only on hover, to save some space on smaller
resolutions (filters can get quite long).

>  - The capitalisation of the options in the filter drop-down boxes is
> inconsistent. For a sensible reason actually but it looked ugly to me. Perhaps
> we should make it consistent somehow?

How do you mean? The case-form is consistent among all available options in the
dropdown, and with the types shown in the list. Also this (uppercase) is how
types are used internally by Adblock Plus.

http://codereview.adblockplus.org/6393086494113792/diff/5665117697998848/chro...
File chrome/devtools/devtools-panel.js (right):

http://codereview.adblockplus.org/6393086494113792/diff/5665117697998848/chro...
chrome/devtools/devtools-panel.js:32: {
On 2015/01/05 17:30:58, kzar wrote:
> This function seems quite ugly, is there not a way we can create a row from a
> template?

We don't use a template system. And I rather don't introduce one, just for this.
Of course I could generate a an HTML string instead using DOM APIs here, but I
don't think that this would be any less ugly. But while seeing this code without
syntax highlighting for the first time, I think we should add some new lines.

[kzar, 2015-01-06 13:07:59]

On 2015/01/05 19:33:58, Sebastian Noack wrote:
> This is consistent with the "Block element" functionality. And "Add filter to
> block request" / "Add filter to whitelist request" would be rather long. And
the
> footer saying "Reload page to see effect of filter changes" makes it obvious
> that you have to reload, to see the effect of the request being
> blocked/whitelisted.

I disagree, with the "Block element" feature the element also disappears
instantly. From the user's point of view "Block element" makes sense. With
"Block request" however this does not work, the request has already been made
and there's no way to hide it until the page is reloaded. I personally found
this confusing so I'm guessing others will too.

> By adding it at the top, we would move the filter options and table header
down,
> which IMO is a bad user experience. I don't think it's a big deal if users
> sometimes don't realize the footer appearing. It's not an urgent
notifications.
> Frankly, I only added it that users don't claim its a bug that
> blocking/whitelisting requests don't show immediate effect. 

Again I disagree about this :( I found it confusing to start with as I had not
noticed the refresh notification. I think it's quite important for the user
experience that the notification about refreshing the page is easy to spot. (My
ideas there were just suggestions, I don't care how it's achieved as long as
it's clear to the user what's happening.)

> For now I stuck to the design used by the built-in devtool
> panels. And frankly, at least as long as this is only for Chrome I'd > prefer
to keep it like that.

I agree that we should use the built in devtool panels for now but that does not
mean we shouldn't consider the user experience carefully. Little things like I
mentioned above can confuse people (I was confused) and with some thought
hopefully that can be avoided.

Perhaps we should put the code review on hold for now until Sven . someone has
worked on the interface?

[Sebastian Noack, 2015-01-06 14:50:21]

On 2015/01/06 13:07:59, kzar wrote:
> On 2015/01/05 19:33:58, Sebastian Noack wrote:
> > This is consistent with the "Block element" functionality. And "Add filter
to
> > block request" / "Add filter to whitelist request" would be rather long. And
> the
> > footer saying "Reload page to see effect of filter changes" makes it obvious
> > that you have to reload, to see the effect of the request being
> > blocked/whitelisted.
> 
> I disagree, with the "Block element" feature the element also disappears
> instantly. From the user's point of view "Block element" makes sense. With
> "Block request" however this does not work, the request has already been made
> and there's no way to hide it until the page is reloaded. I personally found
> this confusing so I'm guessing others will too.
> 
> > By adding it at the top, we would move the filter options and table header
> down,
> > which IMO is a bad user experience. I don't think it's a big deal if users
> > sometimes don't realize the footer appearing. It's not an urgent
> notifications.
> > Frankly, I only added it that users don't claim its a bug that
> > blocking/whitelisting requests don't show immediate effect. 
> 
> Again I disagree about this :( I found it confusing to start with as I had not
> noticed the refresh notification. I think it's quite important for the user
> experience that the notification about refreshing the page is easy to spot.
(My
> ideas there were just suggestions, I don't care how it's achieved as long as
> it's clear to the user what's happening.)

I see your points. But I don't see a way to address them without worsen other UX
aspects. If you have any ideas, I'm happy to hear your suggestions.

> but that does not mean we shouldn't consider the user experience carefully.

I did consider UX carefully (together with mapx). It merely seems that we
focused on different aspects, i.e. to get most out of smaller resolutions,
easily finding particular kind of request/filter, combining the user experience
of the built-in panels with what we have on Firefox in a neat way.

> Little things like I mentioned above can confuse people (I was confused) and
> with some thought hopefully that can be avoided.

This is an advanced feature for advanced users (those who write filters or debug
Adblock Plus). So it's more important that it is usable for those, than that all
it's functionality is intuitive for people using it for the first time. If we
can improve the later without worsen the former, we should do so. But bloating
the UI with more and bigger elements, isn't an option.

> Perhaps we should put the code review on hold for now until Sven

Fine for me. But even if we change the UI, this will only affect the code in
devtools-panel.[html|css|js].

[Sebastian Noack, 2015-01-06 18:10:51]

As discussed with Wladimir, I dropped support for Chrome 28, in order to use
unprefixed flexbox.

I also added two comments to the CSS, where the code might be not obvious.

[Sebastian Noack, 2015-01-07 13:44:07]

Also as discussed with Wladimir I moved the UI out of the to the adblockplusui
repo. Therfore I added a very thin abstraction for chrome.devtools, which mostly
just forwards call. This should be enough for now.

[Sebastian Noack, 2015-02-04 19:41:33]

I rebased on top of the URL changes. Makes sense to merge them first. I also
adapted the code for the messaging changes discussed in the other review. Added
some documentation for public the functions of the "devtools" module. And while
on it, I made them work with page objects where possible.

[Sebastian Noack, 2015-03-12 15:45:14]

I've uploaded a new patch set. Besides rebasing, I also fixed a couple of bugs
mapx and myself found. That is most notably:

* Apparently devtools panels are only associated with tabs when detached from
the browser window. So much for using regular messaging. :( However, I think I
found a good compromise. While we don't come around using message ports those
are hidden deep into the abstraction layer now.
* Some requests didn't show up in the panel when following links. It turned out
in order to bypass all caches we have to reload the page with a special option.
* The devtools panel were cleared when a download were initiated. That was
because due to https://crbug.com/465710 navigation is reported when a download
is initiated. However, we had to move from onBeforeNavigate to onLoading anyway
to reload the page.

[Sebastian Noack, 2016-01-28 19:49:04]

While rabasing the patch I also didn't to tidy up some thing. The most notable
changes are:

* Enabled strict mode for new files
* Removed lib/devtools.js from the Safari builds
* Turned element hiding tracing code in content script into a class

[kzar, 2016-01-31 13:33:54]

Mind sending me a patch / a link to these changes in GitHub or BitBucket? I
think playing with the changes would help me to review them. (I never have much
luck getting patches from Rietveld to apply.)

https://codereview.adblockplus.org/6393086494113792/diff/29335169/background.js
File background.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/background....
background.js:399: devtools.logHiddenElements(sender.page, msg.selectors,
extractHostFromFrame(sender.frame));
Nit: Mind fixing this long line?

https://codereview.adblockplus.org/6393086494113792/diff/29335169/chrome/devt...
File chrome/devtools.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/chrome/devt...
chrome/devtools.js:20: chrome.devtools.panels.create("Adblock Plus",
"icons/detailed/abp-48.png", "devtools-panel.html");
Nit: Another long line.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/chrome/ext/...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/chrome/ext/...
chrome/ext/background.js:627: var panel = new Panel(parseInt(match[1], 10),
port);
Nit: Maybe should use let instead of var for these variables?

https://codereview.adblockplus.org/6393086494113792/diff/29335169/include.pre...
File include.preload.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/include.pre...
include.preload.js:215: for (var i = 0; i < this.selectors.length; i++)
Nit: Could you use `for ... of ...` for these loops instead?

https://codereview.adblockplus.org/6393086494113792/diff/29335169/include.pre...
include.preload.js:243: ext.backgroundPage.sendMessage({type: "trace-elemhide",
selectors: matchedSelectors});
Nit: Mind wrapping this long line?

https://codereview.adblockplus.org/6393086494113792/diff/29335169/lib/devtool...
File lib/devtools.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/lib/devtool...
lib/devtools.js:126: exports.logRequest = function(page, url, type, docDomain,
thirdParty, sitekey, specificOnly, filter)
Nit: Mind wrapping this?

https://codereview.adblockplus.org/6393086494113792/diff/29335169/lib/devtool...
lib/devtools.js:314: ext.devtools.onCreated.addListener(function(panel)
I guess I'm undecided about the ext.devtools abstraction. We're only using it
for Chrome anyway, so couldn't we just listen for `chrome.runtime.onConnect`
here instead?

https://codereview.adblockplus.org/6393086494113792/diff/29335169/popupBlocke...
File popupBlocker.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/popupBlocke...
popupBlocker.js:43: checkPotentialPopup(details.tabId, details.url, sourcePage,
documentHost, specificOnly);
Nit: Mind wrapping this and the long line below?

https://codereview.adblockplus.org/6393086494113792/diff/29335169/popupBlocke...
popupBlocker.js:69: var urlObj = new URL(url || "about:blank");
Shouldn't we put the `url ||  "about:blank"` workaround in the URL polyfill?
It's something that keeps coming up.

[Sebastian Noack, 2016-02-02 10:39:56]

https://codereview.adblockplus.org/6393086494113792/diff/29335169/background.js
File background.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/background....
background.js:399: devtools.logHiddenElements(sender.page, msg.selectors,
extractHostFromFrame(sender.frame));
On 2016/01/31 13:33:50, kzar wrote:
> Nit: Mind fixing this long line?

Done.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/chrome/devt...
File chrome/devtools.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/chrome/devt...
chrome/devtools.js:20: chrome.devtools.panels.create("Adblock Plus",
"icons/detailed/abp-48.png", "devtools-panel.html");
On 2016/01/31 13:33:50, kzar wrote:
> Nit: Another long line.

Done.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/chrome/ext/...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/chrome/ext/...
chrome/ext/background.js:627: var panel = new Panel(parseInt(match[1], 10),
port);
On 2016/01/31 13:33:51, kzar wrote:
> Nit: Maybe should use let instead of var for these variables?

I'd rather keep things conistent. (Except for one inconsistency introduced by
you) we don't use block-scoping in this file or generally in ext.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/include.pre...
File include.preload.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/include.pre...
include.preload.js:215: for (var i = 0; i < this.selectors.length; i++)
On 2016/01/31 13:33:51, kzar wrote:
> Nit: Could you use `for ... of ...` for these loops instead?

This is a content script. So there is no jsHydra magic.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/include.pre...
include.preload.js:243: ext.backgroundPage.sendMessage({type: "trace-elemhide",
selectors: matchedSelectors});
On 2016/01/31 13:33:51, kzar wrote:
> Nit: Mind wrapping this long line?

Done.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/lib/devtool...
File lib/devtools.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/lib/devtool...
lib/devtools.js:126: exports.logRequest = function(page, url, type, docDomain,
thirdParty, sitekey, specificOnly, filter)
On 2016/01/31 13:33:52, kzar wrote:
> Nit: Mind wrapping this?

Done.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/lib/devtool...
lib/devtools.js:314: ext.devtools.onCreated.addListener(function(panel)
On 2016/01/31 13:33:52, kzar wrote:
> I guess I'm undecided about the ext.devtools abstraction. We're only using it
> for Chrome anyway, so couldn't we just listen for `chrome.runtime.onConnect`
> here instead?

Yes, I would much prefer that as well. But note that we need some kind of
abstraction for adblockplusui. Feel free to look at the code there. Suggestions
are welcome.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/popupBlocke...
File popupBlocker.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/popupBlocke...
popupBlocker.js:69: var urlObj = new URL(url || "about:blank");
On 2016/01/31 13:33:52, kzar wrote:
> Shouldn't we put the `url ||  "about:blank"` workaround in the URL polyfill?
> It's something that keeps coming up.

Note that Chrome's native URL object treating '' as 'about:blank' is
non-standard, and inconsistent with Safari (and Firefox). Also even on Chrome
new URL(null) still fails.

https://codereview.adblockplus.org/6393086494113792/diff/29335228/lib/whiteli...
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335228/lib/whiteli...
lib/whitelisting.js:83: // Compatibility with adblockplusui, to be removed when
updating the dependency
I am going to update adblockplusui and remove that code before landing this
patch.

[Sebastian Noack, 2016-02-02 11:22:12]

https://codereview.adblockplus.org/6393086494113792/diff/29335169/lib/devtool...
File lib/devtools.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/lib/devtool...
lib/devtools.js:314: ext.devtools.onCreated.addListener(function(panel)
On 2016/02/02 10:39:53, Sebastian Noack wrote:
> On 2016/01/31 13:33:52, kzar wrote:
> > I guess I'm undecided about the ext.devtools abstraction. We're only using
it
> > for Chrome anyway, so couldn't we just listen for `chrome.runtime.onConnect`
> > here instead?
> 
> Yes, I would much prefer that as well. But note that we need some kind of
> abstraction for adblockplusui. Feel free to look at the code there.
Suggestions
> are welcome.

OK, I looked into it. And indeed, as we are in the background page, we don't
need to bother about the mock implementation in adblockplusui. And using
chrome.runtime.onConnect directly, seems to be more appropriate here. Done.

[kzar, 2016-02-02 15:42:13]

Thanks for the patch, just tried it out and it works great now. The UI seems
good too.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/include.pre...
File include.preload.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/include.pre...
include.preload.js:215: for (var i = 0; i < this.selectors.length; i++)
On 2016/02/02 10:39:52, Sebastian Noack wrote:
> On 2016/01/31 13:33:51, kzar wrote:
> > Nit: Could you use `for ... of ...` for these loops instead?
> 
> This is a content script. So there is no jsHydra magic.

Acknowledged.

https://codereview.adblockplus.org/6393086494113792/diff/29335169/popupBlocke...
File popupBlocker.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/popupBlocke...
popupBlocker.js:69: var urlObj = new URL(url || "about:blank");
On 2016/02/02 10:39:53, Sebastian Noack wrote:
> On 2016/01/31 13:33:52, kzar wrote:
> > Shouldn't we put the `url ||  "about:blank"` workaround in the URL polyfill?
> > It's something that keeps coming up.
> 
> Note that Chrome's native URL object treating '' as 'about:blank' is
> non-standard, and inconsistent with Safari (and Firefox). Also even on Chrome
> new URL(null) still fails.

IMO the polyfill should take care of those inconsistencies for us, but don't
want to delay this landing so won't argue it.

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
File lib/devtools.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
lib/devtools.js:132: * @param {string]   url           The URL of the request
Nit: Typo here `{string]`

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
lib/devtools.js:136: * @param {?string}  sitekey       The active sitekey if
there is any
Nit: Isn't the syntax for optional parameters `@param {type} [name] ...`?

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
lib/devtools.js:205: {
Nit: We don't need the braces here and for the for loop below. (I don't mind too
much, up to you.)

[Sebastian Noack, 2016-02-02 16:33:48]

https://codereview.adblockplus.org/6393086494113792/diff/29335169/popupBlocke...
File popupBlocker.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335169/popupBlocke...
popupBlocker.js:69: var urlObj = new URL(url || "about:blank");
On 2016/02/02 15:42:09, kzar wrote:
> On 2016/02/02 10:39:53, Sebastian Noack wrote:
> > On 2016/01/31 13:33:52, kzar wrote:
> > > Shouldn't we put the `url ||  "about:blank"` workaround in the URL
polyfill?
> > > It's something that keeps coming up.
> > 
> > Note that Chrome's native URL object treating '' as 'about:blank' is
> > non-standard, and inconsistent with Safari (and Firefox). Also even on
Chrome
> > new URL(null) still fails.
> 
> IMO the polyfill should take care of those inconsistencies for us, but don't
> want to delay this landing so won't argue it.

A polyfill is a temporary solution, until browsers adapt. If anything, the
polyfill should wrap the native URL object on Chrome, to throw if an empty
string is passed, since this seems to be the standard behavior. However, that
wouldn't remove any complexity from the code here, but just adds some overhead
when URL objects are natively supported.

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
File lib/devtools.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
lib/devtools.js:132: * @param {string]   url           The URL of the request
On 2016/02/02 15:42:11, kzar wrote:
> Nit: Typo here `{string]`

Done.

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
lib/devtools.js:136: * @param {?string}  sitekey       The active sitekey if
there is any
On 2016/02/02 15:42:11, kzar wrote:
> Nit: Isn't the syntax for optional parameters `@param {type} [name] ...`?

{?string} means "string or null", which seems to be more accurrate, as we always
call logRequest() with all arguments, but we might get null here (as well as for
the other two arguments below).

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
lib/devtools.js:205: {
On 2016/02/02 15:42:10, kzar wrote:
> Nit: We don't need the braces here and for the for loop below. (I don't mind
too
> much, up to you.)

Well, according to the Wladimir-ish notation for using braces, braces are
required for readability when the content of a block requires more than one
line. I don't always agree to that though.

[kzar, 2016-02-02 16:40:33]

LGTM

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
File lib/devtools.js (right):

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
lib/devtools.js:136: * @param {?string}  sitekey       The active sitekey if
there is any
On 2016/02/02 16:33:46, Sebastian Noack wrote:
> On 2016/02/02 15:42:11, kzar wrote:
> > Nit: Isn't the syntax for optional parameters `@param {type} [name] ...`?
> 
> {?string} means "string or null", which seems to be more accurrate, as we
always
> call logRequest() with all arguments, but we might get null here (as well as
for
> the other two arguments below).

Acknowledged.

https://codereview.adblockplus.org/6393086494113792/diff/29335298/lib/devtool...
lib/devtools.js:205: {
On 2016/02/02 16:33:47, Sebastian Noack wrote:
> On 2016/02/02 15:42:10, kzar wrote:
> > Nit: We don't need the braces here and for the for loop below. (I don't mind
> too
> > much, up to you.)
> 
> Well, according to the Wladimir-ish notation for using braces, braces are
> required for readability when the content of a block requires more than one
> line. I don't always agree to that though.

Acknowledged.

[kzar, 2016-02-03 10:52:11]

LGTM