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

fix($sniffer): fix history sniffing in Chrome Packaged Apps #13945

Closed
wants to merge 4 commits into from
Closed

fix($sniffer): fix history sniffing in Chrome Packaged Apps #13945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
566b31c
test($sniffer): make the structure and layout of tests more consistent
gkalpak Feb 4, 2016
87ace90
fix($sniffer): fix history sniffing in Chrome Packaged Apps
gkalpak Feb 4, 2016
6cab4ad
fixup 1
gkalpak Feb 4, 2016
047587d
fixup 2
gkalpak Feb 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 8 additions & 9 deletions src/ng/browser.js
View file
Open in desktop
Expand Up @@ -87,7 +87,14 @@ function Browser(window, document, $log, $sniffer) {
var cachedState, lastHistoryState,
lastBrowserUrl = location.href,
baseElement = document.find('base'),
pendingLocation = null;
pendingLocation = null,
getCurrentState = !$sniffer.history ? noop : function getCurrentState() {
try {
return history.state;
} catch (e) {
// MSIE can reportedly throw when there is no state (UNCONFIRMED).
}
};

cacheState();
lastHistoryState = cachedState;
Expand Down Expand Up @@ -195,14 +202,6 @@ function Browser(window, document, $log, $sniffer) {
fireUrlChange();
}

function getCurrentState() {
try {
return history.state;
} catch (e) {
// MSIE can reportedly throw when there is no state (UNCONFIRMED).
}
}

// This variable should be used *only* inside the cacheState function.
var lastCachedState = null;
function cacheState() {
Expand Down
6 changes: 5 additions & 1 deletion src/ng/sniffer.js
View file
Open in desktop
Expand Up @@ -17,6 +17,10 @@
function $SnifferProvider() {
this.$get = ['$window', '$document', function($window, $document) {
var eventSupport = {},
// Chrome Packaged Apps are not allowed to access `history.pushState`. They can be detected by
// the presence of `chrome.app.runtime` (see https://developer.chrome.com/apps/api_index)
isChromePackagedApp = $window.chrome && $window.chrome.app && $window.chrome.app.runtime,
hasHistoryPushState = !isChromePackagedApp && $window.history && $window.history.pushState,
android =
toInt((/android (\d+)/.exec(lowercase(($window.navigator || {}).userAgent)) || [])[1]),
boxee = /Boxee/i.test(($window.navigator || {}).userAgent),
Expand Down Expand Up @@ -61,7 +65,7 @@ function $SnifferProvider() {
// so let's not use the history API also
// We are purposefully using `!(android < 4)` to cover the case when `android` is undefined
// jshint -W018
history: !!($window.history && $window.history.pushState && !(android < 4) && !boxee),
history: !!(hasHistoryPushState && !(android < 4) && !boxee),
Copy link
Member

Choose a reason for hiding this comment

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

why move the $window.history.... checks out of this expression? Is Chrome more special that android < 4 or boxee?

I would expect it is clearer to simply change this to:

!!(!isChromePackagedApp && $window.history && $window.history.pushState && !(android < 4) && !boxee),

Copy link
Member Author

Choose a reason for hiding this comment

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

Is Chrome more special that android < 4 or boxee

Yeah, Chrome is actually very close to my heart 😛

I put it this way for readability (avoiding !!(! and hitting the 100 chars per line limit, thus having to wrap).
I'm fine having it either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Plus, if I have isChromePackagedApp and hasHistoryPushState close together, it's easier for reference wrt to the comment explaining why Chrome Packaged Apps need special handling when checking for the existence of history.pushState.

Copy link
Member

Choose a reason for hiding this comment

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

OK, how about:

// Chrome Packaged Apps are not allowed to access `history.pushState`. They can be detected by
// the presence of `chrome.app.runtime` (see https://developer.chrome.com/apps/api_index)
isChromePackagedApp = $window.chrome && $window.chrome.app && $window.chrome.app.runtime,
hasHistoryPushState = $window.history && $window.history.pushState,

then later:

history: !isChromePackagedApp && !!hasHistoryPushState && !(android < 4) && !boxee,

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is that we shouldn't even try to touch history.pushState in packaged apps (or there'll be errors).
That's why we need the !isChromePackagedApp short-circuiting at the beginning of hasHistoryPushState.

Copy link
Member

Choose a reason for hiding this comment

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

oh :-(

// jshint +W018
hasEvent: function(event) {
// IE9 implements 'input' event it's so fubared that we rather pretend that it doesn't have
Expand Down
23 changes: 23 additions & 0 deletions test/ng/browserSpecs.js
View file
Open in desktop
Expand Up @@ -537,9 +537,32 @@ describe('browser', function() {
currentHref = fakeWindow.location.href;
});

it('should not access `history.state` when `$sniffer.history` is false', function() {
// In the context of a Chrome Packaged App, although `history.state` is present, accessing it
// is not allowed and logs an error in the console. We should not try to access
// `history.state` in contexts where `$sniffer.history` is false.

var historyStateAccessed = false;
var mockSniffer = {histroy: false};
var mockWindow = new MockWindow();

var _state = mockWindow.history.state;
Object.defineProperty(mockWindow.history, 'state', {
get: function() {
historyStateAccessed = true;
return _state;
}
});

var browser = new Browser(mockWindow, fakeDocument, fakeLog, mockSniffer);

expect(historyStateAccessed).toBe(false);
});

describe('in IE', runTests({msie: true}));
describe('not in IE', runTests({msie: false}));


function runTests(options) {
return function() {
beforeEach(function() {
Expand Down