Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($snifferProvider): allow history for nwjs applications #15633

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/ng/sniffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@
function $SnifferProvider() {
this.$get = ['$window', '$document', function($window, $document) {
var eventSupport = {},
isNw = $window.nw && $window.nw.process,
// Chrome Packaged Apps are not allowed to access `history.pushState`.
// If not sandboxed, they can be detected by the presence of `chrome.app.runtime`
// (see https://developer.chrome.com/apps/api_index). If sandboxed, they can be detected by
// the presence of an extension runtime ID and the absence of other Chrome runtime APIs
// (see https://developer.chrome.com/apps/manifest/sandbox).
isChromePackagedApp =
!isNw &&
$window.chrome &&
($window.chrome.app && $window.chrome.app.runtime ||
!$window.chrome.app && $window.chrome.runtime && $window.chrome.runtime.id),
Expand Down
19 changes: 19 additions & 0 deletions test/ng/snifferSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,25 @@ describe('$sniffer', function() {
});


it('should be true if running inside nw.js', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this test below should be false on Chrome Packaged Apps (or even incorporate it into that one).

Copy link
Contributor Author

@frederikprijck frederikprijck Jan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it there to make sure both tests verifying when history should be true are together. But I can move it a bit lower. However, I wouldn't incorporate it into the same test that verifies history being false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://github.com/angular/angular.js/pull/15633/files#r97973838, isNw is just a subcase of detecting whether something is a Chrome Packaged App. In that sense, it makes sense to have it incorporated into the Chrome Packaged App test.

In other words, we are not doing anything special if it is NW. We have just refined our Chrome Packaged App-detection check.

Copy link
Contributor Author

@frederikprijck frederikprijck Jan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the intend of this PR was to allow history in NW apps, I'd say it's a good idea to have a test that reflects that change and test it (despite the actual technical implementation).

Incorporate this into the test for Chromed Packaged App sounds odd to me, as NW apps are not actual Chromed Packaged Apps (correct me if I'm wrong here).

Apart from that, I'll be happy to change the test and try to incorporate it into the one you're talking about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just a suggestion - what makes more sense to me. But I don't feel too strongly about it, so feel free to do it the way it makes more sense to you.

As the intend of this PR was to allow history in NW apps

I see it more like restoring the previous behavior of not blocking history on NW apps, which was incorrectly changed while trying to disable it for Chrome PAckaged Apps.

But like I said, totally up to you. As long as the test is there (and is testing the correct thing), it doesn't have t be incorporated into the other one.

var mockWindow = {
nw: {
process: noop
},
history: {
pushState: noop
},
chrome: {
app: {
runtime: noop
}
}
};

expect(sniffer(mockWindow).history).toBe(true);
});


it('should be false if history or pushState not defined', function() {
expect(sniffer({}).history).toBe(false);
expect(sniffer({history: {}}).history).toBe(false);
Expand Down