Issue 5093 - Indicate whether request in devtools panel belongs to tab

Issue 5093 - Indicate whether request in devtools panel belongs to tab

Abandoned, as described in the trac issue.

[a.giammarchi, 2018-02-12 16:40:49]

In order to drop the background image hack/work around I've used placeholders
and everything seems to work just fine.

However, there is an outstanding issue I'm apparently unable to figure out: how
to understand if a request was coming from the background or not.

Any hint about the best way to retrieve it would be very appreciated.

Also worth checking if updates and/ore removal of records could happen when
records are not live because in such case I need to pass the placeholder comment
along, if live, or remove it from the DOM, if offline.

Thanks in advance for any sort of hint.

[a.giammarchi, 2018-02-13 12:31:48]

After further investigation, I have realised that both record-update and
record-remove use a message.index mechanism that is not compatible with the
current logic: need more refactoring.

However, there is apparently no way with current received information to
understand if the tab was a background one or not.

Looking for suggestions on how to proceed. Thanks.

[a.giammarchi, 2018-02-13 13:36:46]

Latest update uses `request.docDomain` to distinguish between background tabs
and others.

Also update and remove records now are reflected within the panel through rows,
even if split.

I need to double check if docDomain is enough or, eventually, to ask changes in
devtools.js too.

[Thomas Greiner, 2018-02-28 18:18:09]

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.html
File devtools-panel.html (right):

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.html...
devtools-panel.html:79: <tr id="background-items">
This implementation appears to cause a bit of overhead to ensure that we're not
accidentally removing it from the table (e.g. ignoring background items in
`getCSSSelector()`; `table.insertBefore()`).

So what about instead introducing it as a separate table?

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.js
File devtools-panel.js (right):

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.js#n...
devtools-panel.js:188: /**
In comparison to how we did the record filtering before using CSS, this
placeholder-based approach seems quite a bit more complicated because we have to
keep track of all records and modify the DOM to reflect each change.

So assuming that you're doing this to avoid the issues with `:nth-child(odd)`
when some elements are hidden, what about implementing it the same way as Chrome
does in the DevTools' Network tab and assign the styles via JavaScript (e.g.
`class="odd"`)?
That way all we need to do is update the class names of each row whenever the
user is filtering the list.

We can, of course, also hide/show the rows using JavaScript but that seems to be
out of scope for this particular issue.

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.js#n...
devtools-panel.js:242: function getCSSFilter(dataset)
Detail: This name confused me a bit because (a) "filter" is ambiguous and may
refer to  ad blocking filters and (b) it doesn't indicate what the CSS selector
is for.

What about, for instance, renaming it to something like "getRecordSelector"?

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.js#n...
devtools-panel.js:297: if (message.request.docDomain == null)
Note: We'll be able to check `message.request.pageId == -1` after #6402 lands,
according to the code that's currently under review. No need to make any changes
at this point but I thought it's worth mentioning that this part of the code is
subject to change.

[a.giammarchi, 2018-02-28 19:07:13]

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.js
File devtools-panel.js (right):

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.js#n...
devtools-panel.js:188: /**
On 2018/02/28 18:18:08, Thomas Greiner wrote:
> In comparison to how we did the record filtering before using CSS, this

before we were not doing anything, it was a hack with the background color :-)

> placeholder-based approach seems quite a bit more complicated because we have
to
> keep track of all records and modify the DOM to reflect each change.

not perfect, I agree, but don't we need to track all changes and change all
classes when we filter, if we go 100% JS?

> So assuming that you're doing this to avoid the issues with `:nth-child(odd)`
> when some elements are hidden, what about implementing it the same way as
Chrome
> does in the DevTools' Network tab and assign the styles via JavaScript (e.g.
> `class="odd"`)?

not sure which one is better. I usually prefer styling without JS when possible.

Also I am not sure the the JS version would be better, the placeholders need to
be in place regardless unless we want to trash and recreate the whole table each
time, which I guess might be fine if we don't have thousand records.

I will give it a try +1

> We can, of course, also hide/show the rows using JavaScript but that seems to
be
> out of scope for this particular issue.

hide and show might be actually an alternative to the placeholder. If I have to
setup rows, maybe it's better to hide those filtered out instead of using
placeholders, and to the module 2 dance for visible rows only?

In that case there shouldn't be the need at all for placeholders.

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.js#n...
devtools-panel.js:242: function getCSSFilter(dataset)
On 2018/02/28 18:18:08, Thomas Greiner wrote:
> Detail: This name confused me a bit because (a) "filter" is ambiguous and may
> refer to  ad blocking filters and (b) it doesn't indicate what the CSS
selector
> is for.
> 
> What about, for instance, renaming it to something like "getRecordSelector"?

it actually returns a CSS selector that filters rows but since we're going with
manual classes updates this whole part won't be needed anymore.

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.js#n...
devtools-panel.js:297: if (message.request.docDomain == null)
On 2018/02/28 18:18:08, Thomas Greiner wrote:
> Note: We'll be able to check `message.request.pageId == -1` after #6402 lands,
> according to the code that's currently under review. No need to make any
changes
> at this point but I thought it's worth mentioning that this part of the code
is
> subject to change.

then I prefer making that one a blocking of this one, for the simple reason I
don't think this approach I've implemented is bullet proof at all.

What do you think?

[a.giammarchi, 2018-03-02 09:38:21]

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.html
File devtools-panel.html (right):

https://codereview.adblockplus.org/29695580/diff/29697579/devtools-panel.html...
devtools-panel.html:79: <tr id="background-items">
On 2018/02/28 18:18:08, Thomas Greiner wrote:
> This implementation appears to cause a bit of overhead to ensure that we're
not
> accidentally removing it from the table (e.g. ignoring background items in
> `getCSSSelector()`; `table.insertBefore()`).
> 
> So what about instead introducing it as a separate table?

I've missed this one ... yeah, it was somehow my first idea, I'll try to do this
way.

[a.giammarchi, 2018-03-02 15:56:53]

There is one blocker (#6402) and one missing information (what are dark theme
colors for background items?) but I'd like to know if the current version is
better than before.

Key changes:

  * simplified state handling via JS, no hacks needed
  * no placeholders, just table TRs to deal with
  * all operations via index works through a single records set

Performance over long sessions with hundreds of rows seems to be more than
reasonable.

Hints or suggested improvements appreciated.