chrome/content/ui/firstRun.html - Issue 11533106: Prepared adblockplus for Safari

Delta Between Two Patch Sets: chrome/content/ui/firstRun.html

Issue 11533106: Prepared adblockplus for Safari (Closed)

Left Patch Set: Rebased on upstream Created Oct. 25, 2013, 4:17 p.m.

Right Patch Set: Made first run page always (also in FF) generated Created Oct. 31, 2013, 10:45 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <!--	2 <!--

3 - This file is part of Adblock Plus <http://adblockplus.org/>,	3 - This file is part of Adblock Plus <http://adblockplus.org/>,

4 - Copyright (C) 2006-2013 Eyeo GmbH	4 - Copyright (C) 2006-2013 Eyeo GmbH

5 -	5 -

6 - Adblock Plus is free software: you can redistribute it and/or modify	6 - Adblock Plus is free software: you can redistribute it and/or modify

7 - it under the terms of the GNU General Public License version 3 as	7 - it under the terms of the GNU General Public License version 3 as

8 - published by the Free Software Foundation.	8 - published by the Free Software Foundation.

9 -	9 -

10 - Adblock Plus is distributed in the hope that it will be useful,	10 - Adblock Plus is distributed in the hope that it will be useful,

11 - but WITHOUT ANY WARRANTY; without even the implied warranty of	11 - but WITHOUT ANY WARRANTY; without even the implied warranty of

12 - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the	12 - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

13 - GNU General Public License for more details.	13 - GNU General Public License for more details.

14 -	14 -

15 - You should have received a copy of the GNU General Public License	15 - You should have received a copy of the GNU General Public License

16 - along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.	16 - along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.

17 -->	17 -->

18	18

19 <html>	19 <html>

20 <head>	20 <head>

21 <title class="i18n_firstRun_title"></title>	21 <title class="i18n_firstRun_title"></title>

22 <meta charset="utf-8">	22 <meta charset="utf-8">

23

24 <!-- {% if type == 'safari' %} -->

25 <!-- {{ '<link type="text/css" href="skin/firstRun.css" rel="stylesheet"/>' }} -->

26 <!-- {% else %} -->

27 <link type="text/css" href="/skin/firstRun.css" rel="stylesheet"/>	23 <link type="text/css" href="/skin/firstRun.css" rel="stylesheet"/>

28 <!-- {% endif %} -->	24 {%- if needsExt %}
Wladimir Palant 2013/10/29 12:59:52 As mentioned before, I would prefer a solution wit As mentioned before, I would prefer a solution within build tools here - we shouldn't have to think about all platforms when writing basic code code. The Safari packager can easily process HTML files and replace host-relative links to skin by relative ones. If you don't like that solution, some code triggered from utils.js can check require("info").platform and go through stylesheet links correcting the href attribute - that's at least a generic approach not requiring additional markup. Sebastian Noack 2013/10/29 13:37:23 I would rather not hide the logic from the code of Show quoted text On 2013/10/29 12:59:52, Wladimir Palant wrote: > As mentioned before, I would prefer a solution within build tools here - we > shouldn't have to think about all platforms when writing basic code code. The > Safari packager can easily process HTML files and replace host-relative links to > skin by relative ones. I would rather not hide the logic from the code of the first run page, replacing it with some buildtools magic. Otherwise it can simply break accidentally when changing the first run page. Show quoted text > If you don't like that solution, some code triggered from utils.js can check > require("info").platform and go through stylesheet links correcting the href > attribute - that's at least a generic approach not requiring additional markup. I think that aproach wouldn't work for the js files below (see comment below). So I would rather stay to one solution for both. Wladimir Palant 2013/10/29 14:05:45 We have a number of conventions in place that work Show quoted text On 2013/10/29 13:37:23, sebastian wrote: > I would rather not hide the logic from the code of the first run page, replacing > it with some buildtools magic. Otherwise it can simply break accidentally when > changing the first run page. We have a number of conventions in place that work across all supported platforms thanks to "some buildtools magic". One such convention is: "use host-relative paths to refer to skin files." IMHO, having to keep in mind all the details of all platforms is worse than having to stick to these conventions.
29	25 <script type="text/javascript" src="ext/common.js"></script>

30 <!-- {% if type in ('chrome', 'opera', 'safari') %} -->	26 <script type="text/javascript" src="ext/content.js"></script>

31 <!-- {{ '<script type="text/javascript" src="ext/common.js"></script>' }} -- >	27 {%- endif %}

32 <!-- {{ '<script type="text/javascript" src="ext/content.js"></script>' }} - ->

33 <!-- {% endif %} -->
Wladimir Palant 2013/10/29 12:59:52 If we are adding these script tags dynamically, we If we are adding these script tags dynamically, we can simply do it at the top of utils.js (the one in adblockpluschrome repository, not adblockplus). A templating language for that is an overkill. Sebastian Noack 2013/10/29 13:37:23 I might be wrong. But I don't think that this will Show quoted text On 2013/10/29 12:59:52, Wladimir Palant wrote: > If we are adding these script tags dynamically, we can simply do it at the top > of utils.js (the one in adblockpluschrome repository, not adblockplus). A > templating language for that is an overkill. I might be wrong. But I don't think that this will actually work. Those two js files must be loaded and executed, before any other js code runs. And as far as I know, dynamically injecting a script tag at the top of utils.js, wouldn't block further js execution until those files are loaded and executed. Wladimir Palant 2013/10/29 14:05:45 Setting \|script.async = false\| should actually do Show quoted text On 2013/10/29 13:37:23, sebastian wrote: > I might be wrong. But I don't think that this will actually work. Those two js > files must be loaded and executed, before any other js code runs. And as far as > I know, dynamically injecting a script tag at the top of utils.js, wouldn't > block further js execution until those files are loaded and executed. Setting \|script.async = false\| should actually do what you need. I tested the following in Firefox and Chrome: var s = document.createElement("script"); s.async = false; s.src="data:text/javascript,alert(1)"; document.head.appendChild(s); alert(2); setTimeout(function() {alert(3)}, 0); Firefox will execute the injected script immediately, Chrome will only do it after the execution context is done. I assume that Safari's approach will be similar to Chrome. Unfortunately, this means that loading scripts dynamically cannot be done at the top of utils.js, it rather has to be done by a separate script loading before utils.js. Side-note: now that I've found the implementation of these conditional comments I consider it a huge hack. If kept: a) It shouldn't use special templating syntax but rather keep the standard syntax. Firefox packager should be updated accordingly. b) It shouldn't be hardcoded to run on the first-run page, the metadata file should rather define the files to use Jinja2 on. Sebastian Noack 2013/10/29 15:44:56 They are just wrapped by HTML comments, for backwa Show quoted text On 2013/10/29 14:05:45, Wladimir Palant wrote: > On 2013/10/29 13:37:23, sebastian wrote: > > I might be wrong. But I don't think that this will actually work. Those two js > > files must be loaded and executed, before any other js code runs. And as far > as > > I know, dynamically injecting a script tag at the top of utils.js, wouldn't > > block further js execution until those files are loaded and executed. > > Setting \|script.async = false\| should actually do what you need. I tested the > following in Firefox and Chrome: > > var s = document.createElement("script"); > s.async = false; > s.src="data:text/javascript,alert(1)"; > document.head.appendChild(s); > alert(2); > setTimeout(function() {alert(3)}, 0); > > Firefox will execute the injected script immediately, Chrome will only do it > after the execution context is done. I assume that Safari's approach will be > similar to Chrome. Unfortunately, this means that loading scripts dynamically > cannot be done at the top of utils.js, it rather has to be done by a separate > script loading before utils.js. > > Side-note: now that I've found the implementation of these conditional comments > I consider it a huge hack. If kept: > > a) It shouldn't use special templating syntax but rather keep the standard > syntax. Firefox packager should be updated accordingly. They are just wrapped by HTML comments, for backwards compatibility. It was my plan anyway to make the first run page always generated, later. And as you said you don't insist on having it done before merging this changes. Show quoted text > b) It shouldn't be hardcoded to run on the first-run page, the metadata file > should rather define the files to use Jinja2 on. Agreed, that certainly would be nicer. So it sounds that you have accepted the need for this approach or do you still have a better idea? Wladimir Palant 2013/10/29 15:56:37 For the scripts: yes, processing the page with Jin Show quoted text On 2013/10/29 15:44:56, sebastian wrote: > So it sounds that you have accepted the need for this approach or do you still > have a better idea? For the scripts: yes, processing the page with Jinja2 should be a cleaner solution (at least once the comments are gone). Ideally, soon Firefox won't be "special" any more. For the styles: no, requiring special code for Safari everywhere isn't something I'm happy with. As I said, the host-relative URLs should be converted automatically, either in build tools or in a script like ext/content.js.
34 <script type="text/javascript" src="utils.js"></script>	28 <script type="text/javascript" src="utils.js"></script>

35 <script type="text/javascript" src="i18n.js"></script>	29 <script type="text/javascript" src="i18n.js"></script>

36 <script type="text/javascript" src="firstRun.js"></script>	30 <script type="text/javascript" src="firstRun.js"></script>

37 </head>	31 </head>

38 <body>	32 <body>

39	33

40	34

41 <header>	35 <header>

42 <div id="logo"></div>	36 <div id="logo"></div>

43 <h1 id="title-main" class="i18n_firstRun_title"></h1>	37 <h1 id="title-main" class="i18n_firstRun_title"></h1>

(...skipping 121 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
165 <footer>	159 <footer>

166 <a id="contributors" class="i18n_firstRun_contributor_credits"></a>	160 <a id="contributors" class="i18n_firstRun_contributor_credits"></a>

167 </footer>	161 </footer>

168	162

169	163

170 <div id="glass-pane">	164 <div id="glass-pane">

171 <iframe id="share-popup" scrolling="no"></iframe>	165 <iframe id="share-popup" scrolling="no"></iframe>

172 </div>	166 </div>

173 </body>	167 </body>

174 </html>	168 </html>

LEFT	RIGHT