Issue 6511 - Use messaging to register popup notification clicks

Issue 6511 - Use messaging to register popup notification clicks

Depends on https://codereview.adblockplus.org/29730608/

[kzar, 2018-03-22 15:06:39]

Patch Set 1

[saroyanm, 2018-03-22 17:30:47]

https://codereview.adblockplus.org/29730611/diff/29730612/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29730611/diff/29730612/dependencies#newcode5
dependencies:5: adblockplusui = adblockplusui git:6512-notification-close
I assume this suppose to be updated.

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome#new...
metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js
No need to update the path of desktop-options.js we did introduce modularization
 in #6310, so the generated desktop-options.js be
`adblockplusui/desktop-options.js`

[kzar, 2018-03-22 17:36:50]

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome#new...
metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js
On 2018/03/22 17:30:47, saroyanm wrote:
> No need to update the path of desktop-options.js we did introduce
modularization
>  in #6310, so the generated desktop-options.js be
> `adblockplusui/desktop-options.js` 

Well so far there is no adblockplusui/desktop-options.js file, and now no
desktop-options.css file either. The issue #6310 provides zero integration notes
and hardly any information for me to understand how any of this is supposed to
work.

So I will update this codereview once the adblockplusui dependency got updated
by someone else, but until then I'll leave this path as
adblockplusui/js/desktop-options.js so that I can test that my changes work.

[saroyanm, 2018-03-22 18:03:19]

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome#new...
metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js
On 2018/03/22 17:36:50, kzar wrote:
> On 2018/03/22 17:30:47, saroyanm wrote:
> > No need to update the path of desktop-options.js we did introduce
> modularization
> >  in #6310, so the generated desktop-options.js be
> > `adblockplusui/desktop-options.js` 
> 
> Well so far there is no adblockplusui/desktop-options.js file, and now no
> desktop-options.css file either. The issue #6310 provides zero integration
notes
> and hardly any information for me to understand how any of this is supposed to
> work.
Right, the information is in the README file ->
https://hg.adblockplus.org/adblockplusui/file/tip/README.md#l100 and 
https://hg.adblockplus.org/adblockplusui/file/tip/README.md#l36

> So I will update this codereview once the adblockplusui dependency got updated
> by someone else, but until then I'll leave this path as
> adblockplusui/js/desktop-options.js so that I can test that my changes work.
The file is generated during the build process, alternatively you can build it
manually by running `npm i` inside of adblockplusui, hope this helps.

[Sebastian Noack, 2018-03-22 18:06:26]

Looks good to me, once the rebased on the dependency update.

[a.giammarchi, 2018-03-23 14:24:56]

I was going through reviews and I've noticed a stopper in here.

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome#new...
metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js
As previously discussed, and as mentioned by Manvel, this will break.

We are bundling that file, producing exactly what you expect, which is
`adblockplusui/desktop-options.js`

Please revert this change or we'll have more issues, thanks.

[kzar, 2018-03-23 14:43:12]

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29730611/diff/29730612/metadata.chrome#new...
metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js
On 2018/03/23 14:24:56, a.giammarchi wrote:
> Please revert this change or we'll have more issues, thanks.

As I mentioned I will update this codereview once the dependency update (#6476)
has landed.

[a.giammarchi, 2018-03-23 14:59:07]

On 2018/03/23 14:43:12, kzar wrote:
> As I mentioned I will update this codereview once the dependency update
(#6476)
> has landed.

My main concern was here:

> I'll leave this path as adblockplusui/js/desktop-options.js so that I can test
that my changes work.

If you include that file, nothing would work in ABP UI desktop-option page.

However, if that's irrelevant for your tests, we're all good.

[kzar, 2018-03-23 15:15:27]

On 2018/03/23 14:59:07, a.giammarchi wrote:
> If you include that file, nothing would work in ABP UI desktop-option page.
> 
> However, if that's irrelevant for your tests, we're all good.

But if I didn't include that path the extension wouldn't build at all since the
file didn't exist. Yes I now know it's supposed to be generated at the point the
dependency is updated, and I will update this review when (#6476) lands.

[kzar, 2018-03-23 16:41:52]

Patch Set 2 : Updated dependency

https://codereview.adblockplus.org/29730611/diff/29731614/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29730611/diff/29731614/lib/notificationHel...
lib/notificationHelper.js:129: showOptions((page, port) =>
Due to rebase, from #6476.

[Sebastian Noack, 2018-03-23 22:44:03]

LGTM