curl --silent --user-agent "Mozilla/5.0 (X11; Linux x86_64; rv:128.0) Gecko/20100101 Firefox/128.0" http://phorge.localhost | grep -Po '<body class=".*?"'
curl --silent --user-agent "Mozilla/5.0 (Android 11; Mobile; rv:138.0) Gecko/138.0 Firefox/138.0"   http://phorge.localhost | grep -Po '<body class=".*?"'

Output before the change, observe that the second line is recognized as desktop by mistake:

<body class="device-desktop platform-linux phui-theme-blindigo phabricator-home"
<body class="device-desktop phui-theme-blindigo phabricator-home"

Output after the change, observe that the second line is finally recognized as mobile:

<body class="device-desktop platform-linux phui-theme-blindigo phabricator-home"
<body class="device-phone device phui-theme-blindigo phabricator-home"

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.Fri, May 16, 13:19

Owners added a reviewer: O1: Blessed Committers.Fri, May 16, 13:19

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptFri, May 16, 13:19

aklapper requested review of this revision.Fri, May 16, 13:19

Harbormaster completed remote builds in B1980: Diff 3050.Fri, May 16, 13:19

valerio.bozzolan added inline comments.Fri, May 16, 13:38

src/view/page/PhabricatorStandardPageView.php
629–630	In Chrome the "Mobile" is after the version so it's more complicated, but in Firefox seems easier, we can just match "Mobile.*Firefox" probably P.S. feel free to implode by `\|` and have an array if it helps in getting rid of the linter

but in Firefox seems easier, we can just match "Mobile.*Firefox" probably

I prefer that more specific regex as you never know which completely random user-agent strings are out there. It's consistent with the Chrome pattern.

aklapper marked an inline comment as done.Fri, May 16, 14:54

wrap long string to make lint happy

Harbormaster completed remote builds in B1981: Diff 3051.Fri, May 16, 14:54

valerio.bozzolan added inline comments.Fri, May 16, 15:02

src/view/page/PhabricatorStandardPageView.php
629–630	Thanks. In case you don't like `Mobile.Firefox` (that was proposed just because it's simpler and shorter and faster) at least evaluate `Mobile.Firefox/` (that is still simpler and shorter and faster but more strict with that added slash, since the specification says that there is that slash). (This is just a non blocking tip - the block is because people have to test lol)

There is something wrong now nearby the |((Chrome, that should be enclosed by pipes and just one bracket, like an hamburger of pipes, like @stuff|(Chromestuff)|(Firefox stuff)@

Premising that I do not understand the old legacy code, that could be this, without brackets:

@iPhone|iPod|Android.*Chrome/[.0-9]* Mobile@

So it should really work with just this without the slash:

@iPhone|iPod|Android.*Chrome/[.0-9]* Mobile|Mobile.*Firefox@

or with the slash:

@iPhone|iPod|Android.*Chrome/[.0-9]* Mobile|Mobile.*Firefox/@

or with slash and version:

@iPhone|iPod|Android.*Chrome/[.0-9]* Mobile|Mobile.*Firefox/[.0-9]*@

e.g. https://regex101.com/r/PDyAGh/1 (then the slash, or the slash and the version - but that is)

Right...hmm, now I also wonder. :)
Should probably be iPhone|iPod|Android.*(Chrome/[.0-9]* Mobile|Mobile.*Firefox/[.0-9]*) (it's still that Firefox on Android has that Android string first, thus the brackets).