First-run page on Adblock Plus for IE

First-run page on Adblock Plus for IE

[Thomas Greiner, 2013-06-25 10:47:53]

I left the mock object "AdblockPlus" in firstRun.js for the review to show how
I'd expect the generated JavaScript object to look like.

Obviously, it won't be included in the commit.

[Felix Dahlke, 2013-07-11 11:11:13]

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.css
File html/static/css/firstRun.css (right):

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.cs...
html/static/css/firstRun.css:5: text-align: center;
I don't really get this. You set text-align to center in the body, then you set
it to text-align left in the wrapper (which appears to contain all the content)
and then to center again in the header? Is there no easier way to achieve what
you want?

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.cs...
html/static/css/firstRun.css:14: *display: inline; /* IE6 inline-block fix */
The zoom: 1 below is part of the fix, right? Then I'd prefer having both at the
bottom and the comment above, so it's clear.

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.cs...
html/static/css/firstRun.css:29: display: inline-block;
This is the same in #wrapper, and in several blocks below. Wow about having just
one block for all of them?

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.cs...
html/static/css/firstRun.css:178: @media (max-height: 800px)
This won't work in older IEs, don't know which first introduced support for
media queries. To make this work cross-platform, we'll have to use respond.js as
on adblockplus.org, with separate CSS files.

I doubt it's worth the trouble, better have one font size that looks good
everywhere. I really don't see why we need to support anything below 800x600
either.

http://codereview.adblockplus.org/10989023/diff/1/html/static/js/firstRun.js
File html/static/js/firstRun.js (right):

http://codereview.adblockplus.org/10989023/diff/1/html/static/js/firstRun.js#...
html/static/js/firstRun.js:65: function addListener(object, type, listener,
useCapture)
Since you're always passing useCapture=false, I guess it doesn't need to be a
parameter. It wouldn't happen for attachEvent anyway.

http://codereview.adblockplus.org/10989023/diff/1/html/static/js/firstRun.js#...
html/static/js/firstRun.js:106: window.attachEvent("onmessage",
popupMessageListener);
So you're not using addEventListener here because newer IE versions wouldn't
call attachEvent then, and it has to be attachEvent?

I'd prefer to handle that in addListener/removeListener. Roughly like this:

if (type == "message" && "attachEvent" in object)
  object.attachEvent(...);

http://codereview.adblockplus.org/10989023/diff/1/html/static/js/firstRun.js#...
html/static/js/firstRun.js:108: var timeout = 125;
I think the timeout calculating here is a bit more complex than it has to be.
How about just waiting a couple of times for a fixed interval, and have it time
out after 2-4 seconds?

http://codereview.adblockplus.org/10989023/diff/1/html/templates/firstRun.html
File html/templates/firstRun.html (right):

http://codereview.adblockplus.org/10989023/diff/1/html/templates/firstRun.htm...
html/templates/firstRun.html:2: <!-- saved from url=(0014)about:internet -->
What's with that?

http://codereview.adblockplus.org/10989023/diff/1/html/templates/firstRun.htm...
html/templates/firstRun.html:23: <script type="text/javascript"
src="../static/js/i18n.js"></script>
Just in case you don't know: type="text/javascript" is optional with doctype
html, <script src=""></script> is fine.

http://codereview.adblockplus.org/10989023/diff/1/locales/en.ini
File locales/en.ini (right):

http://codereview.adblockplus.org/10989023/diff/1/locales/en.ini#newcode37
locales/en.ini:37: [firstRun]
Should be first-run to be consistent with the naming above (also in the key
names below).

[Thomas Greiner, 2013-07-12 10:52:52]

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.css
File html/static/css/firstRun.css (right):

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.cs...
html/static/css/firstRun.css:5: text-align: center;
Done. I moved the share elements and the glass-pane out of the wrapper to get
rid of one of those text-aligns.

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.cs...
html/static/css/firstRun.css:14: *display: inline; /* IE6 inline-block fix */
On 2013/07/11 11:11:14, Felix H. Dahlke wrote:
> The zoom: 1 below is part of the fix, right? Then I'd prefer having both at
the
> bottom and the comment above, so it's clear.

Done.

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.cs...
html/static/css/firstRun.css:29: display: inline-block;
On 2013/07/11 11:11:14, Felix H. Dahlke wrote:
> This is the same in #wrapper, and in several blocks below. Wow about having
just
> one block for all of them?

Done.

http://codereview.adblockplus.org/10989023/diff/1/html/static/css/firstRun.cs...
html/static/css/firstRun.css:178: @media (max-height: 800px)
Thought of it more as a nice improvement on newer browser versions rather than
something essential.

Removed it anyway and set font-size to 1.1em;

http://codereview.adblockplus.org/10989023/diff/1/html/static/js/firstRun.js
File html/static/js/firstRun.js (right):

http://codereview.adblockplus.org/10989023/diff/1/html/static/js/firstRun.js#...
html/static/js/firstRun.js:65: function addListener(object, type, listener,
useCapture)
On 2013/07/11 11:11:14, Felix H. Dahlke wrote:
> Since you're always passing useCapture=false, I guess it doesn't need to be a
> parameter. It wouldn't happen for attachEvent anyway.

Done.

http://codereview.adblockplus.org/10989023/diff/1/html/static/js/firstRun.js#...
html/static/js/firstRun.js:106: window.attachEvent("onmessage",
popupMessageListener);
On 2013/07/11 11:11:14, Felix H. Dahlke wrote:
> So you're not using addEventListener here because newer IE versions wouldn't
> call attachEvent then, and it has to be attachEvent?
> 
> I'd prefer to handle that in addListener/removeListener. Roughly like this:
> 
> if (type == "message" && "attachEvent" in object)
>   object.attachEvent(...);

Done.

http://codereview.adblockplus.org/10989023/diff/1/html/static/js/firstRun.js#...
html/static/js/firstRun.js:108: var timeout = 125;
On 2013/07/11 11:11:14, Felix H. Dahlke wrote:
> I think the timeout calculating here is a bit more complex than it has to be.
> How about just waiting a couple of times for a fixed interval, and have it
time
> out after 2-4 seconds?

Done.

http://codereview.adblockplus.org/10989023/diff/1/html/templates/firstRun.html
File html/templates/firstRun.html (right):

http://codereview.adblockplus.org/10989023/diff/1/html/templates/firstRun.htm...
html/templates/firstRun.html:2: <!-- saved from url=(0014)about:internet -->
On 2013/07/11 11:11:14, Felix H. Dahlke wrote:
> What's with that?

It was required in the settings page. Therefore I also included it in here.
http://msdn.microsoft.com/en-us/library/ms537628(v=vs.85).aspx

http://codereview.adblockplus.org/10989023/diff/1/html/templates/firstRun.htm...
html/templates/firstRun.html:23: <script type="text/javascript"
src="../static/js/i18n.js"></script>
On 2013/07/11 11:11:14, Felix H. Dahlke wrote:
> Just in case you don't know: type="text/javascript" is optional with doctype
> html, <script src=""></script> is fine.

Done.

http://codereview.adblockplus.org/10989023/diff/1/locales/en.ini
File locales/en.ini (right):

http://codereview.adblockplus.org/10989023/diff/1/locales/en.ini#newcode37
locales/en.ini:37: [firstRun]
Done. Also changed "_" to "-" to be more consistent. ;)

http://codereview.adblockplus.org/10989023/diff/14002/html/static/js/firstRun.js
File html/static/js/firstRun.js (right):

http://codereview.adblockplus.org/10989023/diff/14002/html/static/js/firstRun...
html/static/js/firstRun.js:85: object.detachEvent("on" + type, listener);
Spotted and fix this.

[Felix Dahlke, 2013-07-18 12:29:30]

LGTM :)