Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also .

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also .
Choose a Base Repository
hypothesis/h
40a/h
AFDudley/h
BigBlueHat/h
BinaryStars/h
CCH543/h
Cinemacloud/h
Ericgood/h
FTG-003/h
Forethinker/h
GratefulTony/h
HGldJ1966/h
JJediny/h
John-Williams/h
Laurian/h
LittleFancy/h
MattyQ/h
Mishkin2015/h
RichardLitt/h
Staffan1/h
SteelWagstaff/h
TowerBR/h
VanyTang/h
abigailricarte/h
ackermann/h
alecchap/h
alesarrett/h
alexsegura/h
almereyda/h
alon/h
andzi/h
angelicxsoul/h
ansmoh/h
apurvajalit/h
arjunvasan/h
asdevor/h
bZichett/h
badgettrg/Webmarks
balmas/h
balupton/h
bbarker/h
bennlich/h
benthor/h
blakewest/h
bogste/h
bradparks/h
brittanystoroz/h
buiquangchien/h
cdchapman/h
charblanc/h
chowsamihq/h
chr7stos/Webmarks
chrber/h
chrismPssina/h
christinaphamAD/h
cmbirk/h
codeaudit/h
coolcool21/h
cove/h
csillag/h
danjimilk/h
dannyhope/h
daredream/h
davidmcclure/h
dennisplucinik/h
dezynetechnologies/h
diegodlh/h
djcun95/h
donsequitur/h
edsu/h
eiro10/h
emckean/h
ercchy/h
eshellman/h
fangang123/h
fchasen/h
fcrimins/h
fhirsch/h
ficolo/h
fragkopoulos/h
gauravkeerthi/h
geass/h
gergely-ujvari/h
gitter-badger/h
gnott/h
gobengo/h
gorinovic/h
gus3000/h
hashin/h
helemaalbigt/h
hmstepanek/h
hwasiti/h
hylhero/h
hyperstudio/h
iHDeveloper/h
imeysam/h
jackspaceBerkeley/h
jarey/h
jasdeep/h
jason790/h
jasonzou/j
jazahn/h
jccr/h
jean/h
jeka57/h
jeremydean/h
jermnelson/h
jibe-b/h
jnishiyama/h
jojksd/h
jpadilla/h
jtremback/h
judell/h
juli-so/h
kabacs/h
karissa/h
kaushikvijay/h
kaydoh/h
kill4uk/h
klopiinas/h
klrkdekira/h
koulihong311/h
krassif/h
krstnkngs/h
leoqmp/h
linhua55/h
lucadealfaro/h
lyspooner/h
lyzadanger/h
m1yag1/h
magee/h
mambocab/h
manunymous/h
maraino/h
mari-ja/h
markbarratt/h
martinq/h
mbbaig/h
mcarv63/h
meawoppl/h
meflyup/h
metasj/h
mgasner/h
mgax/h
mollycr/h
mrchrisadams/h
mrienstra/h
mshavlovsky/h
muddasani/h
nagyist/hyphothesis-h
nagyistoce/hypothesis-h
nanxio/h
neozhangthe1/h
ningyifan/h
nkingsley/h
nlholdem/h
nlisgo/h
noscripter/h
nshkuro/h
odnodn/h
oliversauter/h
openbizgit/h
opengovfoundation/h
openstax/hypothesis-server
ouroboros8/h
pablomarti/h
pamo/h
philipn/h
philschatz/h
pinballwonder/h
plainspace/h
raowl/h
rickyhan/h
rmoorman/h
rmtsukuru/h
robertknight/h
rowhit/h
rsarxiv/h
saakaifoundry/h
samrose/h
scharf/h
shepazu/h
sherah/h
shofheinz/h
soapdog/h
ssin122/test-h
st-fresh/h
stuk88/h
sylvanmist/h
tetratorus/h
tilgovi/h
tomnar/h
trivenews/h
truthadjustr/h
utngz/h
voidfiles/h
wenchen/h
yargevad/h
yumatch/h
zshen777/h
Nothing to show
Choose a Head Repository
hypothesis/h
40a/h
AFDudley/h
BigBlueHat/h
BinaryStars/h
CCH543/h
Cinemacloud/h
Ericgood/h
FTG-003/h
Forethinker/h
GratefulTony/h
HGldJ1966/h
JJediny/h
John-Williams/h
Laurian/h
LittleFancy/h
MattyQ/h
Mishkin2015/h
RichardLitt/h
Staffan1/h
SteelWagstaff/h
TowerBR/h
VanyTang/h
abigailricarte/h
ackermann/h
alecchap/h
alesarrett/h
alexsegura/h
almereyda/h
alon/h
andzi/h
angelicxsoul/h
ansmoh/h
apurvajalit/h
arjunvasan/h
asdevor/h
bZichett/h
badgettrg/Webmarks
balmas/h
balupton/h
bbarker/h
bennlich/h
benthor/h
blakewest/h
bogste/h
bradparks/h
brittanystoroz/h
buiquangchien/h
cdchapman/h
charblanc/h
chowsamihq/h
chr7stos/Webmarks
chrber/h
chrismPssina/h
christinaphamAD/h
cmbirk/h
codeaudit/h
coolcool21/h
cove/h
csillag/h
danjimilk/h
dannyhope/h
daredream/h
davidmcclure/h
dennisplucinik/h
dezynetechnologies/h
diegodlh/h
djcun95/h
donsequitur/h
edsu/h
eiro10/h
emckean/h
ercchy/h
eshellman/h
fangang123/h
fchasen/h
fcrimins/h
fhirsch/h
ficolo/h
fragkopoulos/h
gauravkeerthi/h
geass/h
gergely-ujvari/h
gitter-badger/h
gnott/h
gobengo/h
gorinovic/h
gus3000/h
hashin/h
helemaalbigt/h
hmstepanek/h
hwasiti/h
hylhero/h
hyperstudio/h
iHDeveloper/h
imeysam/h
jackspaceBerkeley/h
jarey/h
jasdeep/h
jason790/h
jasonzou/j
jazahn/h
jccr/h
jean/h
jeka57/h
jeremydean/h
jermnelson/h
jibe-b/h
jnishiyama/h
jojksd/h
jpadilla/h
jtremback/h
judell/h
juli-so/h
kabacs/h
karissa/h
kaushikvijay/h
kaydoh/h
kill4uk/h
klopiinas/h
klrkdekira/h
koulihong311/h
krassif/h
krstnkngs/h
leoqmp/h
linhua55/h
lucadealfaro/h
lyspooner/h
lyzadanger/h
m1yag1/h
magee/h
mambocab/h
manunymous/h
maraino/h
mari-ja/h
markbarratt/h
martinq/h
mbbaig/h
mcarv63/h
meawoppl/h
meflyup/h
metasj/h
mgasner/h
mgax/h
mollycr/h
mrchrisadams/h
mrienstra/h
mshavlovsky/h
muddasani/h
nagyist/hyphothesis-h
nagyistoce/hypothesis-h
nanxio/h
neozhangthe1/h
ningyifan/h
nkingsley/h
nlholdem/h
nlisgo/h
noscripter/h
nshkuro/h
odnodn/h
oliversauter/h
openbizgit/h
opengovfoundation/h
openstax/hypothesis-server
ouroboros8/h
pablomarti/h
pamo/h
philipn/h
philschatz/h
pinballwonder/h
plainspace/h
raowl/h
rickyhan/h
rmoorman/h
rmtsukuru/h
robertknight/h
rowhit/h
rsarxiv/h
saakaifoundry/h
samrose/h
scharf/h
shepazu/h
sherah/h
shofheinz/h
soapdog/h
ssin122/test-h
st-fresh/h
stuk88/h
sylvanmist/h
tetratorus/h
tilgovi/h
tomnar/h
trivenews/h
truthadjustr/h
utngz/h
voidfiles/h
wenchen/h
yargevad/h
yumatch/h
zshen777/h
Nothing to show
  • 1 commit
  • 1 file changed
  • 2 commit comments
  • 1 contributor
Commits on Jan 06, 2016
Improve PDF viewer detection in Chrome
Rather than relying on the tab URL containing '.pdf',
check whether the current tab is displaying Chrome's native
PDF viewer and open Hypothesis' PDF.js based viewer in that
case since we the sidebar cannot be injected into the native viewer.

We detect the Chrome PDF viewer by checking for an
<embed ... type="application/pdf"> tag which appears at the top
of the body.

If the user is using a PDF.js or other HTML-based viewer,
then Hypothesis can work with that directly.

An alternative approach would be to detect the mime type of the
loaded document but that requires intercepting all page
load requests in order to read the Content-Type header, which
is not available after the page has loaded OR firing a network
request.

See also this upstream issue: https://code.google.com/p/chromium/issues/detail?id=242575
Showing with 41 additions and 8 deletions.
  1. +41 −8 h/browser/chrome/lib/sidebar-injector.js
@@ -6,6 +6,27 @@ var messageTypes = require('./message-types');
var settings = require('./settings');
var util = require('./util');
var CONTENT_TYPE_PDF = 'PDF';
// a function which is executed as a content script
// to determine the type of content being displayed in a tab
function detectContentType() {
// check if this is the Chrome PDF viewer
if (document.querySelector('embed[type="application/pdf"]')) {
return {
type: 'PDF',
};
} else {
return {
type: 'HTML',
};
}
}
function toIIFEString(fn) {
return '(' + fn.toString() + ')()';
}
/* The SidebarInjector is used to deploy and remove the Hypothesis sidebar
* from tabs. It also deals with loading PDF documents into the PDF.js viewer
* when applicable.
@@ -75,8 +96,12 @@ function SidebarInjector(chromeTabs, dependencies) {
return PDF_VIEWER_URL + '?file=' + encodeURIComponent(url);
}
function isPDFURL(url) {
return url.toLowerCase().indexOf('.pdf') > 0;
function detectTabContentType(tab) {
return executeScriptFn(tab.id, {
code: toIIFEString(detectContentType)
}).then(function (frameResults) {
return frameResults[0].type;
});
}
function isPDFViewerURL(url) {
@@ -95,15 +120,23 @@ function SidebarInjector(chromeTabs, dependencies) {
}
function injectIntoLocalDocument(tab) {
if (isPDFURL(tab.url)) {
return injectIntoLocalPDF(tab);
} else {
return Promise.reject(new errors.LocalFileError('Local non-PDF files are not supported'));
}
return detectTabContentType(tab).then(function (type) {
if (type === CONTENT_TYPE_PDF) {
return injectIntoLocalPDF(tab);
} else {
return Promise.reject(new errors.LocalFileError('Local non-PDF files are not supported'));
}
});
}
function injectIntoRemoteDocument(tab) {
return isPDFURL(tab.url) ? injectIntoPDF(tab) : injectIntoHTML(tab);
return detectTabContentType(tab).then(function (type) {
if (type === CONTENT_TYPE_PDF) {
return injectIntoPDF(tab);
} else {
return injectIntoHTML(tab);
}
});
}
function injectIntoPDF(tab) {

Showing you all comments on commits in this comparison.

@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Jan 6, 2016

Contributor

I think the Content-Type header is available after the page has loaded, for what it's worth (or if it's not we would just listen for it on page load all the time, whether h is active or not, and store it).

Will this (PDF viewer detection) technique work when we want to share code between extensions for different browsers? We'd have to add code to detect each different browser's PDF viewer, right?

If you need to inject a content script into the page in order to detect the PDF viewer, doesn't that mean you need the <all_urls> permission to be able to inject content scripts into any page? (The same permission that we need to look at Content-Type.)

Contributor

seanh commented on 0c7d55c Jan 6, 2016

I think the Content-Type header is available after the page has loaded, for what it's worth (or if it's not we would just listen for it on page load all the time, whether h is active or not, and store it).

Will this (PDF viewer detection) technique work when we want to share code between extensions for different browsers? We'd have to add code to detect each different browser's PDF viewer, right?

If you need to inject a content script into the page in order to detect the PDF viewer, doesn't that mean you need the <all_urls> permission to be able to inject content scripts into any page? (The same permission that we need to look at Content-Type.)

@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Jan 6, 2016

Contributor

If you need to inject a content script into the page in order to detect the PDF viewer,
doesn't that mean you need the <all_urls> permission to be able to inject content scripts into any page?

Not if you have the Chrome implicit "activeTab" permission

Will this (PDF viewer detection) technique work when we want to share code between
extensions for different browsers? We'd have to add code to detect each different browser's PDF viewer, right?

In Firefox we can definitely detect PDF.js, although we might not need to replace the viewer if it is already HTML based. I'm unsure about Safari and Edge. My inclination is that we can cross that bridge when we come to it - and possibly take a different approach in other browsers if necessary.

Contributor

robertknight commented on 0c7d55c Jan 6, 2016

If you need to inject a content script into the page in order to detect the PDF viewer,
doesn't that mean you need the <all_urls> permission to be able to inject content scripts into any page?

Not if you have the Chrome implicit "activeTab" permission

Will this (PDF viewer detection) technique work when we want to share code between
extensions for different browsers? We'd have to add code to detect each different browser's PDF viewer, right?

In Firefox we can definitely detect PDF.js, although we might not need to replace the viewer if it is already HTML based. I'm unsure about Safari and Edge. My inclination is that we can cross that bridge when we come to it - and possibly take a different approach in other browsers if necessary.