Issue 6948 - Begun simplifying the "block element" context menu logic

Issue 6948 - Begun simplifying the "block element" context menu logic

[kzar, 2018-09-28 15:22:55]

Patch Set 1

[kzar, 2018-09-28 15:37:35]

So far I've just smoke tested this on Chrome, and it already fixes the issue for
me. I'll do some testing on Firefox desktop + mobile next week.

https://codereview.adblockplus.org/29894584/diff/29894585/lib/filterComposer.js
File lib/filterComposer.js (left):

https://codereview.adblockplus.org/29894584/diff/29894585/lib/filterComposer....
lib/filterComposer.js:39: exports.isPageReady = page =>
I coudln't see where `isPageReady` was being used in adblockpluschrome,
adblockpluscore or adblockplusui, so I just removed it.

https://codereview.adblockplus.org/29894584/diff/29894585/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29894584/diff/29894585/lib/filterComposer....
lib/filterComposer.js:298: if (shouldShowContextMenu && !showingContextMenu)
The previous logic wasn't great, I noticed it was adding and removing the
context menu more often than was necessary. I think this new logic should work
more reliably.

https://codereview.adblockplus.org/29894584/diff/29894585/lib/filterComposer....
lib/filterComposer.js:300: browser.contextMenus.create({
I originally kept the `contextMenuItem` variable, then passed that through to
`browser.contextMenues.create`. Surprisingly however, I noticed that the API
mutates the passed in object, adding a generated ID property. Then, the API
throws an exception the next time it's called with that Object since the
generated ID property already exists...

https://codereview.adblockplus.org/29894584/diff/29894585/lib/filterComposer....
lib/filterComposer.js:333: // Firefox for Android does not support
browser.windows.
Like we discussed, it's kind of pointless to worry about Firefox for Android in
this file at all... however since we're stuck doing that I've gone with feature
detection like you suggested.

[Sebastian Noack, 2018-09-28 16:24:25]

I cannot see any obvious issues with that change, and it's always great to get
rid of some more logic from the legacy abstraction layer.

However, can you help me understand how exactly this change addresses the bug
described in #6948?

Also, please add hints for testers to the issue, covering each scenario where
the context menu is supposed to be updated.

[kzar, 2018-10-01 11:38:54]

On 2018/09/28 16:24:25, Sebastian Noack wrote:
> I cannot see any obvious issues with that change, and it's always great to get
> rid of some more logic from the legacy abstraction layer.
> 
> However, can you help me understand how exactly this change addresses the bug
> described in #6948?

Fair enough, it seems kind of weird to rewrite a bunch of code instead of just
fixing the bug. The thing is, I realised the existing logic was quite hard to
grok and not working correctly, so instead of figuring out exactly why the (race
condition?) problem was happening I just started again. Especially since we're
trying to get rid of the ext code. Anyway, to see what I mean, put this code in
the background page and try to reproduce the issue:

for (let name of ["create", "remove", "removeAll"])
{
  let real = browser.contextMenus[name];
  browser.contextMenus[name] = (...args) =>
  {
    console.log(name, JSON.stringify(...args));
    real.apply(browser.contextMenus, args);
  };
}

You'll see browser.contextMenus.removeAll and create being called multiple times
for no good reason. I think that's the root of our problem. With this change,
you just see create and remove being called once at the appropriate times.

Since I got upgraded to Chrome 69 I'm no longer able to reproduce the problem
reliably, but last week I could reliably reproduce the problem before my change
and not at all with my change.

> Also, please add hints for testers to the issue, covering each scenario where
> the context menu is supposed to be updated.

Sure, done that. https://issues.adblockplus.org/ticket/6948#Hintsfortesters

[Sebastian Noack, 2018-10-01 11:48:39]

LGTM