Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore compatibility with Chrome 38 #2109

Merged
merged 1 commit into from
Jan 22, 2021
Merged

Restore compatibility with Chrome 38 #2109

merged 1 commit into from
Jan 22, 2021

Conversation

hmdne
Copy link
Member

@hmdne hmdne commented Jul 19, 2020

This is an ancient browser, I know, but it's the basis of many smartphone default browsers.

@ggrossetie
Copy link
Member

defineProperty is supported since Chrome 5 according to MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty#Browser_compatibility

So I'm not sure what you are trying to solve... Do you have a stacktrace? But in any case, I don't think that we want to support a 6 years old browser.

@hmdne
Copy link
Member Author

hmdne commented Sep 30, 2020

This browser is used as WebView on some older (though still in use) smartphones. It's not about defineProperty, it's about just this particular use of defineProperty. I wouldn't call it a full support, just a prevention of a full failure. I can get a stacktrace, sure.

https://hastebin.com/emimopifar

Merging this little patch will reduce my local delta a little bit :)

@hmdne
Copy link
Member Author

hmdne commented Sep 30, 2020

By the way it was tested on 53593f7

@hmdne
Copy link
Member Author

hmdne commented Sep 30, 2020

And it all boils down to this:
Object.defineProperty(function(){}, "length", {value: 10})

@hmdne
Copy link
Member Author

hmdne commented Jan 2, 2021

By the way, to get a browser that fails this problem, I have the following podman/docker Dockerfile:

FROM debian:7

COPY sources.list /etc/apt/sources.list

RUN apt-get -o Acquire::Check-Valid-Until=false update && \
    apt-get -y install tightvncserver chromium openbox psutils iproute rxvt-unicode iceweasel xfonts-base; \
    apt-get clean

RUN echo 8736159e0a4d04d90f3de03f5d8b7d08 > /etc/machine-id; mkdir /root/.vnc; echo '' | vncpasswd -f > /root/.vnc/passwd; chmod 0700 /root/.vnc; chmod 0600 /root/.vnc/passwd

ENV USER=root

CMD vncserver :0; DISPLAY=:0 openbox
EXPOSE 5900/tcp

This requires the following sources.list file:

deb http://archive.debian.org/debian wheezy main contrib non-free
deb http://archive.debian.org/debian-security wheezy/updates main contrib non-free

Afterwards, one needs to build the image (replace podman with docker if you want to use docker):

podman build -t oldbrowsers/d7 .

And run it:

podman run -dt --rm --shm-size=512M -p 127.0.0.1:5900:5900 $DIR oldbrowsers/d7 $X
sleep 1
vncviewer :0

@elia elia added this to the v1.1 milestone Jan 19, 2021
This is an ancient browser, I know, but it's the basis
of many smartphone default browsers.
@elia
Copy link
Member

elia commented Jan 21, 2021

@hmdne I rebased this PR but for some reason it's failing on minitest, although it works locally, you have any clue on why this happens?

@hmdne
Copy link
Member Author

hmdne commented Jan 22, 2021

@elia Just some entropy got between the cogwheels of the CI. I reran the tests and they work now.

@elia elia merged commit f1b3c70 into opal:master Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants