Fixed reload issue in Chrome

Fixed reload issue in Chrome

[Sebastian Noack, 2013-11-15 13:16:11]

There is no event in Safari that notifies about that the page started to load.
So I used the beforeNavigate event which is similar, but only fires when the URL
changed too, and implemented that behavior also in Chrome for consistence.

However it turned out that this isn't sufficient for our use case, because of we
have to update the icon and context menu every time the page loads, not only
when it is about to navigate to a different URL.

So I have replaced onBeforeNavigate with onLoading and emulated it in Safari by
sending a message to the background page from the injected script, which is
executed at the very beginning, every time the page (re)loads.

[Felix Dahlke, 2013-11-15 13:28:37]

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
File safari/background.js (right):

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
safari/background.js:47: if (event.name == "loading" && event.message ==
event.target.url)
Shouldn't we check if event.target is an instance of SafariBrowserTab here, as
for the other events?

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
safari/content.js:18: safari.self.tab.dispatchMessage("loading",
document.location.href);
Why not move this into the IIFE?

[Sebastian Noack, 2013-11-15 13:45:29]

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
File safari/background.js (right):

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
safari/background.js:47: if (event.name == "loading" && event.message ==
event.target.url)
On 2013/11/15 13:28:38, Felix H. Dahlke wrote:
> Shouldn't we check if event.target is an instance of SafariBrowserTab here, as
> for the other events?

Shouldn't be required here. Only tabs can send messages to the background page.
While the other events can also have an instance of SafariBrowserWindow in
event.target, and therefore require that check.

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
safari/content.js:18: safari.self.tab.dispatchMessage("loading",
document.location.href);
On 2013/11/15 13:28:38, Felix H. Dahlke wrote:
> Why not move this into the IIFE?

As you wish. I didn't put it into the IIFE in the first place, because of that
code doesn't define variables and the only purpose of the IIFE below is to hide
variables, that don't need to be global.

[Felix Dahlke, 2013-11-15 13:48:49]

LGTM

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
File safari/content.js (right):

http://codereview.adblockplus.org/6369485355220992/diff/5629499534213120/safa...
safari/content.js:18: safari.self.tab.dispatchMessage("loading",
document.location.href);
On 2013/11/15 13:45:29, sebastian wrote:
> On 2013/11/15 13:28:38, Felix H. Dahlke wrote:
> > Why not move this into the IIFE?
> 
> As you wish. I didn't put it into the IIFE in the first place, because of that
> code doesn't define variables and the only purpose of the IIFE below is to
hide
> variables, that don't need to be global.

Oh, I actually meant "under" the IIFE (whether it's in there or not, we
typically have definitions first, then code.)

But it actually doesn't matter, it's up to you.