Issue 3487 - Add links to the help tab

Issue 3487 - Add links to the help tab

[saroyanm, 2017-01-27 17:23:43]

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js#newc...
new-options.js:855: ext.backgroundPage.sendMessage(
I think we create a function, similar to the "getDocLink" for "app.get"
requests.

[Thomas Greiner, 2017-01-31 14:29:23]

https://codereview.adblockplus.org/29373674/diff/29373675/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29373674/diff/29373675/messageResponder.js...
messageResponder.js:190: return info.platform;
See line 192.

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js#newc...
new-options.js:852: });
We should definitely try to make this more elegant and more efficient (same
applies for setting doclinks in the first-run page) but feel free to tackle that
in a separate ticket.

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js#newc...
new-options.js:855: ext.backgroundPage.sendMessage(
On 2017/01/27 17:23:43, saroyanm wrote:
> I think we create a function, similar to the "getDocLink" for "app.get"
> requests.

I guess you're referring to
https://hg.adblockplus.org/adblockpluschrome/file/tip/options.js#l63 but I don't
think that's necessary unless we use a specific type of message more often.

Instead, we could've just implemented an abstraction layer if we wanted to hide
away all `ext.backgroundPage.sendMessage()` calls but for some reason we didn't.
So if it turns out that those calls are way too numerous and tedious, I'd rather
introduce one than making custom (inconsistent) functions for each individual
message.

https://codereview.adblockplus.org/29373674/diff/29373675/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29373674/diff/29373675/skin/new-options.cs...
skin/new-options.css:1147: #share-chinese:lang(zh)
I'd prefer using `:not()` instead to avoid having to set the "display" property.

[saroyanm, 2017-02-01 13:22:18]

https://codereview.adblockplus.org/29373674/diff/29373675/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29373674/diff/29373675/messageResponder.js...
messageResponder.js:190: return info.platform;
On 2017/01/31 14:29:22, Thomas Greiner wrote:
> See line 192.

Crap, how I could have missed that, thanks.

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js#newc...
new-options.js:852: });
On 2017/01/31 14:29:22, Thomas Greiner wrote:
> We should definitely try to make this more elegant and more efficient (same
> applies for setting doclinks in the first-run page) but feel free to tackle
that
> in a separate ticket.

Agree, we can discuss details in #4856

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js#newc...
new-options.js:855: ext.backgroundPage.sendMessage(
On 2017/01/31 14:29:22, Thomas Greiner wrote:
> On 2017/01/27 17:23:43, saroyanm wrote:
> > I think we create a function, similar to the "getDocLink" for "app.get"
> > requests.
> 
> I guess you're referring to
> https://hg.adblockplus.org/adblockpluschrome/file/tip/options.js#l63 but I
don't
> think that's necessary unless we use a specific type of message more often.
> 
> Instead, we could've just implemented an abstraction layer if we wanted to
hide
> away all `ext.backgroundPage.sendMessage()` calls but for some reason we
didn't.
> So if it turns out that those calls are way too numerous and tedious, I'd
rather
> introduce one than making custom (inconsistent) functions for each individual
> message.

Agree, you want me to tackle it here, or in a separate issue ?

https://codereview.adblockplus.org/29373674/diff/29373675/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29373674/diff/29373675/skin/new-options.cs...
skin/new-options.css:1147: #share-chinese:lang(zh)
On 2017/01/31 14:29:22, Thomas Greiner wrote:
> I'd prefer using `:not()` instead to avoid having to set the "display"
property.

Agree, we might want to fix that in firstRun page as well in future.

[Thomas Greiner, 2017-02-02 12:28:47]

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js#newc...
new-options.js:852: });
On 2017/02/01 13:22:18, saroyanm wrote:
> On 2017/01/31 14:29:22, Thomas Greiner wrote:
> > We should definitely try to make this more elegant and more efficient (same
> > applies for setting doclinks in the first-run page) but feel free to tackle
> that
> > in a separate ticket.
> 
> Agree, we can discuss details in #4856

Thanks.

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js#newc...
new-options.js:855: ext.backgroundPage.sendMessage(
On 2017/02/01 13:22:18, saroyanm wrote:
> On 2017/01/31 14:29:22, Thomas Greiner wrote:
> > On 2017/01/27 17:23:43, saroyanm wrote:
> > > I think we create a function, similar to the "getDocLink" for "app.get"
> > > requests.
> > 
> > I guess you're referring to
> > https://hg.adblockplus.org/adblockpluschrome/file/tip/options.js#l63 but I
> don't
> > think that's necessary unless we use a specific type of message more often.
> > 
> > Instead, we could've just implemented an abstraction layer if we wanted to
> hide
> > away all `ext.backgroundPage.sendMessage()` calls but for some reason we
> didn't.
> > So if it turns out that those calls are way too numerous and tedious, I'd
> rather
> > introduce one than making custom (inconsistent) functions for each
individual
> > message.
> 
> Agree, you want me to tackle it here, or in a separate issue ?

Let's tackle this separately. It does look like nobody is happy with the way the
messaging API should be used so finding a better solution is way out of scope
for this review.

[saroyanm, 2017-02-08 13:13:29]

New patch uploaded.

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29373674/diff/29373675/new-options.js#newc...
new-options.js:855: ext.backgroundPage.sendMessage(
On 2017/02/02 12:28:47, Thomas Greiner wrote:
> On 2017/02/01 13:22:18, saroyanm wrote:
> > On 2017/01/31 14:29:22, Thomas Greiner wrote:
> > > On 2017/01/27 17:23:43, saroyanm wrote:
> > > > I think we create a function, similar to the "getDocLink" for "app.get"
> > > > requests.
> > > 
> > > I guess you're referring to
> > > https://hg.adblockplus.org/adblockpluschrome/file/tip/options.js#l63 but I
> > don't
> > > think that's necessary unless we use a specific type of message more
often.
> > > 
> > > Instead, we could've just implemented an abstraction layer if we wanted to
> > hide
> > > away all `ext.backgroundPage.sendMessage()` calls but for some reason we
> > didn't.
> > > So if it turns out that those calls are way too numerous and tedious, I'd
> > rather
> > > introduce one than making custom (inconsistent) functions for each
> individual
> > > message.
> > 
> > Agree, you want me to tackle it here, or in a separate issue ?
> 
> Let's tackle this separately. It does look like nobody is happy with the way
the
> messaging API should be used so finding a better solution is way out of scope
> for this review.

I'm having a hard time of thinking about the new ticket description, can you
please create a ticket if you have one generic idea in mind, or we can discuss
together how we can improve the usage of "messaging API", I feel like I don't
have all details considering `It does look like nobody is happy with the way the
messaging API should be used` point.

[Thomas Greiner, 2017-03-02 15:17:47]

LGTM, just added a small comment.

In regards to the discussion about the messaging API we should keep it on our
radar. The requirements for it are still unclear at this point, however, so it
seems too early to even create a ticket for it.

https://codereview.adblockplus.org/29373674/diff/29374828/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29373674/diff/29374828/new-options.js#newc...
new-options.js:828: //Help tab
Detail: Usually, we put a space between `//` and the start of the comment.

[Thomas Greiner, 2017-03-02 19:13:26]

FYI: There is a ticket already in which we're discussing changes to the
messaging API (see https://issues.adblockplus.org/ticket/4492).