Issue 2381 - Added share overlay to options page

To avoid code duplication I also created common.js and common.css for code that's shared between the first-run page and the options page.

[saroyanm, 2015-09-08 17:33:50]

In general looks good, I can see that first-run page needs to be updated to be
more consistent with options page, because I can see that while we will add
common scripts and styles things can become more confusing.
But I do think that it's separate topic.

https://codereview.adblockplus.org/29321336/diff/29322930/common.js
File common.js (right):

https://codereview.adblockplus.org/29321336/diff/29322930/common.js#newcode17
common.js:17: 
I think we should use "strict" here as well, but not sure why it's missing in
background.js and i18n.js ? Is there a reason, or we forget to add ?

https://codereview.adblockplus.org/29321336/diff/29322930/common.js#newcode122
common.js:122: iframe.className = "visible";
This implementation is inconsistent, with other similar implementation on the
page, but it's not the part of the review and it just about reuse the overlay,
so we can fix this also afterward.

https://codereview.adblockplus.org/29321336/diff/29322930/options.js
File options.js (right):

https://codereview.adblockplus.org/29321336/diff/29322930/options.js#newcode676
options.js:676: function updateShareLink()
Shouldn't this implementation also be shared between scripts ?

https://codereview.adblockplus.org/29321336/diff/29322930/skin/common.css
File skin/common.css (right):

https://codereview.adblockplus.org/29321336/diff/29322930/skin/common.css#new...
skin/common.css:35: -webkit-transition-duration: 0.2s;
We are using "ms" in options.css and "s" for durations in first-run.css, It's
not part of this review, but we should consider that and make the code somehow
consistent.

[Thomas Greiner, 2015-09-23 14:03:06]

I also rebased it to the latest revision so ignore the changes in firstRun.html
and firstRun.css.

https://codereview.adblockplus.org/29321336/diff/29322930/common.js
File common.js (right):

https://codereview.adblockplus.org/29321336/diff/29322930/common.js#newcode17
common.js:17: 
On 2015/09/08 17:33:49, saroyanm wrote:
> I think we should use "strict" here as well, but not sure why it's missing in
> background.js and i18n.js ? Is there a reason, or we forget to add ? 

The reason for that is that adding strict mode after the fact can cause
unexpected errors. Since most of this code comes from a file that didn't use
strict mode before, I didn't add it.

Anyway, I tested all functions in this file now and added the statement.

https://codereview.adblockplus.org/29321336/diff/29322930/common.js#newcode122
common.js:122: iframe.className = "visible";
On 2015/09/08 17:33:49, saroyanm wrote:
> This implementation is inconsistent, with other similar implementation on the
> page, but it's not the part of the review and it just about reuse the overlay,
> so we can fix this also afterward.

Agreed, let's work on consistency changes separately.

https://codereview.adblockplus.org/29321336/diff/29322930/options.js
File options.js (right):

https://codereview.adblockplus.org/29321336/diff/29322930/options.js#newcode676
options.js:676: function updateShareLink()
On 2015/09/08 17:33:49, saroyanm wrote:
> Shouldn't this implementation also be shared between scripts ?

No, the difference is that on the first-run page we check for a specific
resource while here we check for all three of them.

The reason for that is that we still want to show the share overlay when someone
clicks on the Twitter button even if the Facebook script is blocked.

[saroyanm, 2015-09-23 16:09:42]

On 2015/09/23 14:03:06, Thomas Greiner wrote:
> I also rebased it to the latest revision so ignore the changes in
firstRun.html
> and firstRun.css.
> 
> https://codereview.adblockplus.org/29321336/diff/29322930/common.js
> File common.js (right):
> 
> https://codereview.adblockplus.org/29321336/diff/29322930/common.js#newcode17
> common.js:17: 
> On 2015/09/08 17:33:49, saroyanm wrote:
> > I think we should use "strict" here as well, but not sure why it's missing
in
> > background.js and i18n.js ? Is there a reason, or we forget to add ? 
> 
> The reason for that is that adding strict mode after the fact can cause
> unexpected errors. Since most of this code comes from a file that didn't use
> strict mode before, I didn't add it.
> 
> Anyway, I tested all functions in this file now and added the statement.
> 
> https://codereview.adblockplus.org/29321336/diff/29322930/common.js#newcode122
> common.js:122: iframe.className = "visible";
> On 2015/09/08 17:33:49, saroyanm wrote:
> > This implementation is inconsistent, with other similar implementation on
the
> > page, but it's not the part of the review and it just about reuse the
overlay,
> > so we can fix this also afterward.
> 
> Agreed, let's work on consistency changes separately.
> 
> https://codereview.adblockplus.org/29321336/diff/29322930/options.js
> File options.js (right):
> 
>
https://codereview.adblockplus.org/29321336/diff/29322930/options.js#newcode676
> options.js:676: function updateShareLink()
> On 2015/09/08 17:33:49, saroyanm wrote:
> > Shouldn't this implementation also be shared between scripts ?
> 
> No, the difference is that on the first-run page we check for a specific
> resource while here we check for all three of them.
> 
> The reason for that is that we still want to show the share overlay when
someone
> clicks on the Twitter button even if the Facebook script is blocked.

LGTM