notification.html - Issue 5767063142400000: notification fixes

Side by Side Diff: notification.html

Issue 5767063142400000: notification fixes (Closed)

Patch Set: removed import of ext/background.js from notification.html Created March 4, 2014, 4:19 p.m.

Left:
Right:

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

Jump to:

View unified diff | Download patch

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <html>	2 <html>

3 <head>	3 <head>

4 <style>	4 <style>

5 body	5 body

6 {	6 {

7 background-color: #f8f6f2;	7 background-color: #f8f6f2;

8 font-family: Arial, sans;	8 font-family: Arial, sans;

9 font-size: 12px;	9 font-size: 12px;

10 }	10 }

11	11

12 #notification	12 #notification

13 {	13 {

14 position: absolute;	14 position: absolute;

15 top: 5px;	15 top: 5px;

16 left: 65px;	16 left: 65px;

17 }	17 }

18	18

19 #notification>h1	19 #notification>h1

20 {	20 {

21 font-size: 16px;	21 font-size: 16px;

22 }	22 }

23	23

24 #notification>*	24 #notification>*

25 {	25 {

26 margin: 5px;	26 margin: 5px;

27 }	27 }

28 </style>	28 </style>

29 <script src="ext/common.js"></script>	29 <script src="ext/common.js"></script>
	Thomas Greiner 2014/03/05 13:24:47 We don't need ext/common.js anymore if we remove n We don't need ext/common.js anymore if we remove notification.html from Safari and get rid of references to ext.. saroyanm* 2014/03/05 13:44:28 notification.js also imported in popup.html, so we Show quoted text On 2014/03/05 13:24:47, Thomas Greiner wrote: > We don't need ext/common.js anymore if we remove notification.html from Safari > and get rid of references to ext.. notification.js also imported in popup.html, so we can't use only chrome API there I guess, maybe we can still have this import here, what you think ? Thomas Greiner* 2014/03/05 13:51:48 Good point. In that case I'd be in favor of keepin Show quoted text On 2014/03/05 13:44:28, saroyanm wrote: > On 2014/03/05 13:24:47, Thomas Greiner wrote: > > We don't need ext/common.js anymore if we remove notification.html from Safari > > and get rid of references to ext.. > > notification.js also imported in popup.html, so we can't use only chrome API > there I guess, maybe we can still have this import here, what you think ? Good point. In that case I'd be in favor of keeping common.js in here. @Sebastian What do you think? Sebastian Noack* 2014/03/05 14:11:56 Don't we also need ext.windows in order to open a Show quoted text On 2014/03/05 13:51:48, Thomas Greiner wrote: > On 2014/03/05 13:44:28, saroyanm wrote: > > On 2014/03/05 13:24:47, Thomas Greiner wrote: > > > We don't need ext/common.js anymore if we remove notification.html from > Safari > > > and get rid of references to ext.. > > > > notification.js also imported in popup.html, so we can't use only chrome API > > there I guess, maybe we can still have this import here, what you think ? > > Good point. In that case I'd be in favor of keeping common.js in here. > @Sebastian What do you think? Don't we also need ext.windows in order to open a new page? That can't be moved to common.js. However in popup.html we already have ext. And in notification.html (which is still only used on Chrome) you can retrieve ext via chrome.extension.getBackgroundPage().ext. saroyanm* 2014/03/05 14:36:26 In that case maybe we can still import ext/common. Show quoted text On 2014/03/05 14:11:56, Sebastian Noack wrote: > On 2014/03/05 13:51:48, Thomas Greiner wrote: > > On 2014/03/05 13:44:28, saroyanm wrote: > > > On 2014/03/05 13:24:47, Thomas Greiner wrote: > > > > We don't need ext/common.js anymore if we remove notification.html from > > Safari > > > > and get rid of references to ext.. > > > > > > notification.js also imported in popup.html, so we can't use only chrome API > > > there I guess, maybe we can still have this import here, what you think ? > > > > Good point. In that case I'd be in favor of keeping common.js in here. > > @Sebastian What do you think? > Don't we also need ext.windows in order to open a new page? That can't be moved > to common.js. However in popup.html we already have ext. And in > notification.html (which is still only used on Chrome) you can retrieve ext via > chrome.extension.getBackgroundPage().ext. In that case maybe we can still import ext/common.js into notification.html and use: backgroundPage.ext.windows.getLastFocused(function(win) { win.openTab(link.href); }); to open new tab, not to make changes in notification.js while both safari and chrome use it in popup.html ? Sebastian Noack* 2014/03/05 18:48:15 I would still prefer something like this at the to Show quoted text On 2014/03/05 14:36:26, saroyanm wrote: > On 2014/03/05 14:11:56, Sebastian Noack wrote: > > On 2014/03/05 13:51:48, Thomas Greiner wrote: > > > On 2014/03/05 13:44:28, saroyanm wrote: > > > > On 2014/03/05 13:24:47, Thomas Greiner wrote: > > > > > We don't need ext/common.js anymore if we remove notification.html from > > > Safari > > > > > and get rid of references to ext.*. > > > > > > > > notification.js also imported in popup.html, so we can't use only chrome > API > > > > there I guess, maybe we can still have this import here, what you think ? > > > > > > Good point. In that case I'd be in favor of keeping common.js in here. > > > @Sebastian What do you think? > > Don't we also need ext.windows in order to open a new page? That can't be > moved > > to common.js. However in popup.html we already have ext. And in > > notification.html (which is still only used on Chrome) you can retrieve ext > via > > chrome.extension.getBackgroundPage().ext. > In that case maybe we can still import ext/common.js into notification.html and > use: > backgroundPage.ext.windows.getLastFocused(function(win) { > win.openTab(link.href); }); > to open new tab, not to make changes in notification.js while both safari and > chrome use it in popup.html ? I would still prefer something like this at the top of notification.js: if (!("ext" in window)) window.ext = chrome.extension.getBackgroundPage().ext; Until now in all JavaScript files we have either the content script part or the background page part of the extension API exposed by ext. And having only the common subset available in notification.html is just not consistent, but confusing and might rather quickly lead to bugs when working on that code in the future.
30 <script src="ext/background.js"></script>

31 <script src="notification.js"></script>	30 <script src="notification.js"></script>

32 </head>	31 </head>

33 <body>	32 <body>

34 <img src="icons/abp-48.png">	33 <img src="icons/abp-48.png">

35 <div id="notification">	34 <div id="notification">

36 <h1 id="notification-title"></h1>	35 <h1 id="notification-title"></h1>

37 <p id="notification-message"></p>	36 <p id="notification-message"></p>

38 </div>	37 </div>

39 </body>	38 </body>

40 </html>	39 </html>

OLD	NEW

« no previous file with comments | « chrome/ext/background.js ('k') | notification.js » ('j') | notification.js » ('J')