Delta Between Two Patch Sets: notification.html
Issue 6098518317989888:
Fix: Notification popup is broken (Closed)
Left Patch Set: Changes after Thomas notes
Created Feb. 18, 2014, 10:13 a.m.
Right Patch Set: Change back activeNotification unset
Created Feb. 26, 2014, 6:31 p.m.
| Left: | ||
| Right: |
Use n/p to move between diff chunks;
N/P to move between comments.
« chrome/ext/background.js ('K')
|
« metadata.chrome ('k')
|
no next file »
|
no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
| LEFT | RIGHT |
|---|---|
| 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/02/18 14:26:17
Well spotted. However, notifications.js also makes
Well spotted. However, notifications.js also makes a call to
ext.windows.getLastFocused() which is not included in common.js
saroyanm
2014/02/18 16:20:28
Good point.
On 2014/02/18 14:26:17, Thomas Greiner wrote:
> Well spotted. However, notifications.js also makes a call to
> ext.windows.getLastFocused() which is not included in common.js
Good point.
| |
| 30 <script src="ext/background.js"></script> | |
|
Felix Dahlke
2014/02/27 14:14:43
Guess I'm missing something, but what is this nece
Guess I'm missing something, but what is this necessary for?
Thomas Greiner
2014/02/27 15:10:37
This fixes notification.js which depends on ext.*
On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> Guess I'm missing something, but what is this necessary for?
This fixes notification.js which depends on ext.*
Sebastian Noack
2014/02/28 09:56:03
No, background.js must be only executed once in th
On 2014/02/27 15:10:37, Thomas Greiner wrote:
> On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > Guess I'm missing something, but what is this necessary for?
>
> This fixes notification.js which depends on ext.*
No, background.js must be only executed once in the background page, otherwise
things will break. Maybe you can use window.parent.ext?
saroyanm
2014/02/28 10:56:12
Not sure why the things will be broken, actually w
On 2014/02/28 09:56:03, Sebastian Noack wrote:
> On 2014/02/27 15:10:37, Thomas Greiner wrote:
> > On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > > Guess I'm missing something, but what is this necessary for?
> >
> > This fixes notification.js which depends on ext.*
>
> No, background.js must be only executed once in the background page, otherwise
> things will break. Maybe you can use window.parent.ext?
Not sure why the things will be broken, actually we are loading the HTML page
only in webkitNotifications.createHTMLNotification method and it's open html
page in desktop notification window. and in future I'm pretty sure that it will
be removed while it's a deprecated method, also not sure how I can use
window.parent.ext in this case - the Script that uses ext api is loaded in
desktop notification window. I've tested this with the browser that support
webkitNotifications.createHTMLNotification and seams nothing broken. Please let
me know if I didn't get your point.
Sebastian Noack
2014/02/28 12:35:03
This will run code twice, that is supposed to run
On 2014/02/28 10:56:12, saroyanm wrote:
> On 2014/02/28 09:56:03, Sebastian Noack wrote:
> > On 2014/02/27 15:10:37, Thomas Greiner wrote:
> > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > > > Guess I'm missing something, but what is this necessary for?
> > >
> > > This fixes notification.js which depends on ext.*
> >
> > No, background.js must be only executed once in the background page,
otherwise
> > things will break. Maybe you can use window.parent.ext?
>
> Not sure why the things will be broken, actually we are loading the HTML page
> only in webkitNotifications.createHTMLNotification method and it's open html
> page in desktop notification window.
This will run code twice, that is supposed to run only once, since
ext/background.js is already loaded by the background page itself. That already
makes things slower and using more memory than it would need. But when code that
persists data runs twice, it will lead to serious issues. This is currently more
a problem on Safari than on Chrome, but can also quickly lead to problems on
Chrome.
> also not sure how I can use
> window.parent.ext in this case - the Script that uses ext api is loaded in
> desktop notification window.
The idea was that it might be possible to access the window that created the
notification (the real background page in this case), via window.parent (like it
works inside frames). So you could just put "var ext = window.parent.ext;" at
the top of notification.js, and remove the includes for ext/common.js and
ext/background.js from notification.html. However I don't know if that works.
But if not, there might be an other way to pass an object to the page running
inside the notification.
saroyanm
2014/02/28 13:47:04
Yes I agree, but it will run twice for the users w
On 2014/02/28 12:35:03, Sebastian Noack wrote:
> On 2014/02/28 10:56:12, saroyanm wrote:
> > On 2014/02/28 09:56:03, Sebastian Noack wrote:
> > > On 2014/02/27 15:10:37, Thomas Greiner wrote:
> > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > > > > Guess I'm missing something, but what is this necessary for?
> > > >
> > > > This fixes notification.js which depends on ext.*
> > >
> > > No, background.js must be only executed once in the background page,
> otherwise
> > > things will break. Maybe you can use window.parent.ext?
> >
> > Not sure why the things will be broken, actually we are loading the HTML
page
> > only in webkitNotifications.createHTMLNotification method and it's open html
> > page in desktop notification window.
>
> This will run code twice, that is supposed to run only once, since
> ext/background.js is already loaded by the background page itself. That
already
> makes things slower and using more memory than it would need. But when code
that
> persists data runs twice, it will lead to serious issues. This is currently
more
> a problem on Safari than on Chrome, but can also quickly lead to problems on
> Chrome.
Yes I agree, but it will run twice for the users with old browser version, and
in future I think we will get rid of the createHTMLNotification.
And I'm pretty sure that this method will be deprecated for safari while it's
deprecated from w3c Notificaition API.
http://www.w3.org/TR/notifications/
> > also not sure how I can use
> > window.parent.ext in this case - the Script that uses ext api is loaded in
> > desktop notification window.
>
> The idea was that it might be possible to access the window that created the
> notification (the real background page in this case), via window.parent (like
it
> works inside frames). So you could just put "var ext = window.parent.ext;" at
> the top of notification.js, and remove the includes for ext/common.js and
> ext/background.js from notification.html. However I don't know if that works.
> But if not, there might be an other way to pass an object to the page running
> inside the notification.
>
Unfortunately I've tried to test that, but didn't worked for me, so I think It's
not possible,
And seams like it's only accept one parameter and It's URL:
http://www.chromium.org/developers/design-documents/desktop-notifications/api...
I'm not sure if the desktop notification has connection with extension.
Also no success by accessing background page using
chrome.extension.getBackgroundPage and chrome.extension.getViews.
So I don't see another way how we can access unfortunately.
Sebastian Noack
2014/02/28 14:23:49
I've just realized that webkitNotification.createH
On 2014/02/28 13:47:04, saroyanm wrote:
> On 2014/02/28 12:35:03, Sebastian Noack wrote:
> > On 2014/02/28 10:56:12, saroyanm wrote:
> > > On 2014/02/28 09:56:03, Sebastian Noack wrote:
> > > > On 2014/02/27 15:10:37, Thomas Greiner wrote:
> > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > > > > > Guess I'm missing something, but what is this necessary for?
> > > > >
> > > > > This fixes notification.js which depends on ext.*
> > > >
> > > > No, background.js must be only executed once in the background page,
> > otherwise
> > > > things will break. Maybe you can use window.parent.ext?
> > >
> > > Not sure why the things will be broken, actually we are loading the HTML
> page
> > > only in webkitNotifications.createHTMLNotification method and it's open
html
> > > page in desktop notification window.
> >
> > This will run code twice, that is supposed to run only once, since
> > ext/background.js is already loaded by the background page itself. That
> already
> > makes things slower and using more memory than it would need. But when code
> that
> > persists data runs twice, it will lead to serious issues. This is currently
> more
> > a problem on Safari than on Chrome, but can also quickly lead to problems on
> > Chrome.
>
> Yes I agree, but it will run twice for the users with old browser version, and
> in future I think we will get rid of the createHTMLNotification.
> And I'm pretty sure that this method will be deprecated for safari while it's
> deprecated from w3c Notificaition API.
> http://www.w3.org/TR/notifications/
>
> > > also not sure how I can use
> > > window.parent.ext in this case - the Script that uses ext api is loaded in
> > > desktop notification window.
> >
> > The idea was that it might be possible to access the window that created the
> > notification (the real background page in this case), via window.parent
(like
> it
> > works inside frames). So you could just put "var ext = window.parent.ext;"
at
> > the top of notification.js, and remove the includes for ext/common.js and
> > ext/background.js from notification.html. However I don't know if that
works.
> > But if not, there might be an other way to pass an object to the page
running
> > inside the notification.
> >
>
> Unfortunately I've tried to test that, but didn't worked for me, so I think
It's
> not possible,
> And seams like it's only accept one parameter and It's URL:
>
http://www.chromium.org/developers/design-documents/desktop-notifications/api...
> I'm not sure if the desktop notification has connection with extension.
> Also no success by accessing background page using
> chrome.extension.getBackgroundPage and chrome.extension.getViews.
> So I don't see another way how we can access unfortunately.
I've just realized that webkitNotification.createHTMLNotification doesn't exist
in the latest version of Chrome on Linux, Windows and Mac OS X, and also not in
the latest version of Safari. So either I missed something or notification.html
is actually never used?
saroyanm
2014/02/28 14:34:25
As I've mentioned in my previous comment It's depr
On 2014/02/28 14:23:49, Sebastian Noack wrote:
> On 2014/02/28 13:47:04, saroyanm wrote:
> > On 2014/02/28 12:35:03, Sebastian Noack wrote:
> > > On 2014/02/28 10:56:12, saroyanm wrote:
> > > > On 2014/02/28 09:56:03, Sebastian Noack wrote:
> > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote:
> > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > > > > > > Guess I'm missing something, but what is this necessary for?
> > > > > >
> > > > > > This fixes notification.js which depends on ext.*
> > > > >
> > > > > No, background.js must be only executed once in the background page,
> > > otherwise
> > > > > things will break. Maybe you can use window.parent.ext?
> > > >
> > > > Not sure why the things will be broken, actually we are loading the HTML
> > page
> > > > only in webkitNotifications.createHTMLNotification method and it's open
> html
> > > > page in desktop notification window.
> > >
> > > This will run code twice, that is supposed to run only once, since
> > > ext/background.js is already loaded by the background page itself. That
> > already
> > > makes things slower and using more memory than it would need. But when
code
> > that
> > > persists data runs twice, it will lead to serious issues. This is
currently
> > more
> > > a problem on Safari than on Chrome, but can also quickly lead to problems
on
> > > Chrome.
> >
> > Yes I agree, but it will run twice for the users with old browser version,
and
> > in future I think we will get rid of the createHTMLNotification.
> > And I'm pretty sure that this method will be deprecated for safari while
it's
> > deprecated from w3c Notificaition API.
> > http://www.w3.org/TR/notifications/
> >
> > > > also not sure how I can use
> > > > window.parent.ext in this case - the Script that uses ext api is loaded
in
> > > > desktop notification window.
> > >
> > > The idea was that it might be possible to access the window that created
the
> > > notification (the real background page in this case), via window.parent
> (like
> > it
> > > works inside frames). So you could just put "var ext = window.parent.ext;"
> at
> > > the top of notification.js, and remove the includes for ext/common.js and
> > > ext/background.js from notification.html. However I don't know if that
> works.
> > > But if not, there might be an other way to pass an object to the page
> running
> > > inside the notification.
> > >
> >
> > Unfortunately I've tried to test that, but didn't worked for me, so I think
> It's
> > not possible,
> > And seams like it's only accept one parameter and It's URL:
> >
>
http://www.chromium.org/developers/design-documents/desktop-notifications/api...
> > I'm not sure if the desktop notification has connection with extension.
> > Also no success by accessing background page using
> > chrome.extension.getBackgroundPage and chrome.extension.getViews.
> > So I don't see another way how we can access unfortunately.
>
> I've just realized that webkitNotification.createHTMLNotification doesn't
exist
> in the latest version of Chrome on Linux, Windows and Mac OS X, and also not
in
> the latest version of Safari. So either I missed something or
notification.html
> is actually never used?
As I've mentioned in my previous comment It's deprecated Method but it's still
used for old versions of browsers, I'm not sure from which version exactly it
was removed but I'v tested it with version 26 Chrome And it's still there I can
not get the newer version of chrome because of the Chrome policy that standalone
versions are stopped to appear in sites with trusted source, so I can't tell
from which exactly version it was removed.
So you are right, for most of people It's not available.
Sebastian Noack
2014/02/28 16:59:22
I'm very much in the favor of getting rid of that
On 2014/02/28 14:34:25, saroyanm wrote:
> On 2014/02/28 14:23:49, Sebastian Noack wrote:
> > On 2014/02/28 13:47:04, saroyanm wrote:
> > > On 2014/02/28 12:35:03, Sebastian Noack wrote:
> > > > On 2014/02/28 10:56:12, saroyanm wrote:
> > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote:
> > > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote:
> > > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > > > > > > > Guess I'm missing something, but what is this necessary for?
> > > > > > >
> > > > > > > This fixes notification.js which depends on ext.*
> > > > > >
> > > > > > No, background.js must be only executed once in the background page,
> > > > otherwise
> > > > > > things will break. Maybe you can use window.parent.ext?
> > > > >
> > > > > Not sure why the things will be broken, actually we are loading the
HTML
> > > page
> > > > > only in webkitNotifications.createHTMLNotification method and it's
open
> > html
> > > > > page in desktop notification window.
> > > >
> > > > This will run code twice, that is supposed to run only once, since
> > > > ext/background.js is already loaded by the background page itself. That
> > > already
> > > > makes things slower and using more memory than it would need. But when
> code
> > > that
> > > > persists data runs twice, it will lead to serious issues. This is
> currently
> > > more
> > > > a problem on Safari than on Chrome, but can also quickly lead to
problems
> on
> > > > Chrome.
> > >
> > > Yes I agree, but it will run twice for the users with old browser version,
> and
> > > in future I think we will get rid of the createHTMLNotification.
> > > And I'm pretty sure that this method will be deprecated for safari while
> it's
> > > deprecated from w3c Notificaition API.
> > > http://www.w3.org/TR/notifications/
> > >
> > > > > also not sure how I can use
> > > > > window.parent.ext in this case - the Script that uses ext api is
loaded
> in
> > > > > desktop notification window.
> > > >
> > > > The idea was that it might be possible to access the window that created
> the
> > > > notification (the real background page in this case), via window.parent
> > (like
> > > it
> > > > works inside frames). So you could just put "var ext =
window.parent.ext;"
> > at
> > > > the top of notification.js, and remove the includes for ext/common.js
and
> > > > ext/background.js from notification.html. However I don't know if that
> > works.
> > > > But if not, there might be an other way to pass an object to the page
> > running
> > > > inside the notification.
> > > >
> > >
> > > Unfortunately I've tried to test that, but didn't worked for me, so I
think
> > It's
> > > not possible,
> > > And seams like it's only accept one parameter and It's URL:
> > >
> >
>
http://www.chromium.org/developers/design-documents/desktop-notifications/api...
> > > I'm not sure if the desktop notification has connection with extension.
> > > Also no success by accessing background page using
> > > chrome.extension.getBackgroundPage and chrome.extension.getViews.
> > > So I don't see another way how we can access unfortunately.
> >
> > I've just realized that webkitNotification.createHTMLNotification doesn't
> exist
> > in the latest version of Chrome on Linux, Windows and Mac OS X, and also not
> in
> > the latest version of Safari. So either I missed something or
> notification.html
> > is actually never used?
>
> As I've mentioned in my previous comment It's deprecated Method but it's still
> used for old versions of browsers, I'm not sure from which version exactly it
> was removed but I'v tested it with version 26 Chrome And it's still there I
can
> not get the newer version of chrome because of the Chrome policy that
standalone
> versions are stopped to appear in sites with trusted source, so I can't tell
> from which exactly version it was removed.
> So you are right, for most of people It's not available.
I'm very much in the favor of getting rid of that code, now. It will make
problems rather sooner than later. And I don't think that supporting a minor
feature on an outdated browser if worse maintaining that code.
Sebastian Noack
2014/02/28 17:01:00
s/if worse/is worth/
On 2014/02/28 16:59:22, Sebastian Noack wrote:
> On 2014/02/28 14:34:25, saroyanm wrote:
> > On 2014/02/28 14:23:49, Sebastian Noack wrote:
> > > On 2014/02/28 13:47:04, saroyanm wrote:
> > > > On 2014/02/28 12:35:03, Sebastian Noack wrote:
> > > > > On 2014/02/28 10:56:12, saroyanm wrote:
> > > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote:
> > > > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote:
> > > > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > > > > > > > > Guess I'm missing something, but what is this necessary for?
> > > > > > > >
> > > > > > > > This fixes notification.js which depends on ext.*
> > > > > > >
> > > > > > > No, background.js must be only executed once in the background
page,
> > > > > otherwise
> > > > > > > things will break. Maybe you can use window.parent.ext?
> > > > > >
> > > > > > Not sure why the things will be broken, actually we are loading the
> HTML
> > > > page
> > > > > > only in webkitNotifications.createHTMLNotification method and it's
> open
> > > html
> > > > > > page in desktop notification window.
> > > > >
> > > > > This will run code twice, that is supposed to run only once, since
> > > > > ext/background.js is already loaded by the background page itself.
That
> > > > already
> > > > > makes things slower and using more memory than it would need. But when
> > code
> > > > that
> > > > > persists data runs twice, it will lead to serious issues. This is
> > currently
> > > > more
> > > > > a problem on Safari than on Chrome, but can also quickly lead to
> problems
> > on
> > > > > Chrome.
> > > >
> > > > Yes I agree, but it will run twice for the users with old browser
version,
> > and
> > > > in future I think we will get rid of the createHTMLNotification.
> > > > And I'm pretty sure that this method will be deprecated for safari while
> > it's
> > > > deprecated from w3c Notificaition API.
> > > > http://www.w3.org/TR/notifications/
> > > >
> > > > > > also not sure how I can use
> > > > > > window.parent.ext in this case - the Script that uses ext api is
> loaded
> > in
> > > > > > desktop notification window.
> > > > >
> > > > > The idea was that it might be possible to access the window that
created
> > the
> > > > > notification (the real background page in this case), via
window.parent
> > > (like
> > > > it
> > > > > works inside frames). So you could just put "var ext =
> window.parent.ext;"
> > > at
> > > > > the top of notification.js, and remove the includes for ext/common.js
> and
> > > > > ext/background.js from notification.html. However I don't know if that
> > > works.
> > > > > But if not, there might be an other way to pass an object to the page
> > > running
> > > > > inside the notification.
> > > > >
> > > >
> > > > Unfortunately I've tried to test that, but didn't worked for me, so I
> think
> > > It's
> > > > not possible,
> > > > And seams like it's only accept one parameter and It's URL:
> > > >
> > >
> >
>
http://www.chromium.org/developers/design-documents/desktop-notifications/api...
> > > > I'm not sure if the desktop notification has connection with extension.
> > > > Also no success by accessing background page using
> > > > chrome.extension.getBackgroundPage and chrome.extension.getViews.
> > > > So I don't see another way how we can access unfortunately.
> > >
> > > I've just realized that webkitNotification.createHTMLNotification doesn't
> > exist
> > > in the latest version of Chrome on Linux, Windows and Mac OS X, and also
not
> > in
> > > the latest version of Safari. So either I missed something or
> > notification.html
> > > is actually never used?
> >
> > As I've mentioned in my previous comment It's deprecated Method but it's
still
> > used for old versions of browsers, I'm not sure from which version exactly
it
> > was removed but I'v tested it with version 26 Chrome And it's still there I
> can
> > not get the newer version of chrome because of the Chrome policy that
> standalone
> > versions are stopped to appear in sites with trusted source, so I can't tell
> > from which exactly version it was removed.
> > So you are right, for most of people It's not available.
>
> I'm very much in the favor of getting rid of that code, now. It will make
> problems rather sooner than later. And I don't think that supporting a minor
> feature on an outdated browser if worse maintaining that code.
s/if worse/is worth/
saroyanm
2014/03/03 07:08:51
Sebastian I see your point and yes while It's not
On 2014/02/28 17:01:00, Sebastian Noack wrote:
> On 2014/02/28 16:59:22, Sebastian Noack wrote:
> > On 2014/02/28 14:34:25, saroyanm wrote:
> > > On 2014/02/28 14:23:49, Sebastian Noack wrote:
> > > > On 2014/02/28 13:47:04, saroyanm wrote:
> > > > > On 2014/02/28 12:35:03, Sebastian Noack wrote:
> > > > > > On 2014/02/28 10:56:12, saroyanm wrote:
> > > > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote:
> > > > > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote:
> > > > > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > > > > > > > > > Guess I'm missing something, but what is this necessary for?
> > > > > > > > >
> > > > > > > > > This fixes notification.js which depends on ext.*
> > > > > > > >
> > > > > > > > No, background.js must be only executed once in the background
> page,
> > > > > > otherwise
> > > > > > > > things will break. Maybe you can use window.parent.ext?
> > > > > > >
> > > > > > > Not sure why the things will be broken, actually we are loading
the
> > HTML
> > > > > page
> > > > > > > only in webkitNotifications.createHTMLNotification method and it's
> > open
> > > > html
> > > > > > > page in desktop notification window.
> > > > > >
> > > > > > This will run code twice, that is supposed to run only once, since
> > > > > > ext/background.js is already loaded by the background page itself.
> That
> > > > > already
> > > > > > makes things slower and using more memory than it would need. But
when
> > > code
> > > > > that
> > > > > > persists data runs twice, it will lead to serious issues. This is
> > > currently
> > > > > more
> > > > > > a problem on Safari than on Chrome, but can also quickly lead to
> > problems
> > > on
> > > > > > Chrome.
> > > > >
> > > > > Yes I agree, but it will run twice for the users with old browser
> version,
> > > and
> > > > > in future I think we will get rid of the createHTMLNotification.
> > > > > And I'm pretty sure that this method will be deprecated for safari
while
> > > it's
> > > > > deprecated from w3c Notificaition API.
> > > > > http://www.w3.org/TR/notifications/
> > > > >
> > > > > > > also not sure how I can use
> > > > > > > window.parent.ext in this case - the Script that uses ext api is
> > loaded
> > > in
> > > > > > > desktop notification window.
> > > > > >
> > > > > > The idea was that it might be possible to access the window that
> created
> > > the
> > > > > > notification (the real background page in this case), via
> window.parent
> > > > (like
> > > > > it
> > > > > > works inside frames). So you could just put "var ext =
> > window.parent.ext;"
> > > > at
> > > > > > the top of notification.js, and remove the includes for
ext/common.js
> > and
> > > > > > ext/background.js from notification.html. However I don't know if
that
> > > > works.
> > > > > > But if not, there might be an other way to pass an object to the
page
> > > > running
> > > > > > inside the notification.
> > > > > >
> > > > >
> > > > > Unfortunately I've tried to test that, but didn't worked for me, so I
> > think
> > > > It's
> > > > > not possible,
> > > > > And seams like it's only accept one parameter and It's URL:
> > > > >
> > > >
> > >
> >
>
http://www.chromium.org/developers/design-documents/desktop-notifications/api...
> > > > > I'm not sure if the desktop notification has connection with
extension.
> > > > > Also no success by accessing background page using
> > > > > chrome.extension.getBackgroundPage and chrome.extension.getViews.
> > > > > So I don't see another way how we can access unfortunately.
> > > >
> > > > I've just realized that webkitNotification.createHTMLNotification
doesn't
> > > exist
> > > > in the latest version of Chrome on Linux, Windows and Mac OS X, and also
> not
> > > in
> > > > the latest version of Safari. So either I missed something or
> > > notification.html
> > > > is actually never used?
> > >
> > > As I've mentioned in my previous comment It's deprecated Method but it's
> still
> > > used for old versions of browsers, I'm not sure from which version exactly
> it
> > > was removed but I'v tested it with version 26 Chrome And it's still there
I
> > can
> > > not get the newer version of chrome because of the Chrome policy that
> > standalone
> > > versions are stopped to appear in sites with trusted source, so I can't
tell
> > > from which exactly version it was removed.
> > > So you are right, for most of people It's not available.
> >
> > I'm very much in the favor of getting rid of that code, now. It will make
> > problems rather sooner than later. And I don't think that supporting a minor
> > feature on an outdated browser if worse maintaining that code.
>
> s/if worse/is worth/
Sebastian I see your point and yes while It's not available for newer version of
chrome and safari, but there is a discussion where me and Thomas agreed to use
createHTMLNotification to support people with old browser version:
http://codereview.adblockplus.org/6098518317989888/#msg1
So would also like to know what Thomas think about this.
@Thomas what you think about removing case for createHTMLNotification support
for old browsers, while as far as I understood from Sebastian notes that this
will be a problem for safari (loading scripts in notification.html) and it worth
to use createNotification for old browser versions as well and maybe remove
notification.html file from revision control while we will no more use it ?
Thomas Greiner
2014/03/04 09:28:15
We should not remove the case for createHTMLNotifi
On 2014/03/03 07:08:51, saroyanm wrote:
> On 2014/02/28 17:01:00, Sebastian Noack wrote:
> > On 2014/02/28 16:59:22, Sebastian Noack wrote:
> > > On 2014/02/28 14:34:25, saroyanm wrote:
> > > > On 2014/02/28 14:23:49, Sebastian Noack wrote:
> > > > > On 2014/02/28 13:47:04, saroyanm wrote:
> > > > > > On 2014/02/28 12:35:03, Sebastian Noack wrote:
> > > > > > > On 2014/02/28 10:56:12, saroyanm wrote:
> > > > > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote:
> > > > > > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote:
> > > > > > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote:
> > > > > > > > > > > Guess I'm missing something, but what is this necessary
for?
> > > > > > > > > >
> > > > > > > > > > This fixes notification.js which depends on ext.*
> > > > > > > > >
> > > > > > > > > No, background.js must be only executed once in the background
> > page,
> > > > > > > otherwise
> > > > > > > > > things will break. Maybe you can use window.parent.ext?
> > > > > > > >
> > > > > > > > Not sure why the things will be broken, actually we are loading
> the
> > > HTML
> > > > > > page
> > > > > > > > only in webkitNotifications.createHTMLNotification method and
it's
> > > open
> > > > > html
> > > > > > > > page in desktop notification window.
> > > > > > >
> > > > > > > This will run code twice, that is supposed to run only once, since
> > > > > > > ext/background.js is already loaded by the background page itself.
> > That
> > > > > > already
> > > > > > > makes things slower and using more memory than it would need. But
> when
> > > > code
> > > > > > that
> > > > > > > persists data runs twice, it will lead to serious issues. This is
> > > > currently
> > > > > > more
> > > > > > > a problem on Safari than on Chrome, but can also quickly lead to
> > > problems
> > > > on
> > > > > > > Chrome.
> > > > > >
> > > > > > Yes I agree, but it will run twice for the users with old browser
> > version,
> > > > and
> > > > > > in future I think we will get rid of the createHTMLNotification.
> > > > > > And I'm pretty sure that this method will be deprecated for safari
> while
> > > > it's
> > > > > > deprecated from w3c Notificaition API.
> > > > > > http://www.w3.org/TR/notifications/
> > > > > >
> > > > > > > > also not sure how I can use
> > > > > > > > window.parent.ext in this case - the Script that uses ext api is
> > > loaded
> > > > in
> > > > > > > > desktop notification window.
> > > > > > >
> > > > > > > The idea was that it might be possible to access the window that
> > created
> > > > the
> > > > > > > notification (the real background page in this case), via
> > window.parent
> > > > > (like
> > > > > > it
> > > > > > > works inside frames). So you could just put "var ext =
> > > window.parent.ext;"
> > > > > at
> > > > > > > the top of notification.js, and remove the includes for
> ext/common.js
> > > and
> > > > > > > ext/background.js from notification.html. However I don't know if
> that
> > > > > works.
> > > > > > > But if not, there might be an other way to pass an object to the
> page
> > > > > running
> > > > > > > inside the notification.
> > > > > > >
> > > > > >
> > > > > > Unfortunately I've tried to test that, but didn't worked for me, so
I
> > > think
> > > > > It's
> > > > > > not possible,
> > > > > > And seams like it's only accept one parameter and It's URL:
> > > > > >
> > > > >
> > > >
> > >
> >
>
http://www.chromium.org/developers/design-documents/desktop-notifications/api...
> > > > > > I'm not sure if the desktop notification has connection with
> extension.
> > > > > > Also no success by accessing background page using
> > > > > > chrome.extension.getBackgroundPage and chrome.extension.getViews.
> > > > > > So I don't see another way how we can access unfortunately.
> > > > >
> > > > > I've just realized that webkitNotification.createHTMLNotification
> doesn't
> > > > exist
> > > > > in the latest version of Chrome on Linux, Windows and Mac OS X, and
also
> > not
> > > > in
> > > > > the latest version of Safari. So either I missed something or
> > > > notification.html
> > > > > is actually never used?
> > > >
> > > > As I've mentioned in my previous comment It's deprecated Method but it's
> > still
> > > > used for old versions of browsers, I'm not sure from which version
exactly
> > it
> > > > was removed but I'v tested it with version 26 Chrome And it's still
there
> I
> > > can
> > > > not get the newer version of chrome because of the Chrome policy that
> > > standalone
> > > > versions are stopped to appear in sites with trusted source, so I can't
> tell
> > > > from which exactly version it was removed.
> > > > So you are right, for most of people It's not available.
> > >
> > > I'm very much in the favor of getting rid of that code, now. It will make
> > > problems rather sooner than later. And I don't think that supporting a
minor
> > > feature on an outdated browser if worse maintaining that code.
> >
> > s/if worse/is worth/
>
> Sebastian I see your point and yes while It's not available for newer version
of
> chrome and safari, but there is a discussion where me and Thomas agreed to use
> createHTMLNotification to support people with old browser version:
> http://codereview.adblockplus.org/6098518317989888/#msg1
>
> So would also like to know what Thomas think about this.
> @Thomas what you think about removing case for createHTMLNotification support
> for old browsers, while as far as I understood from Sebastian notes that this
> will be a problem for safari (loading scripts in notification.html) and it
worth
> to use createNotification for old browser versions as well and maybe remove
> notification.html file from revision control while we will no more use it ?
We should not remove the case for createHTMLNotification because I think we
should always aim for the best user experience even if not a lot of users will
see it anymore. If it turns out to be impossible to implement in Safari (which I
doubt since that would've come up when we ported it over from Chrome) we could
think of making this case Chrome-only.
Sebastian Noack
2014/03/05 10:45:49
createHTMLNotification isn't available for any ver
On 2014/03/03 07:08:51, saroyanm wrote:
> Sebastian I see your point and yes while It's not available for newer version
of
> chrome and safari
createHTMLNotification isn't available for any version of Safari.
> while as far as I understood from Sebastian notes that this
> will be a problem for safari
It turned out by now, that Safari isn't actually affected, because the absence
of createHTMLNotification. However as explained above, including
ext/background.js a second time, is also problematic on Chrome.
> to use createNotification for old browser versions as well
Isn't createNotification useless for this case, because it doesn't support UI
elements like buttons?
On 2014/03/04 09:28:15, Thomas Greiner wrote:
> we could think of making this case Chrome-only.
Since notification.html is only used if createHTMLNotification exist, this is
already Chrome-only. However it might be a good idea to don't even include
notification.html and notification.js in the Safari build. Feel free to do that.
Thomas Greiner
2014/03/05 11:30:51
It's still better than window.confirm and it can h
> > to use createNotification for old browser versions as well
> Isn't createNotification useless for this case, because it doesn't support UI
> elements like buttons?
It's still better than window.confirm and it can have an onclick listener.
However, for the new question-type that I'm working on, createNotification will
not be used.
> Since notification.html is only used if createHTMLNotification exist, this is
> already Chrome-only. However it might be a good idea to don't even include
> notification.html and notification.js in the Safari build. Feel free to do
that.
I agree. As you said on IRC, moving the notification files into ext/ and adding
it to metadata.chrome (possibly also metadata.opera if createHTMLNotification
has been available in any of the Opera versions) should do the job.
saroyanm
2014/03/05 11:33:25
chrome.notifications Are stable since chrome v28 s
> > to use createNotification for old browser versions as well
> Isn't createNotification useless for this case, because it doesn't support UI
> elements like buttons?
chrome.notifications Are stable since chrome v28 so we are not sure from which
exact chrome version webkitNotifications.createHTMLNotification removed from
chrome, so we are using webkitNotifications.createNotification to be sure that
the notification will be shown in case both previous are not available.
> > we could think of making this case Chrome-only.
> Since notification.html is only used if createHTMLNotification exist, this is
> already Chrome-only. However it might be a good idea to don't even include
> notification.html and notification.js in the Safari build. Feel free to do
that.
So if it's not included in any version of safari that make sense. Thanks for
pointing that.
| |
| 30 <script src="notification.js"></script> | 31 <script src="notification.js"></script> |
| 31 </head> | 32 </head> |
| 32 <body> | 33 <body> |
| 33 <img src="icons/abp-48.png"> | 34 <img src="icons/abp-48.png"> |
| 34 <div id="notification"> | 35 <div id="notification"> |
| 35 <h1 id="notification-title"></h1> | 36 <h1 id="notification-title"></h1> |
| 36 <p id="notification-message"></p> | 37 <p id="notification-message"></p> |
| 37 </div> | 38 </div> |
| 38 </body> | 39 </body> |
| 39 </html> | 40 </html> |
| LEFT | RIGHT |
« metadata.chrome ('k')
|
no next file »
|
Toggle Intra-line Diffs ('i')
|
Expand Comments ('e')
|
Collapse Comments ('c')
|
Toggle Comments ('s')
